mbox series

[v1,0/9] fw_devlink improvements

Message ID 20220810060040.321697-1-saravanak@google.com
Headers show
Series fw_devlink improvements | expand

Message

Saravana Kannan Aug. 10, 2022, 6 a.m. UTC
This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It
   figures this our more dynamically. The only expectation is that
   fwnode that are converted to devices actually get probed by a driver
   for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device
   links from the consumer to the actual resource's device (if it has one,
   Eg: gpio_device) instead of the parent supplier device. This improves
   things like async suspend/resume ordering, potentially remove the need
   for frameworks to create device links, more parallelized async probing,
   and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets
   populated as a device before its parent firmware node AND actually
   supplies a non-optional resource to the parent firmware node's
   device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific
   code.

This took way too long to get done due to typo bugs I had in my rewrite or
corner cases I had to find and handle. But it's fairly well tested at this
point and I expect this to work properly.

Abel & Doug,

This should fix your cyclic dependency issues with your display. Can you
give it a shot please?

Alexander,

This should fix your issue where the power domain device not having a
compatible property. Can you give it a shot please?

Tony,

This should handle the odd case of the child being the supplier of the
parent. Can you please give this a shot? I want to make sure the cycle
detection code handles this properly and treats it like it's NOT a cycle.

Geert,

Can you test the renesas stuff I changed please? They should continue
working like before. Any other sanity test on other hardware would be
great too.

Sudeep,

I don't think there are any unfixed issues you had reported in my other
patches that this series might fix, but it'll be nice if you could give
this a sanity test.

Guenter,

I don't think this will fix the issue you reported in the amba patch, but
it's worth a shot because it improves a bunch of corner case handling. So
it might be better at handling whatever corner cases you might have in the
qemu platforms.

Thanks,
Saravana

Cc: Abel Vesa <abel.vesa@linaro.org>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: John Stultz <jstultz@google.com>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Guenter Roeck <linux@roeck-us.net>

Saravana Kannan (9):
  driver core: fw_devlink: Don't purge child fwnode's consumer links
  driver core: fw_devlink: Improve check for fwnode with no
    device/driver
  soc: renesas: Move away from using OF_POPULATED for fw_devlink
  gpiolib: Clear the gpio_device's fwnode initialized flag before adding
  driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
  driver core: fw_devlink: Allow marking a fwnode link as being part of
    a cycle
  driver core: fw_devlink: Consolidate device link flag computation
  driver core: fw_devlink: Make cycle detection more robust
  of: property: Simplify of_link_to_phandle()

 drivers/base/core.c             | 437 +++++++++++++++++++++-----------
 drivers/gpio/gpiolib.c          |   6 +
 drivers/of/property.c           |  84 +-----
 drivers/soc/renesas/rcar-sysc.c |   2 +-
 include/linux/device.h          |   1 +
 include/linux/fwnode.h          |  12 +-
 6 files changed, 323 insertions(+), 219 deletions(-)

Comments

Tony Lindgren Aug. 12, 2022, 9:49 a.m. UTC | #1
* Saravana Kannan <saravanak@google.com> [220810 05:54]:
> Tony,
> 
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.

Yup, this series works for me, so feel free to add:

Tested-by: Tony Lindgren <tony@atomide.com>

I have some concerns though on how do we get a working -rc1 with the
earlier series applied? See the comments in the last patch of this
series.

Regards,

Tony
Saravana Kannan Aug. 13, 2022, 12:51 a.m. UTC | #2
On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * Saravana Kannan <saravanak@google.com> [220810 05:54]:
> > Tony,
> >
> > This should handle the odd case of the child being the supplier of the
> > parent. Can you please give this a shot? I want to make sure the cycle
> > detection code handles this properly and treats it like it's NOT a cycle.
>
> Yup, this series works for me, so feel free to add:
>
> Tested-by: Tony Lindgren <tony@atomide.com>

Thanks for testing!

Btw, out of curiosity, how many different boards did you test this on?
IIRC you had an issue only in one board, right? Not to say I didn't
break anything else, I'm just trying to see how much confidence we
have on this series so far. I'm hoping the rest of the folks I listed
in the email will get around to testing this series.

-Saravana

> I have some concerns though on how do we get a working -rc1 with the
> earlier series applied? See the comments in the last patch of this
> series.

I tried to reply, but not sure if it helps. We'll continue the discussion there.

-Saravana
Saravana Kannan Aug. 14, 2022, 5:59 a.m. UTC | #3
+Naresh Kamboju

On Tue, Aug 9, 2022 at 11:00 PM Saravana Kannan <saravanak@google.com> wrote:
>
> This patch series improves fw_devlink in the following ways:
>
> 1. It no longer cares about a fwnode having a "compatible" property. It
>    figures this our more dynamically. The only expectation is that
>    fwnode that are converted to devices actually get probed by a driver
>    for the dependencies to be enforced correctly.
>
> 2. Finer grained dependency tracking. fw_devlink will now create device
>    links from the consumer to the actual resource's device (if it has one,
>    Eg: gpio_device) instead of the parent supplier device. This improves
>    things like async suspend/resume ordering, potentially remove the need
>    for frameworks to create device links, more parallelized async probing,
>    and better sync_state() tracking.
>
> 3. Handle hardware/software quirks where a child firmware node gets
>    populated as a device before its parent firmware node AND actually
>    supplies a non-optional resource to the parent firmware node's
>    device.
>
> 4. Way more robust at cycle handling (see patch for the insane cases).
>
> 5. Stops depending on OF_POPULATED to figure out some corner cases.
>
> 6. Simplifies the work that needs to be done by the firmware specific
>    code.
>
> This took way too long to get done due to typo bugs I had in my rewrite or
> corner cases I had to find and handle. But it's fairly well tested at this
> point and I expect this to work properly.
>
> Abel & Doug,
>
> This should fix your cyclic dependency issues with your display. Can you
> give it a shot please?
>
> Alexander,
>
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?
>
> Tony,
>
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.
>
> Geert,
>
> Can you test the renesas stuff I changed please? They should continue
> working like before. Any other sanity test on other hardware would be
> great too.
>
> Sudeep,
>
> I don't think there are any unfixed issues you had reported in my other
> patches that this series might fix, but it'll be nice if you could give
> this a sanity test.
>
> Guenter,
>
> I don't think this will fix the issue you reported in the amba patch, but
> it's worth a shot because it improves a bunch of corner case handling. So
> it might be better at handling whatever corner cases you might have in the
> qemu platforms.

Hi Naresh,

Thanks for testing these patches in the other thread. Mind giving your
tested-by here? I know you tested these patches in X15, but were there
also other boards these patches were tested on as part of the run?

Thanks,
Saravana


