Re: [PATCH v1 0/9] fw_devlink improvements

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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