diff mbox series

[v5,15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

Message ID 20220613195658.5607-16-brad@pensando.io
State New
Headers show
Series Support AMD Pensando Elba SoC | expand

Commit Message

Brad Larson June 13, 2022, 7:56 p.m. UTC
From: Brad Larson <blarson@amd.com>

This patch adds the reset controller functionality for the
AMD Pensando Elba System Resource Chip.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 drivers/reset/Kconfig                         |  9 ++
 drivers/reset/Makefile                        |  1 +
 drivers/reset/reset-elbasr.c                  | 94 +++++++++++++++++++
 .../reset/amd,pensando-elba-reset.h           | 11 +++
 4 files changed, 115 insertions(+)
 create mode 100644 drivers/reset/reset-elbasr.c
 create mode 100644 include/dt-bindings/reset/amd,pensando-elba-reset.h

Comments

Andy Shevchenko June 14, 2022, 11:46 a.m. UTC | #1
On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
>
> From: Brad Larson <blarson@amd.com>
>
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.

...

> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>

> +#include <linux/of.h>

There is no user of this header. But there are missed ones, such as
mod_devicetable.h.

Keep them ordered to easily find such issues.

...

> +       ret = devm_reset_controller_register(&pdev->dev, &elbar->rcdev);
> +
> +       return ret;

It is simply `return devm_...(...);`. Looking through your patches I
can tell that you may easily drop LoCs by 10%. Please do so in the
next version.

...

> +static const struct of_device_id elba_reset_dt_match[] = {
> +       { .compatible = "amd,pensando-elbasr-reset", },
> +       { /* sentinel */ },

No comma.

> +};
Philipp Zabel June 14, 2022, 2:49 p.m. UTC | #2
Hi Brad,

On Mo, 2022-06-13 at 12:56 -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
[...]
> diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> new file mode 100644
> index 000000000000..6e429cb11466
> --- /dev/null
> +++ b/drivers/reset/reset-elbasr.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 AMD Pensando
> + */
> +
> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +#include <dt-bindings/reset/amd,pensando-elba-reset.h>
> +
> +struct elbasr_reset {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +};
> +
> +static inline struct elbasr_reset *to_elbasr_rst(struct reset_controller_dev *rc)
> +{
> +	return container_of(rc, struct elbasr_reset, rcdev);
> +}
> +
> +static inline int elbasr_reset_shift(unsigned long id)
> +{
> +	switch (id) {
> +	case EMMC_HW_RESET:

Are there more reset controls than EMMC_HW_RESET?
If so, please list them all.
If not, why is this a function with a switch statement for a single
reset bit?

> +		return 6;
> +	default:
> +		return -EINVAL;

The error return value is never checked.
This can't be reached, since ELBASR_NR_RESETS == 1. So id will only
ever be 0.

> +static int elbasr_reset_probe(struct platform_device *pdev)
> +{
> +	struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);

Peeking into the MFD driver's private data structure seems unnecessary.
Consider using dev_get_regmap() instead.


regards
Philipp
Rob Herring (Arm) June 14, 2022, 9:34 p.m. UTC | #3
On Mon, Jun 13, 2022 at 12:56:58PM -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  drivers/reset/Kconfig                         |  9 ++
>  drivers/reset/Makefile                        |  1 +
>  drivers/reset/reset-elbasr.c                  | 94 +++++++++++++++++++
>  .../reset/amd,pensando-elba-reset.h           | 11 +++

This goes with the binding patch

>  4 files changed, 115 insertions(+)
>  create mode 100644 drivers/reset/reset-elbasr.c
>  create mode 100644 include/dt-bindings/reset/amd,pensando-elba-reset.h
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 93c8d07ee328..13f5a8ca0f03 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -66,6 +66,15 @@ config RESET_BRCMSTB_RESCAL
>  	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
>  	  BCM7216.
>  
> +config RESET_ELBASR
> +	tristate "Pensando Elba System Resource reset controller"
> +	depends on MFD_PENSANDO_ELBASR || COMPILE_TEST
> +	help
> +	  This option enables support for the external reset functions
> +	  on the Pensando Elba System Resource Chip.  Reset control
> +	  of peripherals is accessed over SPI to the system resource
> +	  chip device registers using CS0.
> +
>  config RESET_HSDK
>  	bool "Synopsys HSDK Reset Driver"
>  	depends on HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a80a9c4008a7..c0fe12b9950e 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
>  obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>  obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> +obj-$(CONFIG_RESET_ELBASR) += reset-elbasr.o
>  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> new file mode 100644
> index 000000000000..6e429cb11466
> --- /dev/null
> +++ b/drivers/reset/reset-elbasr.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

Kernel code is GPL-2.0-only generally.
Brad Larson July 3, 2022, 10:03 p.m. UTC | #4
Hi Philipp,

On Tue, Jun 14, 2022 at 7:49 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Brad,
>
> On Mo, 2022-06-13 at 12:56 -0700, Brad Larson wrote:
> > From: Brad Larson <blarson@amd.com>
> >
> > This patch adds the reset controller functionality for the
> > AMD Pensando Elba System Resource Chip.
> >
> > Signed-off-by: Brad Larson <blarson@amd.com>
> [...]
> > diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> ...
> > +static inline int elbasr_reset_shift(unsigned long id)
> > +{
> > +     switch (id) {
> > +     case EMMC_HW_RESET:
>
> Are there more reset controls than EMMC_HW_RESET?
> If so, please list them all.
> If not, why is this a function with a switch statement for a single
> reset bit?
>
> > +             return 6;
> > +     default:
> > +             return -EINVAL;

There are others but only emmc hardware reset is currently needed/used.  Removed
the switch and just using BIT(6) and removed file amd,pensando-elba-reset.h.

> The error return value is never checked.
> This can't be reached, since ELBASR_NR_RESETS == 1. So id will only
> ever be 0.
>
> > +static int elbasr_reset_probe(struct platform_device *pdev)
> > +{
> > +     struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);
>
> Peeking into the MFD driver's private data structure seems unnecessary.
> Consider using dev_get_regmap() instead.

Prefer to keep it this way as it follows the approach of existing
driver reset-a10sr.c

Regards,
Brad
Brad Larson July 3, 2022, 10:06 p.m. UTC | #5
Hi Andy,

On Tue, Jun 14, 2022 at 4:47 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
> >
> > From: Brad Larson <blarson@amd.com>
> >
> > This patch adds the reset controller functionality for the
> > AMD Pensando Elba System Resource Chip.
>
> ...
>
> > +#include <linux/mfd/pensando-elbasr.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/regmap.h>
> > +#include <linux/err.h>
>
> > +#include <linux/of.h>
>
> There is no user of this header. But there are missed ones, such as
> mod_devicetable.h.
>
> Keep them ordered to easily find such issues.

Removed of.h and added mod_devicetable.h.

> ...
> > +       ret = devm_reset_controller_register(&pdev->dev, &elbar->rcdev);
> > +
> > +       return ret;
>
> It is simply `return devm_...(...);`. Looking through your patches I
> can tell that you may easily drop LoCs by 10%. Please do so in the
> next version.

Changed to return devm...(...)

> ...
>
> > +static const struct of_device_id elba_reset_dt_match[] = {
> > +       { .compatible = "amd,pensando-elbasr-reset", },
> > +       { /* sentinel */ },
>
> No comma.

Removed comma

Regards,
Brad
Brad Larson July 3, 2022, 11:24 p.m. UTC | #6
Hi Rob,

On Tue, Jun 14, 2022 at 2:34 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 12:56:58PM -0700, Brad Larson wrote:
> > From: Brad Larson <blarson@amd.com>
> >
> > This patch adds the reset controller functionality for the
> > AMD Pensando Elba System Resource Chip.
> >
> > Signed-off-by: Brad Larson <blarson@amd.com>
> > ---
> >  drivers/reset/Kconfig                         |  9 ++
> >  drivers/reset/Makefile                        |  1 +
> >  drivers/reset/reset-elbasr.c                  | 94 +++++++++++++++++++
> >  .../reset/amd,pensando-elba-reset.h           | 11 +++
>
> This goes with the binding patch

I must have misinterpreted an earlier request to put the bindings
separately up front in the patch set.  For a new driver the binding
and driver should be in one patch which I'll change for the next version.

> ...
> > --- /dev/null
> > +++ b/drivers/reset/reset-elbasr.c
> > @@ -0,0 +1,94 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>
> Kernel code is GPL-2.0-only generally.

Did something change versus earlier request for dual license?

> Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support
> - by Rob Herring @ 2021-10-27 21:37 UTC [8%]

> > +// SPDX-License-Identifier: GPL-2.0

> Do you care about using with non-GPL OS? Dual license is preferred.

Regards,
Brad
diff mbox series

Patch

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 93c8d07ee328..13f5a8ca0f03 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -66,6 +66,15 @@  config RESET_BRCMSTB_RESCAL
 	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
 	  BCM7216.
 
+config RESET_ELBASR
+	tristate "Pensando Elba System Resource reset controller"
+	depends on MFD_PENSANDO_ELBASR || COMPILE_TEST
+	help
+	  This option enables support for the external reset functions
+	  on the Pensando Elba System Resource Chip.  Reset control
+	  of peripherals is accessed over SPI to the system resource
+	  chip device registers using CS0.
+
 config RESET_HSDK
 	bool "Synopsys HSDK Reset Driver"
 	depends on HAS_IOMEM
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index a80a9c4008a7..c0fe12b9950e 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -10,6 +10,7 @@  obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
 obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
 obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
+obj-$(CONFIG_RESET_ELBASR) += reset-elbasr.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
 obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
 obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
new file mode 100644
index 000000000000..6e429cb11466
--- /dev/null
+++ b/drivers/reset/reset-elbasr.c
@@ -0,0 +1,94 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2022 AMD Pensando
+ */
+
+#include <linux/mfd/pensando-elbasr.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/regmap.h>
+#include <linux/err.h>
+#include <linux/of.h>
+
+#include <dt-bindings/reset/amd,pensando-elba-reset.h>
+
+struct elbasr_reset {
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+};
+
+static inline struct elbasr_reset *to_elbasr_rst(struct reset_controller_dev *rc)
+{
+	return container_of(rc, struct elbasr_reset, rcdev);
+}
+
+static inline int elbasr_reset_shift(unsigned long id)
+{
+	switch (id) {
+	case EMMC_HW_RESET:
+		return 6;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int elbasr_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct elbasr_reset *elbar = to_elbasr_rst(rcdev);
+	u32 mask = 1 << elbasr_reset_shift(id);
+
+	return regmap_update_bits(elbar->regmap, ELBASR_CTRL0_REG, mask, mask);
+}
+
+static int elbasr_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	struct elbasr_reset *elbar = to_elbasr_rst(rcdev);
+	u32 mask = 1 << elbasr_reset_shift(id);
+
+	return regmap_update_bits(elbar->regmap, ELBASR_CTRL0_REG, mask, 0);
+}
+
+static const struct reset_control_ops elbasr_reset_ops = {
+	.assert	= elbasr_reset_assert,
+	.deassert = elbasr_reset_deassert,
+};
+
+static int elbasr_reset_probe(struct platform_device *pdev)
+{
+	struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);
+	struct elbasr_reset *elbar;
+	int ret;
+
+	elbar = devm_kzalloc(&pdev->dev, sizeof(struct elbasr_reset),
+			     GFP_KERNEL);
+	if (!elbar)
+		return -ENOMEM;
+
+	elbar->rcdev.owner = THIS_MODULE;
+	elbar->rcdev.nr_resets = ELBASR_NR_RESETS;
+	elbar->rcdev.ops = &elbasr_reset_ops;
+	elbar->rcdev.of_node = pdev->dev.of_node;
+	elbar->regmap = elbasr->elbasr_regs;
+
+	platform_set_drvdata(pdev, elbar);
+
+	ret = devm_reset_controller_register(&pdev->dev, &elbar->rcdev);
+
+	return ret;
+}
+
+static const struct of_device_id elba_reset_dt_match[] = {
+	{ .compatible = "amd,pensando-elbasr-reset", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver elbasr_reset_driver = {
+	.probe	= elbasr_reset_probe,
+	.driver = {
+		.name = "pensando_elbasr_reset",
+		.of_match_table	= elba_reset_dt_match,
+	},
+};
+builtin_platform_driver(elbasr_reset_driver);
diff --git a/include/dt-bindings/reset/amd,pensando-elba-reset.h b/include/dt-bindings/reset/amd,pensando-elba-reset.h
new file mode 100644
index 000000000000..68d69a98e750
--- /dev/null
+++ b/include/dt-bindings/reset/amd,pensando-elba-reset.h
@@ -0,0 +1,11 @@ 
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2022, AMD Pensando
+ */
+
+#ifndef _DT_BINDINGS_RESET_AMD_PENSANDO_ELBA_RESET_H
+#define _DT_BINDINGS_RESET_AMD_PENSANDO_ELBA_RESET_H
+
+#define EMMC_HW_RESET		0
+
+#endif