[PATCH 0/3] Adding the Sparx5 Switch Reset Driver

[PATCH 0/3] Adding the Sparx5 Switch Reset Driver

[Steen Hegelund]

This series provides the Microchip Sparx5 Switch Reset Driver

The Sparx5 Switch SoC has a number of components that can be reset
individually, but at least the Switch Core needs to be in a well defined
state at power on, when any of the Sparx5 drivers starts to access the
Switch Core, this reset driver is available.

The reset driver is loaded early via the postcore_initcall interface, and
will then be available for the other Sparx5 drivers (SGPIO, SwitchDev etc)
that are loaded next, and the first of them to be loaded can perform the
one-time Switch Core reset that is needed.

The driver has protection so that the system busses, DDR controller, PCI-E
and ARM A53 CPU and a few other subsystems are not touched by the reset.

The Sparx5 Chip Register Model can be browsed at this location:
https://github.com/microchip-ung/sparx-5_reginfo

Steen Hegelund (3):
  dt-bindings: reset: microchip sparx5 reset driver bindings
  reset: mchp: sparx5: add switch reset driver
  arm64: dts: reset: add microchip sparx5 switch reset driver

 .../bindings/reset/microchip,rst.yaml         |  52 +++++++
 arch/arm64/boot/dts/microchip/sparx5.dtsi     |  13 +-
 drivers/reset/Kconfig                         |   8 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-microchip-sparx5.c        | 145 ++++++++++++++++++
 5 files changed, 216 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/microchip,rst.yaml
 create mode 100644 drivers/reset/reset-microchip-sparx5.c

--
2.29.2

[PATCH 1/3] dt-bindings: reset: microchip sparx5 reset driver bindings

[Steen Hegelund]

Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
 .../bindings/reset/microchip,rst.yaml         | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/microchip,rst.yaml

diff --git a/Documentation/devicetree/bindings/reset/microchip,rst.yaml b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
new file mode 100644
index 000000000000..b5526753e85d
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/reset/microchip,rst.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Microchip Sparx5 Switch Reset Controller
+
+maintainers:
+  - Steen Hegelund <steen.hegelund@microchip.com>
+  - Lars Povlsen <lars.povlsen@microchip.com>
+
+description: |
+  The Microchip Sparx5 Switch provides reset control and implements the following
+  functions
+    - One Time Switch Core Reset (Soft Reset)
+
+properties:
+  $nodename:
+    pattern: "^reset-controller@[0-9a-f]+$"
+
+  compatible:
+    const: microchip,sparx5-switch-reset
+
+  reg:
+    maxItems: 1
+
+  "#reset-cells":
+    const: 1
+
+  syscons:
+    $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    description: Array of syscons used to access reset registers
+    minItems: 2
+
+required:
+  - compatible
+  - reg
+  - "#reset-cells"
+  - syscons
+
+additionalProperties: false
+
+examples:
+  - |
+    reset: reset-controller@0 {
+        compatible = "microchip,sparx5-switch-reset";
+        reg = <0x0 0x0>;
+        #reset-cells = <1>;
+        syscons = <&cpu_ctrl>,<&gcb_ctrl>;
+    };
+
-- 
2.29.2

[PATCH 2/3] reset: mchp: sparx5: add switch reset driver

[Steen Hegelund]

Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
 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

[PATCH 3/3] arm64: dts: reset: add microchip sparx5 switch reset driver

[Steen Hegelund]

Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
 arch/arm64/boot/dts/microchip/sparx5.dtsi | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi
index 380281f312d8..6f0a21c362e3 100644
--- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
@@ -132,9 +132,16 @@ mux: mux-controller {
 			};
 		};
 
