diff mbox series

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

Message ID 20210113201915.2734205-3-steen.hegelund@microchip.com (mailing list archive)
State New, archived
Headers show
Series Adding the Sparx5 Switch Reset Driver | expand

Commit Message

Steen Hegelund Jan. 13, 2021, 8:19 p.m. UTC 
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

Comments

Andrew Lunn Jan. 13, 2021, 11:23 p.m. UTC | #1 
> +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
Steen Hegelund Jan. 14, 2021, 8:30 a.m. UTC | #2 
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
Philipp Zabel Jan. 14, 2021, 9:39 a.m. UTC | #3 
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
Steen Hegelund Jan. 14, 2021, 1:36 p.m. UTC | #4 
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
diff mbox series

Patch

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);