Re: [PATCH 2/2] of: property: fw_devlink: Set 'optional_con_dev' for parse_power_domains
From: Ulf Hansson <hidden>
Date: 2021-09-08 10:41:35
Also in:
linux-arm-kernel, linux-devicetree, lkml
[...]
quoted
quoted
Device-A { compatible="foo"; Device-B { compatible="flam"; power-domains = <&Device-C>; } } Device-C { compatible="bar"; Device-D { compatible="baz"; power-domains = <&Device-A>; } } Legend: I'll use X -> Y to indicate links where X is the consumer and Y is the supplier. I'll call out the link types as fwnode or device links. If I don't explicitly state otherwise, when I say device links, I mean stateful/managed device link that is NOT sync-state-only device links. I think your first question is asking about fwnode link. So I'll answer that. fwnode links are created from the actual nodes that list the dependencies. So in this case from device-B -> device-C and device-D -> device-A. It needs to be done this way for a couple of reasons. But to answer your question on "why do this when Device-B doesn't have a compatible string?": 1. Not all devices have compatible strings (in an ideal world this won't be the case). So Device-A would create a struct device for Device-B, set the of_node/fwnode to point to Device-B DT node. Then device-B gets probed, etc. In those cases, we want the device links to be created between device-B -> device-C and NOT from device-A -> device-C. Because if we did follow that logic, we'll get device-A -> device-C and device-C -> device-A. This obviously can't work because it's a cyclic dependency and fw_devlink will have to give up on these. 2. When device-C is added (assuming device-A is added already), we need to create a sync-state-only device link from device-A to device-C as a proxy for the future device-B -> device-C device link that'll come up. This is to make sure device-C's sync_state() doesn't fire too early. So the way fw_devlink can tell apart device-A's real dependency (none in this case) vs device-B's dependency it's proxying for is by the fact the fwnode link is from device-B DT -> device-C DT. Hope that makes sense.Yes, it does and I understand that it may become complicated in some cases. If you get the time to put together an LWN article about fw_devlinks, I would certainly read it. :-) However, at least for power-domains, the DT example you describe above is an invalid description of a HW. It doesn't make sense to try to support if for fw_devlink, at least in my opinion. Let me elaborate. So, I assume you have left out the #power-domain-cells property (for simplicity) for Device-A and Device-C, as those seem to be the power-domain providers.Yes, but also because I don't want you to take these dependencies too literally. I should have just used "depends-on =" as a standing in fake property to make my point. And what "depends-on" maps to in each DT node could be any one of the properties that point to a supplier. The TLDR for this entire email is: You can't transfer the dependency requirement of a child to its parent just because the child doesn't have a "compatible" property (that's exactly what your patch was doing). The incorrect creation of a cyclic dependency is one example of why it's wrong.quoted
*) If Device-B is a consumer of Device-C, it also means that Device-A must be assigned as the child-power-domain to Device-C's power-domain.This statement doesn't make any sense. If Device-B is the actual consumer of device-C, why the heck should Device-A be assigned as the child-power domain of device-C. Device-B should be assigned as the child-power domain of device-C. Device-A could be on a completely different power domain and not depend on Device-C for anything.quoted
**) If Device-D is the consumer of Device-A, it also means that Device-C must be assigned as the child-power-domain to Device-A's power-domain.Similar comment here about device-D being the child power domain to Device-A. Read further below about cycles.
Well, I assumed the usual way of how we treat child nodes for power-domains. In any case, the description is wrong from the HW point of view - power-domains can't be described like that.
quoted
This simply can't be right from the HW point of view - and we don't support this in the Linux kernel anyway.That's my point. By doing what you wanted to do, you are making Device-A dependent on Device-C and Device-C dependent on Device-A. Which makes no sense.
Exactly. If that configuration exists in DT, why should we bother to support it with fw_devlinks, it's broken anyway.
quoted
A power-domain can not be both parent and child to another power-domain. In other words, cyclic dependencies can't exist for power-domains, as it would be a wrong description of the HW.Real cyclic dependencies can't exist between any HW -- doesn't matter if it's a power domain or not. That'd just be wrong.quoted
I wonder if the similar reasoning is applicable for other types of resources, like clocks and regulators, for example.So the example I gave definitely happens between two PMIC in one of the MSM chips. Forgot which one. If you follow what you suggested, we'll end up with both the devices not probing because they are waiting on each other to probe. Also, to go back to my main point, don't focus too much on one framework/property. In my example above, Device-D could be dependent on Device-A for a clock and you'll hit the same problem.
Well, again, that would not be a correct description of the HW, but I get your point. [...]
quoted
quoted
quoted
Would you mind elaborating for my understanding and perhaps point me to an example where it will break?So if you did this, it'll break: (1) the probe of device-A/device-C due to cyclic dependencies. Really no, because fw_devlink will just stop enforcing ordering between device-A and device-C if it detects a cycle. But if there was a real dependency (can me multiple links deep) between device-A -> device-C, that would no longer get enforced.As I said above, cyclic dependencies don't exist for power-domains.As I said above, *real* cyclic dependencies don't exist for anything.quoted
quoted
(2) It'd break sync_state() correctness for device-B -> device-C dependency.I don't see that. Again, because power-domain providers can't be described in a cyclic way in DT.I think I answered this above. Change one of the "power-domains" property to clocks (or one of the many properties fw_devlink supports) and you'll have the same issue I described.quoted
quoted
Hope that helps.Perhaps, renaming the flag to "non-cyclic" would be an option? As it seems like that is what this boils done to, right?No property is truly wanting to create a cycle. So if you were to create such a flag, every property should set it. See my TLDR above.
Well, I assume there are some valid cases where cyclic dependencies are okay, like the "remote-endpoint" DT property, for example? No? My point is, we are assuming there may be cyclic dependencies for all the DT properties we parse for fw_devlink, while in fact those should exist only for a few cases, right? Doesn't the additional parsing and creation of links, to deal with cyclic dependencies come with an overhead? If so - an option could be to let it hurt only those properties that really need it. [...] Kind regards Uffe