-		reset@611010008 {
-			compatible = "microchip,sparx5-chip-reset";
-			reg = <0x6 0x11010008 0x4>;
+		gcb_ctrl: syscon@611010000 {
+			compatible = "microchip,sparx5-gcb-syscon", "syscon";
+			reg = <0x6 0x11010000 0x10000>;
+		};
+
+		reset: reset-controller@0 {
+			compatible = "microchip,sparx5-switch-reset";
+			reg = <0x6 0x0 0x0>;
+			#reset-cells = <1>;
+			syscons = <&cpu_ctrl>,<&gcb_ctrl>;
 		};
 
 		uart0: serial@600100000 {
-- 
2.29.2

Re: [PATCH 2/3] reset: mchp: sparx5: add switch reset driver

[Andrew Lunn]

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

Re: [PATCH 2/3] reset: mchp: sparx5: add switch reset driver

[Steen Hegelund]

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

Re: [PATCH 2/3] reset: mchp: sparx5: add switch reset driver

[Philipp Zabel]

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 <steen.hegelund@microchip.com>
> ---
>  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

Re: [PATCH 1/3] dt-bindings: reset: microchip sparx5 reset driver bindings

[Philipp Zabel]

Hi Steen,

On Wed, 2021-01-13 at 21:19 +0100, Steen Hegelund wrote:
> Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
> ---
>  .../bindings/reset/microchip,rst.yaml         | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/microchip,rst.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/microchip,rst.yaml b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> new file mode 100644
> index 000000000000..b5526753e85d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/reset/microchip,rst.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Microchip Sparx5 Switch Reset Controller
> +
> +maintainers:
> +  - Steen Hegelund <steen.hegelund@microchip.com>
> +  - Lars Povlsen <lars.povlsen@microchip.com>
> +
> +description: |
> +  The Microchip Sparx5 Switch provides reset control and implements the following
> +  functions
> +    - One Time Switch Core Reset (Soft Reset)
> +
> +properties:
> +  $nodename:
> +    pattern: "^reset-controller@[0-9a-f]+$"
> +
> +  compatible:
> +    const: microchip,sparx5-switch-reset
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#reset-cells":
> +    const: 1
> +
> +  syscons:
> +    $ref: "/schemas/types.yaml#/definitions/phandle-array"
> +    description: Array of syscons used to access reset registers
> +    minItems: 2

The order seems to be important in the driver, so this should specify
which is the CPU syscon and which is the GCB syscon. I'm not sure if it
would be better to have two separately named syscon properties with a
single phandle each.

regards
Philipp

Re: [PATCH 1/3] dt-bindings: reset: microchip sparx5 reset driver bindings

[Steen Hegelund]

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,
> 
> On Wed, 2021-01-13 at 21:19 +0100, Steen Hegelund wrote:
> > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
> > ---
> >  .../bindings/reset/microchip,rst.yaml         | 52
> > +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/reset/microchip,rst.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> > b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> > new file mode 100644
> > index 000000000000..b5526753e85d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/reset/microchip,rst.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Microchip Sparx5 Switch Reset Controller
> > +
> > +maintainers:
> > +  - Steen Hegelund <steen.hegelund@microchip.com>
> > +  - Lars Povlsen <lars.povlsen@microchip.com>
> > +
> > +description: |
> > +  The Microchip Sparx5 Switch provides reset control and
> > implements the following
> > +  functions
> > +    - One Time Switch Core Reset (Soft Reset)
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^reset-controller@[0-9a-f]+$"
> > +
> > +  compatible:
> > +    const: microchip,sparx5-switch-reset
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#reset-cells":
> > +    const: 1
> > +
> > +  syscons:
> > +    $ref: "/schemas/types.yaml#/definitions/phandle-array"
> > +    description: Array of syscons used to access reset registers
> > +    minItems: 2
> 
> The order seems to be important in the driver, so this should specify
> which is the CPU syscon and which is the GCB syscon. I'm not sure if
> it
> would be better to have two separately named syscon properties with a
> single phandle each.

Yes you got a point.  I will change that.

> 
> regards
> Philipp

BR
Steen

Re: [PATCH 2/3] reset: mchp: sparx5: add switch reset driver

[Steen Hegelund]

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 <steen.hegelund@microchip.com>
> > ---
> >  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