Hi Sean,
On Thu, 28 May 2020 at 13:29, Sean Anderson <sean...@gmail.com> wrote:
>
> 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
Oops sorry.
>
>
> >
> > 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.
Yes that's right...it is a test for dev_read_addr_ptr(), although in
one case it calls the latter.
>
>
> >
> > 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
>
Regards,
Simon
