[U-Boot] [PATCH] power: extend prefix match to regulator-name property

Tue Oct 24 06:30:34 UTC 2017

On 23.10.2017 22:34, Chen-Yu Tsai wrote:
> On Tue, Oct 24, 2017 at 12:36 AM, Felix Brack <fb at ltec.ch> wrote:
>> On 23.10.2017 16:36, Chen-Yu Tsai wrote:
>>> Hi,
>>>
>>> On Thu, Oct 12, 2017 at 11:40 PM, Felix Brack <fb at ltec.ch> wrote:
>>>> On 12.10.2017 14:53, Chen-Yu Tsai wrote:
>>>>> On Thu, Oct 12, 2017 at 8:24 PM, Felix Brack <fb at ltec.ch> wrote:
>>>>>> On 12.10.2017 04:46, Chen-Yu Tsai wrote:
>>>>>>> On Mon, Oct 9, 2017 at 6:04 PM, Felix Brack <fb at ltec.ch> wrote:
>>>>>>>> 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.
>>>>>
>>>>> 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.
>>>
>>>>>>> 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.
>>>>>
>>>> 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.
>>>
>>> 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.
>>
>>>>> 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 ;-).
>>
>>>>
>>>>> 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.
>>>
>>> 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.
>>
>>> 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".
>>
>>> 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.
> 
> 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/).

>>>>> 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.
>>
>>>>>>
>>>>>>> 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.
>>>>>
>>>> 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.
>>>
>> 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.
> 
>> 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
> 

regards, Felix