Re: [PATCH] mux: mmio: use reg property when parent device is not a syscon

[Andrew Davis]

On 5/16/23 1:33 PM, Krzysztof Kozlowski wrote:
> On 16/05/2023 19:47, Andrew Davis wrote:
> > On 5/16/23 11:49 AM, Krzysztof Kozlowski wrote:
> > > On 16/05/2023 18:29, Andrew Davis wrote:
> > > > On 5/16/23 11:19 AM, Krzysztof Kozlowski wrote:
> > > > > On 16/05/2023 17:18, Andrew Davis wrote:
> > > > > > On 5/15/23 4:14 PM, Peter Rosin wrote:
> > > > > > > Hi!
> > > > > > >
> > > > > > > 2023-05-15 at 21:19, Andrew Davis wrote:
> > > > > > > > The DT binding for the reg-mux compatible states it can be used when the
> > > > > > > > "parent device of mux controller is not syscon device". It also allows
> > > > > > > > for a reg property. When the parent device is indeed not a syscon device,
> > > > > > > > nor is it a regmap provider, we should fallback to using that reg
> > > > > > > > property to identify the address space to use for this mux.
> > > > > > >
> > > > > > > We should? Says who?
> > > > > > >
> > > > > > > Don't get me wrong, I'm not saying the change is bad or wrong, I would just
> > > > > > > like to see an example where it matters. Or, at least some rationale for why
> > > > > > > the code needs to change other than covering some case that looks like it
> > > > > > > could/should be possible based on the binding. I.e., why is it not better to
> > > > > > > "close the hole" in the binding instead?
> > > > > >
> > > > > > Sure, so this all stated when I was building a checker to make sure that drivers
> > > > > > are not mapping overlapping register spaces. I noticed syscon nodes are a source
> > > > > > of that so I'm trying to look into their usage.
> > > > > >
> > > > > > To start, IHMO there is only one valid use for syscon and that is when more than
> > > > > > one driver needs to access shared bits in a single register. DT has no way to
> > > > >
> > > > > It has... what about all existing efuse/nvmem devices?
> > > > >
> > > > > > describe down to the bit granular level, so one must give that register to
> > > > > > a "syscon node", then have the device node use a phandle to the syscon node:
> > > > > >
> > > > > > common_reg: syscon@10000 {
> > > > > > 	compatible = "syscon";
> > > > > > 	reg = <0x10000 0x4>;
> > > > > > };
> > > > > >
> > > > > > consumer@1 {
> > > > > > 	syscon-efuse = <&common_reg 0x1>;
> > > > > > };
> > > > > >
> > > > > > consumer@2 {
> > > > > > 	syscon-efuse = <&common_reg 0x2>;
> > > > > > };
> > > > > >
> > > > > > Something like that, then regmap will take care of synchronizing access.
> > > > >
> > > > > Syscon is not for this.
> > > >
> > > > That is how it is used today, and in 5 other ways too and there is
> > > > no guidance on it. Let me know what syscon is for then.
> > >
> > > Like described in its bindings (syscon.yaml). The main case is: some
> > > part of address space (dedicated) for various purposes.
> >
> > That is a "simple-bus", you could use the same reasoning and make the
> > whole address space one big "syscon" node instead then just poke
> > registers from drivers all over.
>
> Yes and both are discouraged.
>
> > It is not clear where to draw the line, and for that reason I would
> > like to discourage "syscon" as much as possible and use the normal DT
> > scheme of node per device/register space.
>
> We all keep discouraging using syscon, so I agree. What exactly do you mean?

Great, then we are in alignment. This thread started as Peter asked for
the "why" and this was my explanation, basically "syscon is discouraged".

> > > Secondary case is a device, with its address space, which has few
> > > registers from other domain, so it needs to expose these to the other
> > > devices.
> >
> > That is not the case for "reg-mux"; neither case is. So you would
> > agree that "reg-mux" nodes should not be syscon nodes
>
> I don't understand. reg-mux is not a syscon. No syscon compatible in:
> Documentation/devicetree/bindings/mux/reg-mux.yaml

This was more a segue into a patch that does fix an instance of that I
just posted here:

https://lore.kernel.org/lkml/20230516184626.154892-2-afd@xxxxxx/

> > nor should
> > they force their parents to be one when they do not meet the above
> > two cases?
>
> reg-mux does not force the parent to be syscon. You are now mistaking it
> with mmio-mux, which apparently for our Linux implementation it expects
> parent to be syscon.

I might have been mistaking it with "ti,phy-gmii-sel" which does force
that and I am fixing here[0].

https://www.spinics.net/lists/kernel/msg4789373.html

> > > efuse is not syscon, because it is not writeable. efuse has entirely
> > > different purpose with its own defined purpose/type - efuse/OTP etc.
> >
> > That was just one example I found, I have not found a standard way
> > to describe down to the bit level in DT, only to the word/register.
> > Anything more granular needs non-standard ways of describing which
> > bits belong to which nodes/devices and using syscon to fetch the
> > common registers.
> >
> > > > > >
> > > > >
> > > > > ...
> > > > >
> > > > > > Ideally DT nodes all describe their register space in a "reg"
> > > > > > property and all the "large collection of devices" spaces become
> > > > > > "simple-bus" nodes. "syscon" nodes can then be limited to only the
> > > > > > rare case when multiple devices share bits in a single register.
> > > > > >
> > > > > > If Rob and Krzysztof agree I can send a patch with the above
> > > > > > guidance to the Devicetree Specification repo also.
> > > > >
> > > > > Agree on what?
> > > >
> > > > That we should provide the above guidance on when and how to use syscon
> > > > nodes. Right now it is a free for all and it is causing issues.
> > >
> > > Sure, providing more guidance seems good. We already provide guidance
> > > via review, but we can codify it more. Where? syscon.yaml? It's already
> > > describing everything needed to know...
> > >
> > > What particular problems do you see which need to be solved?
> >
> > My issue is the guidance is not clear, nor being followed. For instance
> > this is listed as a requirement:
> >
> > "The registers are not cohesive enough to represent as any specific type of device."
> >
> > Take "ti,j721e-system-controller" for instance, today this region is modeled
> > as a "syscon" node but it actually is a region of well defined register spaces
> > and devices. Like PHYs, clock controllers, and our even our pinmux controller.
>
> Then it should not be syscon. The platform maintainer should tell
> submitter: this is not syscon, please stop this nonsense.
>
> We do not have access to your datasheets and we do not have time to
> investigate each one of device, so we, DT maintainers, cannot really
> judge. Submitters want everything to be syscon because they can write
> code much faster and shove into kernel poor quality drivers which do not
> adhere to any design principles.

And that is the point of the guidance I'd like to add, it should be: "here are
the only correct uses for syscon", and every other use is just hacking around
making proper device nodes.

> > Most of these devices use the normal "reg" property to claim their registers and
> > so this space should be a "simple-bus" but we are forced to make it one big
> > "syscon" node because a couple devices in this area have a Linux driver that
> > requires their parent node to be a syscon node.
>
> I don't think it is requirement. You could have a device which has
> children, gives them regmap, but is not really syscon.

Sure, but at least currently making a node "syscon" is the easiest
way to have a parent get a regmap that can be fetched, so most do
that. I have some fixes for that too..

Andrew

> > That is the point of this patch, to relax that restriction in this driver.
> > It doesn't even change the binding, it only makes the driver match what
> > the binding allows.
>
> Hm, we might be talking about different topics, I don't know. I did not
> look at the driver as it does not fall into category of bindings at all
> and is fully ignored by my filters.
>
> Best regards,
> Krzysztof