>
> Thanks,
> Saravana
>
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: John Stultz <jstultz@google.com>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
>
> Saravana Kannan (9):
>   driver core: fw_devlink: Don't purge child fwnode's consumer links
>   driver core: fw_devlink: Improve check for fwnode with no
>     device/driver
>   soc: renesas: Move away from using OF_POPULATED for fw_devlink
>   gpiolib: Clear the gpio_device's fwnode initialized flag before adding
>   driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
>   driver core: fw_devlink: Allow marking a fwnode link as being part of
>     a cycle
>   driver core: fw_devlink: Consolidate device link flag computation
>   driver core: fw_devlink: Make cycle detection more robust
>   of: property: Simplify of_link_to_phandle()
>
>  drivers/base/core.c             | 437 +++++++++++++++++++++-----------
>  drivers/gpio/gpiolib.c          |   6 +
>  drivers/of/property.c           |  84 +-----
>  drivers/soc/renesas/rcar-sysc.c |   2 +-
>  include/linux/device.h          |   1 +
>  include/linux/fwnode.h          |  12 +-
>  6 files changed, 323 insertions(+), 219 deletions(-)
>
> --
> 2.37.1.559.g78731f0fdb-goog
>
Naresh Kamboju Aug. 15, 2022, 10:17 a.m. UTC | #4
On Sun, 14 Aug 2022 at 11:29, Saravana Kannan <saravanak@google.com> wrote:
>
> +Naresh Kamboju
>
> On Tue, Aug 9, 2022 at 11:00 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > This patch series improves fw_devlink in the following ways:
> >
> > 1. It no longer cares about a fwnode having a "compatible" property. It
> >    figures this our more dynamically. The only expectation is that
> >    fwnode that are converted to devices actually get probed by a driver
> >    for the dependencies to be enforced correctly.
> >
> > 2. Finer grained dependency tracking. fw_devlink will now create device
> >    links from the consumer to the actual resource's device (if it has one,
> >    Eg: gpio_device) instead of the parent supplier device. This improves
> >    things like async suspend/resume ordering, potentially remove the need
> >    for frameworks to create device links, more parallelized async probing,
> >    and better sync_state() tracking.
> >
> > 3. Handle hardware/software quirks where a child firmware node gets
> >    populated as a device before its parent firmware node AND actually
> >    supplies a non-optional resource to the parent firmware node's
> >    device.
> >
> > 4. Way more robust at cycle handling (see patch for the insane cases).
> >
> > 5. Stops depending on OF_POPULATED to figure out some corner cases.
> >
> > 6. Simplifies the work that needs to be done by the firmware specific
> >    code.
> >
> > This took way too long to get done due to typo bugs I had in my rewrite or
> > corner cases I had to find and handle. But it's fairly well tested at this
> > point and I expect this to work properly.
> >
> > Abel & Doug,
> >
> > This should fix your cyclic dependency issues with your display. Can you
> > give it a shot please?
> >
> > Alexander,
> >
> > This should fix your issue where the power domain device not having a
> > compatible property. Can you give it a shot please?
> >
> > Tony,
> >
> > This should handle the odd case of the child being the supplier of the
> > parent. Can you please give this a shot? I want to make sure the cycle
> > detection code handles this properly and treats it like it's NOT a cycle.
> >
> > Geert,
> >
> > Can you test the renesas stuff I changed please? They should continue
> > working like before. Any other sanity test on other hardware would be
> > great too.
> >
> > Sudeep,
> >
> > I don't think there are any unfixed issues you had reported in my other
> > patches that this series might fix, but it'll be nice if you could give
> > this a sanity test.
> >
> > Guenter,
> >
> > I don't think this will fix the issue you reported in the amba patch, but
> > it's worth a shot because it improves a bunch of corner case handling. So
> > it might be better at handling whatever corner cases you might have in the
> > qemu platforms.
>
> Hi Naresh,
>
> Thanks for testing these patches in the other thread. Mind giving your
> tested-by here? I know you tested these patches in X15, but were there
> also other boards these patches were tested on as part of the run?

I have tested your patches and boot is successful on x15 device and Juno-r2.

Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

> > Cc: Abel Vesa <abel.vesa@linaro.org>
> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: John Stultz <jstultz@google.com>
> > Cc: Doug Anderson <dianders@chromium.org>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> >
> > Saravana Kannan (9):
> >   driver core: fw_devlink: Don't purge child fwnode's consumer links
> >   driver core: fw_devlink: Improve check for fwnode with no
> >     device/driver
> >   soc: renesas: Move away from using OF_POPULATED for fw_devlink
> >   gpiolib: Clear the gpio_device's fwnode initialized flag before adding
> >   driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
> >   driver core: fw_devlink: Allow marking a fwnode link as being part of
> >     a cycle
> >   driver core: fw_devlink: Consolidate device link flag computation
> >   driver core: fw_devlink: Make cycle detection more robust
> >   of: property: Simplify of_link_to_phandle()
> >
> >  drivers/base/core.c             | 437 +++++++++++++++++++++-----------
> >  drivers/gpio/gpiolib.c          |   6 +
> >  drivers/of/property.c           |  84 +-----
> >  drivers/soc/renesas/rcar-sysc.c |   2 +-
> >  include/linux/device.h          |   1 +
> >  include/linux/fwnode.h          |  12 +-
> >  6 files changed, 323 insertions(+), 219 deletions(-)
> >
> > --
> > 2.37.1.559.g78731f0fdb-goog

- Naresh
Tony Lindgren Aug. 15, 2022, 10:33 a.m. UTC | #5
* Saravana Kannan <saravanak@google.com> [220813 00:45]:
> On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Saravana Kannan <saravanak@google.com> [220810 05:54]:
> > > Tony,
> > >
> > > This should handle the odd case of the child being the supplier of the
> > > parent. Can you please give this a shot? I want to make sure the cycle
> > > detection code handles this properly and treats it like it's NOT a cycle.
> >
> > Yup, this series works for me, so feel free to add:
> >
> > Tested-by: Tony Lindgren <tony@atomide.com>
> 
> Thanks for testing!
> 
> Btw, out of curiosity, how many different boards did you test this on?
> IIRC you had an issue only in one board, right? Not to say I didn't
> break anything else, I'm just trying to see how much confidence we
> have on this series so far. I'm hoping the rest of the folks I listed
> in the email will get around to testing this series.

Sorry if I was not clear earlier. The issue affects several generations
of TI 32-bit SoCs at least, not just one board.

> > I have some concerns though on how do we get a working -rc1 with the
> > earlier series applied? See the comments in the last patch of this
> > series.
> 
> I tried to reply, but not sure if it helps. We'll continue the discussion there.

Ack.

