power: extend prefix match to regulator-name property

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

Signed-off-by Felix Brack <[email protected]>

---

 drivers/power/pmic/pmic-uclass.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

drivers/power/pmic/pmic-uclass.c +13 -3

index 64964e4..5a034f0 100644
@@ -26,6 +26,7 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent,
        struct driver *drv;
        struct udevice *child;
        const char *node_name;
+       const char *reg_name;
        int bind_count = 0;
        ofnode node;
        int prefix_len;

@@ -44,8 +45,18 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent,
                        debug("  - compatible prefix: '%s'\n", info->prefix);
 
                        prefix_len = strlen(info->prefix);
-                       if (strncmp(info->prefix, node_name, prefix_len))
-                               continue;
+                       if (strncmp(info->prefix, node_name, prefix_len)) {
+                               reg_name = ofnode_read_string(node,
+                                                             "regulator-name");
+                               if (reg_name) {
+                                       if (strncmp(info->prefix, reg_name,
+                                                   prefix_len)) {
+                                               continue;
+                                       }
+                               } else {
+                                       continue;
+                               }
+                       }
 
                        drv = lists_driver_lookup_name(info->driver);
                        if (!drv) {
--

2.7.4

Hi Felix,

Quoted 8 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

Signed-off-by Felix Brack <[email protected]>

---

Quoted 2 lines

 drivers/power/pmic/pmic-uclass.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Can you please add a sandbox test and documentation for this?

See:

test/dm/pmic.c
doc/driver-model/pmic-framework.txt

Quoted 32 lines

drivers/power/pmic/pmic-uclass.c +13 -2

index 64964e4..5a034f0 100644
@@ -26,6 +26,7 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent,
        struct driver *drv;
        struct udevice *child;
        const char *node_name;
+       const char *reg_name;
        int bind_count = 0;
        ofnode node;
        int prefix_len;

@@ -44,8 +45,18 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent,
                        debug("  - compatible prefix: '%s'\n", info->prefix);
 
                        prefix_len = strlen(info->prefix);
-                       if (strncmp(info->prefix, node_name, prefix_len))
-                               continue;
+                       if (strncmp(info->prefix, node_name, prefix_len)) {
+                               reg_name = ofnode_read_string(node,
+                                                             "regulator-name");
+                               if (reg_name) {
+                                       if (strncmp(info->prefix, reg_name,
+                                                   prefix_len)) {
+                                               continue;
+                                       }
+                               } else {
+                                       continue;
+                               }
+                       }
 

Quoted 5 lines

drivers/power/pmic/pmic-uclass.c +0 -1

                        drv = lists_driver_lookup_name(info->driver);
                        if (!drv) {
--

2.7.4

Regards,
Simon

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

If you have multiple regulator nodes which need to be differentiated, you need to use the deprecated "regulator-compatible" property, or just use the standard compatible property.

Quoted 11 lines · Chen-Yu Tsai

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

The actual regulator.txt states:

Optional properties:
- regulator-name: a string, required by the regulator uclass

This was the reason for choosing the regulator-name property.

Quoted 4 lines

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

Exactly. I totally agree but as stated in an earlier post: I did not define the names for the regulators and modifying them is almost certainly not the right way to go. Let me explain this briefly. The regulator names I'm trying to match are those from tps65910.dtsi, an include file. The exact same file is part of the LINUX kernel. Therefore I resigned suggesting the modification of the node names.

Quoted 5 lines

If you have multiple regulator nodes which need to be differentiated, you need to use the deprecated "regulator-compatible" property, or just use the standard compatible property.

Good point. I would not use a deprecated property but the compatible property seems reasonable to me. So you agree that the patch's concept could be retained while substituting the node-name property by the compatible property?

Quoted 14 lines · Felix Brack

Quoted 11 lines · Chen-Yu Tsai

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

The actual regulator.txt states:

Quoted 4 lines

Optional properties:
- regulator-name: a string, required by the regulator uclass

This was the reason for choosing the regulator-name property.

Mine was from the Linux Kernel. Are there two sets of bindings? Shouldn't there be just one?

Quoted 10 lines

Quoted 4 lines

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

Exactly. I totally agree but as stated in an earlier post: I did not define the names for the regulators and modifying them is almost certainly not the right way to go. Let me explain this briefly. The regulator names I'm trying to match are those from tps65910.dtsi, an include file. The exact same file is part of the LINUX kernel. Therefore I resigned suggesting the modification of the node names.

First, it is an include file. Board files are free to override the name. We did that for sunxi at first, have the .dtsi file provide some default names, then have board .dts files override them with names that make more sense. Later on we just dropped the default names altogether.

The same pattern is also seen in tps65910.dtsi and am3517-craneboard.dts. And also am335x-evm.dts, which the tps65910.dtsi was tested with.

Now I couldn't find the binding document for tps65910, but looking at tps65217 instead, it says:

- regulators: list of regulators provided by this controller, must be named after their hardware counterparts: dcdc[1-3] and ldo[1-4]

And indeed that is what's used in the example.

So clearly the dts files aren't following the binding.

Quoted 9 lines

Quoted 5 lines

If you have multiple regulator nodes which need to be differentiated, you need to use the deprecated "regulator-compatible" property, or just use the standard compatible property.

Good point. I would not use a deprecated property but the compatible property seems reasonable to me. So you agree that the patch's concept could be retained while substituting the node-name property by the compatible property?

If you're going to modify the binding and/or device tree files, please do it in both places. Ideally the platform should have one canonical source for them.

Linux's standard regulator DT matching code only matches against node names and the deprecated "regulator-compatible" property. Not even the standard "compatible" is used, though I see a few PMICs using that. So even if you do get them working this way in U-boot, it's still not going to work in Linux, and you might get other comments when pushing your changes.

Quoted 18 lines · Chen-Yu Tsai

Quoted 17 lines · Felix Brack

Quoted 11 lines · Chen-Yu Tsai

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

The actual regulator.txt states:

Optional properties:
- regulator-name: a string, required by the regulator uclass

Quoted 5 lines

Quoted 1 line

This was the reason for choosing the regulator-name property.

Mine was from the Linux Kernel. Are there two sets of bindings? Shouldn't there be just one?

Mine was from U-Boot as this is the U-Boot mailing list and things do not seem to be fully synchronized between LINUX and U-Boot.

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

Exactly. I totally agree but as stated in an earlier post: I did not define the names for the regulators and modifying them is almost certainly not the right way to go. Let me explain this briefly. The regulator names I'm trying to match are those from tps65910.dtsi, an include file. The exact same file is part of the LINUX kernel. Therefore I resigned suggesting the modification of the node names.

Quoted 6 lines

First, it is an include file. Board files are free to override the name. We did that for sunxi at first, have the .dtsi file provide some default names, then have board .dts files override them with names that make more sense. Later on we just dropped the default names altogether.

I am definitely confused now, maybe we are using same terms for different things. When I'm talking about a 'name' I mean the 'node name'
according to DT specification (as for instance returned by
ofnode_get_name). I'm not talking about a property or a node label. The following code defines a node name 'regulator at 2' ore more precisely a node named 'regulator' having unit-address 2:

vdd1_reg: regulator at 2 {
        reg = <2>;
        regulator-compatible = "vdd1";
};

This is found in tps65910.dtsi include file. You say "board files are free to override the name". Hence I could include the tps65910.dtsi into my board dts file and kind of rename node 'regulator at 2' by let's say 'buck_vdd1 at 2'?
The only way of "overriding" I can think of is by not including the dtsi file and redefining all nodes.

Quoted 3 lines

The same pattern is also seen in tps65910.dtsi and am3517-craneboard.dts. And also am335x-evm.dts, which the tps65910.dtsi was tested with.

Looking at the am3517-craneboard.dts file I don't see how node names are getting overwritten. What gets set, changed or overwritten is the node property regulator-name.

Quoted 9 lines

Now I couldn't find the binding document for tps65910, but looking at tps65217 instead, it says:

- regulators: list of regulators provided by this controller, must be named after their hardware counterparts: dcdc[1-3] and ldo[1-4]

And indeed that is what's used in the example.

Yes, exactly and correct. Again, this tps65217.txt does only exist in LINUX and not in U-Boot.

Quoted 2 lines

So clearly the dts files aren't following the binding.

And what about the dtsi files? Are they correct in defining node names as 'regulator@[unit-address]'?

Quoted 14 lines

Quoted 9 lines

Quoted 5 lines

If you have multiple regulator nodes which need to be differentiated, you need to use the deprecated "regulator-compatible" property, or just use the standard compatible property.

Good point. I would not use a deprecated property but the compatible property seems reasonable to me. So you agree that the patch's concept could be retained while substituting the node-name property by the compatible property?

If you're going to modify the binding and/or device tree files, please do it in both places. Ideally the platform should have one canonical source for them.

Quoted 7 lines

Linux's standard regulator DT matching code only matches against node names and the deprecated "regulator-compatible" property. Not even the standard "compatible" is used, though I see a few PMICs using that. So even if you do get them working this way in U-boot, it's still not going to work in Linux, and you might get other comments when pushing your changes.

I guess if LINUX would not match against the regulator-compatible property but only against the node name things would not work properly. Again looking the file tps65217.dtsi shows that every regulator node (named regulator at 0 to regulator at 6) defines the regulator-compatible property. Only matching against this property therefore makes things working.

Felix

Hi,

Quoted 19 lines · Felix Brack

Quoted 18 lines · Chen-Yu Tsai

Quoted 17 lines · Felix Brack

Quoted 11 lines · Chen-Yu Tsai

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

The actual regulator.txt states:

Optional properties:
- regulator-name: a string, required by the regulator uclass

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

This was the reason for choosing the regulator-name property.

Mine was from the Linux Kernel. Are there two sets of bindings? Shouldn't there be just one?

Mine was from U-Boot as this is the U-Boot mailing list and things do not seem to be fully synchronized between LINUX and U-Boot.

That seems to be the underlying problem. They really should be the same. I'm not sure which platforms really follow through with this though.

Quoted 29 lines

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

Exactly. I totally agree but as stated in an earlier post: I did not define the names for the regulators and modifying them is almost certainly not the right way to go. Let me explain this briefly. The regulator names I'm trying to match are those from tps65910.dtsi, an include file. The exact same file is part of the LINUX kernel. Therefore I resigned suggesting the modification of the node names.

Quoted 6 lines

First, it is an include file. Board files are free to override the name. We did that for sunxi at first, have the .dtsi file provide some default names, then have board .dts files override them with names that make more sense. Later on we just dropped the default names altogether.

I am definitely confused now, maybe we are using same terms for different things. When I'm talking about a 'name' I mean the 'node name'
according to DT specification (as for instance returned by
ofnode_get_name). I'm not talking about a property or a node label. The following code defines a node name 'regulator at 2' ore more precisely a node named 'regulator' having unit-address 2:

vdd1_reg: regulator at 2 {
        reg = <2>;
        regulator-compatible = "vdd1";
};

Quoted 6 lines

This is found in tps65910.dtsi include file. You say "board files are free to override the name". Hence I could include the tps65910.dtsi into my board dts file and kind of rename node 'regulator at 2' by let's say 'buck_vdd1 at 2'?
The only way of "overriding" I can think of is by not including the dtsi file and redefining all nodes.

I'm talking about the "name" as defined in the "regulator-name" property, which is what you want to match against in your patch.

So boards are definitely free to override the property by setting a new value for it. And indeed boards do that.

Quoted 6 lines

Quoted 3 lines

The same pattern is also seen in tps65910.dtsi and am3517-craneboard.dts. And also am335x-evm.dts, which the tps65910.dtsi was tested with.

Looking at the am3517-craneboard.dts file I don't see how node names are getting overwritten. What gets set, changed or overwritten is the node property regulator-name.

Yes. That's what I'm referring to. Doesn't this work against your attempt to bind pmic-uclass against regulator-name properties?

Quoted 9 lines

Quoted 9 lines

Now I couldn't find the binding document for tps65910, but looking at tps65217 instead, it says:

- regulators: list of regulators provided by this controller, must be named after their hardware counterparts: dcdc[1-3] and ldo[1-4]

And indeed that is what's used in the example.

Quoted 2 lines

Yes, exactly and correct. Again, this tps65217.txt does only exist in LINUX and not in U-Boot.

Device tree bindings are a set of rules, contracts, ABIs, whatever you call it, between the hardware description that is the device tree, and software that implement support for the hardware. Having two or more diverging sets is not an improvement. Communities should work together to have a common set of bindings.

FreeBSD seems to be importing device tree bindings from Linux. There was also talk about importing bindings from other projects that have already created and implemented support for bindings that do not exist in Linux. This has been raised for the Device Tree Workshop this Thursday at OSSE/ELCE:

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004833.html
https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004891.html

I've also raised the issue of diverging device trees and bindings:

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004849.html

Quoted 4 lines

Quoted 2 lines

So clearly the dts files aren't following the binding.

And what about the dtsi files? Are they correct in defining node names as 'regulator@[unit-address]'?

They are broken. Someone needs to fix them.

Quoted 21 lines

Quoted 14 lines

Quoted 9 lines

Quoted 5 lines

If you have multiple regulator nodes which need to be differentiated, you need to use the deprecated "regulator-compatible" property, or just use the standard compatible property.

Good point. I would not use a deprecated property but the compatible property seems reasonable to me. So you agree that the patch's concept could be retained while substituting the node-name property by the compatible property?

If you're going to modify the binding and/or device tree files, please do it in both places. Ideally the platform should have one canonical source for them.

Quoted 7 lines

Linux's standard regulator DT matching code only matches against node names and the deprecated "regulator-compatible" property. Not even the standard "compatible" is used, though I see a few PMICs using that. So even if you do get them working this way in U-boot, it's still not going to work in Linux, and you might get other comments when pushing your changes.

Quoted 6 lines

I guess if LINUX would not match against the regulator-compatible property but only against the node name things would not work properly. Again looking the file tps65217.dtsi shows that every regulator node (named regulator at 0 to regulator at 6) defines the regulator-compatible property. Only matching against this property therefore makes things working.

The dtsi file is not following the binding. It should be fixed. But I suppose that is an issue for the platform maintainer, and any concerned users.

Regards
ChenYu

Quoted 2 lines

Felix

Quoted 69 lines · Chen-Yu Tsai

Hi,

Quoted 19 lines · Felix Brack

Quoted 18 lines · Chen-Yu Tsai

Quoted 17 lines · Felix Brack

Quoted 11 lines · Chen-Yu Tsai

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

The actual regulator.txt states:

Optional properties:
- regulator-name: a string, required by the regulator uclass

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

This was the reason for choosing the regulator-name property.

Mine was from the Linux Kernel. Are there two sets of bindings? Shouldn't there be just one?

Mine was from U-Boot as this is the U-Boot mailing list and things do not seem to be fully synchronized between LINUX and U-Boot.

That seems to be the underlying problem. They really should be the same. I'm not sure which platforms really follow through with this though.

Quoted 29 lines

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

Exactly. I totally agree but as stated in an earlier post: I did not define the names for the regulators and modifying them is almost certainly not the right way to go. Let me explain this briefly. The regulator names I'm trying to match are those from tps65910.dtsi, an include file. The exact same file is part of the LINUX kernel. Therefore I resigned suggesting the modification of the node names.

Quoted 6 lines

First, it is an include file. Board files are free to override the name. We did that for sunxi at first, have the .dtsi file provide some default names, then have board .dts files override them with names that make more sense. Later on we just dropped the default names altogether.

I am definitely confused now, maybe we are using same terms for different things. When I'm talking about a 'name' I mean the 'node name'
according to DT specification (as for instance returned by
ofnode_get_name). I'm not talking about a property or a node label. The following code defines a node name 'regulator at 2' ore more precisely a node named 'regulator' having unit-address 2:

vdd1_reg: regulator at 2 {
        reg = <2>;
        regulator-compatible = "vdd1";
};

Quoted 6 lines

This is found in tps65910.dtsi include file. You say "board files are free to override the name". Hence I could include the tps65910.dtsi into my board dts file and kind of rename node 'regulator at 2' by let's say 'buck_vdd1 at 2'?
The only way of "overriding" I can think of is by not including the dtsi file and redefining all nodes.

Quoted 6 lines

I'm talking about the "name" as defined in the "regulator-name" property, which is what you want to match against in your patch.

So boards are definitely free to override the property by setting a new value for it. And indeed boards do that.

This is the driving idea behind my patch.

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

The same pattern is also seen in tps65910.dtsi and am3517-craneboard.dts. And also am335x-evm.dts, which the tps65910.dtsi was tested with.

Looking at the am3517-craneboard.dts file I don't see how node names are getting overwritten. What gets set, changed or overwritten is the node property regulator-name.

Yes. That's what I'm referring to. Doesn't this work against your attempt to bind pmic-uclass against regulator-name properties?

Well, yes, my patch works like charm. I didn't mention that it doesn't work ;-).

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Now I couldn't find the binding document for tps65910, but looking at tps65217 instead, it says:

- regulators: list of regulators provided by this controller, must be named after their hardware counterparts: dcdc[1-3] and ldo[1-4]

And indeed that is what's used in the example.

Quoted 9 lines

Quoted 2 lines

Yes, exactly and correct. Again, this tps65217.txt does only exist in LINUX and not in U-Boot.

Device tree bindings are a set of rules, contracts, ABIs, whatever you call it, between the hardware description that is the device tree, and software that implement support for the hardware. Having two or more diverging sets is not an improvement. Communities should work together to have a common set of bindings.

I totally agree.

Quoted 9 lines

FreeBSD seems to be importing device tree bindings from Linux. There was also talk about importing bindings from other projects that have already created and implemented support for bindings that do not exist in Linux. This has been raised for the Device Tree Workshop this Thursday at OSSE/ELCE:

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004833.html
https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004891.html

If that wish from Andrew would become true it would be the ultimate
solution: "I’d like it if there was a common repo for all devicetree consumers to share".

Quoted 2 lines

I've also raised the issue of diverging device trees and bindings:

Quoted 2 lines

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004849.html

Thanks! I hope this helps moving things into the right direction. And yes, there is quite a lot of truth in calling my patch a "dirty workaround" for broken dts and even worse dtsi files. But for U-Boot this patch could help to bridge the time gap until the real fix. Sadly I now that it could also have the exact opposite effect and lead to even more broken (with respect to binding rules) dts and dtsi files.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

So clearly the dts files aren't following the binding.

And what about the dtsi files? Are they correct in defining node names as 'regulator@[unit-address]'?

They are broken. Someone needs to fix them.

Agreed.

Quoted 28 lines

Quoted 21 lines

Quoted 14 lines

Quoted 9 lines

Quoted 5 lines

If you have multiple regulator nodes which need to be differentiated, you need to use the deprecated "regulator-compatible" property, or just use the standard compatible property.

Good point. I would not use a deprecated property but the compatible property seems reasonable to me. So you agree that the patch's concept could be retained while substituting the node-name property by the compatible property?

If you're going to modify the binding and/or device tree files, please do it in both places. Ideally the platform should have one canonical source for them.

Quoted 7 lines

Linux's standard regulator DT matching code only matches against node names and the deprecated "regulator-compatible" property. Not even the standard "compatible" is used, though I see a few PMICs using that. So even if you do get them working this way in U-boot, it's still not going to work in Linux, and you might get other comments when pushing your changes.

Quoted 6 lines

I guess if LINUX would not match against the regulator-compatible property but only against the node name things would not work properly. Again looking the file tps65217.dtsi shows that every regulator node (named regulator at 0 to regulator at 6) defines the regulator-compatible property. Only matching against this property therefore makes things working.

Quoted 4 lines

The dtsi file is not following the binding. It should be fixed. But I suppose that is an issue for the platform maintainer, and any concerned users.

I am not a platform maintainer but of course I am a concerned user. If I had been confident that fixing the include file tps65910.dtsi (and all its dependencies) in the U-Boot project would have been sufficient, I would have tried that first before sending in my patch. In my case, following the dt binding rules for this file is the most correct solution to the problem, not just a workaround.
But this brings at least U-Boot and LINUX tps65910.dtsi files out of sync and I'm pretty sure that such a patch would be rejected for this reason.
So we definitely have a chicken-egg problem here: who takes the lead, U-Boot or LINUX? And what about the others you mentioned FreeBSD or how Andrew said: "device-tree consumers"?

I'm ending up in voting (like many others I guess) for a central dts/dtsi/dt-binding-rules repository.

Quoted 126 lines · Felix Brack

Quoted 69 lines · Chen-Yu Tsai

Hi,

Quoted 19 lines · Felix Brack

Quoted 18 lines · Chen-Yu Tsai

Quoted 17 lines · Felix Brack

Quoted 11 lines · Chen-Yu Tsai

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

The actual regulator.txt states:

Optional properties:
- regulator-name: a string, required by the regulator uclass

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

This was the reason for choosing the regulator-name property.

Mine was from the Linux Kernel. Are there two sets of bindings? Shouldn't there be just one?

Mine was from U-Boot as this is the U-Boot mailing list and things do not seem to be fully synchronized between LINUX and U-Boot.

That seems to be the underlying problem. They really should be the same. I'm not sure which platforms really follow through with this though.

Quoted 29 lines

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

Exactly. I totally agree but as stated in an earlier post: I did not define the names for the regulators and modifying them is almost certainly not the right way to go. Let me explain this briefly. The regulator names I'm trying to match are those from tps65910.dtsi, an include file. The exact same file is part of the LINUX kernel. Therefore I resigned suggesting the modification of the node names.

Quoted 6 lines

First, it is an include file. Board files are free to override the name. We did that for sunxi at first, have the .dtsi file provide some default names, then have board .dts files override them with names that make more sense. Later on we just dropped the default names altogether.

I am definitely confused now, maybe we are using same terms for different things. When I'm talking about a 'name' I mean the 'node name'
according to DT specification (as for instance returned by
ofnode_get_name). I'm not talking about a property or a node label. The following code defines a node name 'regulator at 2' ore more precisely a node named 'regulator' having unit-address 2:

vdd1_reg: regulator at 2 {
        reg = <2>;
        regulator-compatible = "vdd1";
};

Quoted 6 lines

This is found in tps65910.dtsi include file. You say "board files are free to override the name". Hence I could include the tps65910.dtsi into my board dts file and kind of rename node 'regulator at 2' by let's say 'buck_vdd1 at 2'?
The only way of "overriding" I can think of is by not including the dtsi file and redefining all nodes.

Quoted 6 lines

I'm talking about the "name" as defined in the "regulator-name" property, which is what you want to match against in your patch.

So boards are definitely free to override the property by setting a new value for it. And indeed boards do that.

This is the driving idea behind my patch.

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

The same pattern is also seen in tps65910.dtsi and am3517-craneboard.dts. And also am335x-evm.dts, which the tps65910.dtsi was tested with.

Looking at the am3517-craneboard.dts file I don't see how node names are getting overwritten. What gets set, changed or overwritten is the node property regulator-name.

Yes. That's what I'm referring to. Doesn't this work against your attempt to bind pmic-uclass against regulator-name properties?

Well, yes, my patch works like charm. I didn't mention that it doesn't work ;-).

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Now I couldn't find the binding document for tps65910, but looking at tps65217 instead, it says:

