On 5/28/20 10:32 AM, Simon Glass wrote:
> On Wed, 27 May 2020 at 00:45, Ovidiu Panait <ovidiu.pan...@windriver.com>
> wrote:
>>
>> According to the description of devfdt_get_addr_ptr, this function should
>> return NULL on failure, but currently it returns (void *)FDT_ADDR_T_NONE.
>>
>> This is also a problem because there are two definitions for
>> dev_read_addr_ptr, depending on CONFIG_DM_DEV_READ_INLINE:
>>
>> 1. one returning NULL on failure (drivers/core/read.c):
>> void *dev_read_addr_ptr(const struct udevice *dev)
>> {
>> fdt_addr_t addr = dev_read_addr(dev);
>>
>> return (addr == FDT_ADDR_T_NONE) ? NULL : map_sysmem(addr, 0);
>> }
>>
>> 2. another one, which is a wrapper over devfdt_get_addr_ptr, returning
>> (void *)FDT_ADDR_T_NONE (include/dm/read.h)
>>
>> static inline void *dev_read_addr_ptr(const struct udevice *dev)
>> {
>> return devfdt_get_addr_ptr(dev);
>> }
>>
>> Currently, some drivers which make use of devfdt_get_addr_ptr check the
>> return value for NULL:
>> drivers/i2c/mvtwsi.c
>> drivers/i2c/designware_i2c.c
>> drivers/usb/host/ehci-zynq.c
>>
>> while others check the return value for (void *)FDT_ADDR_T_NONE:
>> drivers/pinctrl/mvebu/pinctrl-mvebu.c
>> drivers/timer/ast_timer.c
>> drivers/watchdog/ast_wdt.c
>>
>> Fix this by making devfdt_get_addr_ptr return NULL on failure, as
>> described in the function comments. Also, update the drivers currently
>> checking (void *)FDT_ADDR_T_NONE to check for NULL.
>>
>> Signed-off-by: Ovidiu Panait <ovidiu.pan...@windriver.com>
>> ---
>>
>> drivers/clk/aspeed/clk_ast2500.c | 4 ++--
>> drivers/core/fdtaddr.c | 5 ++++-
>> drivers/i2c/ast_i2c.c | 4 ++--
>> drivers/pinctrl/mvebu/pinctrl-mvebu.c | 2 +-
>> drivers/timer/ast_timer.c | 4 ++--
>> drivers/watchdog/ast_wdt.c | 4 ++--
>> 6 files changed, 13 insertions(+), 10 deletions(-)
>>
>
> +Sean Anderson
It looks like I never actually got CC'd
>
> I did already review another patch on this topic[1]. I'm not sure
> when that is going to land though. Perhaps this one should be rebased
> on that? Or Matthias can check with the other custodian to see what is
> happening with that series.
Hopefully soon; I believe the last holdup was passing CI.
>
> I've sent a patch for a test which detects the inconsistency[2]. It is
> very easy to write a new test so I encourage people to do that
> whenever there is a fix like this. So with that:
I think that's a good idea. However, from what I can see, that patch
tests dev_read_addr_ptr, not devfdt_get_addr_ptr. You may also want to
check the error case, to make sure FDT_ADDR_T_NONE gets returned.
>
> Tested on sandbox
> Tested-by: Simon Glass <s...@chromium.org>
>
> [1]
> http://patchwork.ozlabs.org/project/uboot/patch/20200521161503.384823-10-sean...@gmail.com/
> [2]
> http://patchwork.ozlabs.org/project/uboot/patch/20200528082045.1.Ibd999a0382164a37d1e59c00c8c7a9ff92b8f53a@changeid/
>
--Sean
