diff mbox series

[v3,14/42] power: reset: Add a driver for the ep93xx reset

Message ID 20230605-ep93xx-v3-14-3d63a5f1103e@maquefel.me
State Changes Requested
Headers show
Series ep93xx device tree conversion | expand

Commit Message

Nikita Shubin via B4 Relay July 20, 2023, 11:29 a.m. UTC 
From: Nikita Shubin <nikita.shubin@maquefel.me>

Implement the reset behaviour of the various EP93xx SoCS in drivers/power/reset.

It used to be located in arch/arm/mach-ep93xx.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
Acked-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/power/reset/Kconfig          | 10 +++++
 drivers/power/reset/Makefile         |  1 +
 drivers/power/reset/ep93xx-restart.c | 86 ++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+)

Comments

Andy Shevchenko July 21, 2023, 4:37 p.m. UTC | #1 
On Thu, Jul 20, 2023 at 02:29:14PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Implement the reset behaviour of the various EP93xx SoCS in drivers/power/reset.
> 
> It used to be located in arch/arm/mach-ep93xx.

...

> +// SPDX-License-Identifier: (GPL-2.0)

Are you sure this is correct form? Have you checked your patches?

...

> +#include <linux/of_device.h>

Do you need this?
Or maybe you need another (of*.h) one?

...

> +	/* Issue the reboot */
> +	ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> +	ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);


> +	mdelay(1000);

Atomic?! Such a huge delay must be explained, esp. why it's atomic.

> +	pr_emerg("Unable to restart system\n");
> +	return NOTIFY_DONE;

...

> +	err = register_restart_handler(&priv->restart_handler);
> +	if (err)
> +		return dev_err_probe(dev, err, "can't register restart notifier\n");

> +	return err;

	return 0;
Alexander Sverdlin Nov. 11, 2023, 6:18 p.m. UTC | #2 
Hi Andy,

On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> > +       /* Issue the reboot */
> > +       ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> > +       ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
> 
> 
> > +       mdelay(1000);
> 
> Atomic?! Such a huge delay must be explained, esp. why it's atomic.

atomic or not, SoC is supposed to reset itself here.
However there is an errata [1] and the SoC can lockup instead.
So even pr_emerg() makes sense to me.

> > +       pr_emerg("Unable to restart system\n");
> > +       return NOTIFY_DONE;

[1] http://web.archive.org/web/20161130230727/http://www.cirrus.com/en/pubs/appNote/AN258REV2.pdf
Andy Shevchenko Nov. 13, 2023, 9:59 a.m. UTC | #3 
On Sat, Nov 11, 2023 at 8:18 PM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
> On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:

...

> > > +       mdelay(1000);
> >
> > Atomic?! Such a huge delay must be explained, esp. why it's atomic.
>
> atomic or not, SoC is supposed to reset itself here.
> However there is an errata [1] and the SoC can lockup instead.

Good, and what I'm saying is that this piece of code must have a
comment explaining this.

> So even pr_emerg() makes sense to me.

This is irrelevant to the comment.

> > > +       pr_emerg("Unable to restart system\n");
> > > +       return NOTIFY_DONE;
>
> [1] http://web.archive.org/web/20161130230727/http://www.cirrus.com/en/pubs/appNote/AN258REV2.pdf
Alexander Sverdlin Nov. 13, 2023, 10:04 a.m. UTC | #4 
Hi Andy!

On Mon, 2023-11-13 at 11:59 +0200, Andy Shevchenko wrote:
> On Sat, Nov 11, 2023 at 8:18 PM Alexander Sverdlin
> <alexander.sverdlin@gmail.com> wrote:
> > On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> 
> ...

[1]

> 
> > > > +       mdelay(1000);
> > > 
> > > Atomic?! Such a huge delay must be explained, esp. why it's atomic.
> > 
> > atomic or not, SoC is supposed to reset itself here.
> > However there is an errata [1] and the SoC can lockup instead.
> 
> Good, and what I'm saying is that this piece of code must have a
> comment explaining this.

And it has, but for some reason you've trimmed it in your reply...
Alexander Sverdlin Nov. 13, 2023, 10:07 a.m. UTC | #5 
Hi Andy!

On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> > +       /* Issue the reboot */
            ^^^^^^^^^^^^^^^^^^^^^^
This is the relevant comment, one can extend it, but looks already quite
informative considering EP93XX_SYSCON_DEVCFG_SWRST register name.

But Nikita would be able to include more verbose comment if
you'd have a suggestion.

