| Message ID | 20230605-ep93xx-v3-14-3d63a5f1103e@maquefel.me |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ep93xx device tree conversion | expand |
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;
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
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
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...
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.
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 --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);