Re: [PATCH v2 10/18] dt-bindings: Document the STM32 pin controller

Re: [PATCH v2 10/18] dt-bindings: Document the STM32 pin controller

[Linus Walleij]

On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
<mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> This adds documentation of device tree bindings for the
> STM32 pin controller.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
(...)
> +Pin controller node:
> +Required properies:
> +- compatible   : "st,stm32-pinctrl"
> +- #address-cells: The value of this property must be 1
> +- #size-cells  : The value of this property must be 1
> +- ranges       : defines mapping between pin controller node (parent) to
> +  gpio-bank node (children).
> +
> +GPIO controller/bank node:
> +Required properties:
> +- gpio-controller : Indicates this device is a GPIO controller
> +- #gpio-cells    : Should be two.
> +                       The first cell is the pin number
> +                       The second one is the polarity:
> +                               - 0 for active high
> +                               - 1 for active low
> +- reg            : The gpio address range, relative to the pinctrl range
> +- st,bank-name   : Should be a name string for this bank as specified in
> +  the datasheet

OK...

(...)
> +               ...
> +               pin-functions nodes follow...
> +       };
> +
> +Contents of function subnode node:
> +----------------------------------
> +
> +Required properties for pin configuration node:
> +- st,pins      : Child node with list of pins with configuration.

Don't use vendor prefix. Use the new standard notation, just "pins".
See Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

> +Below is the format of how each pin conf should look like.
> +
> +<bank offset altmode pull type speed>
> +
> +Every PIO is represented with 4 to 6 parameters.
> +Each parameter is explained as below.
> +
> +- bank   : Should be bank phandle to which this PIO belongs.
> +- offset  : Offset in the PIO bank.

No. Don't hardcode register offsets into the DTS files. This is
something the driver should know from the compatible string or
instance number, not by explicit reference.

> +- altmode : Should be mode or alternate function number associated this pin, as
> +described in the datasheet (IN, OUT, ALT0...ALT15, ANALOG)
> +- pull   : Should be either NO_PULL, PULL_UP or PULL_DOWN
> +- type   : Should be either PUSH_PULL or OPEN_DRAIN.
> +           Setting it is not needed for IN and ANALOG modes, or alternate
> +           functions acting as inputs.
> +- speed          : Value taken from the datasheet, depending on the function
> +(LOW_SPEED, MEDIUM_SPEED, FAST_SPEED, HIGH_SPEED)
> +           Setting it is not needed for IN and ANALOG modes, or alternate
> +           functions acting as inputs.

NACK. Make your Kconfig select GENERIC_PINCONF and use the
already existing generic pin configuration specifiers from the standard
bindings. Like bias-pull-up; etc.

This kind of custom pin config is a thing of the past.

> +usart1 {
> +       pinctrl_usart1: usart1-0 {
> +               st,pins {
> +                       tx = <&gpioa 9 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
> +                       rx = <&gpioa 10 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
> +               };
> +       };
> +};

You might have to use separate parameters for muxing and config
in your description.

Yours,
Linus Walleij

Re: [PATCH v2 10/18] dt-bindings: Document the STM32 pin controller

[Linus Walleij]

I saw this other thing:

On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> This adds documentation of device tree bindings for the
> STM32 pin controller.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
(...)
> +- altmode : Should be mode or alternate function number associated this pin, as
> +described in the datasheet (IN, OUT, ALT0...ALT15, ANALOG)

We can now describe muxing (altmodes etc) in two ways as described
in the generic bindings in
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

This is done by strings combining a function with N groups.

We are also discussing having a single config number setting up
all and keeping down the size of the DTB (which
is close to what you're doing here). Please take part in that
discussion to standardize such bindings. Sascha Hauer and
others are involved, don't know the exact topic right now but
it involved using a single "pinmux" parameter in the device treel.

All agree on using the standardized pin config bindings henceforth
so start by migrating to these.

Yours,
Linus Walleij

Re: [PATCH v2 02/18] ARM: ARMv7M: Enlarge vector table to 256 entries

[Uwe Kleine-König]

On Fri, Feb 20, 2015 at 07:01:01PM +0100, Maxime Coquelin wrote:
> From Cortex-M reference manuals, the nvic supports up to 240 interrupts.
> So the number of entries in vectors table is up to 256.
> 
> This patch adds a new config flag to specify the number of external interrupts.
> Some ifdeferies are added in order to respect the natural alignment without
> wasting too much space on smaller systems.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  arch/arm/kernel/entry-v7m.S | 13 +++++++++----
>  arch/arm/mm/Kconfig         | 15 +++++++++++++++
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
> index 8944f49..68cde36 100644
> --- a/arch/arm/kernel/entry-v7m.S
> +++ b/arch/arm/kernel/entry-v7m.S
> @@ -117,9 +117,14 @@ ENTRY(__switch_to)
>  ENDPROC(__switch_to)
>  
>  	.data
> -	.align	8
> +#if CONFIG_CPUV7M_NUM_IRQ <= 112
I would have called this CONFIG_CPU_V7M_NUM_IRQ to match the already
existing CPU_V7M symbol.

> +	.align	9
> +#else
> +	.align	10
> +#endif
> +

Other than that:
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Who do you target to apply this series?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 02/18] ARM: ARMv7M: Enlarge vector table to 256 entries

[Maxime Coquelin]

2015-02-20 20:47 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Fri, Feb 20, 2015 at 07:01:01PM +0100, Maxime Coquelin wrote:
>> From Cortex-M reference manuals, the nvic supports up to 240 interrupts.
>> So the number of entries in vectors table is up to 256.
>>
>> This patch adds a new config flag to specify the number of external interrupts.
>> Some ifdeferies are added in order to respect the natural alignment without
>> wasting too much space on smaller systems.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> ---
>>  arch/arm/kernel/entry-v7m.S | 13 +++++++++----
>>  arch/arm/mm/Kconfig         | 15 +++++++++++++++
>>  2 files changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
>> index 8944f49..68cde36 100644
>> --- a/arch/arm/kernel/entry-v7m.S
>> +++ b/arch/arm/kernel/entry-v7m.S
>> @@ -117,9 +117,14 @@ ENTRY(__switch_to)
>>  ENDPROC(__switch_to)
>>
>>       .data
>> -     .align  8
>> +#if CONFIG_CPUV7M_NUM_IRQ <= 112
> I would have called this CONFIG_CPU_V7M_NUM_IRQ to match the already
> existing CPU_V7M symbol.

That's better indeed.
It will be changed in v3.

>
>> +     .align  9
>> +#else
>> +     .align  10
>> +#endif
>> +
>
> Other than that:
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> Who do you target to apply this series?

I guess it should go through Russell's Patch Tracking System?

Thanks,
Maxime
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 02/18] ARM: ARMv7M: Enlarge vector table to 256 entries

[Maxime Coquelin]

2015-02-23 11:33 GMT+01:00 Maxime Coquelin <mcoquelin.stm32@gmail.com>:
> 2015-02-20 20:47 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> On Fri, Feb 20, 2015 at 07:01:01PM +0100, Maxime Coquelin wrote:
>>> From Cortex-M reference manuals, the nvic supports up to 240 interrupts.
>>> So the number of entries in vectors table is up to 256.
>>>
>>> This patch adds a new config flag to specify the number of external interrupts.
>>> Some ifdeferies are added in order to respect the natural alignment without
>>> wasting too much space on smaller systems.
>>>
>>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>>> ---
>>>  arch/arm/kernel/entry-v7m.S | 13 +++++++++----
>>>  arch/arm/mm/Kconfig         | 15 +++++++++++++++
>>>  2 files changed, 24 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
>>> index 8944f49..68cde36 100644
>>> --- a/arch/arm/kernel/entry-v7m.S
>>> +++ b/arch/arm/kernel/entry-v7m.S
>>> @@ -117,9 +117,14 @@ ENTRY(__switch_to)
>>>  ENDPROC(__switch_to)
>>>
>>>       .data
>>> -     .align  8
>>> +#if CONFIG_CPUV7M_NUM_IRQ <= 112
>> I would have called this CONFIG_CPU_V7M_NUM_IRQ to match the already
>> existing CPU_V7M symbol.
>
> That's better indeed.
> It will be changed in v3.
>
>>
>>> +     .align  9
>>> +#else
>>> +     .align  10
>>> +#endif
>>> +
>>
>> Other than that:
>> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>
>> Who do you target to apply this series?
>
> I guess it should go through Russell's Patch Tracking System?

Sorry, I answered your question too quickly.
I meant this patch should go through Russell's Patch Tracking System.

For the other patches, I think it should be picked by their respective
maintainers.
Or I can create an immutable tag (on github or kernel.org?) that will
be merged by the different sub-systems?
What would you advise?

Thanks,
Maxime

>
> Thanks,
> Maxime
>>
>> Best regards
>> Uwe
>>
>> --
>> Pengutronix e.K.                           | Uwe Kleine-König            |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 02/18] ARM: ARMv7M: Enlarge vector table to 256 entries

[Uwe Kleine-König]

