| Message ID | 20221206073916.1606125-3-jk@codeconstruct.com.au |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Arnd, Thanks for taking a look a this. Just a question about the early approach; I'm not too familiar with the internals of the syscon/regmap infrastructure: > > reset_controller_register() only initializes a few fields in the > > passed rcdev structure and adds it to a static list under a static > > mutex, so there's not much of a limit. > > Ok, in that case I think we should at least leave the option of > doing the reset from an early syscon as well. OK, sounds good - I'll add a direct of_reset_control_get_<variant>() in the early of_syscon_register path, which should work in a similar way to the clocks properties. However: this may conflict with the later platform_device syscon; if the late syscon tries to of_reset_control_get_exclusive() the same reset controller (because it's the same syscon node), that will (of course) fail. Hence a question about the syscon infrastructure: how are the late- and early- syscon registrations supposed to interact? Should I allow for there to be two syscons registered (one through of_syscon_register(), the other through the platform device probe), or do we expect that to never happen? In case of the former, I can just grab a shared handle to the reset controller instead, but I want to make sure that's the correct thing to do. Cheers, Jeremy
On Wed, Dec 7, 2022, at 08:56, Jeremy Kerr wrote: > Hi Arnd, > > Thanks for taking a look a this. Just a question about the early > approach; I'm not too familiar with the internals of the syscon/regmap > infrastructure: > >> > reset_controller_register() only initializes a few fields in the >> > passed rcdev structure and adds it to a static list under a static >> > mutex, so there's not much of a limit. >> >> Ok, in that case I think we should at least leave the option of >> doing the reset from an early syscon as well. > > OK, sounds good - I'll add a direct of_reset_control_get_<variant>() in > the early of_syscon_register path, which should work in a similar way to > the clocks properties. > > However: this may conflict with the later platform_device syscon; if the > late syscon tries to of_reset_control_get_exclusive() the same reset > controller (because it's the same syscon node), that will (of course) > fail. > > Hence a question about the syscon infrastructure: how are the late- and > early- syscon registrations supposed to interact? Should I allow for > there to be two syscons registered (one through of_syscon_register(), > the other through the platform device probe), or do we expect that to > never happen? > > In case of the former, I can just grab a shared handle to the reset > controller instead, but I want to make sure that's the correct thing to do. Hmm, it's clearly not doing what I was remembering it to do ;-) Before 2014 commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices"), it was supposed to be the same regmap in both cases, with the linked list being maintained to ensure we never get more than one instance for device_node. After this commit, I see that the platform_driver no longer matches syscon nodes from devicetree, but only those nodes that have platform_device.dev.name="syscon" and are created from board files. The only user of manually created syscon devices at the time was mach-clps711x, but that has been converted to DT a long time ago, so I don't even see anything using the platform_device at all. This would in turn indicate that we can completely remove the platform_driver code, but I don't see how your RFC patch then had any effect because it wouldn't actually perform the reset for any devices in mainline kernels. Arnd
Hi Arnd, > Hmm, it's clearly not doing what I was remembering it to do ;-) > > Before 2014 commit bdb0066df96e ("mfd: syscon: Decouple syscon > interface > from platform devices"), it was supposed to be the same regmap in > both cases, with the linked list being maintained to ensure we > never get more than one instance for device_node. Yep, that makes sense with your earlier suggestions. > After this commit, I see that the platform_driver no longer matches > syscon nodes from devicetree, but only those nodes that have > platform_device.dev.name="syscon" and are created from board > files. The only user of manually created syscon devices at the > time was mach-clps711x, but that has been converted to DT > a long time ago, so I don't even see anything using the > platform_device at all. > > This would in turn indicate that we can completely remove the > platform_driver code, but I don't see how your RFC patch then > had any effect because it wouldn't actually perform the > reset for any devices in mainline kernels. I've been changing a few things at once, it's entirely possible that my testing is incorrect! So, I'll add the reset controller linkage in just the DT-based code, and leave the platform device as-is. And then make sure that I'm getting the correct regmap <--> reset interactions :D Cheers, Jeremy
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index bdb2ce7ff03b..a0483dbfe17a 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -20,6 +20,7 @@ #include <linux/platform_data/syscon.h> #include <linux/platform_device.h> #include <linux/regmap.h> +#include <linux/reset.h> #include <linux/mfd/syscon.h> #include <linux/slab.h> @@ -31,6 +32,7 @@ static LIST_HEAD(syscon_list); struct syscon { struct device_node *np; struct regmap *regmap; + struct reset_control *reset; struct list_head list; }; @@ -280,6 +282,7 @@ static int syscon_probe(struct platform_device *pdev) struct regmap_config syscon_config = syscon_regmap_config; struct resource *res; void __iomem *base; + int rc; syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); if (!syscon) @@ -302,13 +305,32 @@ static int syscon_probe(struct platform_device *pdev) return PTR_ERR(syscon->regmap); } + syscon->reset = devm_reset_control_get_optional_exclusive(&pdev->dev, + "reset"); + if (IS_ERR(syscon->reset)) + return PTR_ERR(syscon->reset); + platform_set_drvdata(pdev, syscon); + rc = reset_control_deassert(syscon->reset); + if (rc) { + dev_err(dev, "failed to deassert reset line! rc: %d\n", rc); + return rc; + } + dev_dbg(dev, "regmap %pR registered\n", res); return 0; } +static int syscon_remove(struct platform_device *pdev) +{ + struct syscon *syscon = platform_get_drvdata(pdev); + + reset_control_assert(syscon->reset); + return 0; +} + static const struct platform_device_id syscon_ids[] = { { "syscon", }, { } @@ -319,6 +341,7 @@ static struct platform_driver syscon_driver = { .name = "syscon", }, .probe = syscon_probe, + .remove = syscon_remove, .id_table = syscon_ids, };
Simple syscon devices may require deassertion of a reset signal in order to access their register set. Rather than requiring a custom driver to implement this, we can use the generic "resets" specifiers to link a reset line to the syscon. This change adds an optional reset line to the syscon device description, and code to perform the deassertion/assertion on probe/remove. Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> --- drivers/mfd/syscon.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)