amba: Retry adding deferred devices at late_initcall

Commit Message

Rob Herring April 27, 2020, 9:25 p.m. UTC

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 <[e-mail]>
Cc: Saravana Kannan <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: Sudeep Holla <[e-mail]>
Cc: Nicolas Saenz Julienne <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Russell King <[e-mail]>
Signed-off-by: Rob Herring <[e-mail]>
---
 drivers/amba/bus.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Sudeep Holla April 28, 2020, 3:51 p.m. UTC | #1

On Mon, Apr 27, 2020 at 04:25:14PM -0500, Rob Herring 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 <[e-mail]>
> Cc: Saravana Kannan <[e-mail]>
> Cc: Linus Walleij <[e-mail]>
> Cc: Sudeep Holla <[e-mail]>

The change makes sense to me,
Reviewed-by: Sudeep Holla <[e-mail]>

This also fixed the issue I reported @[1] is fixed with this patch, so:
Tested-by: Sudeep Holla <[e-mail]>

--
Regards,
Sudeep

[1] https://lore.kernel.org/linux-arm-kernel/20200423133342.GA10628@bogus/

Linus Walleij April 28, 2020, 8:39 p.m. UTC | #2

On Mon, Apr 27, 2020 at 11:25 PM Rob Herring <[e-mail]> 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 <[e-mail]>
> Cc: Saravana Kannan <[e-mail]>
> Cc: Linus Walleij <[e-mail]>
> Cc: Sudeep Holla <[e-mail]>
> Cc: Nicolas Saenz Julienne <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Russell King <[e-mail]>
> Signed-off-by: Rob Herring <[e-mail]>

Makes sense to me, and the same approach is found
in the generic code in drivers/base/dd.c so
Reviewed-by: Linus Walleij <[e-mail]>

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.

Yours,
Linus Walleij

Marek Szyprowski April 29, 2020, 6:06 a.m. UTC | #3

Hi Linus,

On 28.04.2020 22:39, Linus Walleij wrote:
> On Mon, Apr 27, 2020 at 11:25 PM Rob Herring <[e-mail]> 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 <[e-mail]>
>> Cc: Saravana Kannan <[e-mail]>
>> Cc: Linus Walleij <[e-mail]>
>> Cc: Sudeep Holla <[e-mail]>
>> Cc: Nicolas Saenz Julienne <[e-mail]>
>> Cc: Geert Uytterhoeven <[e-mail]>
>> Cc: Russell King <[e-mail]>
>> Signed-off-by: Rob Herring <[e-mail]>
> Makes sense to me, and the same approach is found
> in the generic code in drivers/base/dd.c so
> Reviewed-by: Linus Walleij <[e-mail]>
>
> 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.

Best regards

Saravana Kannan April 29, 2020, 7:33 a.m. UTC | #4

On Tue, Apr 28, 2020 at 11:06 PM Marek Szyprowski
<[e-mail]> wrote:
>
> Hi Linus,
>
> On 28.04.2020 22:39, Linus Walleij wrote:
> > On Mon, Apr 27, 2020 at 11:25 PM Rob Herring <[e-mail]> 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 <[e-mail]>
> >> Cc: Saravana Kannan <[e-mail]>
> >> Cc: Linus Walleij <[e-mail]>
> >> Cc: Sudeep Holla <[e-mail]>
> >> Cc: Nicolas Saenz Julienne <[e-mail]>
> >> Cc: Geert Uytterhoeven <[e-mail]>
> >> Cc: Russell King <[e-mail]>
> >> Signed-off-by: Rob Herring <[e-mail]>
> > Makes sense to me, and the same approach is found
> > in the generic code in drivers/base/dd.c so
> > Reviewed-by: Linus Walleij <[e-mail]>
> >
> > 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.

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.

-Saravana

Linus Walleij April 29, 2020, 12:26 p.m. UTC | #5

On Wed, Apr 29, 2020 at 8:06 AM Marek Szyprowski
<[e-mail]> wrote:
> [Me]
> > 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:

I see, thanks for the context.

> For me it is fine to get back to late initcall based solution, though.

The idea here is to do both/and initcall and timer deferred probe,
not either/or.

And the dd.c core also does both/and.

And then it does some more things, so I was thinking we may be
missing out on some of them?

Yours,
Linus Walleij

Rob Herring April 29, 2020, 2:01 p.m. UTC | #6

On Wed, Apr 29, 2020 at 7:26 AM Linus Walleij <[e-mail]> wrote:
>
> On Wed, Apr 29, 2020 at 8:06 AM Marek Szyprowski
> <[e-mail]> wrote:
> > [Me]
> > > 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:
>
> I see, thanks for the context.
>
> > For me it is fine to get back to late initcall based solution, though.
>
> The idea here is to do both/and initcall and timer deferred probe,
> not either/or.

