2018-03-13 16:37:15

by Gabriel FERNANDEZ

[permalink] [raw]
Subject: [PATCH 0/2] Introduce STM32MP1 Reset driver

From: Gabriel Fernandez <[email protected]>

This patch-set enables the reset of STM32MP1.
It uses the reset simple driver by introducing the clear register offset
parameter.
STM32MP1 reset IP has a register to assert by writing '1' and another
register to de-assert by writing '1'.
The offset between this two registers is '0x4'.

The patch 'dt-bindings: reset: add STM32MP1 resets' could be squashed
with the patch:
'dt-bindings: Document STM32MP1 Reset Clock Controller (RCC) bindings'
commit 3830681d354f

Gabriel Fernandez (2):
dt-bindings: reset: add STM32MP1 resets
reset: simple: Enable stm32mp1 reset driver

drivers/reset/reset-simple.c | 27 +++++--
drivers/reset/reset-simple.h | 1 +
include/dt-bindings/reset/stm32mp1-resets.h | 108 ++++++++++++++++++++++++++++
3 files changed, 130 insertions(+), 6 deletions(-)
create mode 100644 include/dt-bindings/reset/stm32mp1-resets.h

--
1.9.1



2018-03-13 16:36:57

by Gabriel FERNANDEZ

[permalink] [raw]
Subject: [PATCH 1/2] dt-bindings: reset: add STM32MP1 resets

From: Gabriel Fernandez <[email protected]>

This patch adds the reset binding entry for STM32MP1

Signed-off-by: Gabriel Fernandez <[email protected]>
---
include/dt-bindings/reset/stm32mp1-resets.h | 108 ++++++++++++++++++++++++++++
1 file changed, 108 insertions(+)
create mode 100644 include/dt-bindings/reset/stm32mp1-resets.h

diff --git a/include/dt-bindings/reset/stm32mp1-resets.h b/include/dt-bindings/reset/stm32mp1-resets.h
new file mode 100644
index 0000000..177e658
--- /dev/null
+++ b/include/dt-bindings/reset/stm32mp1-resets.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 or BSD-3-Clause */
+/*
+ * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
+ * Author: Gabriel Fernandez <[email protected]> for STMicroelectronics.
+ */
+
+#ifndef _DT_BINDINGS_STM32MP1_RESET_H_
+#define _DT_BINDINGS_STM32MP1_RESET_H_
+
+#define LTDC_R 3072
+#define DSI_R 3076
+#define DDRPERFM_R 3080
+#define USBPHY_R 3088
+#define SPI6_R 3136
+#define I2C4_R 3138
+#define I2C6_R 3139
+#define USART1_R 3140
+#define STGEN_R 3156
+#define GPIOZ_R 3200
+#define CRYP1_R 3204
+#define HASH1_R 3205
+#define RNG1_R 3206
+#define AXIM_R 3216
+#define GPU_R 3269
+#define ETHMAC_R 3274
+#define FMC_R 3276
+#define QSPI_R 3278
+#define SDMMC1_R 3280
+#define SDMMC2_R 3281
+#define CRC1_R 3284
+#define USBH_R 3288
+#define MDMA_R 3328
+#define MCU_R 8225
+#define TIM2_R 19456
+#define TIM3_R 19457
+#define TIM4_R 19458
+#define TIM5_R 19459
+#define TIM6_R 19460
+#define TIM7_R 19461
+#define TIM12_R 16462
+#define TIM13_R 16463
+#define TIM14_R 16464
+#define LPTIM1_R 19465
+#define SPI2_R 19467
+#define SPI3_R 19468
+#define USART2_R 19470
+#define USART3_R 19471
+#define UART4_R 19472
+#define UART5_R 19473
+#define UART7_R 19474
+#define UART8_R 19475
+#define I2C1_R 19477
+#define I2C2_R 19478
+#define I2C3_R 19479
+#define I2C5_R 19480
+#define SPDIF_R 19482
+#define CEC_R 19483
+#define DAC12_R 19485
+#define MDIO_R 19847
+#define TIM1_R 19520
+#define TIM8_R 19521
+#define TIM15_R 19522
+#define TIM16_R 19523
+#define TIM17_R 19524
+#define SPI1_R 19528
+#define SPI4_R 19529
+#define SPI5_R 19530
+#define USART6_R 19533
+#define SAI1_R 19536
+#define SAI2_R 19537
+#define SAI3_R 19538
+#define DFSDM_R 19540
+#define FDCAN_R 19544
+#define LPTIM2_R 19584
+#define LPTIM3_R 19585
+#define LPTIM4_R 19586
+#define LPTIM5_R 19587
+#define SAI4_R 19592
+#define SYSCFG_R 19595
+#define VREF_R 19597
+#define TMPSENS_R 19600
+#define PMBCTRL_R 19601
+#define DMA1_R 19648
+#define DMA2_R 19649
+#define DMAMUX_R 19650
+#define ADC12_R 19653
+#define USBO_R 19656
+#define SDMMC3_R 19664
+#define CAMITF_R 19712
+#define CRYP2_R 19716
+#define HASH2_R 19717
+#define RNG2_R 19718
+#define CRC2_R 19719
+#define HSEM_R 19723
+#define MBOX_R 19724
+#define GPIOA_R 19776
+#define GPIOB_R 19777
+#define GPIOC_R 19778
+#define GPIOD_R 19779
+#define GPIOE_R 19780
+#define GPIOF_R 19781
+#define GPIOG_R 19782
+#define GPIOH_R 19783
+#define GPIOI_R 19784
+#define GPIOJ_R 19785
+#define GPIOK_R 19786
+
+#endif /* _DT_BINDINGS_STM32MP1_RESET_H_ */
--
1.9.1


