Message-ID: <2a7afcc3-b973-c993-e4f1-0f1888073f28@ti.com>
Date:   Tue, 1 Nov 2022 08:04:15 -0500
From:   Andrew Davis <afd@...com>
To:     Rob Herring <robh@...nel.org>
CC:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Lee Jones <lee@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Daniel Tang <dt.tangr@...il.com>,
        Fabian Vogt <fabian@...ter-vogt.de>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/9] ARM: dts: nspire: Use syscon-reboot to handle
 restart

On 10/31/22 12:14 PM, Rob Herring wrote:
> On Mon, Oct 31, 2022 at 09:30:45AM -0500, Andrew Davis wrote:
>> On 10/27/22 4:27 PM, Krzysztof Kozlowski wrote:
>>> On 27/10/2022 17:07, Andrew Davis wrote:
>>>> On 10/27/22 2:33 PM, Krzysztof Kozlowski wrote:
>>>>> On 27/10/2022 14:13, Andrew Davis wrote:
>>>>>> Writing this bit can be handled by the syscon-reboot driver.
>>>>>> Add this node to DT.
>>>>>>
>>>>>> Signed-off-by: Andrew Davis <afd@...com>
>>>>>> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
>>>>>> Tested-by: Fabian Vogt <fabian@...ter-vogt.de>
>>>>>> Reviewed-by: Fabian Vogt <fabian@...ter-vogt.de>
>>>>>> ---
>>>>>>     arch/arm/boot/dts/nspire.dtsi | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi
>>>>>> index bb240e6a3a6f..48fbc9d533c3 100644
>>>>>> --- a/arch/arm/boot/dts/nspire.dtsi
>>>>>> +++ b/arch/arm/boot/dts/nspire.dtsi
>>>>>> @@ -172,7 +172,14 @@ rtc: rtc@...90000 {
>>>>>>     			};
>>>>>>     			misc: misc@...a0000 {
>>>>>> +				compatible = "ti,nspire-misc", "syscon", "simple-mfd";
>>>>>
>>>>> You have syscon and simple-mfd, but bindings in patch #1 say only syscon.
>>>>>
>>>>
>>>> I'm not following, are you just saying my wording in the patch message just
>>>> wasn't complete?
>>>
>>> Your binding patch adds nspire compatible to the list of two items, so
>>> you have two items in total - nspire followed by syscon.
>>>
>>> What you implemented here is different.
>>>
>>
>> Is there a list of three items I can add this compatible? If instead you
>> mean I should go make a new binding, just say so :)
> 
> An MFD should define its own schema file.
> 
> The original intent of syscon.yaml was for just single nodes with
> 'syscon' (and a specific compatible). Adding in simple-mfd was probably
> a mistake. Certainly we need to rework the schema as you should get a
> warning in this case.
> 
>>>> Or are you saying something more about nodes that are both syscon and simple-mfd?
>>>> In that case, having both syscon and simple-mfd seems rather common, looks like
>>>> you added the rule for it[0].
>>>>
>>>> Thinking on this, they almost represent the same thing. simple-mfd says "my child
>>>> nodes should be considered devices", why do we need that? Couldn't we simply state
>>>> that "syscon" node's children are always devices, I mean what else could they be,
>>>> syscon is an MFD after all (and lives in drivers/mfd/).
>>>
>>> No, syscon is not an MFD. Syscon means system controller and alone it
>>> does not have children.
>>>
>>
>> The binding lives in devicetree/bindings/*mfd*/, it is mentioned as one
>> in devicetree/bindings/mfd/mfd.txt. If it is not an MFD then the bindings
>> are giving out mixed signals here..
>>
>>>>
>>>> "syscon" often just says, others can use the registers within this node, so as a
>>>> different option, make "syscon" a property of "simple-mfd" nodes. I'm seeing all
>>>> these examples of devices that should have been children of the "syscon" device,
>>>> but instead use
>>>>
>>>> regmap = <&x>;
>>>> syscon = <&x>;
>>>>
>>>> or similar and put the device node out somewhere random. And in those cases,
>>>> wouldn't it have been more correct to use the normal "reg" and "regions" to
>>>> define the registers belonging to the child node/device?..
>>>
>>> Sorry, I do not follow. How this is even related to your patch?
>>>
>>> Your bindings say A, DTS say B. A != B. This needs fixing.
>>>
>>
>> I said it was compatible with "syscon", not that it is incompatible
>> with "simple-mfd" devices.
>>
>> What I've done here gives no dtbs_check warnings and
>> "devicetree/bindings/mfd/mfd.txt" explicitly allows what I am doing.
>> Unless we do not consider the old bindings valid?
> 
> Only that the example is not because it doesn't have a specific
> compatible.
> 
> What needs to be clarified is that MFDs must define all the child nodes
> whether they are 'simple' or not.
> 
>> If so, would you
>> like me to convert mfd.txt to yaml, just let me know.
> 
> No, because I don't think there is anything to define as a schema.
> 