Tony
Abel Vesa Aug. 15, 2022, 11:01 a.m. UTC | #6
On 22-08-09 23:00:29, Saravana Kannan wrote:
> This patch series improves fw_devlink in the following ways:
> 
> 1. It no longer cares about a fwnode having a "compatible" property. It
>    figures this our more dynamically. The only expectation is that
>    fwnode that are converted to devices actually get probed by a driver
>    for the dependencies to be enforced correctly.
> 
> 2. Finer grained dependency tracking. fw_devlink will now create device
>    links from the consumer to the actual resource's device (if it has one,
>    Eg: gpio_device) instead of the parent supplier device. This improves
>    things like async suspend/resume ordering, potentially remove the need
>    for frameworks to create device links, more parallelized async probing,
>    and better sync_state() tracking.
> 
> 3. Handle hardware/software quirks where a child firmware node gets
>    populated as a device before its parent firmware node AND actually
>    supplies a non-optional resource to the parent firmware node's
>    device.
> 
> 4. Way more robust at cycle handling (see patch for the insane cases).
> 
> 5. Stops depending on OF_POPULATED to figure out some corner cases.
> 
> 6. Simplifies the work that needs to be done by the firmware specific
>    code.
> 
> This took way too long to get done due to typo bugs I had in my rewrite or
> corner cases I had to find and handle. But it's fairly well tested at this
> point and I expect this to work properly.
> 
> Abel & Doug,
> 
> This should fix your cyclic dependency issues with your display. Can you
> give it a shot please?

Tested the specific case we discussed about here:
https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/raw

Thanks for fixing this.

Tested-by: Abel Vesa <abel.vesa@linaro.org>

> 
> Alexander,
> 
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?
> 
> Tony,
> 
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.
> 
> Geert,
> 
> Can you test the renesas stuff I changed please? They should continue
> working like before. Any other sanity test on other hardware would be
> great too.
> 
> Sudeep,
> 
> I don't think there are any unfixed issues you had reported in my other
> patches that this series might fix, but it'll be nice if you could give
> this a sanity test.
> 
> Guenter,
> 
> I don't think this will fix the issue you reported in the amba patch, but
> it's worth a shot because it improves a bunch of corner case handling. So
> it might be better at handling whatever corner cases you might have in the
> qemu platforms.
> 
> Thanks,
> Saravana
> 
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: John Stultz <jstultz@google.com>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> 
> Saravana Kannan (9):
>   driver core: fw_devlink: Don't purge child fwnode's consumer links
>   driver core: fw_devlink: Improve check for fwnode with no
>     device/driver
>   soc: renesas: Move away from using OF_POPULATED for fw_devlink
>   gpiolib: Clear the gpio_device's fwnode initialized flag before adding
>   driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
>   driver core: fw_devlink: Allow marking a fwnode link as being part of
>     a cycle
>   driver core: fw_devlink: Consolidate device link flag computation
>   driver core: fw_devlink: Make cycle detection more robust
>   of: property: Simplify of_link_to_phandle()
> 
>  drivers/base/core.c             | 437 +++++++++++++++++++++-----------
>  drivers/gpio/gpiolib.c          |   6 +
>  drivers/of/property.c           |  84 +-----
>  drivers/soc/renesas/rcar-sysc.c |   2 +-
>  include/linux/device.h          |   1 +
>  include/linux/fwnode.h          |  12 +-
>  6 files changed, 323 insertions(+), 219 deletions(-)
> 
> -- 
> 2.37.1.559.g78731f0fdb-goog
>
Alexander Stein Aug. 15, 2022, 12:39 p.m. UTC | #7
Hello Saravana,

Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> Alexander,
> 
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?

thanks for the update. Unfortunately this does not work:

