[v2,07/18] drivers: reset: Add STM32 reset driver
diff mbox

Message ID 1424455277-29983-8-git-send-email-mcoquelin.stm32@gmail.com
State New
Headers show

Commit Message

Maxime Coquelin Feb. 20, 2015, 6:01 p.m. UTC
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

Comments

Arnd Bergmann March 10, 2015, 3:02 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin March 10, 2015, 3:41 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin March 10, 2015, 3:44 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann March 10, 2015, 8:21 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin March 10, 2015, 9:20 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philipp Zabel March 11, 2015, 1:08 p.m. UTC | #6
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

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin March 12, 2015, 9:05 p.m. UTC | #7
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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 */
+