Signed-off-by: Steen Hegelund <[email protected]>
---
drivers/reset/Kconfig | 8 ++
drivers/reset/Makefile | 1 +
drivers/reset/reset-microchip-sparx5.c | 145 +++++++++++++++++++++++++
3 files changed, 154 insertions(+)
create mode 100644 drivers/reset/reset-microchip-sparx5.c
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 71ab75a46491..05c240c47a8a 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -101,6 +101,14 @@ config RESET_LPC18XX
help
This enables the reset controller driver for NXP LPC18xx/43xx SoCs.
+config RESET_MCHP_SPARX5
+ bool "Microchip Sparx5 reset driver"
+ depends on HAS_IOMEM || COMPILE_TEST
+ default y if SPARX5_SWITCH
+ select MFD_SYSCON
+ help
+ This driver supports switch core reset for the Microchip Sparx5 SoC.
+
config RESET_MESON
tristate "Meson Reset Driver"
depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 1054123fd187..341fd9ab4bf6 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
+obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
obj-$(CONFIG_RESET_MESON) += reset-meson.o
obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
new file mode 100644
index 000000000000..bb636ebd22d2
--- /dev/null
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Microchip Sparx5 Switch Reset driver
+ *
+ * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries.
+ *
+ * The Sparx5 Chip Register Model can be browsed at this location:
+ * https://github.com/microchip-ung/sparx-5_reginfo
+ */
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/notifier.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+#define PROTECT_REG 0x84
+#define PROTECT_BIT BIT(10)
+#define SOFT_RESET_REG 0x08
+#define SOFT_RESET_BIT BIT(1)
+
+struct mchp_reset_context {
+ struct device *dev;
+ struct regmap *cpu_ctrl;
+ struct regmap *gcb_ctrl;
+ struct reset_controller_dev reset_ctrl;
+};
+
+static u32 sparx5_read_soft_rst(struct mchp_reset_context *ctx)
+{
+ u32 val;
+
+ regmap_read(ctx->gcb_ctrl, SOFT_RESET_REG, &val);
+ return val;
+}
+
+static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ struct mchp_reset_context *ctx =
+ container_of(rcdev, struct mchp_reset_context, reset_ctrl);
+ u32 val;
+
+ /* Make sure the core is PROTECTED from reset */
+ regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT, PROTECT_BIT);
+
+ dev_info(ctx->dev, "soft reset of switchcore\n");
+
+ /* Start soft reset */
+ regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
+
+ /* Wait for soft reset done */
+ return read_poll_timeout(sparx5_read_soft_rst, val,
+ (val & SOFT_RESET_BIT) == 0,
+ 1, 100, false,
+ ctx);
+}
+
+static const struct reset_control_ops sparx5_reset_ops = {
+ .reset = sparx5_switch_reset,
+};
+
+static int mchp_sparx5_reset_config(struct platform_device *pdev,
+ struct mchp_reset_context *ctx)
+{
+ struct device_node *dn = pdev->dev.of_node;
+ struct regmap *cpu_ctrl, *gcb_ctrl;
+ struct device_node *syscon_np;
+ int err;
+
+ syscon_np = of_parse_phandle(dn, "syscons", 0);
+ if (!syscon_np)
+ return -ENODEV;
+ cpu_ctrl = syscon_node_to_regmap(syscon_np);
+ if (IS_ERR(cpu_ctrl))
+ goto err_cpu;
+ of_node_put(syscon_np);
+
+ syscon_np = of_parse_phandle(dn, "syscons", 1);
+ if (!syscon_np)
+ return -ENODEV;
+ gcb_ctrl = syscon_node_to_regmap(syscon_np);
+ if (IS_ERR(gcb_ctrl))
+ goto err_gcb;
+ of_node_put(syscon_np);
+
+ ctx->cpu_ctrl = cpu_ctrl;
+ ctx->gcb_ctrl = gcb_ctrl;
+
+ ctx->reset_ctrl.owner = THIS_MODULE;
+ ctx->reset_ctrl.nr_resets = 1;
+ ctx->reset_ctrl.ops = &sparx5_reset_ops;
+ ctx->reset_ctrl.of_node = dn;
+
+ err = devm_reset_controller_register(&pdev->dev, &ctx->reset_ctrl);
+ if (err)
+ dev_err(&pdev->dev, "could not register reset controller\n");
+ pr_info("%s:%d\n", __func__, __LINE__);
+ return err;
+err_cpu:
+ of_node_put(syscon_np);
+ dev_err(&pdev->dev, "No cpu syscon map\n");
+ return PTR_ERR(cpu_ctrl);
+err_gcb:
+ of_node_put(syscon_np);
+ dev_err(&pdev->dev, "No gcb syscon map\n");
+ return PTR_ERR(gcb_ctrl);
+}
+
+static int mchp_sparx5_reset_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mchp_reset_context *ctx;
+
+ pr_info("%s:%d\n", __func__, __LINE__);
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+ ctx->dev = dev;
+ return mchp_sparx5_reset_config(pdev, ctx);
+}
+
+static const struct of_device_id mchp_sparx5_reset_of_match[] = {
+ {
+ .compatible = "microchip,sparx5-switch-reset",
+ },
+ { /*sentinel*/ }
+};
+
+static struct platform_driver mchp_sparx5_reset_driver = {
+ .probe = mchp_sparx5_reset_probe,
+ .driver = {
+ .name = "sparx5-switch-reset",
+ .of_match_table = mchp_sparx5_reset_of_match,
+ },
+};
+
+static int __init mchp_sparx5_reset_init(void)
+{
+ return platform_driver_register(&mchp_sparx5_reset_driver);
+}
+
+postcore_initcall(mchp_sparx5_reset_init);
--
2.29.2
> +static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct mchp_reset_context *ctx =
> + container_of(rcdev, struct mchp_reset_context, reset_ctrl);
> + u32 val;
> +
> + /* Make sure the core is PROTECTED from reset */
> + regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT, PROTECT_BIT);
> +
> + dev_info(ctx->dev, "soft reset of switchcore\n");
dev_dbg()?
> +
> + /* Start soft reset */
> + regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
> +
> + /* Wait for soft reset done */
> + return read_poll_timeout(sparx5_read_soft_rst, val,
> + (val & SOFT_RESET_BIT) == 0,
> + 1, 100, false,
> + ctx);
> +}
> +static int mchp_sparx5_reset_config(struct platform_device *pdev,
> + struct mchp_reset_context *ctx)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct regmap *cpu_ctrl, *gcb_ctrl;
> + struct device_node *syscon_np;
> + int err;
> +
> + syscon_np = of_parse_phandle(dn, "syscons", 0);
> + if (!syscon_np)
> + return -ENODEV;
> + cpu_ctrl = syscon_node_to_regmap(syscon_np);
> + if (IS_ERR(cpu_ctrl))
> + goto err_cpu;
> + of_node_put(syscon_np);
> +
> + syscon_np = of_parse_phandle(dn, "syscons", 1);
> + if (!syscon_np)
> + return -ENODEV;
> + gcb_ctrl = syscon_node_to_regmap(syscon_np);
> + if (IS_ERR(gcb_ctrl))
> + goto err_gcb;
> + of_node_put(syscon_np);
> +
> + ctx->cpu_ctrl = cpu_ctrl;
> + ctx->gcb_ctrl = gcb_ctrl;
> +
> + ctx->reset_ctrl.owner = THIS_MODULE;
> + ctx->reset_ctrl.nr_resets = 1;
> + ctx->reset_ctrl.ops = &sparx5_reset_ops;
> + ctx->reset_ctrl.of_node = dn;
> +
> + err = devm_reset_controller_register(&pdev->dev, &ctx->reset_ctrl);
> + if (err)
> + dev_err(&pdev->dev, "could not register reset controller\n");
> + pr_info("%s:%d\n", __func__, __LINE__);
> + return err;
> +err_cpu:
> + of_node_put(syscon_np);
> + dev_err(&pdev->dev, "No cpu syscon map\n");
> + return PTR_ERR(cpu_ctrl);
> +err_gcb:
> + of_node_put(syscon_np);
> + dev_err(&pdev->dev, "No gcb syscon map\n");
> + return PTR_ERR(gcb_ctrl);
It would be normal to put the dev_err() before the goto, set err =
PTR_ERR() and then goto out;
> +}
> +
> +static int mchp_sparx5_reset_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mchp_reset_context *ctx;
> +
> + pr_info("%s:%d\n", __func__, __LINE__);
More left over debug.
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> + ctx->dev = dev;
> + return mchp_sparx5_reset_config(pdev, ctx);
> +}
> +
> +static const struct of_device_id mchp_sparx5_reset_of_match[] = {
> + {
> + .compatible = "microchip,sparx5-switch-reset",
> + },
> + { /*sentinel*/ }
> +};
> +static int __init mchp_sparx5_reset_init(void)
> +{
> + return platform_driver_register(&mchp_sparx5_reset_driver);
> +}
> +
> +postcore_initcall(mchp_sparx5_reset_init);
Does it actually need to be postcore? The users of the reset should
look for -EPROBE_DEFER and try again later. And this then becomes just
a normal driver.
Andrew
On Thu, 2021-01-14 at 00:23 +0100, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> > +static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct mchp_reset_context *ctx =
> > + container_of(rcdev, struct mchp_reset_context,
> > reset_ctrl);
> > + u32 val;
> > +
> > + /* Make sure the core is PROTECTED from reset */
> > + regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT,
> > PROTECT_BIT);
> > +
> > + dev_info(ctx->dev, "soft reset of switchcore\n");
>
> dev_dbg()?
I will remove that.
>
> > +
> > + /* Start soft reset */
> > + regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
> > +
> > + /* Wait for soft reset done */
> > + return read_poll_timeout(sparx5_read_soft_rst, val,
> > + (val & SOFT_RESET_BIT) == 0,
> > + 1, 100, false,
> > + ctx);
> > +}
>
> > +static int mchp_sparx5_reset_config(struct platform_device *pdev,
> > + struct mchp_reset_context *ctx)
> > +{
> > + struct device_node *dn = pdev->dev.of_node;
> > + struct regmap *cpu_ctrl, *gcb_ctrl;
> > + struct device_node *syscon_np;
> > + int err;
> > +
> > + syscon_np = of_parse_phandle(dn, "syscons", 0);
> > + if (!syscon_np)
> > + return -ENODEV;
> > + cpu_ctrl = syscon_node_to_regmap(syscon_np);
> > + if (IS_ERR(cpu_ctrl))
> > + goto err_cpu;
> > + of_node_put(syscon_np);
> > +
> > + syscon_np = of_parse_phandle(dn, "syscons", 1);
> > + if (!syscon_np)
> > + return -ENODEV;
> > + gcb_ctrl = syscon_node_to_regmap(syscon_np);
> > + if (IS_ERR(gcb_ctrl))
> > + goto err_gcb;
> > + of_node_put(syscon_np);
> > +
> > + ctx->cpu_ctrl = cpu_ctrl;
> > + ctx->gcb_ctrl = gcb_ctrl;
> > +
> > + ctx->reset_ctrl.owner = THIS_MODULE;
> > + ctx->reset_ctrl.nr_resets = 1;
> > + ctx->reset_ctrl.ops = &sparx5_reset_ops;
> > + ctx->reset_ctrl.of_node = dn;
> > +
> > + err = devm_reset_controller_register(&pdev->dev, &ctx-
> > >reset_ctrl);
> > + if (err)
> > + dev_err(&pdev->dev, "could not register reset
> > controller\n");
> > + pr_info("%s:%d\n", __func__, __LINE__);
> > + return err;
> > +err_cpu:
> > + of_node_put(syscon_np);
> > + dev_err(&pdev->dev, "No cpu syscon map\n");
> > + return PTR_ERR(cpu_ctrl);
> > +err_gcb:
> > + of_node_put(syscon_np);
> > + dev_err(&pdev->dev, "No gcb syscon map\n");
> > + return PTR_ERR(gcb_ctrl);
>
> It would be normal to put the dev_err() before the goto, set err =
> PTR_ERR() and then goto out;
OK. I will change that.
>
>
> > +}
> > +
> > +static int mchp_sparx5_reset_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct mchp_reset_context *ctx;
> > +
> > + pr_info("%s:%d\n", __func__, __LINE__);
>
> More left over debug.
Yes. That will have to go.
>
> > + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > + ctx->dev = dev;
> > + return mchp_sparx5_reset_config(pdev, ctx);
> > +}
> > +
> > +static const struct of_device_id mchp_sparx5_reset_of_match[] = {
> > + {
> > + .compatible = "microchip,sparx5-switch-reset",
> > + },
> > + { /*sentinel*/ }
> > +};
>
> > +static int __init mchp_sparx5_reset_init(void)
> > +{
> > + return platform_driver_register(&mchp_sparx5_reset_driver);
> > +}
> > +
> > +postcore_initcall(mchp_sparx5_reset_init);
>
> Does it actually need to be postcore? The users of the reset should
> look for -EPROBE_DEFER and try again later. And this then becomes
> just
> a normal driver.
I tried using that, but the SGPIO driver bailed out after 3 DEFER
attempts, so that is why I changed it to use the postcore_initcall.
Maybe it is because the SGPIO driver is a builtin_platform_driver?
>
> Andrew
Hi Steen,
thank you for the patch. In addition to Andrew's comments, I have a few
more below:
On Wed, 2021-01-13 at 21:19 +0100, Steen Hegelund wrote:
> Signed-off-by: Steen Hegelund <[email protected]>
> ---
> drivers/reset/Kconfig | 8 ++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-microchip-sparx5.c | 145 +++++++++++++++++++++++++
> 3 files changed, 154 insertions(+)
> create mode 100644 drivers/reset/reset-microchip-sparx5.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 71ab75a46491..05c240c47a8a 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -101,6 +101,14 @@ config RESET_LPC18XX
> help
> This enables the reset controller driver for NXP LPC18xx/43xx SoCs.
>
> +config RESET_MCHP_SPARX5
> + bool "Microchip Sparx5 reset driver"
> + depends on HAS_IOMEM || COMPILE_TEST
> + default y if SPARX5_SWITCH
> + select MFD_SYSCON
> + help
> + This driver supports switch core reset for the Microchip Sparx5 SoC.
> +
> config RESET_MESON
> tristate "Meson Reset Driver"
> depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 1054123fd187..341fd9ab4bf6 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> +obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
> obj-$(CONFIG_RESET_MESON) += reset-meson.o
> obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
> obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
> diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
> new file mode 100644
> index 000000000000..bb636ebd22d2
> --- /dev/null
> +++ b/drivers/reset/reset-microchip-sparx5.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Microchip Sparx5 Switch Reset driver
> + *
> + * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries.
> + *
> + * The Sparx5 Chip Register Model can be browsed at this location:
> + * https://github.com/microchip-ung/sparx-5_reginfo
> + */
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/notifier.h>
Please drop all unused headers.
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#define PROTECT_REG 0x84
> +#define PROTECT_BIT BIT(10)
> +#define SOFT_RESET_REG 0x08
> +#define SOFT_RESET_BIT BIT(1)
> +
> +struct mchp_reset_context {
> + struct device *dev;
> + struct regmap *cpu_ctrl;
> + struct regmap *gcb_ctrl;
> + struct reset_controller_dev reset_ctrl;
For consistency, I'd like this to be called rcdev, or something else
that doesn't sound like this should be a struct reset_control.
> +};
> +
> +static u32 sparx5_read_soft_rst(struct mchp_reset_context *ctx)
> +{
> + u32 val;
> +
> + regmap_read(ctx->gcb_ctrl, SOFT_RESET_REG, &val);
> + return val;
> +}
This can be dropped if you use regmap_read_poll_timeout() below.
> +static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct mchp_reset_context *ctx =
> + container_of(rcdev, struct mchp_reset_context, reset_ctrl);
> + u32 val;
> +
> + /* Make sure the core is PROTECTED from reset */
> + regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT, PROTECT_BIT);
> +
> + dev_info(ctx->dev, "soft reset of switchcore\n");
> +
> + /* Start soft reset */
> + regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
> +
> + /* Wait for soft reset done */
> + return read_poll_timeout(sparx5_read_soft_rst, val,
> + (val & SOFT_RESET_BIT) == 0,
> + 1, 100, false,
> + ctx);
This looks like you could use regmap_read_poll_timeout() here.
> +}
> +
> +static const struct reset_control_ops sparx5_reset_ops = {
> + .reset = sparx5_switch_reset,
> +};
> +
> +static int mchp_sparx5_reset_config(struct platform_device *pdev,
> + struct mchp_reset_context *ctx)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct regmap *cpu_ctrl, *gcb_ctrl;
> + struct device_node *syscon_np;
> + int err;
> +
> + syscon_np = of_parse_phandle(dn, "syscons", 0);
> + if (!syscon_np)
> + return -ENODEV;
> + cpu_ctrl = syscon_node_to_regmap(syscon_np);
> + if (IS_ERR(cpu_ctrl))
> + goto err_cpu;
> + of_node_put(syscon_np);
If you move the of_node_put() up before the IS_ERR() check, you don't
have to repeat it at the err_cpu: label. In fact, if you also move the
error message up here, you can return here and drop the label.
> +
> + syscon_np = of_parse_phandle(dn, "syscons", 1);
> + if (!syscon_np)
> + return -ENODEV;
> + gcb_ctrl = syscon_node_to_regmap(syscon_np);
> + if (IS_ERR(gcb_ctrl))
> + goto err_gcb;
> + of_node_put(syscon_np);
Same as above.
> +
> + ctx->cpu_ctrl = cpu_ctrl;
> + ctx->gcb_ctrl = gcb_ctrl;
> +
> + ctx->reset_ctrl.owner = THIS_MODULE;
> + ctx->reset_ctrl.nr_resets = 1;
> + ctx->reset_ctrl.ops = &sparx5_reset_ops;
> + ctx->reset_ctrl.of_node = dn;
> +
> + err = devm_reset_controller_register(&pdev->dev, &ctx->reset_ctrl);
> + if (err)
> + dev_err(&pdev->dev, "could not register reset controller\n");
> + pr_info("%s:%d\n", __func__, __LINE__);
> + return err;
The only reason devm_reset_controller_register() can fail
unexpectedly is -ENOMEM. I think it would be fine to just
return devm_reset_controller_regster() here.
> +err_cpu:
> + of_node_put(syscon_np);
> + dev_err(&pdev->dev, "No cpu syscon map\n");
> + return PTR_ERR(cpu_ctrl);
> +err_gcb:
> + of_node_put(syscon_np);
> + dev_err(&pdev->dev, "No gcb syscon map\n");
> + return PTR_ERR(gcb_ctrl);
> +}
> +
> +static int mchp_sparx5_reset_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mchp_reset_context *ctx;
> +
> + pr_info("%s:%d\n", __func__, __LINE__);
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> + ctx->dev = dev;
> + return mchp_sparx5_reset_config(pdev, ctx);
> +}
You could fold the contents of mchp_sparx5_reset_config() into
mchp_sparx5_reset_probe() and replace all &pdev->dev with dev.
regards
Philipp
Hi Philipp,
On Thu, 2021-01-14 at 10:39 +0100, Philipp Zabel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Hi Steen,
>
> thank you for the patch. In addition to Andrew's comments, I have a
> few
> more below:
>
> On Wed, 2021-01-13 at 21:19 +0100, Steen Hegelund wrote:
> > Signed-off-by: Steen Hegelund <[email protected]>
> > ---
> > drivers/reset/Kconfig | 8 ++
> > drivers/reset/Makefile | 1 +
> > drivers/reset/reset-microchip-sparx5.c | 145
> > +++++++++++++++++++++++++
> > 3 files changed, 154 insertions(+)
> > create mode 100644 drivers/reset/reset-microchip-sparx5.c
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 71ab75a46491..05c240c47a8a 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -101,6 +101,14 @@ config RESET_LPC18XX
> > help
> > This enables the reset controller driver for NXP
> > LPC18xx/43xx SoCs.
> >
> > +config RESET_MCHP_SPARX5
> > + bool "Microchip Sparx5 reset driver"
> > + depends on HAS_IOMEM || COMPILE_TEST
> > + default y if SPARX5_SWITCH
> > + select MFD_SYSCON
> > + help
> > + This driver supports switch core reset for the Microchip
> > Sparx5 SoC.
> > +
> > config RESET_MESON
> > tristate "Meson Reset Driver"
> > depends on ARCH_MESON || COMPILE_TEST
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index 1054123fd187..341fd9ab4bf6 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> > obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> > obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> > obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> > +obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
> > obj-$(CONFIG_RESET_MESON) += reset-meson.o
> > obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
> > obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
> > diff --git a/drivers/reset/reset-microchip-sparx5.c
> > b/drivers/reset/reset-microchip-sparx5.c
> > new file mode 100644
> > index 000000000000..bb636ebd22d2
> > --- /dev/null
> > +++ b/drivers/reset/reset-microchip-sparx5.c
> > @@ -0,0 +1,145 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Microchip Sparx5 Switch Reset driver
> > + *
> > + * Copyright (c) 2020 Microchip Technology Inc. and its
> > subsidiaries.
> > + *
> > + * The Sparx5 Chip Register Model can be browsed at this location:
> > + * https://github.com/microchip-ung/sparx-5_reginfo
> > + */
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/notifier.h>
>
> Please drop all unused headers.
Will do.
>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#define PROTECT_REG 0x84
> > +#define PROTECT_BIT BIT(10)
> > +#define SOFT_RESET_REG 0x08
> > +#define SOFT_RESET_BIT BIT(1)
> > +
> > +struct mchp_reset_context {
> > + struct device *dev;
> > + struct regmap *cpu_ctrl;
> > + struct regmap *gcb_ctrl;
> > + struct reset_controller_dev reset_ctrl;
>
> For consistency, I'd like this to be called rcdev, or something else
> that doesn't sound like this should be a struct reset_control.
OK.
>
> > +};
> > +
> > +static u32 sparx5_read_soft_rst(struct mchp_reset_context *ctx)
> > +{
> > + u32 val;
> > +
> > + regmap_read(ctx->gcb_ctrl, SOFT_RESET_REG, &val);
> > + return val;
> > +}
>
> This can be dropped if you use regmap_read_poll_timeout() below.
Yes.
>
> > +static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct mchp_reset_context *ctx =
> > + container_of(rcdev, struct mchp_reset_context,
> > reset_ctrl);
> > + u32 val;
> > +
> > + /* Make sure the core is PROTECTED from reset */
> > + regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT,
> > PROTECT_BIT);
> > +
> > + dev_info(ctx->dev, "soft reset of switchcore\n");
> > +
> > + /* Start soft reset */
> > + regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
> > +
> > + /* Wait for soft reset done */
> > + return read_poll_timeout(sparx5_read_soft_rst, val,
> > + (val & SOFT_RESET_BIT) == 0,
> > + 1, 100, false,
> > + ctx);
>
> This looks like you could use regmap_read_poll_timeout() here.
Yes - did not remember that function...
>
> > +}
> > +
> > +static const struct reset_control_ops sparx5_reset_ops = {
> > + .reset = sparx5_switch_reset,
> > +};
> > +
> > +static int mchp_sparx5_reset_config(struct platform_device *pdev,
> > + struct mchp_reset_context *ctx)
> > +{
> > + struct device_node *dn = pdev->dev.of_node;
> > + struct regmap *cpu_ctrl, *gcb_ctrl;
> > + struct device_node *syscon_np;
> > + int err;
> > +
> > + syscon_np = of_parse_phandle(dn, "syscons", 0);
> > + if (!syscon_np)
> > + return -ENODEV;
> > + cpu_ctrl = syscon_node_to_regmap(syscon_np);
> > + if (IS_ERR(cpu_ctrl))
> > + goto err_cpu;
> > + of_node_put(syscon_np);
>
> If you move the of_node_put() up before the IS_ERR() check, you don't
> have to repeat it at the err_cpu: label. In fact, if you also move
> the
> error message up here, you can return here and drop the label.
Yes. I will change this.
>
> > +
> > + syscon_np = of_parse_phandle(dn, "syscons", 1);
> > + if (!syscon_np)
> > + return -ENODEV;
> > + gcb_ctrl = syscon_node_to_regmap(syscon_np);
> > + if (IS_ERR(gcb_ctrl))
> > + goto err_gcb;
> > + of_node_put(syscon_np);
>
> Same as above.
Yes.
>
> > +
> > + ctx->cpu_ctrl = cpu_ctrl;
> > + ctx->gcb_ctrl = gcb_ctrl;
> > +
> > + ctx->reset_ctrl.owner = THIS_MODULE;
> > + ctx->reset_ctrl.nr_resets = 1;
> > + ctx->reset_ctrl.ops = &sparx5_reset_ops;
> > + ctx->reset_ctrl.of_node = dn;
> > +
> > + err = devm_reset_controller_register(&pdev->dev, &ctx-
> > >reset_ctrl);
> > + if (err)
> > + dev_err(&pdev->dev, "could not register reset
> > controller\n");
> > + pr_info("%s:%d\n", __func__, __LINE__);
> > + return err;
>
> The only reason devm_reset_controller_register() can fail
> unexpectedly is -ENOMEM. I think it would be fine to just
> return devm_reset_controller_regster() here.
OK - simpler => nicer
>
> > +err_cpu:
> > + of_node_put(syscon_np);
> > + dev_err(&pdev->dev, "No cpu syscon map\n");
> > + return PTR_ERR(cpu_ctrl);
> > +err_gcb:
> > + of_node_put(syscon_np);
> > + dev_err(&pdev->dev, "No gcb syscon map\n");
> > + return PTR_ERR(gcb_ctrl);
> > +}
> > +
> > +static int mchp_sparx5_reset_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct mchp_reset_context *ctx;
> > +
> > + pr_info("%s:%d\n", __func__, __LINE__);
> > + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > + ctx->dev = dev;
> > + return mchp_sparx5_reset_config(pdev, ctx);
> > +}
>
> You could fold the contents of mchp_sparx5_reset_config() into
> mchp_sparx5_reset_probe() and replace all &pdev->dev with dev.
Yes. I will change that.
>
> regards
> Philipp
Thanks for your comments.
BR
Steen