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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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.

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.

efuse is not syscon, because it is not writeable. efuse has entirely
different purpose with its own defined purpose/type - efuse/OTP etc.

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

Best regards,
Krzysztof