Message-ID: <895c84f5-4a3c-fd2c-2b43-1a1996862e15@ti.com>
Date:   Tue, 16 May 2023 10:18:02 -0500
From:   Andrew Davis <afd@...com>
To:     Peter Rosin <peda@...ntia.se>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>
CC:     <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mux: mmio: use reg property when parent device is not a
 syscon

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@...00 {
	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@...00 {
	compatible = "syscon", "simple-mfd";
	reg = <0x10000 0x4000000>;

	some-device-inside@...00 {
		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@...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);

Powered by blists - more mailing lists