- regulators: list of regulators provided by this controller, must be named after their hardware counterparts: dcdc[1-3] and ldo[1-4]

And indeed that is what's used in the example.

Quoted 9 lines

Quoted 2 lines

Yes, exactly and correct. Again, this tps65217.txt does only exist in LINUX and not in U-Boot.

Device tree bindings are a set of rules, contracts, ABIs, whatever you call it, between the hardware description that is the device tree, and software that implement support for the hardware. Having two or more diverging sets is not an improvement. Communities should work together to have a common set of bindings.

I totally agree.

Quoted 9 lines

FreeBSD seems to be importing device tree bindings from Linux. There was also talk about importing bindings from other projects that have already created and implemented support for bindings that do not exist in Linux. This has been raised for the Device Tree Workshop this Thursday at OSSE/ELCE:

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004833.html
https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004891.html

If that wish from Andrew would become true it would be the ultimate
solution: "I’d like it if there was a common repo for all devicetree consumers to share".

Quoted 2 lines

I've also raised the issue of diverging device trees and bindings:

Quoted 8 lines

Quoted 2 lines

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004849.html

Thanks! I hope this helps moving things into the right direction. And yes, there is quite a lot of truth in calling my patch a "dirty workaround" for broken dts and even worse dtsi files. But for U-Boot this patch could help to bridge the time gap until the real fix. Sadly I now that it could also have the exact opposite effect and lead to even more broken (with respect to binding rules) dts and dtsi files.

