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.hegel...@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
