| Message ID | 20220810060040.321697-1-saravanak@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fw_devlink improvements | expand |
* 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
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
+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 >
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
* 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
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 >
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
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
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. >
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 > >
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
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
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
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
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
Hi Saravana, On Wed, Aug 10, 2022 at 8:00 AM 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? Thanks for to your series! > 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. At first, this series looked like a total disaster, introducing several regressions[1]. However, after applying the additional fix from https://lore.kernel.org/lkml/CAGETcx-JUV1nj8wBJrTPfyvM7=Mre5j_vkVmZojeiumUGG6QZQ@mail.gmail.com all regressions[1] went away, and /sys/kernel/debug/devices_deferred is empty again. Note that the Ethernet PHY interrupt on Koelsch is not fixed, so the issue from https://lore.kernel.org/all/CAMuHMdWo_wRwV-i_iyTxVnEsf3Th9GBAG+wxUQMQGnw1t2ijTg@mail.gmail.com/ is still present. Summary: while this series (+ the additional fix) doesn't seem to introduce any regressions on Renesas ARM platforms, it doesn't seem to fix anything for me neither. Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> [1] R-Mobile APE6 (ape6evm): No "deferred probe pending" messages? # cat /sys/kernel/debug/devices_deferred ee120000.mmc platform: wait for supplier sd1 ee100000.mmc platform: wait for supplier sd0 ee200000.mmc platform: wait for supplier mmc0 keyboard platform: wait for supplier keyboard R-Mobile A1 (armadillo-800-eva): No soundcards found. platform backlight: deferred probe pending i2c 0-0030: deferred probe pending (RTC) platform sound: deferred probe pending platform fe1f0000.sound: deferred probe pending platform keyboard: deferred probe pending (gpio-keys) platform e6850000.mmc: deferred probe pending (SDHI) platform e6bd0000.mmc: deferred probe pending (SDHI) # cat /sys/kernel/debug/devices_deferred backlight platform: wait for supplier backlight 0-0030 i2c: wait for supplier rtc sound asoc-simple-card: parse error fe1f0000.sound platform: wait for supplier sounda keyboard platform: wait for supplier keyboard e6850000.mmc platform: wait for supplier sd0 e6bd0000.mmc platform: wait for supplier mmc0 SH-Mobile AG5 (kzm9g): No soundcards found. platform sound: deferred probe pending platform ec230000.sound: deferred probe pending # cat /sys/kernel/debug/devices_deferred sound asoc-simple-card: parse error ec230000.sound platform: wait for supplier sounda Note: This is the only board where gpio-keys still works! R-Car M2-W (koelsch): i2c-demux-pinctrl i2c-12: failed to setup demux-adapter 0 (-19) i2c-demux-pinctrl i2c-13: failed to setup demux-adapter 0 (-19) i2c-demux-pinctrl i2c-14: failed to setup demux-adapter 0 (-19) i2c-demux-pinctrl i2c-13: Failed to create device link with hdmi-in i2c-demux-pinctrl i2c-13: Failed to create device link with hdmi-out No soundcards found. Some I2C-busses are missing, but not listed in /sys/kernel/debug/devices_deferred? /devices/platform/soc/e6518000.i2c /devices/platform/soc/e6530000.i2c /devices/platform/soc/e6520000.i2c platform keyboard: deferred probe pending platform sound: deferred probe pending platform feb00000.display: deferred probe pending # cat /sys/kernel/debug/devices_deferred keyboard platform: wait for supplier keyboard sound asoc-simple-card: parse error feb00000.display rcar-du: failed to initialize DRM/KMS R-Car Gen3 (Salvator-X(S), Ebisu) platform keys: deferred probe pending (gpio-keys) # cat /sys/kernel/debug/devices_deferred keys platform: wait for supplier keys RZ/A (rskrza1, rza2mevb) No "deferred probe pending" messages? # cat /sys/kernel/debug/devices_deferred keyboard platform: wait for supplier keyboard Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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
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
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(-)