I'm OK with a temporary fix or workaround. But temporary means someone needs to follow up and get it where it needs to go. It should also be noted in the commit log. And the binding should be marked as a workaround for existing (broken) device trees, so people don't adapt it by mistake.

Quoted 41 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

So clearly the dts files aren't following the binding.

And what about the dtsi files? Are they correct in defining node names as 'regulator@[unit-address]'?

They are broken. Someone needs to fix them.

Agreed.

Quoted 28 lines

Quoted 21 lines

Quoted 14 lines

Quoted 9 lines

Quoted 5 lines

If you have multiple regulator nodes which need to be differentiated, you need to use the deprecated "regulator-compatible" property, or just use the standard compatible property.

Good point. I would not use a deprecated property but the compatible property seems reasonable to me. So you agree that the patch's concept could be retained while substituting the node-name property by the compatible property?

If you're going to modify the binding and/or device tree files, please do it in both places. Ideally the platform should have one canonical source for them.

Quoted 7 lines

Linux's standard regulator DT matching code only matches against node names and the deprecated "regulator-compatible" property. Not even the standard "compatible" is used, though I see a few PMICs using that. So even if you do get them working this way in U-boot, it's still not going to work in Linux, and you might get other comments when pushing your changes.

Quoted 6 lines

I guess if LINUX would not match against the regulator-compatible property but only against the node name things would not work properly. Again looking the file tps65217.dtsi shows that every regulator node (named regulator at 0 to regulator at 6) defines the regulator-compatible property. Only matching against this property therefore makes things working.