Yes.

> And the dd.c core also does both/and.

Not really. Deferred probe delays retrying until late_initcall and
then somewhat intelligently retries after successful probes rather
than time based.

> And then it does some more things, so I was thinking we may be
> missing out on some of them?

You mean like timeout (which is getting moved back to 0 as timeout
causes problems with NFS root)? Remember this is deferred add, not
deferred probe. Once the amba devices are added, the deferred probe in
dd.c takes over.

Rob

Ulf Hansson May 4, 2020, 7:10 p.m. UTC | #7

On Wed, 29 Apr 2020 at 09:33, Saravana Kannan <[e-mail]> wrote:
>
> On Tue, Apr 28, 2020 at 11:06 PM Marek Szyprowski
> <[e-mail]> wrote:
> >
> > Hi Linus,
> >
> > On 28.04.2020 22:39, Linus Walleij wrote:
> > > On Mon, Apr 27, 2020 at 11:25 PM Rob Herring <[e-mail]> 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 <[e-mail]>
> > >> Cc: Saravana Kannan <[e-mail]>
> > >> Cc: Linus Walleij <[e-mail]>
> > >> Cc: Sudeep Holla <[e-mail]>
> > >> Cc: Nicolas Saenz Julienne <[e-mail]>
> > >> Cc: Geert Uytterhoeven <[e-mail]>
> > >> Cc: Russell King <[e-mail]>
> > >> Signed-off-by: Rob Herring <[e-mail]>
> > > Makes sense to me, and the same approach is found
> > > in the generic code in drivers/base/dd.c so
> > > Reviewed-by: Linus Walleij <[e-mail]>
> > >
> > > 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.

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.

Kind regards
Uffe

Saravana Kannan May 4, 2020, 7:28 p.m. UTC | #8

On Mon, May 4, 2020 at 12:11 PM Ulf Hansson <[e-mail]> wrote:
>
> On Wed, 29 Apr 2020 at 09:33, Saravana Kannan <[e-mail]> wrote:
> >
> > On Tue, Apr 28, 2020 at 11:06 PM Marek Szyprowski
> > <[e-mail]> wrote:
> > >
> > > Hi Linus,
> > >
> > > On 28.04.2020 22:39, Linus Walleij wrote:
> > > > On Mon, Apr 27, 2020 at 11:25 PM Rob Herring <[e-mail]> 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 <[e-mail]>
> > > >> Cc: Saravana Kannan <[e-mail]>
> > > >> Cc: Linus Walleij <[e-mail]>
> > > >> Cc: Sudeep Holla <[e-mail]>
> > > >> Cc: Nicolas Saenz Julienne <[e-mail]>
> > > >> Cc: Geert Uytterhoeven <[e-mail]>
> > > >> Cc: Russell King <[e-mail]>
> > > >> Signed-off-by: Rob Herring <[e-mail]>
> > > > Makes sense to me, and the same approach is found
> > > > in the generic code in drivers/base/dd.c so
> > > > Reviewed-by: Linus Walleij <[e-mail]>
> > > >
> > > > 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

Marek Szyprowski May 7, 2020, 11:44 a.m. UTC | #9

Hi Saravana,

On 04.05.2020 21:28, Saravana Kannan wrote:
> On Mon, May 4, 2020 at 12:11 PM Ulf Hansson <[e-mail]> wrote:
>> 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.

The time I've prepared my patch I've also considered something like 
that, but I gave up because timer or notifier based approach was much 
simpler. If you have some time to implement your idea I would be happy 
to test it.

Best regards

Saravana Kannan May 7, 2020, 5:39 p.m. UTC | #10

On Thu, May 7, 2020 at 4:44 AM Marek Szyprowski
<[e-mail]> wrote:
>
> Hi Saravana,
>
> On 04.05.2020 21:28, Saravana Kannan wrote:
> > On Mon, May 4, 2020 at 12:11 PM Ulf Hansson <[e-mail]> wrote:
> >> 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.
>
> The time I've prepared my patch I've also considered something like
> that, but I gave up because timer or notifier based approach was much
> simpler.

Maybe I'll reach the same conclusion. We'll see.

> If you have some time to implement your idea I would be happy
> to test it.

Thanks. I'll take a stab at it then. Btw, does this need to support
the non-DT/machine file way of populating device/getting resources
too? Are or all supported configurations DT based?

-Saravana

Rob Herring May 8, 2020, 1:41 p.m. UTC | #11