2018-03-13 16:37:03

by Gabriel FERNANDEZ

[permalink] [raw]
Subject: [PATCH 2/2] reset: simple: Enable stm32mp1 reset driver

From: Gabriel Fernandez <[email protected]>

The stm32mp1 reset driver is quite similar to simple reset driver.
The difference is that stm32mp1 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

Signed-off-by: Gabriel Fernandez <[email protected]>
---
drivers/reset/reset-simple.c | 27 +++++++++++++++++++++------
drivers/reset/reset-simple.h | 1 +
2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index f7ce891..57ecb49 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -41,15 +41,23 @@ static int reset_simple_update(struct reset_controller_dev *rcdev,
int offset = id % (reg_width * BITS_PER_BYTE);
unsigned long flags;
u32 reg;
+ void __iomem *addr;

spin_lock_irqsave(&data->lock, flags);

- reg = readl(data->membase + (bank * reg_width));
- if (assert ^ data->active_low)
- reg |= BIT(offset);
- else
- reg &= ~BIT(offset);
- writel(reg, data->membase + (bank * reg_width));
+ addr = data->membase + (bank * reg_width);
+ if (data->clr_offset) {
+ reg = BIT(offset);
+ if (!assert)
+ addr += data->clr_offset;
+ } else {
+ reg = readl(addr);
+ if (assert ^ data->active_low)
+ reg |= BIT(offset);
+ else
+ reg &= ~BIT(offset);
+ }
+ writel(reg, addr);

spin_unlock_irqrestore(&data->lock, flags);

@@ -103,6 +111,7 @@ struct reset_simple_devdata {
u32 nr_resets;
bool active_low;
bool status_active_low;
+ u32 clr_offset;
};

#define SOCFPGA_NR_BANKS 8
@@ -118,9 +127,14 @@ struct reset_simple_devdata {
.status_active_low = true,
};

+struct reset_simple_devdata reset_stm32mp1 = {
+ .clr_offset = 0x4,
+};
+
static const struct of_device_id reset_simple_dt_ids[] = {
{ .compatible = "altr,rst-mgr", .data = &reset_simple_socfpga },
{ .compatible = "st,stm32-rcc", },
+ { .compatible = "st,stm32mp1-rcc", .data = &reset_stm32mp1},
{ .compatible = "allwinner,sun6i-a31-clock-reset",
.data = &reset_simple_active_low },
{ .compatible = "zte,zx296718-reset",
@@ -163,6 +177,7 @@ static int reset_simple_probe(struct platform_device *pdev)
data->rcdev.nr_resets = devdata->nr_resets;
data->active_low = devdata->active_low;
data->status_active_low = devdata->status_active_low;
+ data->clr_offset = devdata->clr_offset;
}

if (of_device_is_compatible(dev->of_node, "altr,rst-mgr") &&
diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
index 8a49602..0bbdd34 100644
--- a/drivers/reset/reset-simple.h
+++ b/drivers/reset/reset-simple.h
@@ -38,6 +38,7 @@ struct reset_simple_data {
struct reset_controller_dev rcdev;
bool active_low;
bool status_active_low;
+ u32 clr_offset;
};

extern const struct reset_control_ops reset_simple_ops;
--
1.9.1


2018-03-14 09:13:35

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH 0/2] Introduce STM32MP1 Reset driver

Hi Gabriel,

On Tue, 2018-03-13 at 17:34 +0100, [email protected] wrote:
> From: Gabriel Fernandez <[email protected]>
>
> This patch-set enables the reset of STM32MP1.
> It uses the reset simple driver by introducing the clear register offset
> parameter.
> STM32MP1 reset IP has a register to assert by writing '1' and another
> register to de-assert by writing '1'.
> The offset between this two registers is '0x4'.

I worry a bit about feature creep in the simple-reset driver.
Your patch on its own is simple enough, and I'm not opposed to add a
SET/CLR feature on principle, but there are a few issues:

The RESET_SIMPLE Kconfig description currently says:
  "This enables a simple reset controller driver for reset lines that
   that can be asserted and deasserted by toggling bits in a contiguous,
   exclusive register space."
That would have to be extended to mention SET/CLR register pairs as an
alternative.

What about status (reset_simple_status)? Can current reset line status
be read back from the SET register, as is currently tried? If not, is
there a way to read current reset line status back at all?

The data->lock spinlock is only needed to protect the read-modify-write
cycle on a toggle register, for separate SET/CLR register access the
locking is not necessary.

At this point, it may or may not be easier to add a custom reset driver.
Either way you go, this is missing binding documentation for the
st,stm32mp1-rcc compatible in Documentation/devicetree/bindings/reset.

> The patch 'dt-bindings: reset: add STM32MP1 resets' could be squashed
> with the patch:
> 'dt-bindings: Document STM32MP1 Reset Clock Controller (RCC) bindings'
> commit 3830681d354f
>
> Gabriel Fernandez (2):
> dt-bindings: reset: add STM32MP1 resets
> reset: simple: Enable stm32mp1 reset driver
>
> drivers/reset/reset-simple.c | 27 +++++--
> drivers/reset/reset-simple.h | 1 +
> include/dt-bindings/reset/stm32mp1-resets.h | 108 ++++++++++++++++++++++++++++
> 3 files changed, 130 insertions(+), 6 deletions(-)
> create mode 100644 include/dt-bindings/reset/stm32mp1-resets.h

regards
Philipp

2018-03-14 09:44:10

by Gabriel FERNANDEZ

[permalink] [raw]
Subject: Re: [PATCH 0/2] Introduce STM32MP1 Reset driver

Hi Philipp,

Okay, i too support the idea to add custom  reset driver.

Many Thanks Philipp.

Best regards

Gabriel


On 03/14/2018 10:12 AM, Philipp Zabel wrote:
> Hi Gabriel,
>
> On Tue, 2018-03-13 at 17:34 +0100, [email protected] wrote:
>> From: Gabriel Fernandez <[email protected]>
>>
>> This patch-set enables the reset of STM32MP1.
>> It uses the reset simple driver by introducing the clear register offset
>> parameter.
>> STM32MP1 reset IP has a register to assert by writing '1' and another
>> register to de-assert by writing '1'.
>> The offset between this two registers is '0x4'.
> I worry a bit about feature creep in the simple-reset driver.
> Your patch on its own is simple enough, and I'm not opposed to add a
> SET/CLR feature on principle, but there are a few issues:
>
> The RESET_SIMPLE Kconfig description currently says:
>   "This enables a simple reset controller driver for reset lines that
>    that can be asserted and deasserted by toggling bits in a contiguous,
>    exclusive register space."
> That would have to be extended to mention SET/CLR register pairs as an
> alternative.
>
> What about status (reset_simple_status)? Can current reset line status
> be read back from the SET register, as is currently tried? If not, is
> there a way to read current reset line status back at all?
>
> The data->lock spinlock is only needed to protect the read-modify-write
> cycle on a toggle register, for separate SET/CLR register access the
> locking is not necessary.
>
> At this point, it may or may not be easier to add a custom reset driver.
> Either way you go, this is missing binding documentation for the
> st,stm32mp1-rcc compatible in Documentation/devicetree/bindings/reset.
>
>> The patch 'dt-bindings: reset: add STM32MP1 resets' could be squashed
>> with the patch:
>> 'dt-bindings: Document STM32MP1 Reset Clock Controller (RCC) bindings'
>> commit 3830681d354f
>>
>> Gabriel Fernandez (2):
>> dt-bindings: reset: add STM32MP1 resets
>> reset: simple: Enable stm32mp1 reset driver
>>
>> drivers/reset/reset-simple.c | 27 +++++--
>> drivers/reset/reset-simple.h | 1 +
>> include/dt-bindings/reset/stm32mp1-resets.h | 108 ++++++++++++++++++++++++++++
>> 3 files changed, 130 insertions(+), 6 deletions(-)
>> create mode 100644 include/dt-bindings/reset/stm32mp1-resets.h
> regards
> Philipp