It would allow for simple register regions to be 'simple-mfd' without
needing a whole new binding document for each. Same as we already have
with 'syscon.yaml'.

Making every simple MMIO space create a new binding document is not
reasonable. Neither is defining all nodes up front in that binding,
we don't expect that for top level nodes or 'simple-bus', why should
we for 'simple-mfd'?

My point with mfd.txt is that this *was allowed*, and there are already
a large number of users of the existing style.

> 
>>> Unless you are asking me what your device is in general. This I don't
>>> really know, but if you want to use it as regmap provider for system
>>> registers and as a parent of syscon-based reboot device, then your
>>> device is syscon and simple-mfd. With a specific compatible. Was this
>>> your question?
>>>
>>
>> Yes, I would like to use it as a regmap provider, my question here is
>> a much more general one: why do I need to specify that in device tree?
>> That is not a hardware description, my hardware is not "regmap" hardware.
>> This "syscon" stuff feels like a bodge to make the Linux drivers and bus
>> frameworks interact the way we want.
> 
> Bingo! It's a hint for create a regmap. We could just have a compatible
> list in the kernel for compatibles needing a regmap. Maybe that list
> would be too long though. So call it h/w description for this h/w is
> referenced by other places.
> 
> 
>> I know at this point this has little to do with this series, but I'd like
>> to just think this out for a moment. The latest Devicetree Specification
>> talks about "simple-bus" as a special compatible that communicates that
>> child nodes with compatible strings need probed also. ("simple-mfd" seems
>> to be used the same way but without needing a "ranges" property..)
> 
> Yes, both cases are saying there is no dependency or setup of the parent
> needs. If the child nodes depend on the regmap, then it's not a
> 'simple-mfd' IMO. Therefore 'syscon' together with 'simple-mfd' is wrong
> unless it's other nodes that need the regmap. The schema can't really
> check that.
> 

'syscon' also provides for reusing the same single register by multiple
users, such as bit-mapped registers. It also allows re-using the existing
simple syscon device compatibles. Again because people do not like writing
bindings for simple nodes.

Andrew

>> Both of these are properties of a node, not something a device is "compatible"
>> with. "compatibles" are also supposed to be listed "from most specific to
>> most general", so which is more specific, "simple-mfd" or "syscon", etc..
> 
> I would say 'syscon' is more specific if I have to pick. It implies some
> registers exist. 'simple-mfd' should mean there are no parent resources
> (...the children depend on).
> 
> We've probably got enough of a mixture of the order, it wouldn't be
> worth the effort to try to enforce the order here.
> 
>> Seems like Rob might agree[0], these are not really compatibles. We cant fix
>> history, but for new nodes, instead of growing the problem and forcing these to
>> be overloaded compatibles, we allow these to become new standard node properties.
>>
>> For instance:
>>
>> main_conf: syscon@...00000 {
>> 	compatible = "ti,j721e-system-controller";
>> 	reg = <0x0 0x43000000 0x0 0x20000>;
>>
>> 	simple-bus;
>> 	syscon;
> 
> Umm, no. This ship already sailed and we don't need a 2nd way to do
> things.
> 
> Rob

Powered by blists - more mailing lists