On Thu, May 7, 2020 at 12:39 PM Saravana Kannan <[e-mail]> wrote:
>
> On Thu, May 7, 2020 at 4:44 AM Marek Szyprowski
> <[e-mail]> wrote:
> >
> > Hi Saravana,
> >
> > On 04.05.2020 21:28, Saravana Kannan wrote:
> > > On Mon, May 4, 2020 at 12:11 PM Ulf Hansson <[e-mail]> wrote:
> > >> 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.
> >
> > The time I've prepared my patch I've also considered something like
> > that, but I gave up because timer or notifier based approach was much
> > simpler.
>
> Maybe I'll reach the same conclusion. We'll see.
>
> > If you have some time to implement your idea I would be happy
> > to test it.
>
> Thanks. I'll take a stab at it then. Btw, does this need to support
> the non-DT/machine file way of populating device/getting resources
> too? Are or all supported configurations DT based?

Not sure. There may still be a few cases of non-DT. If everything is
DT, we could probably just get rid of AMBA bus and make everything
platform drivers. We have the compatible strings to match on already
(we only use 'arm,primecell' currently).

I'm not really a fan of creating a dummy platform device. There's
other cases of needing to power on or enable devices before probe.
SDIO devices are one case. That's how we ended up with the mmc-pwrseq
binding. I really think we need some bus/driver hook between device
add and probe. Or maybe more precisely it is just between add and the
add uevent.

Rob

Saravana Kannan May 8, 2020, 7:19 p.m. UTC | #12

On Fri, May 8, 2020 at 6:41 AM Rob Herring <[e-mail]> wrote:
>
> On Thu, May 7, 2020 at 12:39 PM Saravana Kannan <[e-mail]> wrote:
> >
> > On Thu, May 7, 2020 at 4:44 AM Marek Szyprowski
> > <[e-mail]> wrote:
> > >
> > > Hi Saravana,
> > >
> > > On 04.05.2020 21:28, Saravana Kannan wrote:
> > > > On Mon, May 4, 2020 at 12:11 PM Ulf Hansson <[e-mail]> wrote:
> > > >> 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.
> > >
> > > The time I've prepared my patch I've also considered something like
> > > that, but I gave up because timer or notifier based approach was much
> > > simpler.
> >
> > Maybe I'll reach the same conclusion. We'll see.
> >
> > > If you have some time to implement your idea I would be happy
> > > to test it.
> >
> > Thanks. I'll take a stab at it then. Btw, does this need to support
> > the non-DT/machine file way of populating device/getting resources
> > too? Are or all supported configurations DT based?
>
> Not sure. There may still be a few cases of non-DT. If everything is
> DT, we could probably just get rid of AMBA bus and make everything
> platform drivers. We have the compatible strings to match on already
> (we only use 'arm,primecell' currently).

Ok

> I'm not really a fan of creating a dummy platform device.

I'm not saying it's awesome, but I think it would make amba code a bit
cleaner and more importantly easier to maintain? It'll get all future
deferred probe fixes automatically instead of having to replicate it
here? Don't have a strong opinion, just thinking out aloud.

> There's
> other cases of needing to power on or enable devices before probe.
> SDIO devices are one case. That's how we ended up with the mmc-pwrseq
> binding.

But how many of them have this weird userspace dependency where you
can't tell userspace about the device (too soon) but also can't probe
without userspace? At least the mmc-pwrseq doesn't seem to have those
issues.

> I really think we need some bus/driver hook between device
> add and probe. Or maybe more precisely it is just between add and the
> add uevent.

That feels kinda odd to me though "Hey, add this device, but don't
tell userspace". Might be better to keep the "complexity" away in the
places that have odd requirements?

-Saravana

Patch

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index fe1523664816..e797995fc65b 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -505,7 +505,7 @@  static DECLARE_DELAYED_WORK(deferred_retry_work, amba_deferred_retry_func);
 
 #define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000))
 
-static void amba_deferred_retry_func(struct work_struct *dummy)
+static int amba_deferred_retry(void)
 {
 	struct deferred_device *ddev, *tmp;
 
@@ -521,11 +521,19 @@  static void amba_deferred_retry_func(struct work_struct *dummy)
 		kfree(ddev);
 	}
 
+	mutex_unlock(&deferred_devices_lock);
+
+	return 0;
+}
+late_initcall(amba_deferred_retry);
+
+static void amba_deferred_retry_func(struct work_struct *dummy)
+{
+	amba_deferred_retry();
+
 	if (!list_empty(&deferred_devices))
 		schedule_delayed_work(&deferred_retry_work,
 				      DEFERRED_DEVICE_TIMEOUT);
-
-	mutex_unlock(&deferred_devices_lock);
 }
 
 /**