Quoted 4 lines

The dtsi file is not following the binding. It should be fixed. But I suppose that is an issue for the platform maintainer, and any concerned users.

Quoted 12 lines

I am not a platform maintainer but of course I am a concerned user. If I had been confident that fixing the include file tps65910.dtsi (and all its dependencies) in the U-Boot project would have been sufficient, I would have tried that first before sending in my patch. In my case, following the dt binding rules for this file is the most correct solution to the problem, not just a workaround.
But this brings at least U-Boot and LINUX tps65910.dtsi files out of sync and I'm pretty sure that such a patch would be rejected for this reason.
So we definitely have a chicken-egg problem here: who takes the lead, U-Boot or LINUX? And what about the others you mentioned FreeBSD or how Andrew said: "device-tree consumers"?

I guess this is mostly per-platform. For Allwinner SoCs, it seems Linux is the preferred source. U-boot typically falls behind in terms of device tree content and support, and in rare cases such as Ethernet support does end up implementing an earlier version of the binding that was not the final version that ended up in the kernel. The BSDs seem to be happy with taking our Linux bindings and implementing support for them. On the other hand, Allwinner is not a fully supported platform, meaning that with each kernel release, support for new peripherals is added, and additions to the device tree files reflect this as well. We also use separate device tree blobs for U-boot and Linux.

