| Message ID | 1424455277-29983-8-git-send-email-mcoquelin.stm32@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 20 February 2015 19:01:06 Maxime Coquelin wrote: > +/* AHB1 */ > +#define GPIOA_RESET 0 > +#define GPIOB_RESET 1 > +#define GPIOC_RESET 2 > +#define GPIOD_RESET 3 > +#define GPIOE_RESET 4 > +#define GPIOF_RESET 5 > +#define GPIOG_RESET 6 > +#define GPIOH_RESET 7 > +#define GPIOI_RESET 8 > +#define GPIOJ_RESET 9 > +#define GPIOK_RESET 10 > As these are just the hardware numbers, it's better to not make them part of the binding at all. Instead, just document in the binding that one is supposed to pass the hardware number as the argument. Arnd
2015-03-10 16:02 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: > On Friday 20 February 2015 19:01:06 Maxime Coquelin wrote: >> +/* AHB1 */ >> +#define GPIOA_RESET 0 >> +#define GPIOB_RESET 1 >> +#define GPIOC_RESET 2 >> +#define GPIOD_RESET 3 >> +#define GPIOE_RESET 4 >> +#define GPIOF_RESET 5 >> +#define GPIOG_RESET 6 >> +#define GPIOH_RESET 7 >> +#define GPIOI_RESET 8 >> +#define GPIOJ_RESET 9 >> +#define GPIOK_RESET 10 >> > > As these are just the hardware numbers, it's better to not make them > part of the binding at all. Instead, just document in the binding that > one is supposed to pass the hardware number as the argument. The reset controller is part of the RCC (Reset & Clock Controller) IP. In this version, I only provided the reset registers to the reset controller driver, but as per Andrea > > Arnd
2015-03-10 16:02 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: > On Friday 20 February 2015 19:01:06 Maxime Coquelin wrote: >> +/* AHB1 */ >> +#define GPIOA_RESET 0 >> +#define GPIOB_RESET 1 >> +#define GPIOC_RESET 2 >> +#define GPIOD_RESET 3 >> +#define GPIOE_RESET 4 >> +#define GPIOF_RESET 5 >> +#define GPIOG_RESET 6 >> +#define GPIOH_RESET 7 >> +#define GPIOI_RESET 8 >> +#define GPIOJ_RESET 9 >> +#define GPIOK_RESET 10 >> > > As these are just the hardware numbers, it's better to not make them > part of the binding at all. Instead, just document in the binding that > one is supposed to pass the hardware number as the argument. The reset controller is part of the RCC (Reset & Clock Controller) IP. In this version, I only provided the reset registers to the reset controller driver, but as per Andreas Färber remark, I should avec a single DT node for both the resets and clocks. In the next version I am preparing, the defines doesn't look as trivial as in this version, GPIOA_RESET being 128 for instance. Is it fine for you if I keep the defines part of the binding? Br, Maxime > > Arnd
On Tuesday 10 March 2015 16:44:24 Maxime Coquelin wrote: > 2015-03-10 16:02 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: > > On Friday 20 February 2015 19:01:06 Maxime Coquelin wrote: > >> +/* AHB1 */ > >> +#define GPIOA_RESET 0 > >> +#define GPIOB_RESET 1 > >> +#define GPIOC_RESET 2 > >> +#define GPIOD_RESET 3 > >> +#define GPIOE_RESET 4 > >> +#define GPIOF_RESET 5 > >> +#define GPIOG_RESET 6 > >> +#define GPIOH_RESET 7 > >> +#define GPIOI_RESET 8 > >> +#define GPIOJ_RESET 9 > >> +#define GPIOK_RESET 10 > >> > > > > As these are just the hardware numbers, it's better to not make them > > part of the binding at all. Instead, just document in the binding that > > one is supposed to pass the hardware number as the argument. > > The reset controller is part of the RCC (Reset & Clock Controller) IP. > In this version, I only provided the reset registers to the reset > controller driver, but as per Andreas Färber remark, I should avec a > single DT node for both the resets and clocks. > > In the next version I am preparing, the defines doesn't look as > trivial as in this version, GPIOA_RESET being 128 for instance. > > Is it fine for you if I keep the defines part of the binding? > > It's always better to avoid these files entirely, as they are a frequent source of merge dependencies, and they make it less obvious what's going on than having binary values in the dtb that make sense. Arnd
2015-03-10 21:21 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: > On Tuesday 10 March 2015 16:44:24 Maxime Coquelin wrote: >> 2015-03-10 16:02 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: >> > On Friday 20 February 2015 19:01:06 Maxime Coquelin wrote: >> >> +/* AHB1 */ >> >> +#define GPIOA_RESET 0 >> >> +#define GPIOB_RESET 1 >> >> +#define GPIOC_RESET 2 >> >> +#define GPIOD_RESET 3 >> >> +#define GPIOE_RESET 4 >> >> +#define GPIOF_RESET 5 >> >> +#define GPIOG_RESET 6 >> >> +#define GPIOH_RESET 7 >> >> +#define GPIOI_RESET 8 >> >> +#define GPIOJ_RESET 9 >> >> +#define GPIOK_RESET 10 >> >> >> > >> > As these are just the hardware numbers, it's better to not make them >> > part of the binding at all. Instead, just document in the binding that >> > one is supposed to pass the hardware number as the argument. >> >> The reset controller is part of the RCC (Reset & Clock Controller) IP. >> In this version, I only provided the reset registers to the reset >> controller driver, but as per Andreas Färber remark, I should avec a >> single DT node for both the resets and clocks. >> >> In the next version I am preparing, the defines doesn't look as >> trivial as in this version, GPIOA_RESET being 128 for instance. >> >> Is it fine for you if I keep the defines part of the binding? >> >> > > It's always better to avoid these files entirely, as they are > a frequent source of merge dependencies, and they make it less > obvious what's going on than having binary values in the dtb > that make sense. I agree it is always painful to have to have to manage these merge dependencies. What I will do, if Philipp agrees, is to list all the values in the binding documentation. Doing that, the user of a reset won't have to do the calculation, and no more merge dependencies. Maxime > > Arnd
Am Dienstag, den 10.03.2015, 22:20 +0100 schrieb Maxime Coquelin: > 2015-03-10 21:21 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: > > On Tuesday 10 March 2015 16:44:24 Maxime Coquelin wrote: > >> 2015-03-10 16:02 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: > >> > On Friday 20 February 2015 19:01:06 Maxime Coquelin wrote: > >> >> +/* AHB1 */ > >> >> +#define GPIOA_RESET 0 > >> >> +#define GPIOB_RESET 1 > >> >> +#define GPIOC_RESET 2 > >> >> +#define GPIOD_RESET 3 > >> >> +#define GPIOE_RESET 4 > >> >> +#define GPIOF_RESET 5 > >> >> +#define GPIOG_RESET 6 > >> >> +#define GPIOH_RESET 7 > >> >> +#define GPIOI_RESET 8 > >> >> +#define GPIOJ_RESET 9 > >> >> +#define GPIOK_RESET 10 > >> >> > >> > > >> > As these are just the hardware numbers, it's better to not make them > >> > part of the binding at all. Instead, just document in the binding that > >> > one is supposed to pass the hardware number as the argument. > >> > >> The reset controller is part of the RCC (Reset & Clock Controller) IP. > >> In this version, I only provided the reset registers to the reset > >> controller driver, but as per Andreas Färber remark, I should avec a > >> single DT node for both the resets and clocks. > >> > >> In the next version I am preparing, the defines doesn't look as > >> trivial as in this version, GPIOA_RESET being 128 for instance. > >> > >> Is it fine for you if I keep the defines part of the binding? > >> > >> > > > > It's always better to avoid these files entirely, as they are > > a frequent source of merge dependencies, and they make it less > > obvious what's going on than having binary values in the dtb > > that make sense. > > I agree it is always painful to have to have to manage these merge dependencies. > What I will do, if Philipp agrees, is to list all the values in the > binding documentation. > > Doing that, the user of a reset won't have to do the calculation, and > no more merge dependencies. I'd prefer to have #defines for the reset bits if they are named in the documentation and use the names in the dts. But if you want to reference reset bits by number in the device tree instead, I won't insist. Consider using two cells in the phandle for register and bit offset instead of a single number that arbitrarily starts at 128. regards Philipp
2015-03-11 14:08 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>: > Am Dienstag, den 10.03.2015, 22:20 +0100 schrieb Maxime Coquelin: >> 2015-03-10 21:21 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: >> > On Tuesday 10 March 2015 16:44:24 Maxime Coquelin wrote: >> >> 2015-03-10 16:02 GMT+01:00 Arnd Bergmann <arnd@arndb.de>: >> >> > On Friday 20 February 2015 19:01:06 Maxime Coquelin wrote: >> >> >> +/* AHB1 */ >> >> >> +#define GPIOA_RESET 0 >> >> >> +#define GPIOB_RESET 1 >> >> >> +#define GPIOC_RESET 2 >> >> >> +#define GPIOD_RESET 3 >> >> >> +#define GPIOE_RESET 4 >> >> >> +#define GPIOF_RESET 5 >> >> >> +#define GPIOG_RESET 6 >> >> >> +#define GPIOH_RESET 7 >> >> >> +#define GPIOI_RESET 8 >> >> >> +#define GPIOJ_RESET 9 >> >> >> +#define GPIOK_RESET 10 >> >> >> >> >> > >> >> > As these are just the hardware numbers, it's better to not make them >> >> > part of the binding at all. Instead, just document in the binding that >> >> > one is supposed to pass the hardware number as the argument. >> >> >> >> The reset controller is part of the RCC (Reset & Clock Controller) IP. >> >> In this version, I only provided the reset registers to the reset >> >> controller driver, but as per Andreas Färber remark, I should avec a >> >> single DT node for both the resets and clocks. >> >> >> >> In the next version I am preparing, the defines doesn't look as >> >> trivial as in this version, GPIOA_RESET being 128 for instance. >> >> >> >> Is it fine for you if I keep the defines part of the binding? >> >> >> >> >> > >> > It's always better to avoid these files entirely, as they are >> > a frequent source of merge dependencies, and they make it less >> > obvious what's going on than having binary values in the dtb >> > that make sense. >> >> I agree it is always painful to have to have to manage these merge dependencies. >> What I will do, if Philipp agrees, is to list all the values in the >> binding documentation. >> >> Doing that, the user of a reset won't have to do the calculation, and >> no more merge dependencies. > > I'd prefer to have #defines for the reset bits if they are named in the > documentation and use the names in the dts. But if you want to reference > reset bits by number in the device tree instead, I won't insist. > > Consider using two cells in the phandle for register and bit offset > instead of a single number that arbitrarily starts at 128. Thanks for your feedback. I would prefer using a single cell, which is less error prone in my opinion. Will you accept this? Kind regards, Maxime > > regards > Philipp >
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 157d421..aed12d1 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_RESET_CONTROLLER) += core.o obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o +obj-$(CONFIG_ARCH_STM32) += reset-stm32.o obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o obj-$(CONFIG_ARCH_STI) += sti/ diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c new file mode 100644 index 0000000..7a96677 --- /dev/null +++ b/drivers/reset/reset-stm32.c @@ -0,0 +1,124 @@ +/* + * 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 struct reset_control_ops stm32_reset_ops = { + .assert = stm32_reset_assert, + .deassert = stm32_reset_deassert, +}; + +static void stm32_reset_init(struct device_node *np) +{ + struct stm32_reset_data *data; + struct resource res; + resource_size_t size; + int err; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return; + + err = of_address_to_resource(np, 0, &res); + if (err) + goto err_alloc; + + size = resource_size(&res); + if (!request_mem_region(res.start, size, np->name)) { + err = -EINVAL; + goto err_alloc; + } + + data->membase = ioremap(res.start, size); + if (!data->membase) { + err = -ENOMEM; + goto err_alloc; + } + + spin_lock_init(&data->lock); + + data->rcdev.owner = THIS_MODULE; + data->rcdev.nr_resets = size * 8; + data->rcdev.ops = &stm32_reset_ops; + data->rcdev.of_node = np; + + err = reset_controller_register(&data->rcdev); + if (err) + goto err_iomap; + + pr_info("%s: %d reset lines registered\n", np->full_name, + data->rcdev.nr_resets); + return; + +err_iomap: + iounmap(data->membase); +err_alloc: + kfree(data); + pr_err("%s: Reset ctrl registration failed (%d).\n", + np->full_name, err); +} + +RESET_CONTROLLER_OF_DECLARE(stm32, "st,stm32-reset", stm32_reset_init); + diff --git a/include/dt-bindings/reset/st,stm32f429.h b/include/dt-bindings/reset/st,stm32f429.h new file mode 100644 index 0000000..04f2ba8 --- /dev/null +++ b/include/dt-bindings/reset/st,stm32f429.h @@ -0,0 +1,85 @@ +/* + * Copyright (C) Maxime Coquelin 2015 + * Author: Maxime Coquelin <mcoquelin.stm32@gmail.com> + * License terms: GNU General Public License (GPL), version 2 + */ + +#ifndef _DT_BINDINGS_RESET_STM32F429_H +#define _DT_BINDINGS_RESET_STM32F429_H + +/* AHB1 */ +#define GPIOA_RESET 0 +#define GPIOB_RESET 1 +#define GPIOC_RESET 2 +#define GPIOD_RESET 3 +#define GPIOE_RESET 4 +#define GPIOF_RESET 5 +#define GPIOG_RESET 6 +#define GPIOH_RESET 7 +#define GPIOI_RESET 8 +#define GPIOJ_RESET 9 +#define GPIOK_RESET 10 +#define CRC_RESET 12 +#define DMA1_RESET 21 +#define DMA2_RESET 22 +#define DMA2D_RESET 23 +#define ETHMAC_RESET 25 +#define OTGHS_RESET 29 + +/* AHB2 */ +#define DCMI_RESET 32 +#define CRYP_RESET 36 +#define HASH_RESET 37 +#define RNG_RESET 38 +#define OTGFS_RESET 39 + +/* AHB3 */ +#define FMC_RESET 64 + +/* APB1 */ +#define TIM2_RESET 128 +#define TIM3_RESET 129 +#define TIM4_RESET 130 +#define TIM5_RESET 131 +#define TIM6_RESET 132 +#define TIM7_RESET 133 +#define TIM12_RESET 134 +#define TIM13_RESET 135 +#define TIM14_RESET 136 +#define WWDG_RESET 139 +#define SPI2_RESET 142 +#define SPI3_RESET 143 +#define UART2_RESET 145 +#define UART3_RESET 146 +#define UART4_RESET 147 +#define UART5_RESET 148 +#define I2C1_RESET 149 +#define I2C2_RESET 150 +#define I2C3_RESET 151 +#define CAN1_RESET 153 +#define CAN2_RESET 154 +#define PWR_RESET 156 +#define DAC_RESET 157 +#define UART7_RESET 158 +#define UART8_RESET 159 + +/* APB2 */ +#define TIM1_RESET 160 +#define TIM8_RESET 161 +#define USART1_RESET 164 +#define USART6_RESET 165 +#define ADC_RESET 168 +#define SDIO_RESET 171 +#define SPI1_RESET 172 +#define SPI4_RESET 173 +#define SYSCFG_RESET 174 +#define TIM9_RESET 176 +#define TIM10_RESET 177 +#define TIM11_RESET 178 +#define SPI5_RESET 180 +#define SPI6_RESET 181 +#define SAI1_RESET 182 +#define LTDC_RESET 186 + +#endif /* _DT_BINDINGS_RESET_STM32F429_H */ +
The STM32 MCUs family IP can be reset by accessing some shared registers. The specificity is that some reset lines are used by the timers. At timer initialization time, the timer has to be reset, that's why we cannot use a regular driver. Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com> --- drivers/reset/Makefile | 1 + drivers/reset/reset-stm32.c | 124 +++++++++++++++++++++++++++++++ include/dt-bindings/reset/st,stm32f429.h | 85 +++++++++++++++++++++ 3 files changed, 210 insertions(+) create mode 100644 drivers/reset/reset-stm32.c create mode 100644 include/dt-bindings/reset/st,stm32f429.h