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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
+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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
+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?
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.
quoted hunk
++ 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);
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.
quoted hunk
+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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -0,0 +1,52 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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:
@@ -0,0 +1,52 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.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.
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:
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
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>
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.
quoted
++ 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);
+ 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
quoted
+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.