I'm not sure if it's possible to do this to the platform you're working on, as some maintainers do take DT compatibility very seriously. Then again, fixing something that doesn't work (in Linux) in the first place isn't really creating any regressions.

Quoted 2 lines

I'm ending up in voting (like many others I guess) for a central dts/dtsi/dt-binding-rules repository.

I see a few (Linux) people raising concern about synchronization between the kernel and whatever repository the bindings/DT end up in, but that is already what other projects are dealing with. I definitely hope some agreement is achieved.

ChenYu

Quoted 136 lines · Chen-Yu Tsai

Quoted 134 lines · Felix Brack

Quoted 69 lines · Chen-Yu Tsai

Hi,

Quoted 19 lines · Felix Brack

Quoted 18 lines · Chen-Yu Tsai

Quoted 17 lines · Felix Brack

Quoted 11 lines · Chen-Yu Tsai

Quoted 4 lines · Felix Brack

This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator at 1 and regulator at 10 for example.

No. See the regulator bindings:

Optional properties:
- regulator-name: A string used as a descriptive name for regulator outputs

The actual regulator.txt states:

Optional properties:
- regulator-name: a string, required by the regulator uclass

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

This was the reason for choosing the regulator-name property.

Mine was from the Linux Kernel. Are there two sets of bindings? Shouldn't there be just one?

