On Tue, 31 Aug 2021 at 19:51, Saravana Kannan [off-list ref] wrote:

On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

[1] Documentation/devicetree/bindings/power/power-domain.yaml
[2] Documentation/devicetree/bindings/arm/psci.yaml

Signed-off-by: Ulf Hansson <redacted>
---

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.

quoted

...

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.

quoted

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

quoted

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

Kind regards
Uffe