> [    0.774838] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@0
> [    0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed to 
find PM domain: -2
> [    0.775324] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@2
> [    0.775601] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@3
> [    0.775842] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@4
> [    0.776642] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@7
> [    0.776897] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@8
> [    0.777158] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@9
> [    0.777405] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@a
> [    0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach() 
failed to find PM domain: -2
> [    0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed to 
attach power domain "bus"
> [    0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed to 
find PM domain: -2
> [    1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> [    1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> [    1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> [    1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with 
0-0008
> [    1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with 
0-0008

The required power-supply for the power domains is still not yet available.
Does this series require some other patches as well?

Whats worse, starting with commit 9/9 [of: property: Simplify 
of_link_to_phandle()], other drivers fail to probe waiting for pinctrl to be 
available.
> $ cat /sys/kernel/debug/devices_deferred
> gpio-leds       platform: wait for supplier gpioledgrp
> extcon-usbotg0  platform: wait for supplier usb0congrp
> gpio-keys       platform: wait for supplier gpiobuttongrp
> regulator-otg-vbus      platform: wait for supplier reggotgvbusgrp
> regulator-vdd-arm       platform: wait for supplier dvfsgrp

Apparently for some reason they are not probed again, once the pinctrl driver 
probed.

Best reagrds,
Alexander
Sudeep Holla Aug. 15, 2022, 1:52 p.m. UTC | #8
On Tue, Aug 09, 2022 at 11:00:29PM -0700, Saravana Kannan wrote:
> This patch series improves fw_devlink in the following ways:
> 
> 1. It no longer cares about a fwnode having a "compatible" property. It
>    figures this our more dynamically. The only expectation is that
>    fwnode that are converted to devices actually get probed by a driver
>    for the dependencies to be enforced correctly.
> 
> 2. Finer grained dependency tracking. fw_devlink will now create device
>    links from the consumer to the actual resource's device (if it has one,
>    Eg: gpio_device) instead of the parent supplier device. This improves
>    things like async suspend/resume ordering, potentially remove the need
>    for frameworks to create device links, more parallelized async probing,
>    and better sync_state() tracking.
> 
> 3. Handle hardware/software quirks where a child firmware node gets
>    populated as a device before its parent firmware node AND actually
>    supplies a non-optional resource to the parent firmware node's
>    device.
> 
> 4. Way more robust at cycle handling (see patch for the insane cases).
> 
> 5. Stops depending on OF_POPULATED to figure out some corner cases.
> 
> 6. Simplifies the work that needs to be done by the firmware specific
>    code.
> 
> This took way too long to get done due to typo bugs I had in my rewrite or
> corner cases I had to find and handle. But it's fairly well tested at this
> point and I expect this to work properly.
> 
> Abel & Doug,
> 
> This should fix your cyclic dependency issues with your display. Can you
> give it a shot please?
> 
> Alexander,
> 
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?
> 
> Tony,
> 
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.
> 
> Geert,
> 
> Can you test the renesas stuff I changed please? They should continue
> working like before. Any other sanity test on other hardware would be
> great too.
> 
> Sudeep,
> 
> I don't think there are any unfixed issues you had reported in my other
> patches that this series might fix, but it'll be nice if you could give
> this a sanity test.
> 

Sure tested this on Juno on top of v6.0-rc1 and found no regressions.
So,

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

Just wanted to check if the logs are intentional or do you plan to make
them debug. On Juno with hardly few such dependencies I get below extra
logs during boot, it may add loads on other platforms. I am fine either
way, just thought of checking.

| amba 20040000.funnel: Fixed dependency cycle(s) with /etf@20010000/in-ports/port/endpoint
| amba 20120000.replicator: Fixed dependency cycle(s) with /etr@20070000/in-ports/port/endpoint
| amba 20120000.replicator: Fixed dependency cycle(s) with /tpiu@20030000/in-ports/port/endpoint
| amba 220c0000.funnel: Fixed dependency cycle(s) with /etm@22040000/out-ports/port/endpoint
| amba 220c0000.funnel: Fixed dependency cycle(s) with /funnel@20040000/in-ports/port@0/endpoint
| amba 22140000.etm: Fixed dependency cycle(s) with /funnel@220c0000/in-ports/port@1/endpoint
| amba 230c0000.funnel: Fixed dependency cycle(s) with /etm@23040000/out-ports/port/endpoint
| amba 230c0000.funnel: Fixed dependency cycle(s) with /funnel@20040000/in-ports/port@1/endpoint
| amba 23140000.etm: Fixed dependency cycle(s) with /funnel@230c0000/in-ports/port@1/endpoint
| amba 23240000.etm: Fixed dependency cycle(s) with /funnel@230c0000/in-ports/port@2/endpoint
| amba 23340000.etm: Fixed dependency cycle(s) with /funnel@230c0000/in-ports/port@3/endpoint
| amba 20130000.funnel: Fixed dependency cycle(s) with /stm@20100000/out-ports/port/endpoint
| amba 20140000.etf: Fixed dependency cycle(s) with /funnel@20130000/out-ports/port/endpoint
| amba 20150000.funnel: Fixed dependency cycle(s) with /etf@20140000/out-ports/port/endpoint
| amba 20150000.funnel: Fixed dependency cycle(s) with /etf@20010000/out-ports/port/endpoint
| amba 20150000.funnel: Fixed dependency cycle(s) with /replicator@20120000/in-ports/port/endpoint
| i2c 0-0070: Fixed dependency cycle(s) with /hdlcd@7ff60000/port/endpoint
| i2c 0-0071: Fixed dependency cycle(s) with /hdlcd@7ff50000/port/endpoint
Saravana Kannan Aug. 15, 2022, 6:23 p.m. UTC | #9
On Mon, Aug 15, 2022 at 3:33 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * Saravana Kannan <saravanak@google.com> [220813 00:45]:
> > On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > * Saravana Kannan <saravanak@google.com> [220810 05:54]:
> > > > Tony,
> > > >
> > > > This should handle the odd case of the child being the supplier of the
> > > > parent. Can you please give this a shot? I want to make sure the cycle
> > > > detection code handles this properly and treats it like it's NOT a cycle.
> > >
> > > Yup, this series works for me, so feel free to add:
> > >
> > > Tested-by: Tony Lindgren <tony@atomide.com>
> >
> > Thanks for testing!
> >
> > Btw, out of curiosity, how many different boards did you test this on?
> > IIRC you had an issue only in one board, right? Not to say I didn't
> > break anything else, I'm just trying to see how much confidence we
> > have on this series so far. I'm hoping the rest of the folks I listed
> > in the email will get around to testing this series.
>
> Sorry if I was not clear earlier. The issue affects several generations
> of TI 32-bit SoCs at least, not just one board.

But this series fixes the issues for all of them or are you still
seeing some broken boot with this series?

-Saravana

>
> > > I have some concerns though on how do we get a working -rc1 with the
> > > earlier series applied? See the comments in the last patch of this
> > > series.
> >
> > I tried to reply, but not sure if it helps. We'll continue the discussion there.
>
> Ack.
>
> Tony
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
Saravana Kannan Aug. 15, 2022, 6:23 p.m. UTC | #10
On Mon, Aug 15, 2022 at 4:01 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 22-08-09 23:00:29, Saravana Kannan wrote:
> > This patch series improves fw_devlink in the following ways:
> >
> > 1. It no longer cares about a fwnode having a "compatible" property. It
> >    figures this our more dynamically. The only expectation is that
> >    fwnode that are converted to devices actually get probed by a driver
> >    for the dependencies to be enforced correctly.
> >
> > 2. Finer grained dependency tracking. fw_devlink will now create device
> >    links from the consumer to the actual resource's device (if it has one,
> >    Eg: gpio_device) instead of the parent supplier device. This improves
> >    things like async suspend/resume ordering, potentially remove the need
> >    for frameworks to create device links, more parallelized async probing,
> >    and better sync_state() tracking.
> >
> > 3. Handle hardware/software quirks where a child firmware node gets
> >    populated as a device before its parent firmware node AND actually
> >    supplies a non-optional resource to the parent firmware node's
> >    device.
> >
> > 4. Way more robust at cycle handling (see patch for the insane cases).
> >
> > 5. Stops depending on OF_POPULATED to figure out some corner cases.
> >
> > 6. Simplifies the work that needs to be done by the firmware specific
> >    code.
> >
> > This took way too long to get done due to typo bugs I had in my rewrite or
> > corner cases I had to find and handle. But it's fairly well tested at this
> > point and I expect this to work properly.
> >
> > Abel & Doug,
> >
> > This should fix your cyclic dependency issues with your display. Can you
> > give it a shot please?
>
> Tested the specific case we discussed about here:
> https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/raw
>
> Thanks for fixing this.
>
> Tested-by: Abel Vesa <abel.vesa@linaro.org>

Thanks!

-Saravana

>
> >
> > Alexander,
> >
> > This should fix your issue where the power domain device not having a
> > compatible property. Can you give it a shot please?
> >
> > Tony,
> >
> > This should handle the odd case of the child being the supplier of the
> > parent. Can you please give this a shot? I want to make sure the cycle
> > detection code handles this properly and treats it like it's NOT a cycle.
> >
> > Geert,
> >
> > Can you test the renesas stuff I changed please? They should continue
> > working like before. Any other sanity test on other hardware would be
> > great too.
> >
> > Sudeep,
> >
> > I don't think there are any unfixed issues you had reported in my other
> > patches that this series might fix, but it'll be nice if you could give
> > this a sanity test.
> >
> > Guenter,
> >
> > I don't think this will fix the issue you reported in the amba patch, but
> > it's worth a shot because it improves a bunch of corner case handling. So
> > it might be better at handling whatever corner cases you might have in the
> > qemu platforms.
> >
> > Thanks,
> > Saravana
> >
> > Cc: Abel Vesa <abel.vesa@linaro.org>
> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: John Stultz <jstultz@google.com>
> > Cc: Doug Anderson <dianders@chromium.org>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> >
> > Saravana Kannan (9):
> >   driver core: fw_devlink: Don't purge child fwnode's consumer links
> >   driver core: fw_devlink: Improve check for fwnode with no
> >     device/driver
> >   soc: renesas: Move away from using OF_POPULATED for fw_devlink
> >   gpiolib: Clear the gpio_device's fwnode initialized flag before adding
> >   driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
> >   driver core: fw_devlink: Allow marking a fwnode link as being part of
> >     a cycle
> >   driver core: fw_devlink: Consolidate device link flag computation
> >   driver core: fw_devlink: Make cycle detection more robust
> >   of: property: Simplify of_link_to_phandle()
> >
> >  drivers/base/core.c             | 437 +++++++++++++++++++++-----------
> >  drivers/gpio/gpiolib.c          |   6 +
> >  drivers/of/property.c           |  84 +-----
> >  drivers/soc/renesas/rcar-sysc.c |   2 +-
> >  include/linux/device.h          |   1 +
> >  include/linux/fwnode.h          |  12 +-
> >  6 files changed, 323 insertions(+), 219 deletions(-)
> >
> > --
> > 2.37.1.559.g78731f0fdb-goog
> >
Saravana Kannan Aug. 15, 2022, 7:17 p.m. UTC | #11
On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hello Saravana,
>
> Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > Alexander,
> >
> > This should fix your issue where the power domain device not having a
> > compatible property. Can you give it a shot please?
>
> thanks for the update. Unfortunately this does not work:
>
> > [    0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@0
> > [    0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed to
> find PM domain: -2
> > [    0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@2
> > [    0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@3
> > [    0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@4
> > [    0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@7
> > [    0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@8
> > [    0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@9
> > [    0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@a
> > [    0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach()
> failed to find PM domain: -2
> > [    0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed to
> attach power domain "bus"
> > [    0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed to
> find PM domain: -2
> > [    1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> > [    1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > [    1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > [    1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with
> 0-0008
> > [    1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with
> 0-0008
>
> The required power-supply for the power domains is still not yet available.
> Does this series require some other patches as well?

Ah sorry, yeah, this needs additional patches. The one I gave in the
other thread when I debugged this and I also noticed another issue.
Here's the combined diff of what's needed. Can you add this on top of
the series and test it?

diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
index b9c22f764b4d..8a0e82067924 100644
--- a/drivers/irqchip/irq-imx-gpcv2.c
+++ b/drivers/irqchip/irq-imx-gpcv2.c
@@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
device_node *node,
         * later the GPC power domain driver will not be skipped.
         */
        of_node_clear_flag(node, OF_POPULATED);
+       fwnode_dev_initialized(domain->fwnode, false);
        return 0;
 }

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 6383a4edc360..181fbfe5bd4d 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)

                pd_pdev->dev.parent = dev;
                pd_pdev->dev.of_node = np;
+               pd_pdev->dev.fwnode = of_fwnode_handle(np);

                ret = platform_device_add(pd_pdev);
                if (ret) {

With this patch, I'd really expect the power domain dependency to be
handled correctly.

> Whats worse, starting with commit 9/9 [of: property: Simplify
> of_link_to_phandle()], other drivers fail to probe waiting for pinctrl to be
> available.

Heh, Patch 9/9 and all its other dependencies in this series was to
fix your use case. Ironic that it's causing you more issues.

> > $ cat /sys/kernel/debug/devices_deferred
> > gpio-leds       platform: wait for supplier gpioledgrp
> > extcon-usbotg0  platform: wait for supplier usb0congrp
> > gpio-keys       platform: wait for supplier gpiobuttongrp
> > regulator-otg-vbus      platform: wait for supplier reggotgvbusgrp
> > regulator-vdd-arm       platform: wait for supplier dvfsgrp
>
> Apparently for some reason they are not probed again, once the pinctrl driver
> probed.

I'm hoping that this is just some issue due to the missing patch
above, but doesn't sound like it if you say that the pinctrl ended up
probing eventually.

So when device_links_driver_bound() calls
__fw_devlink_pickup_dangling_consumers(), it should have picked up the
consumers of node like gpiobuttongrp and moved it to the pinctrl
device. And right after that we call __fw_devlink_link_to_consumers()
that would have created the device links. And then right after that,
we go through all the consumers and add them to the deferred probe
list. After that deferred probe should have run... either because it's
enabled at late_initcall() or because a new device probed
successfully.

Can you check which one of my expectations isn't true in your case?

Thanks,
Saravana
Saravana Kannan Aug. 15, 2022, 8:56 p.m. UTC | #12
On Mon, Aug 15, 2022 at 12:17 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
> >
> > Hello Saravana,
> >
> > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > Alexander,
> > >
> > > This should fix your issue where the power domain device not having a
> > > compatible property. Can you give it a shot please?
> >
> > thanks for the update. Unfortunately this does not work:
> >
> > > [    0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@0
> > > [    0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed to
> > find PM domain: -2
> > > [    0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@2
> > > [    0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@3
> > > [    0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@4
> > > [    0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@7
> > > [    0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@8
> > > [    0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@9
> > > [    0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@a
> > > [    0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach()
> > failed to find PM domain: -2
> > > [    0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed to
> > attach power domain "bus"
> > > [    0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed to
> > find PM domain: -2
> > > [    1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> > > [    1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > [    1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > [    1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with
> > 0-0008
> > > [    1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with
> > 0-0008
> >
> > The required power-supply for the power domains is still not yet available.
> > Does this series require some other patches as well?
>
> Ah sorry, yeah, this needs additional patches. The one I gave in the
> other thread when I debugged this and I also noticed another issue.
> Here's the combined diff of what's needed. Can you add this on top of
> the series and test it?
>
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index b9c22f764b4d..8a0e82067924 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> device_node *node,
>          * later the GPC power domain driver will not be skipped.
>          */
>         of_node_clear_flag(node, OF_POPULATED);
> +       fwnode_dev_initialized(domain->fwnode, false);
>         return 0;
>  }
>
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 6383a4edc360..181fbfe5bd4d 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
>
>                 pd_pdev->dev.parent = dev;
>                 pd_pdev->dev.of_node = np;
> +               pd_pdev->dev.fwnode = of_fwnode_handle(np);
>
>                 ret = platform_device_add(pd_pdev);
>                 if (ret) {
>
> With this patch, I'd really expect the power domain dependency to be
> handled correctly.
>
> > Whats worse, starting with commit 9/9 [of: property: Simplify
> > of_link_to_phandle()], other drivers fail to probe waiting for pinctrl to be
> > available.
>
> Heh, Patch 9/9 and all its other dependencies in this series was to
> fix your use case. Ironic that it's causing you more issues.
>
> > > $ cat /sys/kernel/debug/devices_deferred
> > > gpio-leds       platform: wait for supplier gpioledgrp
> > > extcon-usbotg0  platform: wait for supplier usb0congrp
> > > gpio-keys       platform: wait for supplier gpiobuttongrp
> > > regulator-otg-vbus      platform: wait for supplier reggotgvbusgrp
> > > regulator-vdd-arm       platform: wait for supplier dvfsgrp
> >
> > Apparently for some reason they are not probed again, once the pinctrl driver
> > probed.
>
> I'm hoping that this is just some issue due to the missing patch
> above, but doesn't sound like it if you say that the pinctrl ended up
> probing eventually.
>
> So when device_links_driver_bound() calls
> __fw_devlink_pickup_dangling_consumers(), it should have picked up the
> consumers of node like gpiobuttongrp and moved it to the pinctrl
> device. And right after that we call __fw_devlink_link_to_consumers()
> that would have created the device links. And then right after that,
> we go through all the consumers and add them to the deferred probe
> list. After that deferred probe should have run... either because it's
> enabled at late_initcall() or because a new device probed
> successfully.
>
> Can you check which one of my expectations isn't true in your case?

Actually I have a hypothesis on what might be happening. It could be a
case of the consumer device getting added after the supplier has been
initialized.

If the patch above doesn't fix everything, can you add this diff on
top of the patch above and see if that fixes everything? If it fixes
the pinctrl issue, can you check my hypothesis be checking in what
order the devices get added and get probed?

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2f012e826986..866755d8ad95 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2068,7 +2068,11 @@ static int fw_devlink_create_devlink(struct device *con,
                device_links_write_unlock();
        }

-       sup_dev = get_dev_from_fwnode(sup_handle);
+       if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
+               sup_dev = fwnode_get_next_parent_dev(sup_handle);
+       else
+               sup_dev = get_dev_from_fwnode(sup_handle);
+
        if (sup_dev) {
                /*
                 * If it's one of those drivers that don't actually bind to

Thanks,
Saravana
Alexander Stein Aug. 16, 2022, 7:17 a.m. UTC | #13
Hello Saravana,

Am Montag, 15. August 2022, 21:17:36 CEST schrieb Saravana Kannan:
> On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hello Saravana,
> > 
> > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > Alexander,
> > > 
> > > This should fix your issue where the power domain device not having a
> > > compatible property. Can you give it a shot please?
> > 
> > thanks for the update. Unfortunately this does not work:
> > > [    0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@0
> > 
> > > [    0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed
> > > to
> > 
> > find PM domain: -2
> > 
> > > [    0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@2
> > 
> > > [    0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@3
> > 
> > > [    0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@4
> > 
> > > [    0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@7
> > 
> > > [    0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@8
> > 
> > > [    0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@9
> > 
> > > [    0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> > 
> > gpc@303a0000/pgc/power-domain@a
> > 
> > > [    0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach()
> > 
> > failed to find PM domain: -2
> > 
> > > [    0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed
> > > to
> > 
> > attach power domain "bus"
> > 
> > > [    0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed
> > > to
> > 
> > find PM domain: -2
> > 
> > > [    1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> > > [    1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > [    1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > [    1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with
> > 
> > 0-0008
> > 
> > > [    1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with
> > 
> > 0-0008
> > 
> > The required power-supply for the power domains is still not yet
> > available.
> > Does this series require some other patches as well?
> 
> Ah sorry, yeah, this needs additional patches. The one I gave in the
> other thread when I debugged this and I also noticed another issue.
> Here's the combined diff of what's needed. Can you add this on top of
> the series and test it?
> 
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c
> b/drivers/irqchip/irq-imx-gpcv2.c index b9c22f764b4d..8a0e82067924 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> device_node *node,
>          * later the GPC power domain driver will not be skipped.
>          */
>         of_node_clear_flag(node, OF_POPULATED);
> +       fwnode_dev_initialized(domain->fwnode, false);
>         return 0;
>  }
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 6383a4edc360..181fbfe5bd4d 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device
> *pdev)
> 
>                 pd_pdev->dev.parent = dev;
>                 pd_pdev->dev.of_node = np;
> +               pd_pdev->dev.fwnode = of_fwnode_handle(np);
> 
>                 ret = platform_device_add(pd_pdev);
>                 if (ret) {
> 
> With this patch, I'd really expect the power domain dependency to be
> handled correctly.

I was out of office so I didn't keep track of any dependencies, sorry.
With these 2 changes above my power domain problem is fixed!

Thanks
Alexander
Alexander Stein Aug. 16, 2022, 7:17 a.m. UTC | #14
Hello Saravana,

Am Montag, 15. August 2022, 22:56:07 CEST schrieb Saravana Kannan:
> On Mon, Aug 15, 2022 at 12:17 PM Saravana Kannan <saravanak@google.com> 
wrote:
> > On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> > 
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hello Saravana,
> > > 
> > > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > > Alexander,
> > > > 
> > > > This should fix your issue where the power domain device not having a
> > > > compatible property. Can you give it a shot please?
> > > 
> > > thanks for the update. Unfortunately this does not work:
> > > > [    0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@0
> > > 
> > > > [    0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach()
> > > > failed to
> > > 
> > > find PM domain: -2
> > > 
> > > > [    0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@2
> > > 
> > > > [    0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@3
> > > 
> > > > [    0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@4
> > > 
> > > > [    0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@7
> > > 
> > > > [    0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@8
> > > 
> > > > [    0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@9
> > > 
> > > > [    0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> > > 
> > > gpc@303a0000/pgc/power-domain@a
> > > 
> > > > [    0.779342] genpd genpd:0:38320000.blk-ctrl:
> > > > __genpd_dev_pm_attach()
> > > 
> > > failed to find PM domain: -2
> > > 
> > > > [    0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed
> > > > to
> > > 
> > > attach power domain "bus"
> > > 
> > > > [    0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach()
> > > > failed to
> > > 
> > > find PM domain: -2
> > > 
> > > > [    1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer:
> > > > 1
> > > > [    1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > > [    1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > > [    1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > with
> > > 
> > > 0-0008
> > > 
> > > > [    1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > with
> > > 
> > > 0-0008
> > > 
> > > The required power-supply for the power domains is still not yet
> > > available.
> > > Does this series require some other patches as well?
> > 
> > Ah sorry, yeah, this needs additional patches. The one I gave in the
> > other thread when I debugged this and I also noticed another issue.
> > Here's the combined diff of what's needed. Can you add this on top of
> > the series and test it?
> > 
> > diff --git a/drivers/irqchip/irq-imx-gpcv2.c
> > b/drivers/irqchip/irq-imx-gpcv2.c index b9c22f764b4d..8a0e82067924 100644
> > --- a/drivers/irqchip/irq-imx-gpcv2.c
> > +++ b/drivers/irqchip/irq-imx-gpcv2.c
> > @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> > device_node *node,
> > 
> >          * later the GPC power domain driver will not be skipped.
> >          */
> >         
> >         of_node_clear_flag(node, OF_POPULATED);
> > 
> > +       fwnode_dev_initialized(domain->fwnode, false);
> > 
> >         return 0;
> >  
> >  }
> > 
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index 6383a4edc360..181fbfe5bd4d 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device
> > *pdev)> 
> >                 pd_pdev->dev.parent = dev;
> >                 pd_pdev->dev.of_node = np;
> > 
> > +               pd_pdev->dev.fwnode = of_fwnode_handle(np);
> > 
> >                 ret = platform_device_add(pd_pdev);
> >                 if (ret) {
> > 
> > With this patch, I'd really expect the power domain dependency to be
> > handled correctly.
> > 
> > > Whats worse, starting with commit 9/9 [of: property: Simplify
> > > of_link_to_phandle()], other drivers fail to probe waiting for pinctrl
> > > to be available.
> > 
> > Heh, Patch 9/9 and all its other dependencies in this series was to
> > fix your use case. Ironic that it's causing you more issues.
> > 
> > > > $ cat /sys/kernel/debug/devices_deferred
> > > > gpio-leds       platform: wait for supplier gpioledgrp
> > > > extcon-usbotg0  platform: wait for supplier usb0congrp
> > > > gpio-keys       platform: wait for supplier gpiobuttongrp
> > > > regulator-otg-vbus      platform: wait for supplier reggotgvbusgrp
> > > > regulator-vdd-arm       platform: wait for supplier dvfsgrp
> > > 
> > > Apparently for some reason they are not probed again, once the pinctrl
> > > driver probed.
> > 
> > I'm hoping that this is just some issue due to the missing patch
> > above, but doesn't sound like it if you say that the pinctrl ended up
> > probing eventually.
> > 
> > So when device_links_driver_bound() calls
> > __fw_devlink_pickup_dangling_consumers(), it should have picked up the
> > consumers of node like gpiobuttongrp and moved it to the pinctrl
> > device. And right after that we call __fw_devlink_link_to_consumers()
> > that would have created the device links. And then right after that,
> > we go through all the consumers and add them to the deferred probe
> > list. After that deferred probe should have run... either because it's
> > enabled at late_initcall() or because a new device probed
> > successfully.
> > 
> > Can you check which one of my expectations isn't true in your case?
> 
> Actually I have a hypothesis on what might be happening. It could be a
> case of the consumer device getting added after the supplier has been
> initialized.
> 
> If the patch above doesn't fix everything, can you add this diff on
> top of the patch above and see if that fixes everything? If it fixes
> the pinctrl issue, can you check my hypothesis be checking in what
> order the devices get added and get probed?
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2f012e826986..866755d8ad95 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2068,7 +2068,11 @@ static int fw_devlink_create_devlink(struct device
> *con, device_links_write_unlock();
>         }
> 
> -       sup_dev = get_dev_from_fwnode(sup_handle);
> +       if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
> +               sup_dev = fwnode_get_next_parent_dev(sup_handle);
> +       else
> +               sup_dev = get_dev_from_fwnode(sup_handle);
> +
>         if (sup_dev) {
>                 /*
>                  * If it's one of those drivers that don't actually bind to
> 

And with this change my pinctrl probing is fixed as well!

Thanks
Alexander
Saravana Kannan Aug. 16, 2022, 6:51 p.m. UTC | #15
On Tue, Aug 16, 2022 at 12:17 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hello Saravana,
>
> Am Montag, 15. August 2022, 22:56:07 CEST schrieb Saravana Kannan:
> > On Mon, Aug 15, 2022 at 12:17 PM Saravana Kannan <saravanak@google.com>
> wrote:
> > > On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> > >
> > > <alexander.stein@ew.tq-group.com> wrote:
> > > > Hello Saravana,
> > > >
> > > > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > > > Alexander,
> > > > >
> > > > > This should fix your issue where the power domain device not having a
> > > > > compatible property. Can you give it a shot please?
> > > >
> > > > thanks for the update. Unfortunately this does not work:
> > > > > [    0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@0
> > > >
> > > > > [    0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach()
> > > > > failed to
> > > >
> > > > find PM domain: -2
> > > >
> > > > > [    0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@2
> > > >
> > > > > [    0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@3
> > > >
> > > > > [    0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@4
> > > >
> > > > > [    0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@7
> > > >
> > > > > [    0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@8
> > > >
> > > > > [    0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@9
> > > >
> > > > > [    0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@a
> > > >
> > > > > [    0.779342] genpd genpd:0:38320000.blk-ctrl:
> > > > > __genpd_dev_pm_attach()
> > > >
> > > > failed to find PM domain: -2
> > > >
> > > > > [    0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed
> > > > > to
> > > >
> > > > attach power domain "bus"
> > > >
> > > > > [    0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach()
> > > > > failed to
> > > >
> > > > find PM domain: -2
> > > >
> > > > > [    1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer:
> > > > > 1
> > > > > [    1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > > > [    1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > > > [    1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > > with
> > > >
> > > > 0-0008
> > > >
> > > > > [    1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > > with
> > > >
> > > > 0-0008
> > > >
> > > > The required power-supply for the power domains is still not yet
> > > > available.
> > > > Does this series require some other patches as well?
> > >
> > > Ah sorry, yeah, this needs additional patches. The one I gave in the
> > > other thread when I debugged this and I also noticed another issue.
> > > Here's the combined diff of what's needed. Can you add this on top of
> > > the series and test it?
> > >
> > > diff --git a/drivers/irqchip/irq-imx-gpcv2.c
> > > b/drivers/irqchip/irq-imx-gpcv2.c index b9c22f764b4d..8a0e82067924 100644
> > > --- a/drivers/irqchip/irq-imx-gpcv2.c
> > > +++ b/drivers/irqchip/irq-imx-gpcv2.c
> > > @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> > > device_node *node,
> > >
> > >          * later the GPC power domain driver will not be skipped.
> > >          */
> > >
> > >         of_node_clear_flag(node, OF_POPULATED);
> > >
> > > +       fwnode_dev_initialized(domain->fwnode, false);
> > >
> > >         return 0;
> > >
> > >  }
> > >
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > index 6383a4edc360..181fbfe5bd4d 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device
> > > *pdev)>
> > >                 pd_pdev->dev.parent = dev;
> > >                 pd_pdev->dev.of_node = np;
> > >
> > > +               pd_pdev->dev.fwnode = of_fwnode_handle(np);
> > >
> > >                 ret = platform_device_add(pd_pdev);
> > >                 if (ret) {
> > >
> > > With this patch, I'd really expect the power domain dependency to be
> > > handled correctly.
> > >
> > > > Whats worse, starting with commit 9/9 [of: property: Simplify
> > > > of_link_to_phandle()], other drivers fail to probe waiting for pinctrl
> > > > to be available.
> > >
> > > Heh, Patch 9/9 and all its other dependencies in this series was to
> > > fix your use case. Ironic that it's causing you more issues.
> > >
> > > > > $ cat /sys/kernel/debug/devices_deferred
> > > > > gpio-leds       platform: wait for supplier gpioledgrp
> > > > > extcon-usbotg0  platform: wait for supplier usb0congrp
> > > > > gpio-keys       platform: wait for supplier gpiobuttongrp
> > > > > regulator-otg-vbus      platform: wait for supplier reggotgvbusgrp
> > > > > regulator-vdd-arm       platform: wait for supplier dvfsgrp
> > > >
> > > > Apparently for some reason they are not probed again, once the pinctrl
> > > > driver probed.
> > >
> > > I'm hoping that this is just some issue due to the missing patch
> > > above, but doesn't sound like it if you say that the pinctrl ended up
> > > probing eventually.
> > >
> > > So when device_links_driver_bound() calls
> > > __fw_devlink_pickup_dangling_consumers(), it should have picked up the
> > > consumers of node like gpiobuttongrp and moved it to the pinctrl
> > > device. And right after that we call __fw_devlink_link_to_consumers()
> > > that would have created the device links. And then right after that,
> > > we go through all the consumers and add them to the deferred probe
> > > list. After that deferred probe should have run... either because it's
> > > enabled at late_initcall() or because a new device probed
> > > successfully.
> > >
> > > Can you check which one of my expectations isn't true in your case?
> >
> > Actually I have a hypothesis on what might be happening. It could be a
> > case of the consumer device getting added after the supplier has been
> > initialized.
> >
> > If the patch above doesn't fix everything, can you add this diff on
> > top of the patch above and see if that fixes everything? If it fixes
> > the pinctrl issue, can you check my hypothesis be checking in what
> > order the devices get added and get probed?
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 2f012e826986..866755d8ad95 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2068,7 +2068,11 @@ static int fw_devlink_create_devlink(struct device
> > *con, device_links_write_unlock();
> >         }
> >
> > -       sup_dev = get_dev_from_fwnode(sup_handle);
> > +       if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
> > +               sup_dev = fwnode_get_next_parent_dev(sup_handle);
> > +       else
> > +               sup_dev = get_dev_from_fwnode(sup_handle);
> > +
> >         if (sup_dev) {
> >                 /*
> >                  * If it's one of those drivers that don't actually bind to
> >
>
> And with this change my pinctrl probing is fixed as well!

Thanks for testing these! I'll roll these into v2 of the series.

Glad to see I've fixed all the issues I set out to fix. Now to figure
out what other corner cases I've missed.

-Saravana
Tony Lindgren Aug. 18, 2022, 7:05 a.m. UTC | #16
Hi,

* Saravana Kannan <saravanak@google.com> [220815 18:16]:
> On Mon, Aug 15, 2022 at 3:33 AM Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Saravana Kannan <saravanak@google.com> [220813 00:45]:
> > > On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <tony@atomide.com> wrote:
> > > >
> > > > * Saravana Kannan <saravanak@google.com> [220810 05:54]:
> > > > > Tony,
> > > > >
> > > > > This should handle the odd case of the child being the supplier of the
> > > > > parent. Can you please give this a shot? I want to make sure the cycle
> > > > > detection code handles this properly and treats it like it's NOT a cycle.
> > > >
> > > > Yup, this series works for me, so feel free to add:
> > > >
> > > > Tested-by: Tony Lindgren <tony@atomide.com>
> > >
> > > Thanks for testing!
> > >
> > > Btw, out of curiosity, how many different boards did you test this on?
> > > IIRC you had an issue only in one board, right? Not to say I didn't
> > > break anything else, I'm just trying to see how much confidence we
> > > have on this series so far. I'm hoping the rest of the folks I listed
> > > in the email will get around to testing this series.
> >
> > Sorry if I was not clear earlier. The issue affects several generations
> > of TI 32-bit SoCs at least, not just one board.
> 
> But this series fixes the issues for all of them or are you still
> seeing some broken boot with this series?

Yes. However, I'm now getting confused what exactly you're proposing to fix
the regressions for v6.0-rc series.

I'd like to see just the fixes series for v6.0-rc series. With proper fixes
tags, and possibly reverts.

Then discussing patches for Linux next can be done based on the fixes :)

Regards,

Tony
Greg KH Aug. 18, 2022, 3 p.m. UTC | #17
On Thu, Aug 18, 2022 at 10:05:14AM +0300, Tony Lindgren wrote:
> Hi,
> 
> * Saravana Kannan <saravanak@google.com> [220815 18:16]:
> > On Mon, Aug 15, 2022 at 3:33 AM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > * Saravana Kannan <saravanak@google.com> [220813 00:45]:
> > > > On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <tony@atomide.com> wrote:
> > > > >
> > > > > * Saravana Kannan <saravanak@google.com> [220810 05:54]:
> > > > > > Tony,
> > > > > >
> > > > > > This should handle the odd case of the child being the supplier of the
> > > > > > parent. Can you please give this a shot? I want to make sure the cycle
> > > > > > detection code handles this properly and treats it like it's NOT a cycle.
> > > > >
> > > > > Yup, this series works for me, so feel free to add:
> > > > >
> > > > > Tested-by: Tony Lindgren <tony@atomide.com>
> > > >
> > > > Thanks for testing!
> > > >
> > > > Btw, out of curiosity, how many different boards did you test this on?
> > > > IIRC you had an issue only in one board, right? Not to say I didn't
> > > > break anything else, I'm just trying to see how much confidence we
> > > > have on this series so far. I'm hoping the rest of the folks I listed
> > > > in the email will get around to testing this series.
> > >
> > > Sorry if I was not clear earlier. The issue affects several generations
> > > of TI 32-bit SoCs at least, not just one board.
> > 
> > But this series fixes the issues for all of them or are you still
> > seeing some broken boot with this series?
> 
> Yes. However, I'm now getting confused what exactly you're proposing to fix
> the regressions for v6.0-rc series.

So am I :(

> I'd like to see just the fixes series for v6.0-rc series. With proper fixes
> tags, and possibly reverts.

Agreed, that would help out a lot here.

> Then discussing patches for Linux next can be done based on the fixes :)

Agreed.

I'll drop this whole series from my queue now and wait for a new one.

thanks,

greg k-h