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

[Andrew Davis <afd@ti.com>]

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@ti.com/

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