mux: mmio: use reg property when parent device is not a syscon
PATCH 1/1 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.
--- drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/mux/mmio.c +6 -3
@@ -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);
Signature · 1 line
2.39.2
Hi!
Quoted 5 lines · Andrew Davis
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
Quoted 26 lines
Signed-off-by Andrew Davis <[email protected]>--- drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +6
-3@@ -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);
Quoted 9 lines · Peter Rosin
Hi!
Quoted 5 lines · Andrew Davis
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.
Quoted 8 lines
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/[email protected]/T/
Quoted 29 lines
Cheers,
PeterQuoted 26 lines
Signed-off-by Andrew Davis <[email protected]>--- drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +6
-3@@ -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);
Quoted 19 lines · Andrew Davis
Quoted 9 lines · Peter Rosin
Hi!
Quoted 5 lines · Andrew Davis
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.
Quoted 8 lines
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?
Quoted 6 lines
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?
Quoted 12 lines
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>; };
Quoted 5 lines
consumer@2 { syscon-efuse = <&common_reg 0x2>; };Something like that, then regmap will take care of synchronizing access.
Syscon is not for this.
Quoted 1 line
...
Quoted 8 lines
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
Quoted 41 lines · Krzysztof Kozlowski
Quoted 19 lines · Andrew Davis
Quoted 9 lines · Peter Rosin
Hi!
Quoted 5 lines · Andrew Davis
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.
Quoted 8 lines
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?
Quoted 6 lines
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?
Quoted 12 lines
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>; };
Quoted 8 lines
Quoted 5 lines
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.
Quoted 10 lines
Quoted 1 line
...
Quoted 6 lines
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.
Quoted 5 lines
Quoted 2 lines
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
Quoted 4 lines
Best regards,
Krzysztof
Quoted 46 lines · Andrew Davis
Quoted 45 lines · Krzysztof Kozlowski
Quoted 19 lines · Andrew Davis
Quoted 9 lines · Peter Rosin
Hi!
Quoted 5 lines · Andrew Davis
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.
Quoted 8 lines
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?
Quoted 6 lines
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?
Quoted 16 lines
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>; };
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
...
Quoted 6 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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
Quoted 49 lines · Krzysztof Kozlowski
Quoted 48 lines · Andrew Davis
Quoted 47 lines · Krzysztof Kozlowski
Quoted 19 lines · Andrew Davis
Quoted 9 lines · Peter Rosin
Hi!
Quoted 5 lines · Andrew Davis
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.
Quoted 8 lines
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?
Quoted 6 lines
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?
Quoted 17 lines
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.
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 4 lines
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?
Quoted 3 lines
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.
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 2 lines
...
Quoted 8 lines
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?
Quoted 9 lines
Quoted 2 lines
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
Quoted 3 lines
Best regards,
Krzysztof
Quoted 56 lines · Andrew Davis
Quoted 55 lines · Krzysztof Kozlowski
Quoted 53 lines · Andrew Davis
Quoted 41 lines · Krzysztof Kozlowski
Quoted 19 lines · Andrew Davis
Quoted 9 lines · Peter Rosin
Hi!
Quoted 5 lines · Andrew Davis
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.
Quoted 8 lines
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?
Quoted 6 lines
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?
Quoted 12 lines
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>; };Quoted 8 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 4 lines
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?
Quoted 8 lines
Quoted 5 lines
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
Quoted 3 lines
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.
Quoted 37 lines
Quoted 3 lines
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.
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 2 lines
...
Quoted 8 lines
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?
Quoted 9 lines
Quoted 2 lines
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?
Quoted 8 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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
Quoted 65 lines · Krzysztof Kozlowski
Quoted 56 lines · Andrew Davis
Quoted 55 lines · Krzysztof Kozlowski
Quoted 53 lines · Andrew Davis
Quoted 41 lines · Krzysztof Kozlowski
Quoted 19 lines · Andrew Davis
Quoted 9 lines · Peter Rosin
Hi!
Quoted 5 lines · Andrew Davis
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.
Quoted 8 lines
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?
Quoted 6 lines
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?
Quoted 12 lines
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>; };Quoted 8 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 9 lines
Yes and both are discouraged.
Quoted 4 lines
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".
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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.
Quoted 6 lines
Quoted 2 lines
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/[email protected]/
Quoted 9 lines
Quoted 4 lines
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
Quoted 46 lines
Quoted 37 lines
Quoted 3 lines
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.
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 2 lines
...
Quoted 8 lines
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?
Quoted 9 lines
Quoted 2 lines
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?
Quoted 8 lines
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.
Quoted 9 lines
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.
Quoted 8 lines
Quoted 4 lines
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
Quoted 12 lines
Quoted 4 lines
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
Quoted 18 lines · Andrew Davis
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 <[email protected]>--- drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +0
-0@@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)int ret;int i;
drivers/mux/mmio.c static int mux_mmio_probe(struct platform_device *pdev) · line 52
Quoted 10 lines
drivers/mux/mmio.c +5
-3-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
Quoted 20 lines · Krzysztof Kozlowski
Quoted 19 lines · Andrew Davis
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 <[email protected]>--- drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +0
-0@@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)int ret;int i;
Quoted 14 lines
drivers/mux/mmio.c static int mux_mmio_probe(struct platform_device *pdev) · line 52
Quoted 9 lines
drivers/mux/mmio.c +5
-3-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
Quoted 41 lines · Krzysztof Kozlowski
Quoted 20 lines · Krzysztof Kozlowski
Quoted 19 lines · Andrew Davis
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 <[email protected]>--- drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +0
-0@@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)int ret;int i;Quoted 14 lines
drivers/mux/mmio.c static int mux_mmio_probe(struct platform_device *pdev) · line 52
Quoted 9 lines
drivers/mux/mmio.c +5
-3-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.
Quoted 6 lines
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
Quoted 3 lines
Best regards,
Krzysztof