Re: [PATCH 2/2] of: property: fw_devlink: Set 'optional_con_dev' for parse_power_domains

[Ulf Hansson <ulf.hansson@linaro.org>]

On Tue, 31 Aug 2021 at 19:51, Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > The power-domain DT bindings [1] doesn't enforce a compatible string for a
> > provider node, even if this is common to use. In particular, when
> > describing a hierarchy with parent/child power-domains, as the psci DT
> > bindings [2] for example, it's sometimes not applicable to use a compatible
> > string.
>
> Ok, and fw_devlink handles that -- provider not having a compatible
> string is pretty common. In these cases, the parent node is the actual
> device that gets probed and registers the provider. So fw_devlink will
> create a link from the consumer to the parent device node.

Yes, correct. That is working fine and isn't a problem.

The first problem (I think) is that fw_devlink creates a fw_devlink
from a child provider node (consumer without compatible string) to a
parent node (supplier with a compatible string). I don't understand
the reason why this is needed, perhaps you can elaborate on why?

I come to the second and follow up problem from this behaviour, see below.

>
> > Therefore, let's set the 'optional_con_dev' to true to avoid creating
> > incorrect fw_devlinks for power-domains.
>
> This part doesn't make sense or is incomplete. What is being done incorrectly?

See below.

>
> >
> > [1] Documentation/devicetree/bindings/power/power-domain.yaml
> > [2] Documentation/devicetree/bindings/arm/psci.yaml
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Some more details of what goes on here. I have added a debug print in
> > of_link_to_phandle() to see the fw_devlinks that gets created.
> >
> > This is what happens on Dragonboard 410c when 'optional_con_dev' isn't set:
> > ...
> > [    0.041274] device: 'psci': device_add
> > [    0.041366] OF: Linking power-domain-cpu0 (consumer) to psci (supplier)
> > [    0.041395] OF: Linking power-domain-cpu1 (consumer) to psci (supplier)
> > [    0.041423] OF: Linking power-domain-cpu2 (consumer) to psci (supplier)
> > [    0.041451] OF: Linking power-domain-cpu3 (consumer) to psci (supplier)
> > [    0.041494] device: 'platform:psci--platform:psci': device_add
> > [    0.041556] platform psci: Linked as a sync state only consumer to psci

Because we created a fw_devlink for the child provider nodes
(consumer) that lacks compatible properties, we end up creating a sync
state only devlink. I don't think it serves a purpose, but I may be
wrong!?

Additionally, the actual devlink that is created, has the same
supplier and consumer device, which indicates that this isn't the
right thing to do.

> > ...
> >
> > This is what happens on Dragonboard 410c when 'optional_con_dev' is set:
> > ...
> > [    0.041179] device: 'psci': device_add
> > [    0.041265] OF: Not linking psci to psci - is descendant
> > [    0.041293] OF: Not linking psci to psci - is descendant
> > [    0.041319] OF: Not linking psci to psci - is descendant
> > [    0.041346] OF: Not linking psci to psci - is descendant
> > ...
>
> Can you please explain what exactly is going on that's wrong here? I
> notice that psci is not probed as a device at all. And when you aren't
> setting this flag the only difference I see is the creating of a sync
> state only link -- which shouldn't matter here because you don't even
> have a driver implemented.

See above.

>
> > The relevant dtsi file:
> > arch/arm64/boot/dts/qcom/msm8916.dtsi
> >
> > Kind regards
> > Ulf Hansson
> >
> > ---
> >  drivers/of/property.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 2babb1807228..4d607fdbea24 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1356,7 +1356,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_io_channels, },
> >         { .parse_prop = parse_interrupt_parent, },
> >         { .parse_prop = parse_dmas, .optional = true, },
> > -       { .parse_prop = parse_power_domains, },
> > +       { .parse_prop = parse_power_domains, .optional_con_dev = true, },
>
> This change is just shooting in dark/completely unrelated to the
> commit text. This is just saying the actual consumer is a level up
> from where the property is listed (eg: remote-endpoint). It just
> happens to fix your case for unrelated reasons.

Again, see above.

>
> Definite Nak as this *will* break other cases.

In what way will this break other cases?

Would you mind elaborating for my understanding and perhaps point me
to an example where it will break?

>
> -Saravana
>
>
> >         { .parse_prop = parse_hwlocks, },
> >         { .parse_prop = parse_extcon, },
> >         { .parse_prop = parse_nvmem_cells, },
> > --
> > 2.25.1
> >

Kind regards
Uffe