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(+)

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,
 };
 
-- 
2.35.1

On Tue, Dec 6, 2022, at 08:39, Jeremy Kerr wrote:
> 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>

I see that this will only work after the device has been registered,
but not for early users of the syscon framework that bypass the
device logic and just call device_node_to_regmap() or 
syscon_regmap_lookup*() during early boot.

It should be possible to solve this by adding the reset logic
into the of_syscon_register() function and using the
of_reset_control_get*() helpers instead of the devm_* ones,
but I'm not sure if that causes other problems with probe
order, or if that helps at all, if reset drivers already
require the device subsystem to be running.

Philipp, what is the earliest point at which
reset_controller_register() can be called? Is that
possible before postcore_initcall() or driver_register()?

     Arnd

On Di, 2022-12-06 at 09:41 +0100, Arnd Bergmann wrote:
> On Tue, Dec 6, 2022, at 08:39, Jeremy Kerr wrote:
> > 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>
> 
> I see that this will only work after the device has been registered,
> but not for early users of the syscon framework that bypass the
> device logic and just call device_node_to_regmap() or 
> syscon_regmap_lookup*() during early boot.
> 
> It should be possible to solve this by adding the reset logic
> into the of_syscon_register() function and using the
> of_reset_control_get*() helpers instead of the devm_* ones,
> but I'm not sure if that causes other problems with probe
> order, or if that helps at all, if reset drivers already
> require the device subsystem to be running.
> 
> Philipp, what is the earliest point at which
> reset_controller_register() can be called? Is that
> possible before postcore_initcall() or driver_register()?

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.

However, reset controllers that choose to register early without
creating a platform device may run into issues with devlink inhibiting
reset consumers' probe [1].

[1] a1467faa1041 ("ARM: imx: register reset controller from a platform driver")
    https://lore.kernel.org/linux-arm-kernel/20211005100618.730907-1-p.zabel@pengutronix.de/

regards
Philipp

On Tue, Dec 6, 2022, at 10:25, Philipp Zabel wrote:
> On Di, 2022-12-06 at 09:41 +0100, Arnd Bergmann wrote:
>> Philipp, what is the earliest point at which
>> reset_controller_register() can be called? Is that
>> possible before postcore_initcall() or driver_register()?
>
> 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.

> However, reset controllers that choose to register early without
> creating a platform device may run into issues with devlink inhibiting
> reset consumers' probe [1].

Right. I think the is generally a problem of "early" code, so
device drivers should avoid doing this if at all possible, but at
the same time it makes sense for infrastructure to tolerate drivers
doing it.

     Arnd

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