Mine was from U-Boot as this is the U-Boot mailing list and things do not seem to be fully synchronized between LINUX and U-Boot.

That seems to be the underlying problem. They really should be the same. I'm not sure which platforms really follow through with this though.

Quoted 29 lines

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

This can vary from board to board. The name should match the power rail name of the board (which may not be the same as the regulator chip's output name).

Exactly. I totally agree but as stated in an earlier post: I did not define the names for the regulators and modifying them is almost certainly not the right way to go. Let me explain this briefly. The regulator names I'm trying to match are those from tps65910.dtsi, an include file. The exact same file is part of the LINUX kernel. Therefore I resigned suggesting the modification of the node names.

Quoted 6 lines

First, it is an include file. Board files are free to override the name. We did that for sunxi at first, have the .dtsi file provide some default names, then have board .dts files override them with names that make more sense. Later on we just dropped the default names altogether.

I am definitely confused now, maybe we are using same terms for different things. When I'm talking about a 'name' I mean the 'node name'
according to DT specification (as for instance returned by
ofnode_get_name). I'm not talking about a property or a node label. The following code defines a node name 'regulator at 2' ore more precisely a node named 'regulator' having unit-address 2:

vdd1_reg: regulator at 2 {
        reg = <2>;
        regulator-compatible = "vdd1";
};

Quoted 6 lines

This is found in tps65910.dtsi include file. You say "board files are free to override the name". Hence I could include the tps65910.dtsi into my board dts file and kind of rename node 'regulator at 2' by let's say 'buck_vdd1 at 2'?
The only way of "overriding" I can think of is by not including the dtsi file and redefining all nodes.

Quoted 6 lines

I'm talking about the "name" as defined in the "regulator-name" property, which is what you want to match against in your patch.

So boards are definitely free to override the property by setting a new value for it. And indeed boards do that.

This is the driving idea behind my patch.

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

The same pattern is also seen in tps65910.dtsi and am3517-craneboard.dts. And also am335x-evm.dts, which the tps65910.dtsi was tested with.

Looking at the am3517-craneboard.dts file I don't see how node names are getting overwritten. What gets set, changed or overwritten is the node property regulator-name.

Yes. That's what I'm referring to. Doesn't this work against your attempt to bind pmic-uclass against regulator-name properties?

Well, yes, my patch works like charm. I didn't mention that it doesn't work ;-).

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Now I couldn't find the binding document for tps65910, but looking at tps65217 instead, it says:

