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

[Andrew Davis]

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.

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.

> 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 nor should
they force their parents to be one when they do not meet the above
two cases?

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

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.

Andrew

> Best regards,
> Krzysztof