> > +       ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> > +       ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
> 
> 
> > +       mdelay(1000);
> 
> Atomic?! Such a huge delay must be explained, esp. why it's atomic.
Andy Shevchenko Nov. 13, 2023, 10:22 a.m. UTC | #6 
On Mon, Nov 13, 2023 at 12:07 PM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
> On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> > > +       /* Issue the reboot */
>             ^^^^^^^^^^^^^^^^^^^^^^
> This is the relevant comment, one can extend it, but looks already quite
> informative considering EP93XX_SYSCON_DEVCFG_SWRST register name.

This does not explain the necessity of the mdelay() below.

> But Nikita would be able to include more verbose comment if
> you'd have a suggestion.

Please,add one.

> > > +       ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> > > +       ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
> >
> >
> > > +       mdelay(1000);
> >
> > Atomic?! Such a huge delay must be explained, esp. why it's atomic.
diff mbox series

Patch

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index fff07b2bd77b..9c2aa9218841 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -75,6 +75,16 @@  config POWER_RESET_BRCMSTB
 	  Say Y here if you have a Broadcom STB board and you wish
 	  to have restart support.
 
+config POWER_RESET_EP93XX
+	bool "Cirrus EP93XX reset driver" if COMPILE_TEST
+	depends on MFD_SYSCON
+	default ARCH_EP93XX
+	help
+	  This driver provides restart support for Cirrus EP93XX SoC.
+
+	  Say Y here if you have a Cirrus EP93XX SoC and you wish
+	  to have restart support.
+
 config POWER_RESET_GEMINI_POWEROFF
 	bool "Cortina Gemini power-off driver"
 	depends on ARCH_GEMINI || COMPILE_TEST
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index d763e6735ee3..61f4e11619b2 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -7,6 +7,7 @@  obj-$(CONFIG_POWER_RESET_ATC260X) += atc260x-poweroff.o
 obj-$(CONFIG_POWER_RESET_AXXIA) += axxia-reset.o
 obj-$(CONFIG_POWER_RESET_BRCMKONA) += brcm-kona-reset.o
 obj-$(CONFIG_POWER_RESET_BRCMSTB) += brcmstb-reboot.o
+obj-$(CONFIG_POWER_RESET_EP93XX) += ep93xx-restart.o
 obj-$(CONFIG_POWER_RESET_GEMINI_POWEROFF) += gemini-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
diff --git a/drivers/power/reset/ep93xx-restart.c b/drivers/power/reset/ep93xx-restart.c
new file mode 100644
index 000000000000..1f5be7de8719
--- /dev/null
+++ b/drivers/power/reset/ep93xx-restart.c
@@ -0,0 +1,86 @@ 
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Cirrus EP93xx SoC reset driver
+ *
+ * Copyright (C) 2021 Nikita Shubin <nikita.shubin@maquefel.me>
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/notifier.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+
+#include <linux/soc/cirrus/ep93xx.h>
+
+#define EP93XX_SYSCON_DEVCFG_SWRST	BIT(31)
+
+struct ep93xx_restart {
+	struct regmap *map;
+	struct notifier_block restart_handler;
+};
+
+static int ep93xx_restart_handle(struct notifier_block *this,
+				 unsigned long mode, void *cmd)
+{
+	struct ep93xx_restart *priv =
+		container_of(this, struct ep93xx_restart, restart_handler);
+
+	/* Issue the reboot */
+	ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
+	ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
+
+	mdelay(1000);
+
+	pr_emerg("Unable to restart system\n");
+	return NOTIFY_DONE;
+}
+
+static int ep93xx_reboot_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ep93xx_restart *priv;
+	struct device_node *parent;
+	int err;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	parent = of_get_parent(dev->of_node);
+	if (!parent)
+		return dev_err_probe(dev, -EINVAL, "no syscon parent for reboot node\n");
+
+	priv->map = syscon_node_to_regmap(parent);
+	of_node_put(parent);
+	if (IS_ERR(priv->map))
+		return dev_err_probe(dev, -EINVAL, "no syscon regmap\n");
+
+	priv->restart_handler.notifier_call = ep93xx_restart_handle;
+	priv->restart_handler.priority = 128;
+
+	err = register_restart_handler(&priv->restart_handler);
+	if (err)
+		return dev_err_probe(dev, err, "can't register restart notifier\n");
+
+	return err;
+}
+
+static const struct of_device_id ep93xx_reboot_of_match[] = {
+	{
+		.compatible = "cirrus,ep9301-reboot",
+	},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver ep93xx_reboot_driver = {
+	.probe = ep93xx_reboot_probe,
+	.driver = {
+		.name = "ep9301-reboot",
+		.of_match_table = ep93xx_reboot_of_match,
+	},
+};
+builtin_platform_driver(ep93xx_reboot_driver);
+MODULE_IMPORT_NS(EP93XX_SOC);