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

[Krzysztof Kozlowski]

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?

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


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

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

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

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