On Thu, Feb 26, 2015 at 11:29:52AM +0100, Maxime Coquelin wrote:
> 2015-02-23 11:33 GMT+01:00 Maxime Coquelin <mcoquelin.stm32@gmail.com>:
> > 2015-02-20 20:47 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> Who do you target to apply this series?
> >
> > I guess it should go through Russell's Patch Tracking System?
> 
> Sorry, I answered your question too quickly.
> I meant this patch should go through Russell's Patch Tracking System.
> 
> For the other patches, I think it should be picked by their respective
> maintainers.
> Or I can create an immutable tag (on github or kernel.org?) that will
> be merged by the different sub-systems?
> What would you advise?
Depends on the interdependencies of your patches. If each maintainer can
just pick up the patches affecting his area on top of (say) 4.0-rc1 that
would be good.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 02/18] ARM: ARMv7M: Enlarge vector table to 256 entries

[Stefan Agner]

On 2015-02-20 19:01, Maxime Coquelin wrote:
> From Cortex-M reference manuals, the nvic supports up to 240 interrupts.
> So the number of entries in vectors table is up to 256.
> 
> This patch adds a new config flag to specify the number of external interrupts.
> Some ifdeferies are added in order to respect the natural alignment without
> wasting too much space on smaller systems.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  arch/arm/kernel/entry-v7m.S | 13 +++++++++----
>  arch/arm/mm/Kconfig         | 15 +++++++++++++++
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
> index 8944f49..68cde36 100644
> --- a/arch/arm/kernel/entry-v7m.S
> +++ b/arch/arm/kernel/entry-v7m.S
> @@ -117,9 +117,14 @@ ENTRY(__switch_to)
>  ENDPROC(__switch_to)
>  
>  	.data
> -	.align	8
> +#if CONFIG_CPUV7M_NUM_IRQ <= 112
> +	.align	9
> +#else
> +	.align	10
> +#endif
> +
>  /*
> - * Vector table (64 words => 256 bytes natural alignment)
> + * Vector table (Natural alignment need to be ensured)
>   */
>  ENTRY(vector_table)
>  	.long	0			@ 0 - Reset stack pointer
> @@ -138,6 +143,6 @@ ENTRY(vector_table)
>  	.long	__invalid_entry		@ 13 - Reserved
>  	.long	__pendsv_entry		@ 14 - PendSV
>  	.long	__invalid_entry		@ 15 - SysTick
> -	.rept	64 - 16
> -	.long	__irq_entry		@ 16..64 - External Interrupts
> +	.rept	CONFIG_CPUV7M_NUM_IRQ
> +	.long	__irq_entry		@ External Interrupts
>  	.endr
> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index c43c714..27eb835 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -604,6 +604,21 @@ config CPU_USE_DOMAINS
>  	  This option enables or disables the use of domain switching
>  	  via the set_fs() function.
>  
> +config CPUV7M_NUM_IRQ
> +	int "Number of external interrupts connected to the NVIC"
> +	depends on CPU_V7M
> +	default 90 if ARCH_STM32
> +	default 38 if ARCH_EFM32
> +	default 240
> +	help
> +	  This option indicates the number of interrupts connected to the NVIC.
> +	  The value can be larger than the real number of interrupts supported
> +	  by the system, but must not be lower.
> +	  The default value is 240, corresponding to the maximum number of
> +	  interrupts supported by the NVIC on Cortex-M family.
> +
> +	  If unsure, keep default value.
> +
>  #
>  # CPU supports 36-bit I/O
>  #

I sent a patch which extended that vector table some weeks ago:
https://lkml.org/lkml/2014/12/29/296

But your solution is definitely more flexible, and given that we deal
with small devices here, it's worth saving memory.

Acked-by: Stefan Agner <stefan@agner.ch>

Re: [PATCH v2 02/18] ARM: ARMv7M: Enlarge vector table to 256 entries

[Maxime Coquelin]

2015-03-09 1:29 GMT+01:00 Stefan Agner <stefan@agner.ch>:
> On 2015-02-20 19:01, Maxime Coquelin wrote:
>> From Cortex-M reference manuals, the nvic supports up to 240 interrupts.
>> So the number of entries in vectors table is up to 256.
>>
>> This patch adds a new config flag to specify the number of external interrupts.
>> Some ifdeferies are added in order to respect the natural alignment without
>> wasting too much space on smaller systems.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> ---
>>  arch/arm/kernel/entry-v7m.S | 13 +++++++++----
>>  arch/arm/mm/Kconfig         | 15 +++++++++++++++
>>  2 files changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
>> index 8944f49..68cde36 100644
>> --- a/arch/arm/kernel/entry-v7m.S
>> +++ b/arch/arm/kernel/entry-v7m.S
>> @@ -117,9 +117,14 @@ ENTRY(__switch_to)
>>  ENDPROC(__switch_to)
>>
>>       .data
>> -     .align  8
>> +#if CONFIG_CPUV7M_NUM_IRQ <= 112
>> +     .align  9
>> +#else
>> +     .align  10
>> +#endif
>> +
>>  /*
>> - * Vector table (64 words => 256 bytes natural alignment)
>> + * Vector table (Natural alignment need to be ensured)
>>   */
>>  ENTRY(vector_table)
>>       .long   0                       @ 0 - Reset stack pointer
>> @@ -138,6 +143,6 @@ ENTRY(vector_table)
>>       .long   __invalid_entry         @ 13 - Reserved
>>       .long   __pendsv_entry          @ 14 - PendSV
>>       .long   __invalid_entry         @ 15 - SysTick
>> -     .rept   64 - 16
>> -     .long   __irq_entry             @ 16..64 - External Interrupts
>> +     .rept   CONFIG_CPUV7M_NUM_IRQ
>> +     .long   __irq_entry             @ External Interrupts
>>       .endr
>> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
>> index c43c714..27eb835 100644
>> --- a/arch/arm/mm/Kconfig
>> +++ b/arch/arm/mm/Kconfig
>> @@ -604,6 +604,21 @@ config CPU_USE_DOMAINS
>>         This option enables or disables the use of domain switching
>>         via the set_fs() function.
>>
>> +config CPUV7M_NUM_IRQ
>> +     int "Number of external interrupts connected to the NVIC"
>> +     depends on CPU_V7M
>> +     default 90 if ARCH_STM32
>> +     default 38 if ARCH_EFM32
>> +     default 240
>> +     help
>> +       This option indicates the number of interrupts connected to the NVIC.
>> +       The value can be larger than the real number of interrupts supported
>> +       by the system, but must not be lower.
>> +       The default value is 240, corresponding to the maximum number of
>> +       interrupts supported by the NVIC on Cortex-M family.
>> +
>> +       If unsure, keep default value.
>> +
>>  #
>>  # CPU supports 36-bit I/O
>>  #
>
> I sent a patch which extended that vector table some weeks ago:
> https://lkml.org/lkml/2014/12/29/296

I did something similar in my first version.

>
> But your solution is definitely more flexible, and given that we deal
> with small devices here, it's worth saving memory.

Yes, it is worth for these small devices.

>
> Acked-by: Stefan Agner <stefan@agner.ch>
>

Thanks for the review,
Maxime

Re: [PATCH v2 16/18] ARM: dts: Introduce STM32F429 MCU

[Maxime Coquelin]

Hi Andreas,

Thanks for this detailed review.

2015-03-02 18:42 GMT+01:00 Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>:
> Hi Maxime,
>
> Please don't put the whole world in To, that makes replying to you much
> harder. You can put maintainers in CC instead.
Ok.
>
> Am 20.02.2015 um 19:01 schrieb Maxime Coquelin:
>> The STMicrolectornics's STM32F419 MCU has the following main features:
>>  - Cortex-M4 core running up to @180MHz
>>  - 2MB internal flash, 256KBytes internal RAM
>>  - FMC controller to connect SDRAM, NOR and NAND memories
>>  - SD/MMC/SDIO support
>>  - Ethernet controller
>>  - USB OTFG FS & HS controllers
>>  - I2C, SPI, CAN busses support
>>  - Several 16 & 32 bits general purpose timers
>>  - Serial Audio interface
>>  - LCD controller
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/Makefile            |   1 +
>>  arch/arm/boot/dts/stm32f429-disco.dts |  41 ++++
>>  arch/arm/boot/dts/stm32f429.dtsi      | 396 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 438 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/stm32f429-disco.dts
>>  create mode 100644 arch/arm/boot/dts/stm32f429.dtsi
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 91bd5bd..d7da0ef 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -442,6 +442,7 @@ dtb-$(CONFIG_ARCH_STI)+= stih407-b2120.dtb \
>>       stih416-b2000.dtb \
>>       stih416-b2020.dtb \
>>       stih416-b2020e.dtb
>> +dtb-$(CONFIG_ARCH_STM32)+= stm32f429-disco.dtb
>>  dtb-$(CONFIG_MACH_SUN4I) += \
>>       sun4i-a10-a1000.dtb \
>>       sun4i-a10-ba10-tvbox.dtb \
>> diff --git a/arch/arm/boot/dts/stm32f429-disco.dts b/arch/arm/boot/dts/stm32f429-disco.dts
>> new file mode 100644
>> index 0000000..0e79cc1
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/stm32f429-disco.dts
>> @@ -0,0 +1,41 @@
>
> Add a suitable license header here? There had been attempts to
> dual-license as GPL and MIT/X11.

I agree to add the dual GPL and MIT/X11 licence.

>
>> +/dts-v1/;
>> +#include "stm32f429.dtsi"
>> +
>> +/ {
>> +     model = "STMicroelectronics's STM32F429i-DISCO board";
>
> "'s" seems uncommon here and "s's" looks wrong. Just use
> "STMicroelectronics STM32F429I-DISCO board"?

Ok, it will be fixed in v3.

>
>> +     compatible = "st,stm32f429i-disco", "st,stm32f429";
>> +
>> +     chosen {
>> +             bootargs = "console=ttyS0,115200 root=/dev/ram rdinit=/linuxrc";
>> +             linux,stdout-path = &usart1;
>
> I have actually been using USART3, following a guide I found on GitHub.
> Which pins do you use for USART1, and is one better than the other?

I used U-boot from Emcraft, which uses USART1, that is the only reason
I used this one.
Regarding USART3, I had a look at your boot-wrapper, and you configure
the USART3 to PC10/PC11 pins.

From the schematics, I see the PC10 pin is connected to the LCD.


>
>> +     };
>> +
>> +     memory {
>> +             reg = <0xd0000000 0x800000>;
>
> I have it at 0x90000000!

Ok. But you use a specific setting:
https://github.com/afaerber/afboot-stm32/blob/master/foo.c#L338

By default this is the PC Card bank (Bank 4) at 0x90000000.

Maybe you could change your boot-wrapper to use also the default setting?

>
>> +     };
>> +
>> +     aliases {
>> +             ttyS0 = &usart1;
>
> Why ttyS0 here? Shouldn't that be serial0 by convention?

I was not aware of this. Do you know whether it is documented somewhere?
Anyway, I will change to serial0 in next version.

>
>> +     };
>> +
>> +     soc {
>> +             usart1: usart@40011000 {
>> +                     status = "okay";
>> +             };
>
> This is a new target, so please use the new-style &usart1 {...};
> reference rather than replicating the hierarchy.
Ok.

>
>> +
>> +             leds {
>
> These LEDs are definitely not on the SoC, they're on the board. Please
> move one level up.

Ok.

>
>> +                     compatible = "gpio-leds";
>
> In this file you're being parsimonious with newline, yet in the .dtsi
> you unnecessarily add newlines before the trailing status property.
>
>> +                     red {
>> +                             #gpio-cells = <2>;
>
> This looks weird. Drop it?

Yes.

>
>> +                             label = "Front Panel LED";
>
> "Front Panel"? Both LEDs are on the front, and several other
> architectures avoid spaces in the label for accessing it from /sys.

That's an unfortunate copy/paste. For sure it has no meaning here.

>
>> +                             gpios = <&gpiog 14 0>;
>
> GPIO_ACTIVE_HIGH
Ok.

>
>> +                             linux,default-trigger = "heartbeat";
>
> Suggest to leave both off by default.
Ok.

>
>> +                     };
>> +                     green {
>> +                             #gpio-cells = <2>;
>
> Ditto.
>
>> +                             gpios = <&gpiog 13 0>;
>
> Ditto.
>
>> +                             default-state = "off";
>> +                     };
>> +             };
>> +     };
>> +};
>> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
>> new file mode 100644
>> index 0000000..5b3442a
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/stm32f429.dtsi
>> @@ -0,0 +1,396 @@
>> +/*
>> + * Device tree for STM32F429
>
> License?

Yes. I will do the same as for the board file.

>
>> + */
>> +#include "armv7-m.dtsi"
>> +#include <dt-bindings/pinctrl/pinctrl-stm32.h>
>> +#include <dt-bindings/reset/st,stm32f429.h>
>> +
>> +/ {
>> +
>> +     aliases {
>> +             gpio0 = &gpioa;
>> +             gpio1 = &gpiob;
>> +             gpio2 = &gpioc;
>> +             gpio3 = &gpiod;
>> +             gpio4 = &gpioe;
>> +             gpio5 = &gpiof;
>> +             gpio6 = &gpiog;
>> +             gpio7 = &gpioh;
>> +             gpio8 = &gpioi;
>> +             gpio9 = &gpioj;
>> +             gpio10 = &gpiok;
>> +     };
>> +
>> +     clocks {
>> +             clk_sysclk: clk-sysclk {
>> +                     #clock-cells = <0>;
>> +                     compatible = "fixed-clock";
>> +                     clock-frequency = <180000000>;
>> +             };
>> +
>> +             clk_hclk: clk-hclk {
>> +                     #clock-cells = <0>;
>> +                     compatible = "fixed-clock";
>> +                     clock-frequency = <180000000>;
>> +             };
>> +
>> +             clk_pclk1: clk-pclk1 {
>> +                     #clock-cells = <0>;
>> +                     compatible = "fixed-clock";
>> +                     clock-frequency = <45000000>;
>> +             };
>> +
>> +             clk_pclk2: clk-pclk2 {
>> +                     #clock-cells = <0>;
>> +                     compatible = "fixed-clock";
>> +                     clock-frequency = <90000000>;
>> +             };
>> +
>> +             clk_pmtr1: clk-pmtr1 {
>> +                     #clock-cells = <0>;
>> +                     compatible = "fixed-clock";
>> +                     clock-frequency = <90000000>;
>> +             };
>> +
>> +             clk_pmtr2: clk-pmtr2 {
>> +                     #clock-cells = <0>;
>> +                     compatible = "fixed-clock";
>> +                     clock-frequency = <180000000>;
>> +             };
>> +
>> +             clk_systick: clk-systick {
>> +                     compatible = "fixed-factor-clock";
>> +                     clocks = <&clk_hclk>;
>> +                     #clock-cells = <0>;
>> +                     clock-div = <8>;
>> +                     clock-mult = <1>;
>> +             };
>
> Most of these should be replaceable with my clk driver.
>

I agree, but adding more drivers is likely to delay the STM32 machine
to be merged.
Once this first round accepted, it will be easier for others to contribute.
It will help to have sooner a good support of the machine.

Does that look reasonable for you?

>> +     };
>> +
>> +     systick: timer@e000e010 {
>> +             clocks = <&clk_systick>;
>> +
>> +             status = "okay";
>> +     };
>
> &systick {...};
Ok.

>
>> +
>> +     soc {
>> +             reset: reset@40023810 {
>> +                     #reset-cells = <1>;
>> +                     compatible = "st,stm32-reset";
>> +                     reg = <0x40023810 0x18>;
>> +             };
>
> Still, this feels wrong, given that it's an RCC IP block...
Ok, this was already pointed out by Philipp.
I will rework the driver so that the node covers the full RCC IP.


>
>> +
>> +             pin-controller {
>> +                     #address-cells = <1>;
>> +                     #size-cells = <1>;
>> +                     compatible = "st,stm32-pinctrl";
>> +                     ranges = <0 0x40020000 0x3000>;
>> +
>> +                     gpioa: gpio@40020000 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x0 0x400>;
>> +                             resets = <&reset GPIOA_RESET>;
>> +                             st,bank-name = "GPIOA";
>> +                     };
>> +
>> +                     gpiob: gpio@40020400 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x400 0x400>;
>> +                             resets = <&reset GPIOB_RESET>;
>> +                             st,bank-name = "GPIOB";
>> +                     };
>> +
>> +                     gpioc: gpio@40020800 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x800 0x400>;
>> +                             resets = <&reset GPIOC_RESET>;
>> +                             st,bank-name = "GPIOC";
>> +                     };
>> +
>> +                     gpiod: gpio@40020c00 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0xc00 0x400>;
>> +                             resets = <&reset GPIOD_RESET>;
>> +                             st,bank-name = "GPIOD";
>> +                     };
>> +
>> +                     gpioe: gpio@40021000 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x1000 0x400>;
>> +                             resets = <&reset GPIOE_RESET>;
>> +                             st,bank-name = "GPIOE";
>> +                     };
>> +
>> +                     gpiof: gpio@40021400 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x1400 0x400>;
>> +                             resets = <&reset GPIOF_RESET>;
>> +                             st,bank-name = "GPIOF";
>> +                     };
>> +
>> +                     gpiog: gpio@40021800 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x1800 0x400>;
>> +                             resets = <&reset GPIOG_RESET>;
>> +                             st,bank-name = "GPIOG";
>> +                     };
>> +
>> +                     gpioh: gpio@40021c00 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x1c00 0x400>;
>> +                             resets = <&reset GPIOH_RESET>;
>> +                             st,bank-name = "GPIOH";
>> +                     };
>> +
>> +                     gpioi: gpio@40022000 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x2000 0x400>;
>> +                             resets = <&reset GPIOI_RESET>;
>> +                             st,bank-name = "GPIOI";
>> +                     };
>> +
>> +                     gpioj: gpio@40022400 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x2400 0x400>;
>> +                             resets = <&reset GPIOJ_RESET>;
>> +                             st,bank-name = "GPIOJ";
>> +                     };
>> +
>> +                     gpiok: gpio@40022800 {
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                             reg = <0x2800 0x400>;
>> +                             resets = <&reset GPIOK_RESET>;
>> +                             st,bank-name = "GPIOK";
>> +                     };
>> +
>> +                     usart1 {
>> +                             pinctrl_usart1: usart1-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpioa 9 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpioa 10 ALT7 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>> +
>> +                     usart2 {
>> +                             pinctrl_usart2: usart2-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpioa 2 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpioa 3 ALT7 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>> +
>> +                     usart3 {
>> +                             pinctrl_usart3: usart3-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpiob 10 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpiob 11 ALT7 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>> +
>> +                     usart4 {
>> +                             pinctrl_usart4: usart4-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpioa 0 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpioa 1 ALT8 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>> +
>> +                     usart5 {
>> +                             pinctrl_usart5: usart5-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpioc 12 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpiod 2 ALT8 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>> +
>> +                     usart6 {
>> +                             pinctrl_usart6: usart6-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpioc 6 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpioc 7 ALT8 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>> +
>> +                     usart7 {
>> +                             pinctrl_usart7: usart7-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpioe 8 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpioe 7 ALT8 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>> +
>> +                     usart8 {
>> +                             pinctrl_usart8: usart8-0 {
>> +                                     st,pins {
>> +                                             tx = <&gpioe 1 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
>> +                                             rx = <&gpioe 0 ALT8 NO_PULL>;
>> +                                     };
>> +                             };
>> +                     };
>
> Can't the outer level be avoided here to save space and indentation?
Yes. I will rework this too.

>
>> +             };
>> +
>> +             timer2: timer@40000000 {
>> +                     compatible = "st,stm32-timer";
>> +                     reg = <0x40000000 0x400>;
>> +                     interrupts = <28>;
>> +                     resets = <&reset TIM2_RESET>;
>> +                     clocks = <&clk_pmtr1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             timer3: timer@40000400 {
>> +                     compatible = "st,stm32-timer";
>> +                     reg = <0x40000400 0x400>;
>> +                     interrupts = <29>;
>> +                     resets = <&reset TIM3_RESET>;
>> +                     clocks = <&clk_pmtr1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             timer4: timer@40000800 {
>> +                     compatible = "st,stm32-timer";
>> +                     reg = <0x40000800 0x400>;
>> +                     interrupts = <30>;
>> +                     resets = <&reset TIM4_RESET>;
>> +                     clocks = <&clk_pmtr1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             timer5: timer@40000c00 {
>> +                     compatible = "st,stm32-timer";
>> +                     reg = <0x40000c00 0x400>;
>> +                     interrupts = <50>;
>> +                     resets = <&reset TIM5_RESET>;
>> +                     clocks = <&clk_pmtr1>;
>> +             };
>> +
>> +             timer6: timer@40001000 {
>> +                     compatible = "st,stm32-timer";
>> +                     reg = <0x40001000 0x400>;
>> +                     interrupts = <54>;
>> +                     resets = <&reset TIM6_RESET>;
>> +                     clocks = <&clk_pmtr1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             timer7: timer@40001400 {
>> +                     compatible = "st,stm32-timer";
>> +                     reg = <0x40001400 0x400>;
>> +                     interrupts = <55>;
>> +                     resets = <&reset TIM7_RESET>;
>> +                     clocks = <&clk_pmtr1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart1: usart@40011000 {
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40011000 0x400>;
>> +                     interrupts = <37>;
>> +                     clocks = <&clk_pclk2>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart2: usart@40004400 {
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40004400 0x400>;
>> +                     interrupts = <38>;
>> +                     clocks = <&clk_pclk1>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>
> Copy&paste, also below. Is this really .dtsi material?

Sorry, I am not sure to understand what you mean.
Could you elaborate please?

>
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart3: usart@40004800 {
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40004800 0x400>;
>> +                     interrupts = <39>;
>> +                     clocks = <&clk_pclk1>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart4: usart@40004c00 {
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40004c00 0x400>;
>> +                     interrupts = <52>;
>> +                     clocks = <&clk_pclk1>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart5: usart@40005000 {
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40005000 0x400>;
>> +                     interrupts = <53>;
>> +                     clocks = <&clk_pclk1>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart6: usart@40011400 {
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40011400 0x400>;
>> +                     interrupts = <71>;
>> +                     clocks = <&clk_pclk2>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart7: usart@40007800 {
>
> Please order all nodes by unit address, not by label.
Ok.

>
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40007800 0x400>;
>> +                     interrupts = <82>;
>> +                     clocks = <&clk_pclk1>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             usart8: usart@40007c00 {
>> +                     compatible = "st,stm32-usart";
>> +                     reg = <0x40007c00 0x400>;
>> +                     interrupts = <83>;
>> +                     clocks = <&clk_pclk1>;
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&pinctrl_usart1>;
>> +
>> +                     status = "disabled";
>> +             };
>
> I remember two of them not being USART but UART? Similarly, two of them
> support flow control (or two don't?), for which we would either need a
> property or two different compatible strings.

I have to check what would best fit to handle these differences.
I will come back to you later on this point.

Thanks!
Maxime

>
>> +     };
>> +};
>
> Regards,
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> Graham Norton; HRB 21284 (AG Nürnberg)
>

Re: [PATCH v2 16/18] ARM: dts: Introduce STM32F429 MCU

[Linus Walleij]

On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> The STMicrolectornics's STM32F419 MCU has the following main features:
>  - Cortex-M4 core running up to @180MHz
>  - 2MB internal flash, 256KBytes internal RAM
>  - FMC controller to connect SDRAM, NOR and NAND memories
>  - SD/MMC/SDIO support
>  - Ethernet controller
>  - USB OTFG FS & HS controllers
>  - I2C, SPI, CAN busses support
>  - Several 16 & 32 bits general purpose timers
>  - Serial Audio interface
>  - LCD controller
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
(...)
> +       soc {
> +               usart1: usart@40011000 {
> +                       status = "okay";
> +               };
> +
> +               leds {
> +                       compatible = "gpio-leds";
> +                       red {
> +                               #gpio-cells = <2>;
> +                               label = "Front Panel LED";
> +                               gpios = <&gpiog 14 0>;
> +                               linux,default-trigger = "heartbeat";
> +                       };
> +                       green {
> +                               #gpio-cells = <2>;
> +                               gpios = <&gpiog 13 0>;
> +                               default-state = "off";
> +                       };
> +               };
> +       };

The LEDs are mounted on the board not the SoC, right?

So move them outside of the soc node, to the top level
of the DTS file.

Yours,
Linus Walleij

Re: [PATCH v2 16/18] ARM: dts: Introduce STM32F429 MCU

[Maxime Coquelin]

2015-03-09 15:39 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>
>> The STMicrolectornics's STM32F419 MCU has the following main features:
>>  - Cortex-M4 core running up to @180MHz
>>  - 2MB internal flash, 256KBytes internal RAM
>>  - FMC controller to connect SDRAM, NOR and NAND memories
>>  - SD/MMC/SDIO support
>>  - Ethernet controller
>>  - USB OTFG FS & HS controllers
>>  - I2C, SPI, CAN busses support
>>  - Several 16 & 32 bits general purpose timers
>>  - Serial Audio interface
>>  - LCD controller
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> (...)
>> +       soc {
>> +               usart1: usart@40011000 {
>> +                       status = "okay";
>> +               };
>> +
>> +               leds {
>> +                       compatible = "gpio-leds";
>> +                       red {
>> +                               #gpio-cells = <2>;
>> +                               label = "Front Panel LED";
>> +                               gpios = <&gpiog 14 0>;
>> +                               linux,default-trigger = "heartbeat";
>> +                       };
>> +                       green {
>> +                               #gpio-cells = <2>;
>> +                               gpios = <&gpiog 13 0>;
>> +                               default-state = "off";
>> +                       };
>> +               };
>> +       };
>
> The LEDs are mounted on the board not the SoC, right?
>
> So move them outside of the soc node, to the top level
> of the DTS file.

Yes, you are right.
Andreas already made the same remark, and the top-level move is
already here in the upcoming v3.

Thanks,
Maxime
>
> Yours,
> Linus Walleij

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Uwe Kleine-König]

Hello,

On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:
> This patch adds clocksource support for ARMv7-M's System timer,
> also known as SysTick.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  drivers/clocksource/Kconfig          |  7 ++++
>  drivers/clocksource/Makefile         |  1 +
>  drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+)
>  create mode 100644 drivers/clocksource/armv7m_systick.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index fc01ec2..fb6011e 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>  	help
>  	 Use ARM global timer clock source as sched_clock
>  
> +config ARMV7M_SYSTICK
> +	bool
I assume this symbol is enabled later in the series. Would it make sense
to allow enabing the symbol for compile test coverage?

> +	select CLKSRC_OF if OF
What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
compile?

> +	select CLKSRC_MMIO
> +	help
> +	  This options enables support for the ARMv7M system timer unit
the right spelling is ARMv7-M.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Paul Bolle]

On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote:
> On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:
> > This patch adds clocksource support for ARMv7-M's System timer,
> > also known as SysTick.
> > 
> > Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> > ---
> >  drivers/clocksource/Kconfig          |  7 ++++
> >  drivers/clocksource/Makefile         |  1 +
> >  drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 86 insertions(+)
> >  create mode 100644 drivers/clocksource/armv7m_systick.c
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index fc01ec2..fb6011e 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> >  	help
> >  	 Use ARM global timer clock source as sched_clock
> >  
> > +config ARMV7M_SYSTICK
> > +	bool
> I assume this symbol is enabled later in the series.

Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family
machine").

> Would it make sense
> to allow enabing the symbol for compile test coverage?
> 
> > +	select CLKSRC_OF if OF
> What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
> compile?
> 
> > +	select CLKSRC_MMIO
> > +	help
> > +	  This options enables support for the ARMv7M system timer unit
> the right spelling is ARMv7-M.

This Kconfig entry has no prompt, so no one is going to see this text
during make *config. Perhaps this should be made a comment. In that case
the right spelling should still be used.

Thanks,


Paul Bolle

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Maxime Coquelin]

Hi Paul, Uwe,

2015-02-20 22:48 GMT+01:00 Paul Bolle <pebolle@tiscali.nl>:
> On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote:
>> On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:
>> > This patch adds clocksource support for ARMv7-M's System timer,
>> > also known as SysTick.
>> >
>> > Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> > ---
>> >  drivers/clocksource/Kconfig          |  7 ++++
>> >  drivers/clocksource/Makefile         |  1 +
>> >  drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 86 insertions(+)
>> >  create mode 100644 drivers/clocksource/armv7m_systick.c
>> >
>> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> > index fc01ec2..fb6011e 100644
>> > --- a/drivers/clocksource/Kconfig
>> > +++ b/drivers/clocksource/Kconfig
>> > @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>> >     help
>> >      Use ARM global timer clock source as sched_clock
>> >
>> > +config ARMV7M_SYSTICK
>> > +   bool
>> I assume this symbol is enabled later in the series.
>
> Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family
> machine").
>
>> Would it make sense
>> to allow enabing the symbol for compile test coverage?
>>
>> > +   select CLKSRC_OF if OF
>> What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
>> compile?
>>
>> > +   select CLKSRC_MMIO
>> > +   help
>> > +     This options enables support for the ARMv7M system timer unit
>> the right spelling is ARMv7-M.
>
> This Kconfig entry has no prompt, so no one is going to see this text
> during make *config. Perhaps this should be made a comment. In that case
> the right spelling should still be used.

Yes, you are right.
Do you agree if I define it like this:

config ARMV7M_SYSTICK
    bool "Clocksource driver for ARMv7-M System timer"
    depends on OF && (CPU_V7M || COMPILE_TEST)
    select CLKSRC_OF
    select CLKSRC_MMIO
    help
      This options enables clocksource support for the ARMv7-M system
      timer unit.

Thanks,
Maxime
>
> Thanks,
>
>
> Paul Bolle
>

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Paul Bolle]

Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]:
> Do you agree if I define it like this:
> 
> config ARMV7M_SYSTICK
>     bool "Clocksource driver for ARMv7-M System timer"
>     depends on OF && (CPU_V7M || COMPILE_TEST)
>     select CLKSRC_OF
>     select CLKSRC_MMIO
>     help
>       This options enables clocksource support for the ARMv7-M system
>       timer unit.

I don't really have strong feelings on whatever way you choose to fix
the, well, minor problem I pointed out.

Having said that, if a Kconfig entry without a prompt (and therefor,
without help) actually does what you want it to do, why bother adding a
prompt and a one line help text?


Paul Bolle

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Maxime Coquelin]

2015-03-03 20:43 GMT+01:00 Paul Bolle <pebolle@tiscali.nl>:
> Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]:
>> Do you agree if I define it like this:
>>
>> config ARMV7M_SYSTICK
>>     bool "Clocksource driver for ARMv7-M System timer"
>>     depends on OF && (CPU_V7M || COMPILE_TEST)
>>     select CLKSRC_OF
>>     select CLKSRC_MMIO
>>     help
>>       This options enables clocksource support for the ARMv7-M system
>>       timer unit.
>
> I don't really have strong feelings on whatever way you choose to fix
> the, well, minor problem I pointed out.
>
> Having said that, if a Kconfig entry without a prompt (and therefor,
> without help) actually does what you want it to do, why bother adding a
> prompt and a one line help text?

This is because I added also support for COMPILE_TEST coverage as per
Uwe advice,
and thought it was necessary to have an entry for this.
Maybe I'm just wrong?

Thanks,
Maxime

>
>
> Paul Bolle
>

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Paul Bolle]

On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote:
> This is because I added also support for COMPILE_TEST coverage as per
> Uwe advice,
> and thought it was necessary to have an entry for this.
> Maybe I'm just wrong?

I missed that you added COMPILE_TEST.

A quick scan of your idea doesn't show any obvious issues. (Note that I
don't really know how people actually use COMPILE_TEST. I guess things
like "make allyesconfig" are involved.)


Paul Bolle

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Uwe Kleine-König]

Hello,

On Mon, Mar 09, 2015 at 10:12:32PM +0100, Paul Bolle wrote:
> On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote:
> > This is because I added also support for COMPILE_TEST coverage as per
> > Uwe advice,
> > and thought it was necessary to have an entry for this.
> > Maybe I'm just wrong?
> 
> I missed that you added COMPILE_TEST.
> 
> A quick scan of your idea doesn't show any obvious issues. (Note that I
> don't really know how people actually use COMPILE_TEST. I guess things
> like "make allyesconfig" are involved.)
Maybe this can clearify the purpose of COMPILE_TEST:

diff --git a/init/Kconfig b/init/Kconfig
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -67,6 +67,26 @@ config COMPILE_TEST
 	  here. If you are a user/distributor, say N here to exclude useless
 	  drivers to be distributed.
 
+	# If you are a driver author consider to adjust your driver's
+	# dependencies to make it buildable with minimal preconditions if
+	# COMPILE_TEST is enabled. This helps contributers and maintainers
+	# that might not have the necessary toolchain or kernel config handy and
+	# also increases compile test coverage. It's your advantage if others can
+	# build your driver more easily! So for a device that is only found on the
+	# foo cpu use:
+	#
+	# 	depends on CPU_FOO || COMPILE_TEST
+	#
+	# . You might have to use
+	#
+	# 	depends on CPU_FOO || (COMPILE_TEST && COOKIE)
+	#
+	# or
+	#
+	#	depends on COOKIE && (CPU_FOO || COMPILE_TEST)
+	#
+	# if your driver uses features that are only available if COOKIE is on.
+
 config LOCALVERSION
 	string "Local version - append to kernel release"
 	help

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Linus Walleij]

On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> This patch adds clocksource support for ARMv7-M's System timer,
> also known as SysTick.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
(...)
> +       /* If no clock found, try to get clock-frequency property */
> +       if (!rate) {
> +               ret = of_property_read_u32(np, "clock-frequency", &rate);
> +               if (ret)
> +                       goto out_unmap;
> +       }

If this driver is only used for this one system, and if on this one system
the clk subsystem will provide the clock rate, then there is no need
to include this hackaround property.

Alternatively there is no point including reading the frequency from
the clk subsystem for this one system.

So which one is it?

Yours,
Linus Walleij

Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

[Maxime Coquelin]

2015-03-09 16:50 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>
>> This patch adds clocksource support for ARMv7-M's System timer,
>> also known as SysTick.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> (...)
>> +       /* If no clock found, try to get clock-frequency property */
>> +       if (!rate) {
>> +               ret = of_property_read_u32(np, "clock-frequency", &rate);
>> +               if (ret)
>> +                       goto out_unmap;
>> +       }
>
> If this driver is only used for this one system, and if on this one system
> the clk subsystem will provide the clock rate, then there is no need
> to include this hackaround property.
>
> Alternatively there is no point including reading the frequency from
> the clk subsystem for this one system.
>
> So which one is it?

In the first version, the "clock-frequency" property handling was not here.
Rob Herring advised me to add its support, as it could be used by
simple systems not selecting CCF.

So, I don't have the name of a system where it could be useful, but I
think Rob's request make sense.

Note that I tested using the clock-frequency property before sending.

Best regards,
Maxime
>
> Yours,
> Linus Walleij

Re: [PATCH v2 05/18] reset: Add reset_controller_of_init() function

[Arnd Bergmann]

On Friday 20 February 2015 19:01:04 Maxime Coquelin wrote:
> Some platforms need to initialize the reset controller before the timers.
> 
> This patch introduces a reset_controller_of_init() function that can be
> called before the timers intialization.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> 

Not sure about this. It seems like the cleanest approach if we get
a lot of these, but then again it is probably very rare, and I'd
like to avoid adding such infrastructure if it's just for one
SoC. Could we add a hack in the machine initialization instead?

I think ideally this would be done in the boot loader before we
even start Linux, but I don't know if that's possible for you.

	Arnd

Re: [PATCH v2 05/18] reset: Add reset_controller_of_init() function

[Maxime Coquelin]

2015-03-10 16:00 GMT+01:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Friday 20 February 2015 19:01:04 Maxime Coquelin wrote:
>> Some platforms need to initialize the reset controller before the timers.
>>
>> This patch introduces a reset_controller_of_init() function that can be
>> called before the timers intialization.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>
> Not sure about this. It seems like the cleanest approach if we get
> a lot of these, but then again it is probably very rare, and I'd
> like to avoid adding such infrastructure if it's just for one
> SoC. Could we add a hack in the machine initialization instead?

Sun6i also need to initialize the reset controller early. Today, they
hack the machine initialization.
With two SoCs having the same need, what should we do?

That said, I'm fine with either way. If reset_controller_of_init gets
accepted, I will send the patch for sun6i.

>
> I think ideally this would be done in the boot loader before we
> even start Linux, but I don't know if that's possible for you.

>From what I understand, the only constraint is to perform it after the
clock is enabled.
So this should be possible to do it in the bootloader, but it means
also adding timers clocks ungating in the bootloader.

Br,
Maxime

>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/18] reset: Add reset_controller_of_init() function

[Arnd Bergmann]

On Tuesday 10 March 2015 16:28:44 Maxime Coquelin wrote:
> 2015-03-10 16:00 GMT+01:00 Arnd Bergmann <arnd@arndb.de>:
> > On Friday 20 February 2015 19:01:04 Maxime Coquelin wrote:
> >> Some platforms need to initialize the reset controller before the timers.
> >>
> >> This patch introduces a reset_controller_of_init() function that can be
> >> called before the timers intialization.
> >>
> >> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> >>
> >
> > Not sure about this. It seems like the cleanest approach if we get
> > a lot of these, but then again it is probably very rare, and I'd
> > like to avoid adding such infrastructure if it's just for one
> > SoC. Could we add a hack in the machine initialization instead?
> 
> Sun6i also need to initialize the reset controller early. Today, they
> hack the machine initialization.
> With two SoCs having the same need, what should we do?

Good question, I'd like to hear some other opinions on this first.

> > I think ideally this would be done in the boot loader before we
> > even start Linux, but I don't know if that's possible for you.
> 
> From what I understand, the only constraint is to perform it after the
> clock is enabled.
> So this should be possible to do it in the bootloader, but it means
> also adding timers clocks ungating in the bootloader.

Ungating the timer clock input seems like a reasonable thing to
do for the bootloader, I think a lot of platforms rely on this
elsewhere (but enough of them don't, which is why we have the
early clk init).

	Arnd

Re: [PATCH v2 05/18] reset: Add reset_controller_of_init() function

[Rob Herring]

On Tue, Mar 10, 2015 at 3:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 10 March 2015 16:28:44 Maxime Coquelin wrote:
>> 2015-03-10 16:00 GMT+01:00 Arnd Bergmann <arnd@arndb.de>:
>> > On Friday 20 February 2015 19:01:04 Maxime Coquelin wrote:
>> >> Some platforms need to initialize the reset controller before the timers.
>> >>
>> >> This patch introduces a reset_controller_of_init() function that can be
>> >> called before the timers intialization.
>> >>
>> >> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> >>
>> >
>> > Not sure about this. It seems like the cleanest approach if we get
>> > a lot of these, but then again it is probably very rare, and I'd
>> > like to avoid adding such infrastructure if it's just for one
>> > SoC. Could we add a hack in the machine initialization instead?
>>
>> Sun6i also need to initialize the reset controller early. Today, they
>> hack the machine initialization.
>> With two SoCs having the same need, what should we do?
>
> Good question, I'd like to hear some other opinions on this first.

2 is still far from the common case.

>> > I think ideally this would be done in the boot loader before we
>> > even start Linux, but I don't know if that's possible for you.
>>
>> From what I understand, the only constraint is to perform it after the
>> clock is enabled.
>> So this should be possible to do it in the bootloader, but it means
>> also adding timers clocks ungating in the bootloader.
>
> Ungating the timer clock input seems like a reasonable thing to
> do for the bootloader, I think a lot of platforms rely on this
> elsewhere (but enough of them don't, which is why we have the
> early clk init).

+1

If the bootloader is u-boot, then you need a timer anyway.

Rob

Re: [PATCH v2 05/18] reset: Add reset_controller_of_init() function

[Maxime Coquelin]

2015-03-10 22:30 GMT+01:00 Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Tue, Mar 10, 2015 at 3:19 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Tuesday 10 March 2015 16:28:44 Maxime Coquelin wrote:
>>> 2015-03-10 16:00 GMT+01:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
>>> > On Friday 20 February 2015 19:01:04 Maxime Coquelin wrote:
>>> >> Some platforms need to initialize the reset controller before the timers.
>>> >>
>>> >> This patch introduces a reset_controller_of_init() function that can be
>>> >> called before the timers intialization.
>>> >>
>>> >> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> >>
>>> >
>>> > Not sure about this. It seems like the cleanest approach if we get
>>> > a lot of these, but then again it is probably very rare, and I'd
>>> > like to avoid adding such infrastructure if it's just for one
>>> > SoC. Could we add a hack in the machine initialization instead?
>>>
>>> Sun6i also need to initialize the reset controller early. Today, they
>>> hack the machine initialization.
>>> With two SoCs having the same need, what should we do?
>>
>> Good question, I'd like to hear some other opinions on this first.
>
> 2 is still far from the common case.
>
>>> > I think ideally this would be done in the boot loader before we
>>> > even start Linux, but I don't know if that's possible for you.
>>>
>>> From what I understand, the only constraint is to perform it after the
>>> clock is enabled.
>>> So this should be possible to do it in the bootloader, but it means
>>> also adding timers clocks ungating in the bootloader.
>>
>> Ungating the timer clock input seems like a reasonable thing to
>> do for the bootloader, I think a lot of platforms rely on this
>> elsewhere (but enough of them don't, which is why we have the
>> early clk init).
>
> +1
>
> If the bootloader is u-boot, then you need a timer anyway.

Ok, I moved it to the bootloader, and use the reset as optional in the
timer driver.

Thanks,
Maxime
>
> Rob

Re: [PATCH v2 07/18] drivers: reset: Add STM32 reset driver

[Arnd Bergmann]

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

Re: [PATCH v2 07/18] drivers: reset: Add STM32 reset driver

[Maxime Coquelin]

2015-03-10 16:02 GMT+01:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/18] drivers: reset: Add STM32 reset driver

[Maxime Coquelin]

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

Re: [PATCH v2 07/18] drivers: reset: Add STM32 reset driver

[Arnd Bergmann]

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

Re: [PATCH v2 07/18] drivers: reset: Add STM32 reset driver

[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.

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

Re: [PATCH v2 07/18] drivers: reset: Add STM32 reset driver

[Philipp Zabel]

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

Re: [PATCH v2 07/18] drivers: reset: Add STM32 reset driver

[Maxime Coquelin]

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
>

Re: [PATCH v2 11/18] pinctrl: Add pinctrl driver for STM32 MCUs

[Linus Walleij]

On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> This driver adds pinctrl and GPIO support to STMicrolectronic's
> STM32 family of MCUs.
>
> Pin muxing and GPIO handling have been tested on STM32F429
> based Discovery board.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>

(...)
> +config PINCTRL_STM32
> +       bool "STMicroelectronics STM32 pinctrl driver"
> +       depends on OF
> +       depends on ARCH_STM32 || COMPILE_TEST
> +       select PINMUX
> +       select PINCONF
> +       select GPIOLIB_IRQCHIP
> +       help
> +         This selects the device tree based generic pinctrl driver for STM32.

Good start! Especially that you use GPIOLIB_IRQCHIP.

But this (as discussed earlier) should select GENERIC_PINCONF

Stopping review here so you can reengineer it a bit using GENERIC_PINCONF
for next submission.

Also think about pinmux in single registers, whether you want to do this
with a single value for a register or using strings to identify groups
and functions.

Yours,
Linus Walleij

Re: [PATCH v2 11/18] pinctrl: Add pinctrl driver for STM32 MCUs

[Maxime Coquelin]

2015-03-06 10:24 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>
>> This driver adds pinctrl and GPIO support to STMicrolectronic's
>> STM32 family of MCUs.
>>
>> Pin muxing and GPIO handling have been tested on STM32F429
>> based Discovery board.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>
> (...)
>> +config PINCTRL_STM32
>> +       bool "STMicroelectronics STM32 pinctrl driver"
>> +       depends on OF
>> +       depends on ARCH_STM32 || COMPILE_TEST
>> +       select PINMUX
>> +       select PINCONF
>> +       select GPIOLIB_IRQCHIP
>> +       help
>> +         This selects the device tree based generic pinctrl driver for STM32.
>
> Good start! Especially that you use GPIOLIB_IRQCHIP.
>
> But this (as discussed earlier) should select GENERIC_PINCONF
>
> Stopping review here so you can reengineer it a bit using GENERIC_PINCONF
> for next submission.
>
> Also think about pinmux in single registers, whether you want to do this
> with a single value for a register or using strings to identify groups
> and functions.

Thanks for the review.
I will digest all this, and come back with another solution :)

Best regards,
Maxime

>
> Yours,
> Linus Walleij

Re: [PATCH v2 11/18] pinctrl: Add pinctrl driver for STM32 MCUs

[Arnd Bergmann]

On Friday 20 February 2015 19:01:10 Maxime Coquelin wrote:
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/pinctrl-stm32.h
> @@ -0,0 +1,43 @@
> +#ifndef _DT_BINDINGS_PINCTRL_STM32_H
> +#define _DT_BINDINGS_PINCTRL_STM32_H
> +
> +/* Modes */
> +#define IN             0
> +#define OUT            1
> +#define ALT            2
> +#define ANALOG         3

I think it's better to prefix all the names with a
string to identify what they are for, otherwise these
are way too generic.

> +/* Alternate functions */
> +#define ALT0           ((0 << 2) | ALT)
> +#define ALT1           ((1 << 2) | ALT)
> +#define ALT2           ((2 << 2) | ALT)
> +#define ALT3           ((3 << 2) | ALT)
> +#define ALT4           ((4 << 2) | ALT)
> +#define ALT5           ((5 << 2) | ALT)
> +#define ALT6           ((6 << 2) | ALT)
> +#define ALT7           ((7 << 2) | ALT)
> +#define ALT8           ((8 << 2) | ALT)
> +#define ALT9           ((9 << 2) | ALT)
> +#define ALT10          ((10 << 2) | ALT)
> +#define ALT11          ((11 << 2) | ALT)
> +#define ALT12          ((12 << 2) | ALT)
> +#define ALT13          ((13 << 2) | ALT)
> +#define ALT14          ((14 << 2) | ALT)
> +#define ALT15          ((15 << 2) | ALT)

You can have a single macro for these like

#define STM32_PIN_ALT(x) ((x << 2) | ALT)

> +/* Pull-Up/Down */
> +#define NO_PULL                0
> +#define PULL_UP                1
> +#define PULL_DOWN      2
> +
> +/* Type */
> +#define PUSH_PULL      (0 << 2)
> +#define OPEN_DRAIN     (1 << 2)
> +

These should probably not be stm32 specific at all, they sound
rather generic, so maybe put the definitions into a common file.



	Arnd

Re: [PATCH v2 11/18] pinctrl: Add pinctrl driver for STM32 MCUs

[Linus Walleij]

On Tue, Mar 10, 2015 at 4:08 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 20 February 2015 19:01:10 Maxime Coquelin wrote:

>> +/* Pull-Up/Down */
>> +#define NO_PULL                0
>> +#define PULL_UP                1
>> +#define PULL_DOWN      2
>> +
>> +/* Type */
>> +#define PUSH_PULL      (0 << 2)
>> +#define OPEN_DRAIN     (1 << 2)
>> +
>
> These should probably not be stm32 specific at all, they sound
> rather generic, so maybe put the definitions into a common file.

It's part of what GENERIC_PINCONF does and it has bindings
in Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Yours,
Linus Walleij

Re: [PATCH v2 12/18] dt-bindings: Document the STM32 USART bindings

[Arnd Bergmann]

On Friday 20 February 2015 19:01:11 Maxime Coquelin wrote:
> +
> +Example:
> +usart1: usart@40011000 {
> +       compatible = "st,stm32-usart";
> 

Please use generic node names everywhere. The standard name for a serial
port is "serial".

	Arnd

Re: [PATCH v2 12/18] dt-bindings: Document the STM32 USART bindings

[Maxime Coquelin]

2015-03-10 16:08 GMT+01:00 Arnd Bergmann <arnd@arndb.de>:
> On Friday 20 February 2015 19:01:11 Maxime Coquelin wrote:
>> +
>> +Example:
>> +usart1: usart@40011000 {
>> +       compatible = "st,stm32-usart";
>>
>
> Please use generic node names everywhere. The standard name for a serial
> port is "serial".

I already had the remark, and it is already fixed in the next version.

Thanks,
Maxime

>
>         Arnd

Re: [PATCH v2 14/18] ARM: Add STM32 family machine

[Uwe Kleine-König]

Hello,

On Fri, Feb 20, 2015 at 07:01:13PM +0100, Maxime Coquelin wrote:
> STMicrolectronics's STM32 series is a family of Cortex-M
> microcontrollers. It is used in various applications, and
> proposes a wide range of peripherals.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  Documentation/arm/stm32/overview.txt           | 32 ++++++++++++++++++++++++++
>  Documentation/arm/stm32/stm32f429-overview.txt | 22 ++++++++++++++++++
>  arch/arm/Kconfig                               | 22 ++++++++++++++++++
>  arch/arm/Makefile                              |  1 +
>  arch/arm/mach-stm32/Makefile                   |  1 +
>  arch/arm/mach-stm32/Makefile.boot              |  0
>  arch/arm/mach-stm32/board-dt.c                 | 31 +++++++++++++++++++++++++
>  7 files changed, 109 insertions(+)
>  create mode 100644 Documentation/arm/stm32/overview.txt
>  create mode 100644 Documentation/arm/stm32/stm32f429-overview.txt
>  create mode 100644 arch/arm/mach-stm32/Makefile
>  create mode 100644 arch/arm/mach-stm32/Makefile.boot
>  create mode 100644 arch/arm/mach-stm32/board-dt.c
> 
> diff --git a/Documentation/arm/stm32/overview.txt b/Documentation/arm/stm32/overview.txt
> new file mode 100644
> index 0000000..d8bf6bb
> --- /dev/null
> +++ b/Documentation/arm/stm32/overview.txt
> @@ -0,0 +1,32 @@
> +			STM32 ARM Linux Overview
> +			==========================
> +
> +Introduction
> +------------
> +
> +  The STMicroelectronics family of Cortex-M based MCUs are supported by the
> +  'STM32' platform of ARM Linux. Currently only the STM32F429 is supported.
> +
> +
> +Configuration
> +-------------
> +
> +  A generic configuration is provided for STM32 family, and can be used as the
> +  default by
> +	make stm32_defconfig
> +
> +Layout
> +------
> +
> +  All the files for multiple machine families are located in the platform code
> +  contained in arch/arm/mach-stm32
> +
> +  There is a generic board board-dt.c in the mach folder which support
> +  Flattened Device Tree, which means, It works with any compatible board with
> +  Device Trees.
> +
> +
> +Document Author
> +---------------
> +
> +  Maxime Coquelin <mcoquelin.stm32@gmail.com>
> diff --git a/Documentation/arm/stm32/stm32f429-overview.txt b/Documentation/arm/stm32/stm32f429-overview.txt
> new file mode 100644
> index 0000000..5206822
> --- /dev/null
> +++ b/Documentation/arm/stm32/stm32f429-overview.txt
> @@ -0,0 +1,22 @@
> +			STM32F429 Overview
> +			==================
> +
> +  Introduction
> +  ------------
> +	The STM32F429 is a Cortex-M4 MCU aimed at various applications.
> +	It features:
> +	- ARM Cortex-M4 up to 180MHz with FPU
> +	- 2MB internal Flash Memory
> +	- External memory support through FMC controller (PSRAM, SDRAM, NOR, NAND)
> +	- I2C, SPI, SAI, CAN, USB OTG, Ethernet controllers
> +	- LCD controller & Camera interface
> +	- Cryptographic processor
> +
> +  Resources
> +  ---------
> +	Datasheet and reference manual are publicly available on ST website:
> +	- http://www.st.com/web/en/catalog/mmc/FM141/SC1169/SS1577/LN1806?ecmp=stm32f429-439_pron_pr-ces2014_nov2013
> +
> +  Document Author
> +  ---------------
> +	Maxime Coquelin <mcoquelin.stm32@gmail.com>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 97d07ed..cfd9532 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -774,6 +774,28 @@ config ARCH_OMAP1
>  	help
>  	  Support for older TI OMAP1 (omap7xx, omap15xx or omap16xx)
>  
> +config ARCH_STM32
> +	bool "STMicrolectronics STM32"
> +	depends on !MMU
> +	select ARCH_REQUIRE_GPIOLIB
> +	select ARM_NVIC
> +	select AUTO_ZRELADDR
> +	select ARCH_HAS_RESET_CONTROLLER
> +	select RESET_CONTROLLER
> +	select PINCTRL
> +	select PINCTRL_STM32
> +	select CLKSRC_OF
> +	select ARMV7M_SYSTICK
> +	select COMMON_CLK
> +	select CPU_V7M
> +	select GENERIC_CLOCKEVENTS
> +	select NO_DMA
> +	select NO_IOPORT_MAP
> +	select SPARSE_IRQ
> +	select USE_OF
Please sort this list alphabetically.

> +	help
> +	  Support for STMicorelectronics STM32 processors.
> +
>  endchoice
>  
>  menu "Multiple platform selection"
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index c1785ee..7d00659 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -196,6 +196,7 @@ machine-$(CONFIG_ARCH_SHMOBILE) 	+= shmobile
>  machine-$(CONFIG_ARCH_SIRF)		+= prima2
>  machine-$(CONFIG_ARCH_SOCFPGA)		+= socfpga
>  machine-$(CONFIG_ARCH_STI)		+= sti
> +machine-$(CONFIG_ARCH_STM32)		+= stm32
>  machine-$(CONFIG_ARCH_SUNXI)		+= sunxi
>  machine-$(CONFIG_ARCH_TEGRA)		+= tegra
>  machine-$(CONFIG_ARCH_U300)		+= u300
> diff --git a/arch/arm/mach-stm32/Makefile b/arch/arm/mach-stm32/Makefile
> new file mode 100644
> index 0000000..bd0b7b5
> --- /dev/null
> +++ b/arch/arm/mach-stm32/Makefile
> @@ -0,0 +1 @@
> +obj-y += board-dt.o
> diff --git a/arch/arm/mach-stm32/Makefile.boot b/arch/arm/mach-stm32/Makefile.boot
> new file mode 100644
> index 0000000..e69de29
Maybe note there why this file exists and can be empty. Feel free to
copy the content of efm32's Makefile.boot.

> diff --git a/arch/arm/mach-stm32/board-dt.c b/arch/arm/mach-stm32/board-dt.c
> new file mode 100644
> index 0000000..1d681b3
> --- /dev/null
> +++ b/arch/arm/mach-stm32/board-dt.c
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright (C) Maxime Coquelin 2015
> + * Author:  Maxime Coquelin <mcoquelin.stm32@gmail.com>
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clocksource.h>
> +#include <linux/reset-controller.h>
> +#include <asm/v7m.h>
> +#include <asm/mach/arch.h>
> +
> +static const char *const stm32_compat[] __initconst = {
> +	"st,stm32f429",
> +	NULL
> +};
> +
> +static void __init stm32_timer_init(void)
> +{
> +	of_clk_init(NULL);
> +	reset_controller_of_init();
> +	clocksource_of_init();
Hmm, if reset_controller_of_init was called automatically you wouldn't
need that, right? Maybe arange for that instead?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 14/18] ARM: Add STM32 family machine

[Paul Bolle]

On Fri, 2015-02-20 at 21:00 +0100, Uwe Kleine-König wrote:
> On Fri, Feb 20, 2015 at 07:01:13PM +0100, Maxime Coquelin wrote:
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 97d07ed..cfd9532 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -774,6 +774,28 @@ config ARCH_OMAP1
> >  	help
> >  	  Support for older TI OMAP1 (omap7xx, omap15xx or omap16xx)
> >  
> > +config ARCH_STM32
> > +	bool "STMicrolectronics STM32"
> > +	depends on !MMU
> > +	select ARCH_REQUIRE_GPIOLIB
> > +	select ARM_NVIC
> > +	select AUTO_ZRELADDR
> > +	select ARCH_HAS_RESET_CONTROLLER
> > +	select RESET_CONTROLLER
> > +	select PINCTRL
> > +	select PINCTRL_STM32
> > +	select CLKSRC_OF
> > +	select ARMV7M_SYSTICK
> > +	select COMMON_CLK
> > +	select CPU_V7M
> > +	select GENERIC_CLOCKEVENTS
> > +	select NO_DMA
> > +	select NO_IOPORT_MAP
> > +	select SPARSE_IRQ
> > +	select USE_OF
> Please sort this list alphabetically.

And drop
    select NO_DMA

You copied that from ARCH_EFM32, but it's pointless on arm (as arch/arm/
doesn't provide a NO_DMA Kconfig symbol).

I submitted a patch last year to drop it from ARCH_EFM32, which Uwe
Acked, but then nothing happened. I'm to blame, as I should have sent a
reminder.

> > +	help
> > +	  Support for STMicorelectronics STM32 processors.
> > +
> >  endchoice
> >  
> >  menu "Multiple platform selection"


Paul Bolle

Re: [PATCH v2 14/18] ARM: Add STM32 family machine

[Maxime Coquelin]

2015-02-20 22:37 GMT+01:00 Paul Bolle <pebolle@tiscali.nl>:
> On Fri, 2015-02-20 at 21:00 +0100, Uwe Kleine-König wrote:
>> On Fri, Feb 20, 2015 at 07:01:13PM +0100, Maxime Coquelin wrote:
>> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> > index 97d07ed..cfd9532 100644
>> > --- a/arch/arm/Kconfig
>> > +++ b/arch/arm/Kconfig
>> > @@ -774,6 +774,28 @@ config ARCH_OMAP1
>> >     help
>> >       Support for older TI OMAP1 (omap7xx, omap15xx or omap16xx)
>> >
>> > +config ARCH_STM32
>> > +   bool "STMicrolectronics STM32"
>> > +   depends on !MMU
>> > +   select ARCH_REQUIRE_GPIOLIB
>> > +   select ARM_NVIC
>> > +   select AUTO_ZRELADDR
>> > +   select ARCH_HAS_RESET_CONTROLLER
>> > +   select RESET_CONTROLLER
>> > +   select PINCTRL
>> > +   select PINCTRL_STM32
>> > +   select CLKSRC_OF
>> > +   select ARMV7M_SYSTICK
>> > +   select COMMON_CLK
>> > +   select CPU_V7M
>> > +   select GENERIC_CLOCKEVENTS
>> > +   select NO_DMA
>> > +   select NO_IOPORT_MAP
>> > +   select SPARSE_IRQ
>> > +   select USE_OF
>> Please sort this list alphabetically.
>
> And drop
>     select NO_DMA
>
> You copied that from ARCH_EFM32, but it's pointless on arm (as arch/arm/
> doesn't provide a NO_DMA Kconfig symbol).

You are right, I will drop NO_DMA in v3.

Thanks,
Maxime

>
> I submitted a patch last year to drop it from ARCH_EFM32, which Uwe
> Acked, but then nothing happened. I'm to blame, as I should have sent a
> reminder.
>
>> > +   help
>> > +     Support for STMicorelectronics STM32 processors.
>> > +
>> >  endchoice
>> >
>> >  menu "Multiple platform selection"
>
>
> Paul Bolle
>

Re: [PATCH v2 14/18] ARM: Add STM32 family machine

[Maxime Coquelin]

2015-02-20 21:00 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Fri, Feb 20, 2015 at 07:01:13PM +0100, Maxime Coquelin wrote:
>> STMicrolectronics's STM32 series is a family of Cortex-M
>> microcontrollers. It is used in various applications, and
>> proposes a wide range of peripherals.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> ---
>>  Documentation/arm/stm32/overview.txt           | 32 ++++++++++++++++++++++++++
>>  Documentation/arm/stm32/stm32f429-overview.txt | 22 ++++++++++++++++++
>>  arch/arm/Kconfig                               | 22 ++++++++++++++++++
>>  arch/arm/Makefile                              |  1 +
>>  arch/arm/mach-stm32/Makefile                   |  1 +
>>  arch/arm/mach-stm32/Makefile.boot              |  0
>>  arch/arm/mach-stm32/board-dt.c                 | 31 +++++++++++++++++++++++++
>>  7 files changed, 109 insertions(+)
>>  create mode 100644 Documentation/arm/stm32/overview.txt
>>  create mode 100644 Documentation/arm/stm32/stm32f429-overview.txt
>>  create mode 100644 arch/arm/mach-stm32/Makefile
>>  create mode 100644 arch/arm/mach-stm32/Makefile.boot
>>  create mode 100644 arch/arm/mach-stm32/board-dt.c
>>

<snip>

>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 97d07ed..cfd9532 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -774,6 +774,28 @@ config ARCH_OMAP1
>>       help
>>         Support for older TI OMAP1 (omap7xx, omap15xx or omap16xx)
>>
>> +config ARCH_STM32
>> +     bool "STMicrolectronics STM32"
>> +     depends on !MMU
>> +     select ARCH_REQUIRE_GPIOLIB
>> +     select ARM_NVIC
>> +     select AUTO_ZRELADDR
>> +     select ARCH_HAS_RESET_CONTROLLER
>> +     select RESET_CONTROLLER
>> +     select PINCTRL
>> +     select PINCTRL_STM32
>> +     select CLKSRC_OF
>> +     select ARMV7M_SYSTICK
>> +     select COMMON_CLK
>> +     select CPU_V7M
>> +     select GENERIC_CLOCKEVENTS
>> +     select NO_DMA
>> +     select NO_IOPORT_MAP
>> +     select SPARSE_IRQ
>> +     select USE_OF
> Please sort this list alphabetically.

Ok, I will do for v3.

>
>> +     help
>> +       Support for STMicorelectronics STM32 processors.
>> +
>>  endchoice
>>
>>  menu "Multiple platform selection"
>> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
>> index c1785ee..7d00659 100644
>> --- a/arch/arm/Makefile
>> +++ b/arch/arm/Makefile
>> @@ -196,6 +196,7 @@ machine-$(CONFIG_ARCH_SHMOBILE)   += shmobile
>>  machine-$(CONFIG_ARCH_SIRF)          += prima2
>>  machine-$(CONFIG_ARCH_SOCFPGA)               += socfpga
>>  machine-$(CONFIG_ARCH_STI)           += sti
>> +machine-$(CONFIG_ARCH_STM32)         += stm32
>>  machine-$(CONFIG_ARCH_SUNXI)         += sunxi
>>  machine-$(CONFIG_ARCH_TEGRA)         += tegra
>>  machine-$(CONFIG_ARCH_U300)          += u300
>> diff --git a/arch/arm/mach-stm32/Makefile b/arch/arm/mach-stm32/Makefile
>> new file mode 100644
>> index 0000000..bd0b7b5
>> --- /dev/null
>> +++ b/arch/arm/mach-stm32/Makefile
>> @@ -0,0 +1 @@
>> +obj-y += board-dt.o
>> diff --git a/arch/arm/mach-stm32/Makefile.boot b/arch/arm/mach-stm32/Makefile.boot
>> new file mode 100644
>> index 0000000..e69de29
> Maybe note there why this file exists and can be empty. Feel free to
> copy the content of efm32's Makefile.boot.

Ok, I will copy efm32's Makefile.boot content.
Do you know why your patch has not been applied yet?

>
>> diff --git a/arch/arm/mach-stm32/board-dt.c b/arch/arm/mach-stm32/board-dt.c
>> new file mode 100644
>> index 0000000..1d681b3
>> --- /dev/null
>> +++ b/arch/arm/mach-stm32/board-dt.c
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Copyright (C) Maxime Coquelin 2015
>> + * Author:  Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/reset-controller.h>
>> +#include <asm/v7m.h>
>> +#include <asm/mach/arch.h>
>> +
>> +static const char *const stm32_compat[] __initconst = {
>> +     "st,stm32f429",
>> +     NULL
>> +};
>> +
>> +static void __init stm32_timer_init(void)
>> +{
>> +     of_clk_init(NULL);
>> +     reset_controller_of_init();
>> +     clocksource_of_init();
> Hmm, if reset_controller_of_init was called automatically you wouldn't
> need that, right? Maybe arange for that instead?

This is what I did in the v1:
http://marc.info/?l=linux-arm-kernel&m=142376341008550&w=2

But Rob advised to put it elsewhere.

Thanks,
Maxime

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

Re: [PATCH v2 14/18] ARM: Add STM32 family machine

[Arnd Bergmann]

On Friday 20 February 2015 19:01:13 Maxime Coquelin wrote:
> +static void __init stm32_timer_init(void)
> +{
> +       of_clk_init(NULL);
> +       reset_controller_of_init();
> +       clocksource_of_init();
> +
> +}

Don't do that. As I said, I'd rather not introduce reset_controller_of_init()
at all, but if we end up doing it, it should be run by default for
any machine that has an empty timer_init callback.

	Arnd