| Message ID | 1521045001-30788-3-git-send-email-gabriel.fernandez@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gabriel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20180309]
[also build test WARNING on v4.16-rc5]
[cannot apply to linus/master v4.16-rc4 v4.16-rc3 v4.16-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/gabriel-fernandez-st-com/Introduce-STM32MP1-Reset-driver/20180316-082607
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/reset/reset-stm32mp1.c:73:32: sparse: symbol 'stm32_reset_ops' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Gabriel, this looks mostly good to me, a few questions and comments below: On Wed, 2018-03-14 at 17:30 +0100, gabriel.fernandez@st.com wrote: > From: Gabriel Fernandez <gabriel.fernandez@st.com> > > stm32mp1 RCC IP 1 has a reset SET register and a reset CLEAR register. > > Writing '0' on reset SET register has no effect > Writing '1' on reset SET register > activates the reset of the corresponding peripheral > > Writing '0' on reset CLEAR register has no effect > Writing '1' on reset CLEAR register > releases the reset of the corresponding peripheral > > See Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.txt > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > --- > drivers/reset/Kconfig | 6 ++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-stm32mp1.c | 122 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 129 insertions(+) > create mode 100644 drivers/reset/reset-stm32mp1.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 1efbc6c..c0b292b 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -97,6 +97,12 @@ config RESET_SIMPLE > - Allwinner SoCs > - ZTE's zx2967 family > > +config RESET_STM32MP157 > + bool "STM32MP157 Reset Driver" if COMPILE_TEST > + default MACH_STM32MP157 > + help > + This enables the RCC reset controller driver for STM32 MPUs. > + > config RESET_SUNXI > bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI > default ARCH_SUNXI > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 132c24f..c1261dc 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_RESET_MESON) += reset-meson.o > obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o > obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o > obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o > +obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o > obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o > obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o > obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o > diff --git a/drivers/reset/reset-stm32mp1.c b/drivers/reset/reset-stm32mp1.c > new file mode 100644 > index 0000000..5e25388 > --- /dev/null > +++ b/drivers/reset/reset-stm32mp1.c > @@ -0,0 +1,122 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved > + * Author: Gabriel Fernandez <gabriel.fernandez@st.com> for STMicroelectronics. > + */ > + > +#include <linux/arm-smccc.h> This does not seem to be necessary. > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/of.h> > +#include <linux/of_device.h> This does not seem to be necessary either. > +#include <linux/platform_device.h> > +#include <linux/reset-controller.h> > + > +#define CLR_OFFSET 0x4 > + > +struct stm32_reset_data { > + struct reset_controller_dev rcdev; > + void __iomem *membase; > +}; > + > +static inline struct stm32_reset_data * > +to_stm32_reset_data(struct reset_controller_dev *rcdev) > +{ > + return container_of(rcdev, struct stm32_reset_data, rcdev); > +} > + > +static int stm32_reset_update(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + struct stm32_reset_data *data = to_stm32_reset_data(rcdev); > + int reg_width = sizeof(u32); > + int bank = id / (reg_width * BITS_PER_BYTE); > + int offset = id % (reg_width * BITS_PER_BYTE); > + void __iomem *addr; > + > + addr = data->membase + (bank * reg_width); > + if (!assert) > + addr += CLR_OFFSET; > + > + writel(BIT(offset), addr); > + > + return 0; > +} > + > +static int stm32_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return stm32_reset_update(rcdev, id, true); > +} > + > +static int stm32_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return stm32_reset_update(rcdev, id, false); > +} > + > +static int stm32_reset_status(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct stm32_reset_data *data = to_stm32_reset_data(rcdev); > + int reg_width = sizeof(u32); > + int bank = id / (reg_width * BITS_PER_BYTE); > + int offset = id % (reg_width * BITS_PER_BYTE); > + u32 reg; > + > + reg = readl(data->membase + (bank * reg_width)); > + > + return !(reg & BIT(offset)); > +} So the SET register can be read back and returns 0 for reset lines that are currently asserted and returns 1 for reset lines that are currently deasserted? > + > +const struct reset_control_ops stm32_reset_ops = { > + .assert = stm32_reset_assert, > + .deassert = stm32_reset_deassert, > + .status = stm32_reset_status, > +}; > + > +static const struct of_device_id stm32_reset_dt_ids[] = { > + { .compatible = "st,stm32mp1-rcc"}, > + { /* sentinel */ }, > +}; From the DT bindings it looks like the clock and reset drivers are sharing the same node. Is there just no clock platform_driver at all? > + > +static int stm32_reset_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct stm32_reset_data *data; > + void __iomem *membase; > + struct resource *res; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + membase = devm_ioremap_resource(dev, res); > + if (IS_ERR(membase)) > + return PTR_ERR(membase); > + > + data->membase = membase; > + data->rcdev.owner = THIS_MODULE; > + data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE; > + data->rcdev.ops = &stm32_reset_ops; > + data->rcdev.of_node = dev->of_node; > + > + return devm_reset_controller_register(dev, &data->rcdev); > +} > + > +static struct platform_driver stm32_reset_driver = { > + .probe = stm32_reset_probe, > + .driver = { > + .name = "stm32mp1-reset", > + .of_match_table = stm32_reset_dt_ids, > + }, > +}; > + > +static int __init stm32_reset_init(void) > +{ > + return platform_driver_register(&stm32_reset_driver); > +} > + > +postcore_initcall(stm32_reset_init); Isn't builtin_platform_driver early enough? regards Philipp
Hi Philipp, Thanks for reviewing. On 03/16/2018 02:29 PM, Philipp Zabel wrote: > Hi Gabriel, > > this looks mostly good to me, a few questions and comments below: > > On Wed, 2018-03-14 at 17:30 +0100, gabriel.fernandez@st.com wrote: >> From: Gabriel Fernandez <gabriel.fernandez@st.com> >> >> stm32mp1 RCC IP 1 has a reset SET register and a reset CLEAR register. >> >> Writing '0' on reset SET register has no effect >> Writing '1' on reset SET register >> activates the reset of the corresponding peripheral >> >> Writing '0' on reset CLEAR register has no effect >> Writing '1' on reset CLEAR register >> releases the reset of the corresponding peripheral >> >> See Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.txt >> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> >> --- >> drivers/reset/Kconfig | 6 ++ >> drivers/reset/Makefile | 1 + >> drivers/reset/reset-stm32mp1.c | 122 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 129 insertions(+) >> create mode 100644 drivers/reset/reset-stm32mp1.c >> >> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig >> index 1efbc6c..c0b292b 100644 >> --- a/drivers/reset/Kconfig >> +++ b/drivers/reset/Kconfig >> @@ -97,6 +97,12 @@ config RESET_SIMPLE >> - Allwinner SoCs >> - ZTE's zx2967 family >> >> +config RESET_STM32MP157 >> + bool "STM32MP157 Reset Driver" if COMPILE_TEST >> + default MACH_STM32MP157 >> + help >> + This enables the RCC reset controller driver for STM32 MPUs. >> + >> config RESET_SUNXI >> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI >> default ARCH_SUNXI >> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile >> index 132c24f..c1261dc 100644 >> --- a/drivers/reset/Makefile >> +++ b/drivers/reset/Makefile >> @@ -15,6 +15,7 @@ obj-$(CONFIG_RESET_MESON) += reset-meson.o >> obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o >> obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o >> obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o >> +obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o >> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o >> obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o >> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o >> diff --git a/drivers/reset/reset-stm32mp1.c b/drivers/reset/reset-stm32mp1.c >> new file mode 100644 >> index 0000000..5e25388 >> --- /dev/null >> +++ b/drivers/reset/reset-stm32mp1.c >> @@ -0,0 +1,122 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved >> + * Author: Gabriel Fernandez <gabriel.fernandez@st.com> for STMicroelectronics. >> + */ >> + >> +#include <linux/arm-smccc.h> > This does not seem to be necessary. right >> +#include <linux/device.h> >> +#include <linux/err.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> > This does not seem to be necessary either. ok >> +#include <linux/platform_device.h> >> +#include <linux/reset-controller.h> >> + >> +#define CLR_OFFSET 0x4 >> + >> +struct stm32_reset_data { >> + struct reset_controller_dev rcdev; >> + void __iomem *membase; >> +}; >> + >> +static inline struct stm32_reset_data * >> +to_stm32_reset_data(struct reset_controller_dev *rcdev) >> +{ >> + return container_of(rcdev, struct stm32_reset_data, rcdev); >> +} >> + >> +static int stm32_reset_update(struct reset_controller_dev *rcdev, >> + unsigned long id, bool assert) >> +{ >> + struct stm32_reset_data *data = to_stm32_reset_data(rcdev); >> + int reg_width = sizeof(u32); >> + int bank = id / (reg_width * BITS_PER_BYTE); >> + int offset = id % (reg_width * BITS_PER_BYTE); >> + void __iomem *addr; >> + >> + addr = data->membase + (bank * reg_width); >> + if (!assert) >> + addr += CLR_OFFSET; >> + >> + writel(BIT(offset), addr); >> + >> + return 0; >> +} >> + >> +static int stm32_reset_assert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + return stm32_reset_update(rcdev, id, true); >> +} >> + >> +static int stm32_reset_deassert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + return stm32_reset_update(rcdev, id, false); >> +} >> + >> +static int stm32_reset_status(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct stm32_reset_data *data = to_stm32_reset_data(rcdev); >> + int reg_width = sizeof(u32); >> + int bank = id / (reg_width * BITS_PER_BYTE); >> + int offset = id % (reg_width * BITS_PER_BYTE); >> + u32 reg; >> + >> + reg = readl(data->membase + (bank * reg_width)); >> + >> + return !(reg & BIT(offset)); >> +} > So the SET register can be read back and returns 0 for reset lines that > are currently asserted and returns 1 for reset lines that are currently > deasserted? yes you have spotted a error, i will replace by'return !!(reg & BIT(offset));' >> + >> +const struct reset_control_ops stm32_reset_ops = { >> + .assert = stm32_reset_assert, >> + .deassert = stm32_reset_deassert, >> + .status = stm32_reset_status, >> +}; >> + >> +static const struct of_device_id stm32_reset_dt_ids[] = { >> + { .compatible = "st,stm32mp1-rcc"}, >> + { /* sentinel */ }, >> +}; > From the DT bindings it looks like the clock and reset drivers are > sharing the same node. Is there just no clock platform_driver at all? there is one hardware block that is used for clock, reset and power, they share the same node but each have it own platform_driver >> + >> +static int stm32_reset_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct stm32_reset_data *data; >> + void __iomem *membase; >> + struct resource *res; >> + >> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + membase = devm_ioremap_resource(dev, res); >> + if (IS_ERR(membase)) >> + return PTR_ERR(membase); >> + >> + data->membase = membase; >> + data->rcdev.owner = THIS_MODULE; >> + data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE; >> + data->rcdev.ops = &stm32_reset_ops; >> + data->rcdev.of_node = dev->of_node; >> + >> + return devm_reset_controller_register(dev, &data->rcdev); >> +} >> + >> +static struct platform_driver stm32_reset_driver = { >> + .probe = stm32_reset_probe, >> + .driver = { >> + .name = "stm32mp1-reset", >> + .of_match_table = stm32_reset_dt_ids, >> + }, >> +}; >> + >> +static int __init stm32_reset_init(void) >> +{ >> + return platform_driver_register(&stm32_reset_driver); >> +} >> + >> +postcore_initcall(stm32_reset_init); > Isn't builtin_platform_driver early enough? ok Best Regards Gabriel > > regards > Philipp
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 1efbc6c..c0b292b 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -97,6 +97,12 @@ config RESET_SIMPLE - Allwinner SoCs - ZTE's zx2967 family +config RESET_STM32MP157 + bool "STM32MP157 Reset Driver" if COMPILE_TEST + default MACH_STM32MP157 + help + This enables the RCC reset controller driver for STM32 MPUs. + config RESET_SUNXI bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI default ARCH_SUNXI diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 132c24f..c1261dc 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_RESET_MESON) += reset-meson.o obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o +obj-$(CONFIG_RESET_STM32MP157) += reset-stm32mp1.o obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o diff --git a/drivers/reset/reset-stm32mp1.c b/drivers/reset/reset-stm32mp1.c new file mode 100644 index 0000000..5e25388 --- /dev/null +++ b/drivers/reset/reset-stm32mp1.c @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved + * Author: Gabriel Fernandez <gabriel.fernandez@st.com> for STMicroelectronics. + */ + +#include <linux/arm-smccc.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset-controller.h> + +#define CLR_OFFSET 0x4 + +struct stm32_reset_data { + struct reset_controller_dev rcdev; + void __iomem *membase; +}; + +static inline struct stm32_reset_data * +to_stm32_reset_data(struct reset_controller_dev *rcdev) +{ + return container_of(rcdev, struct stm32_reset_data, rcdev); +} + +static int stm32_reset_update(struct reset_controller_dev *rcdev, + unsigned long id, bool assert) +{ + struct stm32_reset_data *data = to_stm32_reset_data(rcdev); + int reg_width = sizeof(u32); + int bank = id / (reg_width * BITS_PER_BYTE); + int offset = id % (reg_width * BITS_PER_BYTE); + void __iomem *addr; + + addr = data->membase + (bank * reg_width); + if (!assert) + addr += CLR_OFFSET; + + writel(BIT(offset), addr); + + return 0; +} + +static int stm32_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return stm32_reset_update(rcdev, id, true); +} + +static int stm32_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return stm32_reset_update(rcdev, id, false); +} + +static int stm32_reset_status(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct stm32_reset_data *data = to_stm32_reset_data(rcdev); + int reg_width = sizeof(u32); + int bank = id / (reg_width * BITS_PER_BYTE); + int offset = id % (reg_width * BITS_PER_BYTE); + u32 reg; + + reg = readl(data->membase + (bank * reg_width)); + + return !(reg & BIT(offset)); +} + +const struct reset_control_ops stm32_reset_ops = { + .assert = stm32_reset_assert, + .deassert = stm32_reset_deassert, + .status = stm32_reset_status, +}; + +static const struct of_device_id stm32_reset_dt_ids[] = { + { .compatible = "st,stm32mp1-rcc"}, + { /* sentinel */ }, +}; + +static int stm32_reset_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct stm32_reset_data *data; + void __iomem *membase; + struct resource *res; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + membase = devm_ioremap_resource(dev, res); + if (IS_ERR(membase)) + return PTR_ERR(membase); + + data->membase = membase; + data->rcdev.owner = THIS_MODULE; + data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE; + data->rcdev.ops = &stm32_reset_ops; + data->rcdev.of_node = dev->of_node; + + return devm_reset_controller_register(dev, &data->rcdev); +} + +static struct platform_driver stm32_reset_driver = { + .probe = stm32_reset_probe, + .driver = { + .name = "stm32mp1-reset", + .of_match_table = stm32_reset_dt_ids, + }, +}; + +static int __init stm32_reset_init(void) +{ + return platform_driver_register(&stm32_reset_driver); +} + +postcore_initcall(stm32_reset_init);