| Message ID | 20220613195658.5607-16-brad@pensando.io |
|---|---|
| State | New |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
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. > +};
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
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.
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
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
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 --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