Re: [PATCH] amba: Retry adding deferred devices at late_initcall

[Saravana Kannan <saravanak@google.com>]

On Mon, May 4, 2020 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 29 Apr 2020 at 09:33, Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Apr 28, 2020 at 11:06 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> > >
> > > Hi Linus,
> > >
> > > On 28.04.2020 22:39, Linus Walleij wrote:
> > > > On Mon, Apr 27, 2020 at 11:25 PM Rob Herring <robh@kernel.org> wrote:
> > > >> If amba bus devices defer when adding, the amba bus code simply retries
> > > >> adding the devices every 5 seconds. This doesn't work well as it
> > > >> completely unsynchronized with starting the init process which can
> > > >> happen in less than 5 secs. Add a retry during late_initcall. If the
> > > >> amba devices are added, then deferred probe takes over. If the
> > > >> dependencies have not probed at this point, then there's no improvement
> > > >> over previous behavior. To completely solve this, we'd need to retry
> > > >> after every successful probe as deferred probe does.
> > > >>
> > > >> The list_empty() check now happens outside the mutex, but the mutex
> > > >> wasn't necessary in the first place.
> > > >>
> > > >> This needed to use deferred probe instead of fragile initcall ordering
> > > >> on 32-bit VExpress systems where the apb_pclk has a number of probe
> > > >> dependencies (vexpress-sysregs, vexpress-config).
> > > >>
> > > >> Cc: John Stultz <john.stultz@linaro.org>
> > > >> Cc: Saravana Kannan <saravanak@google.com>
> > > >> Cc: Linus Walleij <linus.walleij@linaro.org>
> > > >> Cc: Sudeep Holla <sudeep.holla@arm.com>
> > > >> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >> Cc: Russell King <linux@armlinux.org.uk>
> > > >> Signed-off-by: Rob Herring <robh@kernel.org>
> > > > Makes sense to me, and the same approach is found
> > > > in the generic code in drivers/base/dd.c so
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > >
> > > > The timer-based re-probe was added by Marek Szyprowski
> > > > in commit a41980f2a3eb33ed7a2636e83498b47e95ceb05b
> > > > do deal with power domains. I guess it mimics dd.c
> > > > deferred probe at this point?
> > > >
> > > > There are a bit of other differences that have piled up,
> > > > should we take a quick look at dd.c so there is not something
> > > > else we're missing? I see some PM code for example.
> > >
> > > Well, late initcall based approach is what earlier version of my patch did:
> > >
> > > https://lkml.org/lkml/2016/4/12/414
> > >
> > > but then it has been requested to solve the issue 'properly':
> > >
> > > https://lkml.org/lkml/2016/4/12/455
> > >
> > > https://lkml.org/lkml/2016/4/14/875
> > >
> > > For me it is fine to get back to late initcall based solution, though.
> > >
> >
> > I haven't really dealt with a platform with AMBA devices and am not
> > too familiar with it, so apologies in advance if any of my suggestions
> > are silly.
> >
> > This whole "don't add a device before the clocks/resources needed to
> > read the PID/CID" seems like something that can be solved by having a
> > dummy device that "probes" when those resources are available. And
> > during its probe, it adds the real amba device. That should avoid all
> > the reinvention of deferred probing that amba/bus.c seems to be
> > attempting.
>
> Sounds like a clever idea.

Thanks

> In principle we should then be able to rely on the regular defered
> probe mechanism, just that it's the dummy device that is being defered
> probed (if we fail to read PID/CID).
>
> >
> > Any reason to not do something like that? I'd think that should clean
> > up a whole lot of this code. Also, if we are primarily dealing with
> > AMBA devices created from DT, then we might even be able to massage
> > the fw_devlink feature to optimize this even more when fw_devlink=on.
> >
> > Just my 2 cents.
>
> Someone should try to implement this to see if it fits well.

I don't mind taking a stab at this if people are actually okay with
this approach and will test and merge it if it works. I have no
platform to test this. I'll wait to hear what others think before I
jump on this.

-Saravana

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel