Re: [PATCH v1 0/9] fw_devlink improvements

From: Alexander Stein
Date: Tue Aug 16 2022 - 05:04:58 EST


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@xxxxxxxxxx>
wrote:
> > On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> >
> > <alexander.stein@xxxxxxxxxxxxxxx> 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