Re: [PATCH v3 2/9] ARM: dts: nspire: Use syscon-reboot to handle restart

[Rob Herring]

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@xxxxxx>
> > > > > Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > > > > Tested-by: Fabian Vogt <fabian@xxxxxxxxxxxxxx>
> > > > > Reviewed-by: Fabian Vogt <fabian@xxxxxxxxxxxxxx>
> > > > > ---
> > > > >    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@90090000 {
> > > > >    			};
> > > > >    			misc: misc@900a0000 {
> > > > > +				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.


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

> 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@43000000 {
> 	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