mux: mmio: use reg property when parent device is not a syscon
PATCH v3 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 reg property is provided, use that to identify the address space for this mux. If not provided fallback to using the parent device as a regmap provider.
---
Changes from v2:
- Rebased on v6.6-rc1
Changes from v1:
- Flip logic as suggested in v1[0]
[0] https://lore.kernel.org/lkml/[email protected]/T/
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 =device_node_to_regmap (np); +if (IS_ERR (regmap)) + regmap =dev_get_regmap (dev->parent,NULL ) ?: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 26 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 reg property is provided, use that to identify the address space for this mux. If not provided fallback to using the parent device as a regmap provider.
Signed-off-by Andrew Davis <[email protected]>Reviewed-by Nishanth Menon <[email protected]>---
Changes from v2:
- Rebased on v6.6-rc1Changes from v1:
- Flip logic as suggested in v1[0][0] https://lore.kernel.org/lkml/[email protected]/T/
drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +0
-0
drivers/mux/mmio.c static int mux_mmio_probe(struct platform_device *pdev) · line 50
Quoted 10 lines
drivers/mux/mmio.c +3
-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 =device_node_to_regmap (np);
I started digging in device_node_to_regmap() to try to find an error that could be used to trigger if the failover to dev_get_regmap() should be tried, instead of always doing the failover on error. I got lost fairly quickly, but it seems device_node_to_regmap() can return -EDEFER_PROBE. While I'm not certain that it is applicable, that case should probably not fall back to dev_get_regmap()...
Are there other error cases that should prevent the failover? I would guess that it's perhaps just a single error that should trigger trying the failover path? But I don't know, and which error if that's the case?
How much badness can be caused if syscon_node_to_regmap() fails for some random obscure reason and the failover path is taken inadvertently? It certainly smells bad for -EDEFER_PROBE, but do you have any insight in other cases?
And after getting to approx that point a while back, I had other things to take care of, and this fell off the table. Sorry!
Cheers,
Peter
Quoted 6 lines
drivers/mux/mmio.c +3
-0+if (IS_ERR (regmap)) + regmap =dev_get_regmap (dev->parent,NULL ) ?:ERR_PTR (-ENODEV ); + }if (IS_ERR (regmap)) { ret =PTR_ERR (regmap);dev_err (dev,"failed to get regmap: %d\n" , ret);
Quoted 47 lines · Peter Rosin
Hi!
Quoted 26 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 reg property is provided, use that to identify the address space for this mux. If not provided fallback to using the parent device as a regmap provider.
Signed-off-by Andrew Davis <[email protected]>Reviewed-by Nishanth Menon <[email protected]>---
Changes from v2:
- Rebased on v6.6-rc1Changes from v1:
- Flip logic as suggested in v1[0][0] https://lore.kernel.org/lkml/[email protected]/T/
drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +0
-0drivers/mux/mmio.c static int mux_mmio_probe(struct platform_device *pdev) · line 50
Quoted 10 lines
drivers/mux/mmio.c +3
-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 =device_node_to_regmap (np);I started digging in
device_node_to_regmap()to try to find an error that could be used to trigger if the failover todev_get_regmap()should be tried, instead of always doing the failover on error. I got lost fairly quickly, but it seemsdevice_node_to_regmap()can return -EDEFER_PROBE. While I'm not certain that it is applicable, that case should probably not fall back todev_get_regmap()...
Quoted 4 lines
Are there other error cases that should prevent the failover? I would guess that it's perhaps just a single error that should trigger trying the failover path? But I don't know, and which error if that's the case?
Ideally the only error that will be returned is ENOMEM, which happens when this node does not have a 'reg' property, and this is also the one case we want to do the failover. So all should be well.
Quoted 5 lines
How much badness can be caused if
syscon_node_to_regmap()fails for some random obscure reason and the failover path is taken inadvertently? It certainly smells bad for -EDEFER_PROBE, but do you have any insight in other cases?
If we take the failover inadvertently then we will check if the parent node is a syscon, if it is then our offset will most likely be wrong (parent will not match child 'reg').
Quoted 3 lines
And after getting to approx that point a while back, I had other things to take care of, and this fell off the table. Sorry!
No problem as long as we can find a way to get this in quickly (lot of DT warning need cleaned up based on this patch).
Thanks
Andrew
Quoted 9 lines
Cheers,
PeterQuoted 6 lines
drivers/mux/mmio.c +3
-0+if (IS_ERR (regmap)) + regmap =dev_get_regmap (dev->parent,NULL ) ?:ERR_PTR (-ENODEV ); + }if (IS_ERR (regmap)) { ret =PTR_ERR (regmap);dev_err (dev,"failed to get regmap: %d\n" , ret);
Hi!
Quoted 47 lines · Andrew Davis
Quoted 46 lines · Peter Rosin
Hi!
Quoted 26 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 reg property is provided, use that to identify the address space for this mux. If not provided fallback to using the parent device as a regmap provider.
Signed-off-by Andrew Davis <[email protected]>Reviewed-by Nishanth Menon <[email protected]>---
Changes from v2:
- Rebased on v6.6-rc1Changes from v1:
- Flip logic as suggested in v1[0][0] https://lore.kernel.org/lkml/[email protected]/T/
drivers/mux/mmio.c| 9 ++++++---1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +0
-0drivers/mux/mmio.c static int mux_mmio_probe(struct platform_device *pdev) · line 50
Quoted 9 lines
drivers/mux/mmio.c +3
-2@@ -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 = device_node_to_regmap(np);I started digging in
device_node_to_regmap()to try to find an error that could be used to trigger if the failover todev_get_regmap()should be tried, instead of always doing the failover on error. I got lost fairly quickly, but it seemsdevice_node_to_regmap()can return -EDEFER_PROBE. While I'm not certain that it is applicable, that case should probably not fall back todev_get_regmap()...
Quoted 8 lines
Quoted 4 lines
Are there other error cases that should prevent the failover? I would guess that it's perhaps just a single error that should trigger trying the failover path? But I don't know, and which error if that's the case?
Ideally the only error that will be returned is ENOMEM, which happens when this node does not have a 'reg' property, and this is also the one case we want to do the failover. So all should be well.
The ideal working case is usually not much of a problem. When I look at what device_node_to_regmap does, I find, appart from -ENOMEM, possibilities of -ENOENT (because no clock), and the clock may theoretically fail to prepare for numerous reasons hidden in clock drivers, but the clock core can trigger at least -EACCES and -EINPROGRESS via runtime PM.
And it definitely looks like the -EPROBE_DEFER case needs to be addressed. I.e., why is this call chain not a problem?
mux_mmio_probe
->device_node_to_regmap
-> device_node_get_regmap
-> of_syscon_register
-> of_hwspin_lock_get_id
<- -EPROBE_DEFER <- ERR_PTR(-EPROBE_DEFER)
<- ERR_PTR(-EPROBE_DEFER)
<- ERR_PTR(-EPROBE_DEFER)
As far as I can tell, if device_node_to_regmap() fails with -EPROBE_DEFER with your patch, then mux_mmio_probe() misbehaves. It should have aborted and failed with -EPROBE_DEFER, but instead throws that error away and goes on to try dev_get_regmap(). That, in turn, is probably futile and will likely error out in some way, breaking a system that might have been ok, if the probe had been retried some time later.
As long as the above is not sufficiently explained away, or fixed, I consider the patch broken.
Quoted 10 lines
Quoted 5 lines
How much badness can be caused if
syscon_node_to_regmap()fails for some random obscure reason and the failover path is taken inadvertently? It certainly smells bad for -EDEFER_PROBE, but do you have any insight in other cases?If we take the failover inadvertently then we will check if the parent node is a syscon, if it is then our offset will most likely be wrong (parent will not match child 'reg').
Quoted 6 lines
Quoted 3 lines
And after getting to approx that point a while back, I had other things to take care of, and this fell off the table. Sorry!
No problem as long as we can find a way to get this in quickly (lot of DT warning need cleaned up based on this patch).
Hold your horses, I need the above explanation first (and perhaps an updated patch).
Cheers,
Peter
Quoted 12 lines
Thanks
AndrewQuoted 9 lines
Cheers,
PeterQuoted 6 lines
drivers/mux/mmio.c +3
-0+ if (IS_ERR(regmap)) + regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV); + } if (IS_ERR(regmap)) { ret = PTR_ERR(regmap); dev_err(dev, "failed to get regmap: %d\n", ret);
Quoted 78 lines · Peter Rosin
Hi!
Quoted 47 lines · Andrew Davis
Quoted 46 lines · Peter Rosin
Hi!
Quoted 26 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 reg property is provided, use that to identify the address space for this mux. If not provided fallback to using the parent device as a regmap provider.
Signed-off-by Andrew Davis <[email protected]>Reviewed-by Nishanth Menon <[email protected]>---
Changes from v2:
- Rebased on v6.6-rc1Changes from v1:
- Flip logic as suggested in v1[0][0] https://lore.kernel.org/lkml/[email protected]/T/
drivers/mux/mmio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/mux/mmio.c +0
-0drivers/mux/mmio.c static int mux_mmio_probe(struct platform_device *pdev) · line 50
Quoted 9 lines
drivers/mux/mmio.c +3
-2@@ -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 = device_node_to_regmap(np);I started digging in
device_node_to_regmap()to try to find an error that could be used to trigger if the failover todev_get_regmap()should be tried, instead of always doing the failover on error. I got lost fairly quickly, but it seemsdevice_node_to_regmap()can return -EDEFER_PROBE. While I'm not certain that it is applicable, that case should probably not fall back todev_get_regmap()...Quoted 8 lines
Quoted 4 lines
Are there other error cases that should prevent the failover? I would guess that it's perhaps just a single error that should trigger trying the failover path? But I don't know, and which error if that's the case?
Ideally the only error that will be returned is ENOMEM, which happens when this node does not have a 'reg' property, and this is also the one case we want to do the failover. So all should be well.
The ideal working case is usually not much of a problem. When I look at what
device_node_to_regmapdoes, I find, appart from-ENOMEM, possibilities of-ENOENT(because no clock), and the clock may theoretically fail to prepare for numerous reasons hidden in clock drivers, but the clock core can trigger at least-EACCESand-EINPROGRESSvia runtime PM.And it definitely looks like the -EPROBE_DEFER case needs to be addressed. I.e., why is this call chain not a problem?
mux_mmio_probe
->device_node_to_regmap
->device_node_get_regmap-> of_syscon_register -> of_hwspin_lock_get_id <- -EPROBE_DEFER <- ERR_PTR(-EPROBE_DEFER)<-
ERR_PTR(-EPROBE_DEFER)
<-ERR_PTR(-EPROBE_DEFER)
Quoted 7 lines
As far as I can tell, if
device_node_to_regmap()fails with -EPROBE_DEFER with your patch, thenmux_mmio_probe()misbehaves. It should have aborted and failed with -EPROBE_DEFER, but instead throws that error away and goes on to trydev_get_regmap(). That, in turn, is probably futile and will likely error out in some way, breaking a system that might have been ok, if the probe had been retried some time later.
This is why I liked the v1 version, dev_get_regmap() just returns a simple NULL on error, no complex EPROBE_DEFER oddness :)
So is EPROBE_DEFER the only one we think should retry and not go down the fallback path? I believe that is the normal assumption for most drivers.
Quoted 17 lines
As long as the above is not sufficiently explained away, or fixed, I consider the patch broken.
Quoted 14 lines
Quoted 5 lines
How much badness can be caused if
syscon_node_to_regmap()fails for some random obscure reason and the failover path is taken inadvertently? It certainly smells bad for -EDEFER_PROBE, but do you have any insight in other cases?If we take the failover inadvertently then we will check if the parent node is a syscon, if it is then our offset will most likely be wrong (parent will not match child 'reg').
Quoted 3 lines
And after getting to approx that point a while back, I had other things to take care of, and this fell off the table. Sorry!
Quoted 6 lines
Quoted 2 lines
No problem as long as we can find a way to get this in quickly (lot of DT warning need cleaned up based on this patch).
Hold your horses, I need the above explanation first (and perhaps an updated patch).
I'm not normally so impatient but this went two whole kernel cycles without any comment until rc6.. v4 on the way.
Andrew
Quoted 15 lines
Cheers,
PeterQuoted 12 lines
Thanks
AndrewQuoted 9 lines
Cheers,
PeterQuoted 6 lines
drivers/mux/mmio.c +3
-0+ if (IS_ERR(regmap)) + regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV); + } if (IS_ERR(regmap)) { ret = PTR_ERR(regmap); dev_err(dev, "failed to get regmap: %d\n", ret);