| Message ID | 1467640052-6770-3-git-send-email-gabriel.fernandez@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gabriel, Am Montag, den 04.07.2016, 15:47 +0200 schrieb gabriel.fernandez@st.com: > From: Gabriel Fernandez <gabriel.fernandez@st.com> Isn't Maxime the author of this driver? > The STM32 MCUs family IPs can be reset by accessing some registers > from the RCC block. > > The list of available reset lines is documented in the DT bindings. > > Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > --- > drivers/reset/Makefile | 1 + > drivers/reset/reset-stm32.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 114 insertions(+) > create mode 100644 drivers/reset/reset-stm32.c > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 03dc1bb..3776b7b 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile [...] > +static const struct reset_control_ops stm32_reset_ops = { > + .assert = stm32_reset_assert, > + .deassert = stm32_reset_deassert, Are the registers not readable, or did you choose not to implement .status on purpose? regards Philipp
Hi Philipp, Thanks for reviewing. On 07/04/2016 07:36 PM, Philipp Zabel wrote: > Hi Gabriel, > > Am Montag, den 04.07.2016, 15:47 +0200 schrieb gabriel.fernandez@st.com: >> From: Gabriel Fernandez <gabriel.fernandez@st.com> > Isn't Maxime the author of this driver? Yes i upstream with his agreement. I only made small modifications (use of devm_reset_controller_register(), make reset_control_ops const...) that's why the author in the git history has been changed... I will use g |it commit --amend --author="Maxime.." for the v2. | >> The STM32 MCUs family IPs can be reset by accessing some registers >> from the RCC block. >> >> The list of available reset lines is documented in the DT bindings. >> >> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> >> --- >> drivers/reset/Makefile | 1 + >> drivers/reset/reset-stm32.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 114 insertions(+) >> create mode 100644 drivers/reset/reset-stm32.c >> >> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile >> index 03dc1bb..3776b7b 100644 >> --- a/drivers/reset/Makefile >> +++ b/drivers/reset/Makefile > [...] >> +static const struct reset_control_ops stm32_reset_ops = { >> + .assert = stm32_reset_assert, >> + .deassert = stm32_reset_deassert, > Are the registers not readable, or did you choose not to > implement .status on purpose? We choose to not implement. Thanks! Best Regards Gabriel > regards > Philipp >
Am Montag, den 04.07.2016, 15:47 +0200 schrieb gabriel.fernandez@st.com: > From: Gabriel Fernandez <gabriel.fernandez@st.com> > > The STM32 MCUs family IPs can be reset by accessing some registers > from the RCC block. > > The list of available reset lines is documented in the DT bindings. > > Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > --- > drivers/reset/Makefile | 1 + > drivers/reset/reset-stm32.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 114 insertions(+) > create mode 100644 drivers/reset/reset-stm32.c > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 03dc1bb..3776b7b 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -4,6 +4,7 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o > obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o > obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o > obj-$(CONFIG_ARCH_MESON) += reset-meson.o > +obj-$(CONFIG_ARCH_STM32) += reset-stm32.o > obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o > obj-$(CONFIG_ARCH_STI) += sti/ > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c > new file mode 100644 > index 0000000..be42bff > --- /dev/null > +++ b/drivers/reset/reset-stm32.c > @@ -0,0 +1,113 @@ > +/* > + * Copyright (C) Maxime Coquelin 2015 > + * Author: Maxime Coquelin <mcoquelin.stm32@gmail.com> > + * License terms: GNU General Public License (GPL), version 2 > + * > + * Heavily based on sunxi driver from Maxime Ripard. > + */ > + > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/reset-controller.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/types.h> > + > +struct stm32_reset_data { > + spinlock_t lock; > + void __iomem *membase; > + struct reset_controller_dev rcdev; > +}; > + > +static int stm32_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct stm32_reset_data *data = container_of(rcdev, > + struct stm32_reset_data, > + rcdev); > + int bank = id / BITS_PER_LONG; > + int offset = id % BITS_PER_LONG; > + unsigned long flags; > + u32 reg; > + > + spin_lock_irqsave(&data->lock, flags); > + > + reg = readl_relaxed(data->membase + (bank * 4)); > + writel_relaxed(reg | BIT(offset), data->membase + (bank * 4)); Please also switch to the non-relaxed variants. It shouldn't make a difference here, and as Arnd points out, reduces the risk of new developers using readl/writel_relaxed without thinking about the consequences. Further, this will make the stm32, sunxi, and socfpga accessors look the same. I'd like to try and combine them after this is merged. regards Philipp
Am Dienstag, den 05.07.2016, 09:29 +0200 schrieb Gabriel Fernandez: [...] > >> +static const struct reset_control_ops stm32_reset_ops = { > >> + .assert = stm32_reset_assert, > >> + .deassert = stm32_reset_deassert, > > Are the registers not readable, or did you choose not to > > implement .status on purpose? > We choose to not implement. Ok. Because of size issues or just because you don't need them in any of your drivers? regards Philipp
Hi Philipp, On 07/05/2016 03:28 PM, Philipp Zabel wrote: > Am Montag, den 04.07.2016, 15:47 +0200 schrieb gabriel.fernandez@st.com: >> From: Gabriel Fernandez <gabriel.fernandez@st.com> >> >> The STM32 MCUs family IPs can be reset by accessing some registers >> from the RCC block. >> >> The list of available reset lines is documented in the DT bindings. >> >> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> >> --- >> drivers/reset/Makefile | 1 + >> drivers/reset/reset-stm32.c | 113 ++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 114 insertions(+) >> create mode 100644 drivers/reset/reset-stm32.c >> >> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile >> index 03dc1bb..3776b7b 100644 >> --- a/drivers/reset/Makefile >> +++ b/drivers/reset/Makefile >> @@ -4,6 +4,7 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o >> obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o >> obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o >> obj-$(CONFIG_ARCH_MESON) += reset-meson.o >> +obj-$(CONFIG_ARCH_STM32) += reset-stm32.o >> obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o >> obj-$(CONFIG_ARCH_STI) += sti/ >> obj-$(CONFIG_ARCH_HISI) += hisilicon/ >> diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c >> new file mode 100644 >> index 0000000..be42bff >> --- /dev/null >> +++ b/drivers/reset/reset-stm32.c >> @@ -0,0 +1,113 @@ >> +/* >> + * Copyright (C) Maxime Coquelin 2015 >> + * Author: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> + * License terms: GNU General Public License (GPL), version 2 >> + * >> + * Heavily based on sunxi driver from Maxime Ripard. >> + */ >> + >> +#include <linux/err.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/platform_device.h> >> +#include <linux/reset-controller.h> >> +#include <linux/slab.h> >> +#include <linux/spinlock.h> >> +#include <linux/types.h> >> + >> +struct stm32_reset_data { >> + spinlock_t lock; >> + void __iomem *membase; >> + struct reset_controller_dev rcdev; >> +}; >> + >> +static int stm32_reset_assert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct stm32_reset_data *data = container_of(rcdev, >> + struct stm32_reset_data, >> + rcdev); >> + int bank = id / BITS_PER_LONG; >> + int offset = id % BITS_PER_LONG; >> + unsigned long flags; >> + u32 reg; >> + >> + spin_lock_irqsave(&data->lock, flags); >> + >> + reg = readl_relaxed(data->membase + (bank * 4)); >> + writel_relaxed(reg | BIT(offset), data->membase + (bank * 4)); > Please also switch to the non-relaxed variants. It shouldn't make a > difference here, and as Arnd points out, reduces the risk of new > developers using readl/writel_relaxed without thinking about the > consequences. > Further, this will make the stm32, sunxi, and socfpga accessors look the > same. I'd like to try and combine them after this is merged. > > regards > Philipp > ok no problem, i will fix it. Thanks Gabriel
Hi Philipp On 07/05/2016 03:29 PM, Philipp Zabel wrote: > Am Dienstag, den 05.07.2016, 09:29 +0200 schrieb Gabriel Fernandez: > [...] >>>> +static const struct reset_control_ops stm32_reset_ops = { >>>> + .assert = stm32_reset_assert, >>>> + .deassert = stm32_reset_deassert, >>> Are the registers not readable, or did you choose not to >>> implement .status on purpose? >> We choose to not implement. > Ok. Because of size issues or just because you don't need them in any of > your drivers? Because i don't need them. BR Gabriel > > regards > Philipp >
Am Mittwoch, den 06.07.2016, 17:39 +0200 schrieb Gabriel Fernandez: > Hi Philipp > > On 07/05/2016 03:29 PM, Philipp Zabel wrote: > > Am Dienstag, den 05.07.2016, 09:29 +0200 schrieb Gabriel Fernandez: > > [...] > >>>> +static const struct reset_control_ops stm32_reset_ops = { > >>>> + .assert = stm32_reset_assert, > >>>> + .deassert = stm32_reset_deassert, > >>> Are the registers not readable, or did you choose not to > >>> implement .status on purpose? > >> We choose to not implement. > > Ok. Because of size issues or just because you don't need them in any of > > your drivers? > Because i don't need them. Ok, thanks for clarifying. regards Philipp
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 03dc1bb..3776b7b 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o obj-$(CONFIG_ARCH_MESON) += reset-meson.o +obj-$(CONFIG_ARCH_STM32) += reset-stm32.o obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o obj-$(CONFIG_ARCH_STI) += sti/ obj-$(CONFIG_ARCH_HISI) += hisilicon/ diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c new file mode 100644 index 0000000..be42bff --- /dev/null +++ b/drivers/reset/reset-stm32.c @@ -0,0 +1,113 @@ +/* + * Copyright (C) Maxime Coquelin 2015 + * Author: Maxime Coquelin <mcoquelin.stm32@gmail.com> + * License terms: GNU General Public License (GPL), version 2 + * + * Heavily based on sunxi driver from Maxime Ripard. + */ + +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/reset-controller.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/types.h> + +struct stm32_reset_data { + spinlock_t lock; + void __iomem *membase; + struct reset_controller_dev rcdev; +}; + +static int stm32_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct stm32_reset_data *data = container_of(rcdev, + struct stm32_reset_data, + rcdev); + int bank = id / BITS_PER_LONG; + int offset = id % BITS_PER_LONG; + unsigned long flags; + u32 reg; + + spin_lock_irqsave(&data->lock, flags); + + reg = readl_relaxed(data->membase + (bank * 4)); + writel_relaxed(reg | BIT(offset), data->membase + (bank * 4)); + + spin_unlock_irqrestore(&data->lock, flags); + + return 0; +} + +static int stm32_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct stm32_reset_data *data = container_of(rcdev, + struct stm32_reset_data, + rcdev); + int bank = id / BITS_PER_LONG; + int offset = id % BITS_PER_LONG; + unsigned long flags; + u32 reg; + + spin_lock_irqsave(&data->lock, flags); + + reg = readl_relaxed(data->membase + (bank * 4)); + writel_relaxed(reg & ~BIT(offset), data->membase + (bank * 4)); + + spin_unlock_irqrestore(&data->lock, flags); + + return 0; +} + +static const struct reset_control_ops stm32_reset_ops = { + .assert = stm32_reset_assert, + .deassert = stm32_reset_deassert, +}; + +static const struct of_device_id stm32_reset_dt_ids[] = { + { .compatible = "st,stm32-rcc", }, + { /* sentinel */ }, +}; + +static int stm32_reset_probe(struct platform_device *pdev) +{ + struct stm32_reset_data *data; + struct resource *res; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + data->membase = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(data->membase)) + return PTR_ERR(data->membase); + + spin_lock_init(&data->lock); + + data->rcdev.owner = THIS_MODULE; + data->rcdev.nr_resets = resource_size(res) * 8; + data->rcdev.ops = &stm32_reset_ops; + data->rcdev.of_node = pdev->dev.of_node; + + return devm_reset_controller_register(&pdev->dev, &data->rcdev); +} + +static struct platform_driver stm32_reset_driver = { + .probe = stm32_reset_probe, + .driver = { + .name = "stm32-rcc-reset", + .of_match_table = stm32_reset_dt_ids, + }, +}; +module_platform_driver(stm32_reset_driver); + +MODULE_AUTHOR("Maxime Coquelin <maxime.coquelin@gmail.com>"); +MODULE_DESCRIPTION("STM32 MCUs Reset Controller Driver"); +MODULE_LICENSE("GPL");