mux: mmio: use reg property when parent device is not a syscon

  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.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/mux/mmio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
  

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?

Cheers,
Peter

> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/mux/mmio.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
> index 44a7a0e885b8..42e00b9fd0a9 100644
> --- a/drivers/mux/mmio.c
> +++ b/drivers/mux/mmio.c
> @@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)
>  	int ret;
>  	int i;
>  
> -	if (of_device_is_compatible(np, "mmio-mux"))
> +	if (of_device_is_compatible(np, "mmio-mux")) {
>  		regmap = syscon_node_to_regmap(np->parent);
> -	else
> -		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
> +	} else {
> +		regmap = dev_get_regmap(dev->parent, NULL);
> +		if (!regmap)
> +			regmap = device_node_to_regmap(np) ?: ERR_PTR(-ENODEV);
> +	}
>  	if (IS_ERR(regmap)) {
>  		ret = PTR_ERR(regmap);
>  		dev_err(dev, "failed to get regmap: %d\n", ret);
  

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

Unfortunately I see lots of syscon anti-patterns in use today. First
type is mapping an entire multi-device space into one big syscon node,
then having all the device nodes live on the top level and simply
point into the syscon node. This does not follow the normal DT
structure and hides the true memory map.

large_collection_of_devices: syscon@10000 {
	compatible = "syscon", "simple-mfd";
	reg = <0x10000 0x4000000>;

	some-device-inside@12000 {
		reg = <0x12000 0x400>;
	};
};

some-device-outside {
	hacky_syscon_pointer = <&large_collection_of_devices 0x8000>;
};

An example of the inside-device would be "ti,am654-chipid", this
is a simple device, sometimes placed as a child of syscon node
and sometimes not. It does not use the syscon parent either
way, and so when placed as a child there ends up two mappings to
the same memory resources. Not a huge problem, but it could be
avoided by removing the overuse of syscon.

Another case is "ti,am654-phy-gmii-sel" which depends on the
parent being a syscon node, but uses the "reg" property to
only define the offset into that space. This isn't necessarily
wrong, but it does contrast with the devices that make their
own regmap, in which case one needs to have the parent syscon
node contain "ranges" to do the translation, this is the more
idiomatic way to use "reg" properies.

"ti,am64-epwm-tbclk" is yet another type of use. It marks
itself as a "syscon" node, just to make use of the helper
syscon_node_to_regmap(). Using it on itself to get a regmap
without all the otherwise required regmap setup (which I like
the idea of having all the info needed to setup a regmap for
a given node handled in DT, but again, inconsistent).

For more examples feel free to scan for "ti,keystone-dsp-gpio"
or  "pinctrl-single" in K3 devices for more syscon oddness.


So what causes this and what is my solution? I believe the
root cause is in devices that force the parent node to be
a syscon node. This then forces that devices' parent to
be a syscon node even when that doesn't make sense in that
given case. "mmio-mux" is one such driver. Luckily we have
"reg-mux" which is the same, but according to the binding,
does not need the parent to be a syscon node, perfect,
let's use that here, not "close the hole" in the binding :)

The issue is that it still does require the parent to be a
regmap provider, and that is what this patch fixes. With
this we can start the cleanup, in fact we already have
patches in-flight that require this patch[0].

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.

Andrew

[0] https://lore.kernel.org/lkml/20230513123313.11462-4-vaishnav.a@ti.com/T/

> Cheers,
> Peter
> 
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   drivers/mux/mmio.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
>> index 44a7a0e885b8..42e00b9fd0a9 100644
>> --- a/drivers/mux/mmio.c
>> +++ b/drivers/mux/mmio.c
>> @@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)
>>   	int ret;
>>   	int i;
>>   
>> -	if (of_device_is_compatible(np, "mmio-mux"))
>> +	if (of_device_is_compatible(np, "mmio-mux")) {
>>   		regmap = syscon_node_to_regmap(np->parent);
>> -	else
>> -		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
>> +	} else {
>> +		regmap = dev_get_regmap(dev->parent, NULL);
>> +		if (!regmap)
>> +			regmap = device_node_to_regmap(np) ?: ERR_PTR(-ENODEV);
>> +	}
>>   	if (IS_ERR(regmap)) {
>>   		ret = PTR_ERR(regmap);
>>   		dev_err(dev, "failed to get regmap: %d\n", ret);
  

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.

> 

...

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


Best regards,
Krzysztof
  

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.

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

Andrew

> 
> Best regards,
> Krzysztof
>
  

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
  

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
>
  

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
  

On 15/05/2023 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.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/mux/mmio.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
> index 44a7a0e885b8..42e00b9fd0a9 100644
> --- a/drivers/mux/mmio.c
> +++ b/drivers/mux/mmio.c
> @@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)
>  	int ret;
>  	int i;
>  
> -	if (of_device_is_compatible(np, "mmio-mux"))
> +	if (of_device_is_compatible(np, "mmio-mux")) {
>  		regmap = syscon_node_to_regmap(np->parent);
> -	else
> -		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
> +	} else {
> +		regmap = dev_get_regmap(dev->parent, NULL);
> +		if (!regmap)
> +			regmap = device_node_to_regmap(np) ?: ERR_PTR(-ENODEV);

Unless I miss something obvious, the original code is simply bogus and
wrong. I would like to give here Rb tag... but maybe I miss something
obvious. Why mux cannot be a device with MMIO itself? Binding allows it
which would be perfectly proper description of hardware.

Best regards,
Krzysztof
  

On 16/05/2023 20:37, Krzysztof Kozlowski wrote:
> On 15/05/2023 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.
>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>  drivers/mux/mmio.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
>> index 44a7a0e885b8..42e00b9fd0a9 100644
>> --- a/drivers/mux/mmio.c
>> +++ b/drivers/mux/mmio.c
>> @@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)
>>  	int ret;
>>  	int i;
>>  
>> -	if (of_device_is_compatible(np, "mmio-mux"))
>> +	if (of_device_is_compatible(np, "mmio-mux")) {
>>  		regmap = syscon_node_to_regmap(np->parent);
>> -	else
>> -		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
>> +	} else {
>> +		regmap = dev_get_regmap(dev->parent, NULL);
>> +		if (!regmap)
>> +			regmap = device_node_to_regmap(np) ?: ERR_PTR(-ENODEV);
> 
> Unless I miss something obvious, the original code is simply bogus and
> wrong. I would like to give here Rb tag... but maybe I miss something
> obvious. Why mux cannot be a device with MMIO itself? Binding allows it
> which would be perfectly proper description of hardware.

OK, I see now original binding and it did not allow 'reg' property, thus
binding was matching driver. Commit 9b358af7c818 ("dt-bindings: mux:
Convert mux controller bindings to schema") introduced reg, which is
reasonable.

The driver change is reasonable as well - we should not need syscon
parent... however, I think the code should be different. If reg is
present you should use it first. If reg is missing, use parent. This
solves the case when node with reg will be put inside something else
which has regmap, but should not be used for reg-mux.

Best regards,
Krzysztof
  

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
>
  

On 5/16/23 1:44 PM, Krzysztof Kozlowski wrote:
> On 16/05/2023 20:37, Krzysztof Kozlowski wrote:
>> On 15/05/2023 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.
>>>
>>> Signed-off-by: Andrew Davis <afd@ti.com>
>>> ---
>>>   drivers/mux/mmio.c | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
>>> index 44a7a0e885b8..42e00b9fd0a9 100644
>>> --- a/drivers/mux/mmio.c
>>> +++ b/drivers/mux/mmio.c
>>> @@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)
>>>   	int ret;
>>>   	int i;
>>>   
>>> -	if (of_device_is_compatible(np, "mmio-mux"))
>>> +	if (of_device_is_compatible(np, "mmio-mux")) {
>>>   		regmap = syscon_node_to_regmap(np->parent);
>>> -	else
>>> -		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
>>> +	} else {
>>> +		regmap = dev_get_regmap(dev->parent, NULL);
>>> +		if (!regmap)
>>> +			regmap = device_node_to_regmap(np) ?: ERR_PTR(-ENODEV);
>>
>> Unless I miss something obvious, the original code is simply bogus and
>> wrong. I would like to give here Rb tag... but maybe I miss something
>> obvious. Why mux cannot be a device with MMIO itself? Binding allows it
>> which would be perfectly proper description of hardware.
> 
> OK, I see now original binding and it did not allow 'reg' property, thus
> binding was matching driver. Commit 9b358af7c818 ("dt-bindings: mux:
> Convert mux controller bindings to schema") introduced reg, which is
> reasonable.
> 
> The driver change is reasonable as well - we should not need syscon
> parent... however, I think the code should be different. If reg is
> present you should use it first. If reg is missing, use parent. This
> solves the case when node with reg will be put inside something else
> which has regmap, but should not be used for reg-mux.
> 

Makes sense to me. I didn't do that to begin with to keep the
functionality the same, but I don't see any users of this with both
"reg" and a parent with regmap today. Will flip the logic for v2.

Thanks,
Andrew

> Best regards,
> Krzysztof
>