- regulators: list of regulators provided by this controller, must be named after their hardware counterparts: dcdc[1-3] and ldo[1-4]

And indeed that is what's used in the example.

Quoted 9 lines

Quoted 2 lines

Yes, exactly and correct. Again, this tps65217.txt does only exist in LINUX and not in U-Boot.

Device tree bindings are a set of rules, contracts, ABIs, whatever you call it, between the hardware description that is the device tree, and software that implement support for the hardware. Having two or more diverging sets is not an improvement. Communities should work together to have a common set of bindings.

I totally agree.

Quoted 9 lines

FreeBSD seems to be importing device tree bindings from Linux. There was also talk about importing bindings from other projects that have already created and implemented support for bindings that do not exist in Linux. This has been raised for the Device Tree Workshop this Thursday at OSSE/ELCE:

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004833.html
https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004891.html

If that wish from Andrew would become true it would be the ultimate
solution: "I’d like it if there was a common repo for all devicetree consumers to share".

Quoted 4 lines

I've also raised the issue of diverging device trees and bindings:

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004849.html

Thanks! I hope this helps moving things into the right direction. And yes, there is quite a lot of truth in calling my patch a "dirty workaround" for broken dts and even worse dtsi files. But for U-Boot this patch could help to bridge the time gap until the real fix. Sadly I now that it could also have the exact opposite effect and lead to even more broken (with respect to binding rules) dts and dtsi files.

Quoted 6 lines

I'm OK with a temporary fix or workaround. But temporary means someone needs to follow up and get it where it needs to go. It should also be noted in the commit log. And the binding should be marked as a workaround for existing (broken) device trees, so people don't adapt it by mistake.

This has already been addressed in v2 patch I posted a week ago
(https://patchwork.ozlabs.org/patch/827507/).