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

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

[Geert Uytterhoeven]

On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> From Cortex-M4 and M7 reference manuals, the nvic supports up to 240
> interrupts. So the number of entries in vectors table is 256.
>
> This patch adds the missing entries, and change the alignement, so that
> vector_table remains naturally aligned.

Shouldn't this depend on ARCH_STM32, or some other M4 or M7 specific
Kconfig option, to avoid wasting the space on other CPUs?

>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  arch/arm/kernel/entry-v7m.S | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
> index 8944f49..29a461b 100644
> --- a/arch/arm/kernel/entry-v7m.S
> +++ b/arch/arm/kernel/entry-v7m.S
> @@ -117,9 +117,9 @@ ENTRY(__switch_to)
>  ENDPROC(__switch_to)
>
>         .data
> -       .align  8
> +       .align  10
>  /*
> - * Vector table (64 words => 256 bytes natural alignment)
> + * Vector table (256 words => 1024 bytes alignment)
>   */
>  ENTRY(vector_table)
>         .long   0                       @ 0 - Reset stack pointer
> @@ -138,6 +138,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   256 - 16
> +       .long   __irq_entry             @ 16..256 - External Interrupts
>         .endr
> --
> 1.9.1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Maxime Coquelin]

Hi Geert,

2015-02-12 21:34 GMT+01:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> From Cortex-M4 and M7 reference manuals, the nvic supports up to 240
>> interrupts. So the number of entries in vectors table is 256.
>>
>> This patch adds the missing entries, and change the alignement, so that
>> vector_table remains naturally aligned.
>
> Shouldn't this depend on ARCH_STM32, or some other M4 or M7 specific
> Kconfig option, to avoid wasting the space on other CPUs?

Actually, the STM32F429 has 90 interrupts, so it would need 106
entries in the vector table.
The maximum of supported interrupts is not only for Cortex-M4 and M7,
this is also true for Cortex-M3.

I see two possibilities:
 1 - We declare the vector table for the maximum supported number of
IRQs, as this patch does.
        - Pro: it will be functionnal with all Cortex-M MCUs
        - Con: Waste of less than 1KB for memory
 2 - We introduce a config flag that provides the number of interrupts
        - Pro: No more memory waste
        - Con: Need to declare a per MCU model config flag.

Then, regarding the natural alignment, is there a way to ensure it
depending on the value of a config flag?
Or we should keep it at the maximum value possible?

Any feedback will be appreciated, especially from Uwe who maintains
the efm32 machine.

Kind regards,
Maxime

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

[Uwe Kleine-König]

On Fri, Feb 13, 2015 at 09:42:46AM +0100, Maxime Coquelin wrote:
> Hi Geert,
> 
> 2015-02-12 21:34 GMT+01:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> > On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
> > <mcoquelin.stm32@gmail.com> wrote:
> >> From Cortex-M4 and M7 reference manuals, the nvic supports up to 240
> >> interrupts. So the number of entries in vectors table is 256.
> >>
> >> This patch adds the missing entries, and change the alignement, so that
> >> vector_table remains naturally aligned.
> >
> > Shouldn't this depend on ARCH_STM32, or some other M4 or M7 specific
> > Kconfig option, to avoid wasting the space on other CPUs?
> 
> Actually, the STM32F429 has 90 interrupts, so it would need 106
> entries in the vector table.
> The maximum of supported interrupts is not only for Cortex-M4 and M7,
> this is also true for Cortex-M3.
> 
> I see two possibilities:
>  1 - We declare the vector table for the maximum supported number of
> IRQs, as this patch does.
>         - Pro: it will be functionnal with all Cortex-M MCUs
>         - Con: Waste of less than 1KB for memory
>  2 - We introduce a config flag that provides the number of interrupts
>         - Pro: No more memory waste
>         - Con: Need to declare a per MCU model config flag.
I'd vote for 2, something like:

	config CPUV7M_NUM_IRQ
		int
		default 90 if STM32F429
		default 38 if EFM32GG
		default 240

then there is a working default and platforms being short on memory can
configure as appropriate. (The only down side is that if we create
multi-platfrom images at some time in the future either all or none of
the supported platforms must provide a value here.)
 
> Then, regarding the natural alignment, is there a way to ensure it
> depending on the value of a config flag?
The exact wording in ARMARMv7-M is:

	The Vector table must be naturally aligned to a power of two
	whose alignment value is greater than or equal
	to (Number of Exceptions supported x 4), with a minimum
	alignment of 128 bytes.

> Or we should keep it at the maximum value possible?
So we need:

	.align x

with x being max(7, ceil(log((CPUV7M_NUM_IRQ + 16) * 4, 2))). So the
alignment needed is between 7 and 10.

If the assembler supports an expression here I'd use that. But before
adding strange hacks to generate the right value there better go for a
static value like:

	/* The vector table must be naturally aligned */
	#if CONFIG_CPUV7M_NUM_IRQ <= 112
	.align 9 /* log2((112 + 16) * 4) */
	#else
	.align 10
	#endif

Further steps would be:

	CONFIG_CPUV7M_NUM_IRQ <= 48 -> .align 8
	CONFIG_CPUV7M_NUM_IRQ <= 16 -> .align 7

Probably it's not worth to add the respective #ifdefs here.

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 02/14] ARM: ARMv7M: Enlarge vector table to 256 entries

[Maxime Coquelin]

2015-02-13 11:00 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Fri, Feb 13, 2015 at 09:42:46AM +0100, Maxime Coquelin wrote:
>> Hi Geert,
>>
>> 2015-02-12 21:34 GMT+01:00 Geert Uytterhoeven <geert@linux-m68k.org>:
>> > On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
>> > <mcoquelin.stm32@gmail.com> wrote:
>> >> From Cortex-M4 and M7 reference manuals, the nvic supports up to 240
>> >> interrupts. So the number of entries in vectors table is 256.
>> >>
>> >> This patch adds the missing entries, and change the alignement, so that
>> >> vector_table remains naturally aligned.
>> >
>> > Shouldn't this depend on ARCH_STM32, or some other M4 or M7 specific
>> > Kconfig option, to avoid wasting the space on other CPUs?
>>
>> Actually, the STM32F429 has 90 interrupts, so it would need 106
>> entries in the vector table.
>> The maximum of supported interrupts is not only for Cortex-M4 and M7,
>> this is also true for Cortex-M3.
>>
>> I see two possibilities:
>>  1 - We declare the vector table for the maximum supported number of
>> IRQs, as this patch does.
>>         - Pro: it will be functionnal with all Cortex-M MCUs
>>         - Con: Waste of less than 1KB for memory
>>  2 - We introduce a config flag that provides the number of interrupts
>>         - Pro: No more memory waste
>>         - Con: Need to declare a per MCU model config flag.
> I'd vote for 2, something like:
>
>         config CPUV7M_NUM_IRQ
>                 int
>                 default 90 if STM32F429
>                 default 38 if EFM32GG
>                 default 240
>
> then there is a working default and platforms being short on memory can
> configure as appropriate. (The only down side is that if we create
> multi-platfrom images at some time in the future either all or none of
> the supported platforms must provide a value here.)

Ok, I'm fine doing this way.
I will implement this in the v2 if Russel is fine with the proposal too.

>
>> Then, regarding the natural alignment, is there a way to ensure it
>> depending on the value of a config flag?
> The exact wording in ARMARMv7-M is:
>
>         The Vector table must be naturally aligned to a power of two
>         whose alignment value is greater than or equal
>         to (Number of Exceptions supported x 4), with a minimum
>         alignment of 128 bytes.
>
>> Or we should keep it at the maximum value possible?
> So we need:
>
>         .align x
>
> with x being max(7, ceil(log((CPUV7M_NUM_IRQ + 16) * 4, 2))). So the
> alignment needed is between 7 and 10.
>
> If the assembler supports an expression here I'd use that. But before
> adding strange hacks to generate the right value there better go for a
> static value like:
>
>         /* The vector table must be naturally aligned */
>         #if CONFIG_CPUV7M_NUM_IRQ <= 112
>         .align 9 /* log2((112 + 16) * 4) */
>         #else
>         .align 10
>         #endif
>
> Further steps would be:
>
>         CONFIG_CPUV7M_NUM_IRQ <= 48 -> .align 8
>         CONFIG_CPUV7M_NUM_IRQ <= 16 -> .align 7
>
> Probably it's not worth to add the respective #ifdefs here.

I will go for the  #ifdefs.

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 02/14] ARM: ARMv7M: Enlarge vector table to 256 entries

[Rob Herring]

On Fri, Feb 13, 2015 at 2:42 AM, Maxime Coquelin
<mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Geert,
>
> 2015-02-12 21:34 GMT+01:00 Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>:
>> On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
>> <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> From Cortex-M4 and M7 reference manuals, the nvic supports up to 240
>>> interrupts. So the number of entries in vectors table is 256.
>>>
>>> This patch adds the missing entries, and change the alignement, so that
>>> vector_table remains naturally aligned.
>>
>> Shouldn't this depend on ARCH_STM32, or some other M4 or M7 specific
>> Kconfig option, to avoid wasting the space on other CPUs?
>
> Actually, the STM32F429 has 90 interrupts, so it would need 106
> entries in the vector table.
> The maximum of supported interrupts is not only for Cortex-M4 and M7,
> this is also true for Cortex-M3.
>
> I see two possibilities:
>  1 - We declare the vector table for the maximum supported number of
> IRQs, as this patch does.
>         - Pro: it will be functionnal with all Cortex-M MCUs
>         - Con: Waste of less than 1KB for memory

The waste depends on the alignment size as well and could be up to
almost 2KB worst case. It varies depending on the padding. We should
try to place it so it always aligned and the wasted space is
minimized.

Rob

>  2 - We introduce a config flag that provides the number of interrupts
>         - Pro: No more memory waste
>         - Con: Need to declare a per MCU model config flag.
>
> Then, regarding the natural alignment, is there a way to ensure it
> depending on the value of a config flag?
> Or we should keep it at the maximum value possible?
>
> Any feedback will be appreciated, especially from Uwe who maintains
> the efm32 machine.
>
> Kind regards,
> Maxime

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

[Maxime Coquelin]

Hi Rob,

2015-02-15 23:42 GMT+01:00 Rob Herring <robherring2@gmail.com>:
> On Fri, Feb 13, 2015 at 2:42 AM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> Hi Geert,
>>
>> 2015-02-12 21:34 GMT+01:00 Geert Uytterhoeven <geert@linux-m68k.org>:
>>> On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
>>> <mcoquelin.stm32@gmail.com> wrote:
>>>> From Cortex-M4 and M7 reference manuals, the nvic supports up to 240
>>>> interrupts. So the number of entries in vectors table is 256.
>>>>
>>>> This patch adds the missing entries, and change the alignement, so that
>>>> vector_table remains naturally aligned.
>>>
>>> Shouldn't this depend on ARCH_STM32, or some other M4 or M7 specific
>>> Kconfig option, to avoid wasting the space on other CPUs?
>>
>> Actually, the STM32F429 has 90 interrupts, so it would need 106
>> entries in the vector table.
>> The maximum of supported interrupts is not only for Cortex-M4 and M7,
>> this is also true for Cortex-M3.
>>
>> I see two possibilities:
>>  1 - We declare the vector table for the maximum supported number of
>> IRQs, as this patch does.
>>         - Pro: it will be functionnal with all Cortex-M MCUs
>>         - Con: Waste of less than 1KB for memory
>
> The waste depends on the alignment size as well and could be up to
> almost 2KB worst case. It varies depending on the padding. We should
> try to place it so it always aligned and the wasted space is
> minimized.

Sorry, I just notice I didn't replied to all. That was my question:

Do you mean by forcing its location in the arch/arm/kernel/vmlinux.lds.S file?

Regards,
Maxime

>
> Rob
>
>>  2 - We introduce a config flag that provides the number of interrupts
>>         - Pro: No more memory waste
>>         - Con: Need to declare a per MCU model config flag.
>>
>> Then, regarding the natural alignment, is there a way to ensure it
>> depending on the value of a config flag?
>> Or we should keep it at the maximum value possible?
>>
>> Any feedback will be appreciated, especially from Uwe who maintains
>> the efm32 machine.
>>
>> Kind regards,
>> Maxime

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

[Rob Herring]

On Thu, Feb 19, 2015 at 10:13 AM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> Hi Rob,
>
> 2015-02-15 23:42 GMT+01:00 Rob Herring <robherring2@gmail.com>:
>> On Fri, Feb 13, 2015 at 2:42 AM, Maxime Coquelin
>> <mcoquelin.stm32@gmail.com> wrote:
>>> Hi Geert,
>>>
>>> 2015-02-12 21:34 GMT+01:00 Geert Uytterhoeven <geert@linux-m68k.org>:
>>>> On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
>>>> <mcoquelin.stm32@gmail.com> wrote:
>>>>> From Cortex-M4 and M7 reference manuals, the nvic supports up to 240
>>>>> interrupts. So the number of entries in vectors table is 256.
>>>>>
>>>>> This patch adds the missing entries, and change the alignement, so that
>>>>> vector_table remains naturally aligned.
>>>>
>>>> Shouldn't this depend on ARCH_STM32, or some other M4 or M7 specific
>>>> Kconfig option, to avoid wasting the space on other CPUs?
>>>
>>> Actually, the STM32F429 has 90 interrupts, so it would need 106
>>> entries in the vector table.
>>> The maximum of supported interrupts is not only for Cortex-M4 and M7,
>>> this is also true for Cortex-M3.
>>>
>>> I see two possibilities:
>>>  1 - We declare the vector table for the maximum supported number of
>>> IRQs, as this patch does.
>>>         - Pro: it will be functionnal with all Cortex-M MCUs
>>>         - Con: Waste of less than 1KB for memory
>>
>> The waste depends on the alignment size as well and could be up to
>> almost 2KB worst case. It varies depending on the padding. We should
>> try to place it so it always aligned and the wasted space is
>> minimized.
>
> Sorry, I just notice I didn't replied to all. That was my question:
>
> Do you mean by forcing its location in the arch/arm/kernel/vmlinux.lds.S file?

Yes, that is one way. Or we might be able to just be smarter about how
we arrange the code. The first thing to do is figure out how much
space we waste and what comes before it.

Rob

>
> Regards,
> Maxime
>
>>
>> Rob
>>
>>>  2 - We introduce a config flag that provides the number of interrupts
>>>         - Pro: No more memory waste
>>>         - Con: Need to declare a per MCU model config flag.
>>>
>>> Then, regarding the natural alignment, is there a way to ensure it
>>> depending on the value of a config flag?
>>> Or we should keep it at the maximum value possible?
>>>
>>> Any feedback will be appreciated, especially from Uwe who maintains
>>> the efm32 machine.
>>>
>>> Kind regards,
>>> Maxime

Re: [PATCH 08/14] pinctrl: Add pinctrl driver for STM32 MCUs

[Geert Uytterhoeven]

On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -125,6 +125,15 @@ config PINCTRL_ST
>         select PINCONF
>         select GPIOLIB_IRQCHIP
>
> +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.
> +
>  config PINCTRL_TEGRA
>         bool
>         select PINMUX

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 08/14] pinctrl: Add pinctrl driver for STM32 MCUs

[Maxime Coquelin]

2015-02-12 21:37 GMT+01:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -125,6 +125,15 @@ config PINCTRL_ST
>>         select PINCONF
>>         select GPIOLIB_IRQCHIP
>>
>> +config PINCTRL_STM32
>> +       bool "STMicroelectronics STM32 pinctrl driver"
>> +       depends on OF
>
> depends on ARCH_STM32 || COMPILE_TEST

Ok, I agree.

It will be part of the v2.

Thanks,
Maxime

Re: [PATCH 12/14] ARM: dts: Introduce STM32F429 MCU

[Philipp Zabel]

Hi Maxime,

Am Donnerstag, den 12.02.2015, 18:46 +0100 schrieb Maxime Coquelin:
[...]
> +	soc {
> +		reset_ahb1: reset@40023810 {
> +			#reset-cells = <1>;
> +			compatible = "st,stm32-reset";
> +			reg = <0x40023810 0x4>;
> +		};
> +
> +		reset_ahb2: reset@40023814 {
> +			#reset-cells = <1>;
> +			compatible = "st,stm32-reset";
> +			reg = <0x40023814 0x4>;
> +		};
> +
> +		reset_ahb3: reset@40023818 {
> +			#reset-cells = <1>;
> +			compatible = "st,stm32-reset";
> +			reg = <0x40023818 0x4>;
> +		};
> +
> +		reset_apb1: reset@40023820 {
> +			#reset-cells = <1>;
> +			compatible = "st,stm32-reset";
> +			reg = <0x40023820 0x4>;
> +		};
> +
> +		reset_apb2: reset@40023824 {
> +			#reset-cells = <1>;
> +			compatible = "st,stm32-reset";
> +			reg = <0x40023824 0x4>;
> +		};

These are mostly consecutive, single registers. I wonder if these are
part of the same IP block and thus should be grouped together into the
same reset controller node?

regards
Philipp

Re: [PATCH 12/14] ARM: dts: Introduce STM32F429 MCU

[Maxime Coquelin]

Hi Philipp,

2015-02-13 12:47 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Hi Maxime,
>
> Am Donnerstag, den 12.02.2015, 18:46 +0100 schrieb Maxime Coquelin:
> [...]
>> +     soc {
>> +             reset_ahb1: reset@40023810 {
>> +                     #reset-cells = <1>;
>> +                     compatible = "st,stm32-reset";
>> +                     reg = <0x40023810 0x4>;
>> +             };
>> +
>> +             reset_ahb2: reset@40023814 {
>> +                     #reset-cells = <1>;
>> +                     compatible = "st,stm32-reset";
>> +                     reg = <0x40023814 0x4>;
>> +             };
>> +
>> +             reset_ahb3: reset@40023818 {
>> +                     #reset-cells = <1>;
>> +                     compatible = "st,stm32-reset";
>> +                     reg = <0x40023818 0x4>;
>> +             };
>> +
>> +             reset_apb1: reset@40023820 {
>> +                     #reset-cells = <1>;
>> +                     compatible = "st,stm32-reset";
>> +                     reg = <0x40023820 0x4>;
>> +             };
>> +
>> +             reset_apb2: reset@40023824 {
>> +                     #reset-cells = <1>;
>> +                     compatible = "st,stm32-reset";
>> +                     reg = <0x40023824 0x4>;
>> +             };
>
> These are mostly consecutive, single registers. I wonder if these are
> part of the same IP block and thus should be grouped together into the
> same reset controller node?

What I could to is to have two instances. One for AHB and one for APB domain.
Doing this, I will have one instance per domain, and only consecutive registers.
Is it fine for you?

Thanks,
Maxime

>
> regards
> Philipp
>

Re: [PATCH 12/14] ARM: dts: Introduce STM32F429 MCU

[Philipp Zabel]

Hi Maxime,

Am Freitag, den 13.02.2015, 16:59 +0100 schrieb Maxime Coquelin:
> Hi Philipp,
> 
> 2015-02-13 12:47 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > Hi Maxime,
> >
> > Am Donnerstag, den 12.02.2015, 18:46 +0100 schrieb Maxime Coquelin:
> > [...]
> >> +     soc {
> >> +             reset_ahb1: reset@40023810 {
> >> +                     #reset-cells = <1>;
> >> +                     compatible = "st,stm32-reset";
> >> +                     reg = <0x40023810 0x4>;
> >> +             };
> >> +
> >> +             reset_ahb2: reset@40023814 {
> >> +                     #reset-cells = <1>;
> >> +                     compatible = "st,stm32-reset";
> >> +                     reg = <0x40023814 0x4>;
> >> +             };
> >> +
> >> +             reset_ahb3: reset@40023818 {
> >> +                     #reset-cells = <1>;
> >> +                     compatible = "st,stm32-reset";
> >> +                     reg = <0x40023818 0x4>;
> >> +             };
> >> +
> >> +             reset_apb1: reset@40023820 {
> >> +                     #reset-cells = <1>;
> >> +                     compatible = "st,stm32-reset";
> >> +                     reg = <0x40023820 0x4>;
> >> +             };
> >> +
> >> +             reset_apb2: reset@40023824 {
> >> +                     #reset-cells = <1>;
> >> +                     compatible = "st,stm32-reset";
> >> +                     reg = <0x40023824 0x4>;
> >> +             };
> >
> > These are mostly consecutive, single registers. I wonder if these are
> > part of the same IP block and thus should be grouped together into the
> > same reset controller node?
> 
> What I could to is to have two instances. One for AHB and one for APB domain.
> Doing this, I will have one instance per domain, and only consecutive registers.
> Is it fine for you?

Looking at
http://www.st.com/web/en/resource/technical/document/reference_manual/DM00031020.pdf
Table 34 (RCC register map and reset values), I'd say there is a single
"Reset and Clock Control" device at 0x40023800 - 0x40023884:

	soc {
		rcc: rcc@40023800 {
			#clock-cells = <1>;
			#reset-cells = <1>;
			compatible = "st,stm32-rcc";
			reg = <0x40023800 0x84>;
		};

		...

If you really want to describe the reset controller parts (offsets +0x10
to +0x24) in a separate node, I won't argue against it too long,
although this is a somewhat arbitrary decision.

In any case, the whole register at offset +0x1c is reserved, so there is
no reason to split the reset controller. It is ok to have unused ranges
as is already the case with reserved bits inside the used registers.

regards
Philipp

Re: [PATCH 12/14] ARM: dts: Introduce STM32F429 MCU

[Maxime Coquelin]

2015-02-13 17:25 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Hi Maxime,
>
> Am Freitag, den 13.02.2015, 16:59 +0100 schrieb Maxime Coquelin:
>> Hi Philipp,
>>
>> 2015-02-13 12:47 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> > Hi Maxime,
>> >
>> > Am Donnerstag, den 12.02.2015, 18:46 +0100 schrieb Maxime Coquelin:
>> > [...]
>> >> +     soc {
>> >> +             reset_ahb1: reset@40023810 {
>> >> +                     #reset-cells = <1>;
>> >> +                     compatible = "st,stm32-reset";
>> >> +                     reg = <0x40023810 0x4>;
>> >> +             };
>> >> +
>> >> +             reset_ahb2: reset@40023814 {
>> >> +                     #reset-cells = <1>;
>> >> +                     compatible = "st,stm32-reset";
>> >> +                     reg = <0x40023814 0x4>;
>> >> +             };
>> >> +
>> >> +             reset_ahb3: reset@40023818 {
>> >> +                     #reset-cells = <1>;
>> >> +                     compatible = "st,stm32-reset";
>> >> +                     reg = <0x40023818 0x4>;
>> >> +             };
>> >> +
>> >> +             reset_apb1: reset@40023820 {
>> >> +                     #reset-cells = <1>;
>> >> +                     compatible = "st,stm32-reset";
>> >> +                     reg = <0x40023820 0x4>;
>> >> +             };
>> >> +
>> >> +             reset_apb2: reset@40023824 {
>> >> +                     #reset-cells = <1>;
>> >> +                     compatible = "st,stm32-reset";
>> >> +                     reg = <0x40023824 0x4>;
>> >> +             };
>> >
>> > These are mostly consecutive, single registers. I wonder if these are
>> > part of the same IP block and thus should be grouped together into the
>> > same reset controller node?
>>
>> What I could to is to have two instances. One for AHB and one for APB domain.
>> Doing this, I will have one instance per domain, and only consecutive registers.
>> Is it fine for you?
>
> Looking at
> http://www.st.com/web/en/resource/technical/document/reference_manual/DM00031020.pdf
> Table 34 (RCC register map and reset values), I'd say there is a single
> "Reset and Clock Control" device at 0x40023800 - 0x40023884:
>
>         soc {
>                 rcc: rcc@40023800 {
>                         #clock-cells = <1>;
>                         #reset-cells = <1>;
>                         compatible = "st,stm32-rcc";
>                         reg = <0x40023800 0x84>;
>                 };
>
>                 ...
>
> If you really want to describe the reset controller parts (offsets +0x10
> to +0x24) in a separate node, I won't argue against it too long,
> although this is a somewhat arbitrary decision.
>
> In any case, the whole register at offset +0x1c is reserved, so there is
> no reason to split the reset controller. It is ok to have unused ranges
> as is already the case with reserved bits inside the used registers.

Ok. I understand your point.
But it will be more difficult at usage, because the node referencing
the fourth reset bit of apb2 register will have to pass 164 as
parameter.
It is error prone IMHO.

Other solution would be to add some defines for each reset line in the
DT-Bindings, as we do today for STi platform.
But it is giving an unneeded constraint between DT and reset trees.

Br,
Maxime

>
> regards
> Philipp
>

Re: [PATCH 12/14] ARM: dts: Introduce STM32F429 MCU

[Philipp Zabel]

Am Freitag, den 13.02.2015, 17:41 +0100 schrieb Maxime Coquelin:
> 2015-02-13 17:25 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > Hi Maxime,
> >
> > Am Freitag, den 13.02.2015, 16:59 +0100 schrieb Maxime Coquelin:
> >> Hi Philipp,
> >>
> >> 2015-02-13 12:47 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
> >> > Hi Maxime,
> >> >
> >> > Am Donnerstag, den 12.02.2015, 18:46 +0100 schrieb Maxime Coquelin:
> >> > [...]
> >> >> +     soc {
> >> >> +             reset_ahb1: reset@40023810 {
> >> >> +                     #reset-cells = <1>;
> >> >> +                     compatible = "st,stm32-reset";
> >> >> +                     reg = <0x40023810 0x4>;
> >> >> +             };
> >> >> +
> >> >> +             reset_ahb2: reset@40023814 {
> >> >> +                     #reset-cells = <1>;
> >> >> +                     compatible = "st,stm32-reset";
> >> >> +                     reg = <0x40023814 0x4>;
> >> >> +             };
> >> >> +
> >> >> +             reset_ahb3: reset@40023818 {
> >> >> +                     #reset-cells = <1>;
> >> >> +                     compatible = "st,stm32-reset";
> >> >> +                     reg = <0x40023818 0x4>;
> >> >> +             };
> >> >> +
> >> >> +             reset_apb1: reset@40023820 {
> >> >> +                     #reset-cells = <1>;
> >> >> +                     compatible = "st,stm32-reset";
> >> >> +                     reg = <0x40023820 0x4>;
> >> >> +             };
> >> >> +
> >> >> +             reset_apb2: reset@40023824 {
> >> >> +                     #reset-cells = <1>;
> >> >> +                     compatible = "st,stm32-reset";
> >> >> +                     reg = <0x40023824 0x4>;
> >> >> +             };
> >> >
> >> > These are mostly consecutive, single registers. I wonder if these are
> >> > part of the same IP block and thus should be grouped together into the
> >> > same reset controller node?
> >>
> >> What I could to is to have two instances. One for AHB and one for APB domain.
> >> Doing this, I will have one instance per domain, and only consecutive registers.
> >> Is it fine for you?
> >
> > Looking at
> > http://www.st.com/web/en/resource/technical/document/reference_manual/DM00031020.pdf
> > Table 34 (RCC register map and reset values), I'd say there is a single
> > "Reset and Clock Control" device at 0x40023800 - 0x40023884:
> >
> >         soc {
> >                 rcc: rcc@40023800 {
> >                         #clock-cells = <1>;
> >                         #reset-cells = <1>;
> >                         compatible = "st,stm32-rcc";
> >                         reg = <0x40023800 0x84>;
> >                 };
> >
> >                 ...
> >
> > If you really want to describe the reset controller parts (offsets +0x10
> > to +0x24) in a separate node, I won't argue against it too long,
> > although this is a somewhat arbitrary decision.
> >
> > In any case, the whole register at offset +0x1c is reserved, so there is
> > no reason to split the reset controller. It is ok to have unused ranges
> > as is already the case with reserved bits inside the used registers.
> 
> Ok. I understand your point.
> But it will be more difficult at usage, because the node referencing
> the fourth reset bit of apb2 register will have to pass 164 as
> parameter.
> It is error prone IMHO.
>
> Other solution would be to add some defines for each reset line in the
> DT-Bindings, as we do today for STi platform.
> But it is giving an unneeded constraint between DT and reset trees.

That is a bit unfortunate, but providing the named constants in
include/dt-bindings/reset/ makes for a much better readable device tree,
so I'd prefer that solution, even if it means having to coordinate pull
requests.

regards
Philipp

Re: [PATCH 12/14] ARM: dts: Introduce STM32F429 MCU

[Maxime Coquelin]

Hi Philipp,

2015-02-13 20:18 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Freitag, den 13.02.2015, 17:41 +0100 schrieb Maxime Coquelin:
>> 2015-02-13 17:25 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> > Hi Maxime,
>> >
>> > Am Freitag, den 13.02.2015, 16:59 +0100 schrieb Maxime Coquelin:
>> >> Hi Philipp,
>> >>
>> >> 2015-02-13 12:47 GMT+01:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> >> > Hi Maxime,
>> >> >
>> >> > Am Donnerstag, den 12.02.2015, 18:46 +0100 schrieb Maxime Coquelin:
>> >> > [...]
>> >> >> +     soc {
>> >> >> +             reset_ahb1: reset@40023810 {
>> >> >> +                     #reset-cells = <1>;
>> >> >> +                     compatible = "st,stm32-reset";
>> >> >> +                     reg = <0x40023810 0x4>;
>> >> >> +             };
>> >> >> +
>> >> >> +             reset_ahb2: reset@40023814 {
>> >> >> +                     #reset-cells = <1>;
>> >> >> +                     compatible = "st,stm32-reset";
>> >> >> +                     reg = <0x40023814 0x4>;
>> >> >> +             };
>> >> >> +
>> >> >> +             reset_ahb3: reset@40023818 {
>> >> >> +                     #reset-cells = <1>;
>> >> >> +                     compatible = "st,stm32-reset";
>> >> >> +                     reg = <0x40023818 0x4>;
>> >> >> +             };
>> >> >> +
>> >> >> +             reset_apb1: reset@40023820 {
>> >> >> +                     #reset-cells = <1>;
>> >> >> +                     compatible = "st,stm32-reset";
>> >> >> +                     reg = <0x40023820 0x4>;
>> >> >> +             };
>> >> >> +
>> >> >> +             reset_apb2: reset@40023824 {
>> >> >> +                     #reset-cells = <1>;
>> >> >> +                     compatible = "st,stm32-reset";
>> >> >> +                     reg = <0x40023824 0x4>;
>> >> >> +             };
>> >> >
>> >> > These are mostly consecutive, single registers. I wonder if these are
>> >> > part of the same IP block and thus should be grouped together into the
>> >> > same reset controller node?
>> >>
>> >> What I could to is to have two instances. One for AHB and one for APB domain.
>> >> Doing this, I will have one instance per domain, and only consecutive registers.
>> >> Is it fine for you?
>> >
>> > Looking at
>> > http://www.st.com/web/en/resource/technical/document/reference_manual/DM00031020.pdf
>> > Table 34 (RCC register map and reset values), I'd say there is a single
>> > "Reset and Clock Control" device at 0x40023800 - 0x40023884:
>> >
>> >         soc {
>> >                 rcc: rcc@40023800 {
>> >                         #clock-cells = <1>;
>> >                         #reset-cells = <1>;
>> >                         compatible = "st,stm32-rcc";
>> >                         reg = <0x40023800 0x84>;
>> >                 };
>> >
>> >                 ...
>> >
>> > If you really want to describe the reset controller parts (offsets +0x10
>> > to +0x24) in a separate node, I won't argue against it too long,
>> > although this is a somewhat arbitrary decision.
>> >
>> > In any case, the whole register at offset +0x1c is reserved, so there is
>> > no reason to split the reset controller. It is ok to have unused ranges
>> > as is already the case with reserved bits inside the used registers.
>>
>> Ok. I understand your point.
>> But it will be more difficult at usage, because the node referencing
>> the fourth reset bit of apb2 register will have to pass 164 as
>> parameter.
>> It is error prone IMHO.
>>
>> Other solution would be to add some defines for each reset line in the
>> DT-Bindings, as we do today for STi platform.
>> But it is giving an unneeded constraint between DT and reset trees.
>
> That is a bit unfortunate, but providing the named constants in
> include/dt-bindings/reset/ makes for a much better readable device tree,
> so I'd prefer that solution, even if it means having to coordinate pull
> requests.

Ok, I will add constants in include/dt-bindings/reset/

Thanks,
Maxime

>
> regards
> Philipp
>

Re: [PATCH 04/14] reset: Add reset_controller_of_init() function

[Philipp Zabel]

Hi Maxime,

Am Donnerstag, den 12.02.2015, 18:45 +0100 schrieb Maxime Coquelin:
> 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>
> ---
>  drivers/reset/core.c              | 20 ++++++++++++++++++++
>  include/asm-generic/vmlinux.lds.h |  4 +++-
>  include/linux/reset-controller.h  |  6 ++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 7955e00..18ee579 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -86,6 +86,26 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
>  }
>  EXPORT_SYMBOL_GPL(reset_controller_unregister);
>  
> +extern struct of_device_id __reset_ctrl_of_table[];
> +
> +static const struct of_device_id __reset_ctrl_of_table_sentinel
> +	__used __section(__reset_ctrl_of_table_end);
> +
> +void __init reset_controller_of_init(void)

The patch looks fine to me, but this function is missing a kerneldoc
comment.

> +{
> +	struct device_node *np;
> +	const struct of_device_id *match;
> +	of_init_fn_1 init_func;
> +
> +	for_each_matching_node_and_match(np, __reset_ctrl_of_table, &match) {
> +		if (!of_device_is_available(np))
> +			continue;
> +
> +		init_func = match->data;
> +		init_func(np);
> +	}
> +}
> +
>  /**
>   * reset_control_reset - reset the controlled device
>   * @rstc: reset controller

regards
Philipp

Re: [PATCH 04/14] reset: Add reset_controller_of_init() function

[Maxime Coquelin]

Hi Philipp,

2015-02-13 12:49 GMT+01:00 Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
> Hi Maxime,
>
> Am Donnerstag, den 12.02.2015, 18:45 +0100 schrieb Maxime Coquelin:
>> 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>
>> ---
>>  drivers/reset/core.c              | 20 ++++++++++++++++++++
>>  include/asm-generic/vmlinux.lds.h |  4 +++-
>>  include/linux/reset-controller.h  |  6 ++++++
>>  3 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index 7955e00..18ee579 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -86,6 +86,26 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
>>  }
>>  EXPORT_SYMBOL_GPL(reset_controller_unregister);
>>
>> +extern struct of_device_id __reset_ctrl_of_table[];
>> +
>> +static const struct of_device_id __reset_ctrl_of_table_sentinel
>> +     __used __section(__reset_ctrl_of_table_end);
>> +
>> +void __init reset_controller_of_init(void)
>
> The patch looks fine to me, but this function is missing a kerneldoc
> comment.

Right! It will be fixed in the v2.

Thanks,
Maxime
--
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 00/14] Add support to STMicroelectronics STM32 family

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 5960 bytes --]

Hi Maxime,

Am 12.02.2015 um 18:45 schrieb Maxime Coquelin:
> This patchset adds basic support for STMicroelectronics STM32 series MCUs.
> 
> STM32 MCUs are Cortex-M CPU, used in various applications (consumer
> electronics, industrial applications, hobbyists...).
> Datasheets, user and programming manuals are publicly available on
> STMicroelectronics website.
> 
> With this series applied, the STM32F419 Discovery can boot succesfully.
> 
> Once this series accepted, next steps will be to add DMA support, as USART,
> I2C and SPI IPs don't have any FIFO. Then will come the clock driver, as today
> the bootloader has to be patched to enable the needed clocks.

This is somewhat unfortunate, as I have been working on the same thing
and have demonstrated the STM32F429 Discovery Kit at ARM TechSymposium
Europe in December and submitted a talk for LinuxCon Japan.

https://github.com/afaerber/afboot-stm32
https://github.com/afaerber/linux/commits/stm32

On a brief look, it seems you are further along in terms of code quality
and documenting. Do you spot anything that's missing in your series and
could be added from my branch? The clk controller maybe? Also I already
started looking into gpio and usb drivers. Me too, I skipped DMA support
though.

Regards,
Andreas

> 
> Maxime Coquelin (14):
>   scripts: link-vmlinux: Don't pass page offset to kallsyms if XIP
>     Kernel
>   ARM: ARMv7M: Enlarge vector table to 256 entries
>   clocksource: Add ARM System timer driver
>   reset: Add reset_controller_of_init() function
>   ARM: call reset_controller_of_init from default time_init handler
>   drivers: reset: Add STM32 reset driver
>   clockevent: Add STM32 Timer driver
>   pinctrl: Add pinctrl driver for STM32 MCUs
>   serial: stm32-usart: Add STM32 USART Driver
>   ARM: Add STM32 family machine
>   ARM: dts: Add ARM System timer as clockevent in armv7m
>   ARM: dts: Introduce STM32F429 MCU
>   ARM: configs: Add STM32 defconfig
>   MAINTAINERS: Add entry for STM32 MCUs
> 
>  Documentation/arm/stm32/overview.txt               |  32 +
>  Documentation/arm/stm32/stm32f429-overview.txt     |  22 +
>  .../devicetree/bindings/arm/system_timer.txt       |  15 +
>  .../devicetree/bindings/pinctrl/pinctrl-stm32.txt  |  99 +++
>  .../devicetree/bindings/reset/st,stm32-reset.txt   |  19 +
>  .../devicetree/bindings/serial/st,stm32-usart.txt  |  18 +
>  .../devicetree/bindings/timer/st,stm32-timer.txt   |  19 +
>  MAINTAINERS                                        |   7 +
>  arch/arm/Kconfig                                   |  22 +
>  arch/arm/Makefile                                  |   1 +
>  arch/arm/boot/dts/Makefile                         |   1 +
>  arch/arm/boot/dts/armv7-m.dtsi                     |   7 +
>  arch/arm/boot/dts/stm32f429-disco.dts              |  41 ++
>  arch/arm/boot/dts/stm32f429.dtsi                   | 279 ++++++++
>  arch/arm/configs/stm32_defconfig                   |  72 ++
>  arch/arm/kernel/entry-v7m.S                        |   8 +-
>  arch/arm/kernel/time.c                             |   4 +
>  arch/arm/mach-stm32/Makefile                       |   1 +
>  arch/arm/mach-stm32/Makefile.boot                  |   0
>  arch/arm/mach-stm32/board-dt.c                     |  19 +
>  drivers/clocksource/Kconfig                        |  16 +
>  drivers/clocksource/Makefile                       |   2 +
>  drivers/clocksource/arm_system_timer.c             |  74 ++
>  drivers/clocksource/timer-stm32.c                  | 187 +++++
>  drivers/pinctrl/Kconfig                            |   9 +
>  drivers/pinctrl/Makefile                           |   1 +
>  drivers/pinctrl/pinctrl-stm32.c                    | 779 +++++++++++++++++++++
>  drivers/reset/Makefile                             |   1 +
>  drivers/reset/core.c                               |  20 +
>  drivers/reset/reset-stm32.c                        | 124 ++++
>  drivers/tty/serial/Kconfig                         |  17 +
>  drivers/tty/serial/Makefile                        |   1 +
>  drivers/tty/serial/stm32-usart.c                   | 695 ++++++++++++++++++
>  include/asm-generic/vmlinux.lds.h                  |   4 +-
>  include/dt-bindings/pinctrl/pinctrl-stm32.h        |  43 ++
>  include/linux/reset-controller.h                   |   6 +
>  include/uapi/linux/serial_core.h                   |   3 +
>  scripts/link-vmlinux.sh                            |   2 +-
>  38 files changed, 2664 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/arm/stm32/overview.txt
>  create mode 100644 Documentation/arm/stm32/stm32f429-overview.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/system_timer.txt
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-stm32.txt
>  create mode 100644 Documentation/devicetree/bindings/reset/st,stm32-reset.txt
>  create mode 100644 Documentation/devicetree/bindings/serial/st,stm32-usart.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/st,stm32-timer.txt
>  create mode 100644 arch/arm/boot/dts/stm32f429-disco.dts
>  create mode 100644 arch/arm/boot/dts/stm32f429.dtsi
>  create mode 100644 arch/arm/configs/stm32_defconfig
>  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
>  create mode 100644 drivers/clocksource/arm_system_timer.c
>  create mode 100644 drivers/clocksource/timer-stm32.c
>  create mode 100644 drivers/pinctrl/pinctrl-stm32.c
>  create mode 100644 drivers/reset/reset-stm32.c
>  create mode 100644 drivers/tty/serial/stm32-usart.c
>  create mode 100644 include/dt-bindings/pinctrl/pinctrl-stm32.h
> 


-- 
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)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 00/14] Add support to STMicroelectronics STM32 family

[Maxime Coquelin]

Hi Andreas,

2015-02-15 16:14 GMT+01:00 Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>:
> Hi Maxime,
>
> Am 12.02.2015 um 18:45 schrieb Maxime Coquelin:
>> This patchset adds basic support for STMicroelectronics STM32 series MCUs.
>>
>> STM32 MCUs are Cortex-M CPU, used in various applications (consumer
>> electronics, industrial applications, hobbyists...).
>> Datasheets, user and programming manuals are publicly available on
>> STMicroelectronics website.
>>
>> With this series applied, the STM32F419 Discovery can boot succesfully.
>>
>> Once this series accepted, next steps will be to add DMA support, as USART,
>> I2C and SPI IPs don't have any FIFO. Then will come the clock driver, as today
>> the bootloader has to be patched to enable the needed clocks.
>
> This is somewhat unfortunate, as I have been working on the same thing
> and have demonstrated the STM32F429 Discovery Kit at ARM TechSymposium
> Europe in December and submitted a talk for LinuxCon Japan.
>
> https://github.com/afaerber/afboot-stm32
> https://github.com/afaerber/linux/commits/stm32

Hmm, I wasn't aware you were also working on it.
The good news is that we are not alone on this task :).
I do it on my spare time, so any contribution is more than welcome.

>
> On a brief look, it seems you are further along in terms of code quality
> and documenting. Do you spot anything that's missing in your series and
> could be added from my branch? The clk controller maybe? Also I already
> started looking into gpio and usb drivers. Me too, I skipped DMA support
> though.

The GPIO support is already part of the pinctrl patch.
The missing thing is the GPIO interrupt feature, but I am working on it.

Maybe you could focus on the clock support, as I see its support is
well advanced in you tree?
I see one bug in it, the timer clocks should be 90MHz, but your patch
indicates 45MHz.

I see you have started the LCD controller driver, maybe that is
another task you could handle?

Regarding USB, have you made it to work?

Kind regards,
Maxime

>
> Regards,
> Andreas
>
>>
>> Maxime Coquelin (14):
>>   scripts: link-vmlinux: Don't pass page offset to kallsyms if XIP
>>     Kernel
>>   ARM: ARMv7M: Enlarge vector table to 256 entries
>>   clocksource: Add ARM System timer driver
>>   reset: Add reset_controller_of_init() function
>>   ARM: call reset_controller_of_init from default time_init handler
>>   drivers: reset: Add STM32 reset driver
>>   clockevent: Add STM32 Timer driver
>>   pinctrl: Add pinctrl driver for STM32 MCUs
>>   serial: stm32-usart: Add STM32 USART Driver
>>   ARM: Add STM32 family machine
>>   ARM: dts: Add ARM System timer as clockevent in armv7m
>>   ARM: dts: Introduce STM32F429 MCU
>>   ARM: configs: Add STM32 defconfig
>>   MAINTAINERS: Add entry for STM32 MCUs
>>
>>  Documentation/arm/stm32/overview.txt               |  32 +
>>  Documentation/arm/stm32/stm32f429-overview.txt     |  22 +
>>  .../devicetree/bindings/arm/system_timer.txt       |  15 +
>>  .../devicetree/bindings/pinctrl/pinctrl-stm32.txt  |  99 +++
>>  .../devicetree/bindings/reset/st,stm32-reset.txt   |  19 +
>>  .../devicetree/bindings/serial/st,stm32-usart.txt  |  18 +
>>  .../devicetree/bindings/timer/st,stm32-timer.txt   |  19 +
>>  MAINTAINERS                                        |   7 +
>>  arch/arm/Kconfig                                   |  22 +
>>  arch/arm/Makefile                                  |   1 +
>>  arch/arm/boot/dts/Makefile                         |   1 +
>>  arch/arm/boot/dts/armv7-m.dtsi                     |   7 +
>>  arch/arm/boot/dts/stm32f429-disco.dts              |  41 ++
>>  arch/arm/boot/dts/stm32f429.dtsi                   | 279 ++++++++
>>  arch/arm/configs/stm32_defconfig                   |  72 ++
>>  arch/arm/kernel/entry-v7m.S                        |   8 +-
>>  arch/arm/kernel/time.c                             |   4 +
>>  arch/arm/mach-stm32/Makefile                       |   1 +
>>  arch/arm/mach-stm32/Makefile.boot                  |   0
>>  arch/arm/mach-stm32/board-dt.c                     |  19 +
>>  drivers/clocksource/Kconfig                        |  16 +
>>  drivers/clocksource/Makefile                       |   2 +
>>  drivers/clocksource/arm_system_timer.c             |  74 ++
>>  drivers/clocksource/timer-stm32.c                  | 187 +++++
>>  drivers/pinctrl/Kconfig                            |   9 +
>>  drivers/pinctrl/Makefile                           |   1 +
>>  drivers/pinctrl/pinctrl-stm32.c                    | 779 +++++++++++++++++++++
>>  drivers/reset/Makefile                             |   1 +
>>  drivers/reset/core.c                               |  20 +
>>  drivers/reset/reset-stm32.c                        | 124 ++++
>>  drivers/tty/serial/Kconfig                         |  17 +
>>  drivers/tty/serial/Makefile                        |   1 +
>>  drivers/tty/serial/stm32-usart.c                   | 695 ++++++++++++++++++
>>  include/asm-generic/vmlinux.lds.h                  |   4 +-
>>  include/dt-bindings/pinctrl/pinctrl-stm32.h        |  43 ++
>>  include/linux/reset-controller.h                   |   6 +
>>  include/uapi/linux/serial_core.h                   |   3 +
>>  scripts/link-vmlinux.sh                            |   2 +-
>>  38 files changed, 2664 insertions(+), 6 deletions(-)
>>  create mode 100644 Documentation/arm/stm32/overview.txt
>>  create mode 100644 Documentation/arm/stm32/stm32f429-overview.txt
>>  create mode 100644 Documentation/devicetree/bindings/arm/system_timer.txt
>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-stm32.txt
>>  create mode 100644 Documentation/devicetree/bindings/reset/st,stm32-reset.txt
>>  create mode 100644 Documentation/devicetree/bindings/serial/st,stm32-usart.txt
>>  create mode 100644 Documentation/devicetree/bindings/timer/st,stm32-timer.txt
>>  create mode 100644 arch/arm/boot/dts/stm32f429-disco.dts
>>  create mode 100644 arch/arm/boot/dts/stm32f429.dtsi
>>  create mode 100644 arch/arm/configs/stm32_defconfig
>>  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
>>  create mode 100644 drivers/clocksource/arm_system_timer.c
>>  create mode 100644 drivers/clocksource/timer-stm32.c
>>  create mode 100644 drivers/pinctrl/pinctrl-stm32.c
>>  create mode 100644 drivers/reset/reset-stm32.c
>>  create mode 100644 drivers/tty/serial/stm32-usart.c
>>  create mode 100644 include/dt-bindings/pinctrl/pinctrl-stm32.h
>>
>
>
> --
> 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 05/14] ARM: call reset_controller_of_init from default time_init handler

[Rob Herring]

On Thu, Feb 12, 2015 at 11:45 AM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> Some DT ARM platforms need the reset controllers to be initialized before
> the timers.
> This is the case of the stm32 and sunxi platforms.

I would say this is the exception, not the rule and therefore should
be handled in a machine desc function. Or it could be part of your
timer setup. Or is the bootloader's problem (like arch timer setup).

We just want to limit how much this mechanism gets used.

Rob

>
> This patch adds a call to reset_controller_of_init() to the default
> .init_time callback when RESET_CONTROLLER is used by the platform.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  arch/arm/kernel/time.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
> index 0cc7e58..4601b1e 100644
> --- a/arch/arm/kernel/time.c
> +++ b/arch/arm/kernel/time.c
> @@ -20,6 +20,7 @@
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/profile.h>
> +#include <linux/reset-controller.h>
>  #include <linux/sched.h>
>  #include <linux/sched_clock.h>
>  #include <linux/smp.h>
> @@ -117,6 +118,9 @@ void __init time_init(void)
>         if (machine_desc->init_time) {
>                 machine_desc->init_time();
>         } else {
> +#ifdef CONFIG_RESET_CONTROLLER
> +               reset_controller_of_init();
> +#endif
>  #ifdef CONFIG_COMMON_CLK
>                 of_clk_init(NULL);
>  #endif
> --
> 1.9.1
>

Re: [PATCH 05/14] ARM: call reset_controller_of_init from default time_init handler

[Russell King - ARM Linux]

On Sun, Feb 15, 2015 at 04:17:31PM -0600, Rob Herring wrote:
> On Thu, Feb 12, 2015 at 11:45 AM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
> > Some DT ARM platforms need the reset controllers to be initialized before
> > the timers.
> > This is the case of the stm32 and sunxi platforms.
> 
> I would say this is the exception, not the rule and therefore should
> be handled in a machine desc function. Or it could be part of your
> timer setup. Or is the bootloader's problem (like arch timer setup).
> 
> We just want to limit how much this mechanism gets used.

Can you clarify please - what is "this mechanism"?  Placing explicit
calls at this location, or the whole OF_DECLARE_* stuff?

Sebastian suggested using the OF_DECLARE_* stuff for the Dove PMU -
so maybe you have a comment on that too?

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 05/14] ARM: call reset_controller_of_init from default time_init handler

[Rob Herring]

On Sun, Feb 15, 2015 at 5:12 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Feb 15, 2015 at 04:17:31PM -0600, Rob Herring wrote:
>> On Thu, Feb 12, 2015 at 11:45 AM, Maxime Coquelin
>> <mcoquelin.stm32@gmail.com> wrote:
>> > Some DT ARM platforms need the reset controllers to be initialized before
>> > the timers.
>> > This is the case of the stm32 and sunxi platforms.
>>
>> I would say this is the exception, not the rule and therefore should
>> be handled in a machine desc function. Or it could be part of your
>> timer setup. Or is the bootloader's problem (like arch timer setup).
>>
>> We just want to limit how much this mechanism gets used.
>
> Can you clarify please - what is "this mechanism"?  Placing explicit
> calls at this location, or the whole OF_DECLARE_* stuff?

Well, it is both really, but I guess OF_DECLARE_ linker sections are
just an implementation detail of scattering various explicit init
calls. My concern is we'll end up with another initcall like mechanism
and the ordering problems associated with them. For example, what if
another platform needs clocks initialized before the reset controller?
I could see wanting to add gpio, pinctrl, syscon, etc. Another example
is the Beagleboard Cape folks want to read an I2C EEPROM early to
apply overlays before drivers probe. Where do we draw the line?

> Sebastian suggested using the OF_DECLARE_* stuff for the Dove PMU -
> so maybe you have a comment on that too?

I have the same position that it is the exception, not the rule, so
use the machine descriptor. I'm willing to be convinced otherwise, but
I think these cases need to be questioned.

Rob

Re: [PATCH 05/14] ARM: call reset_controller_of_init from default time_init handler

[Maxime Coquelin]

2015-02-15 23:17 GMT+01:00 Rob Herring <robherring2@gmail.com>:
> On Thu, Feb 12, 2015 at 11:45 AM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> Some DT ARM platforms need the reset controllers to be initialized before
>> the timers.
>> This is the case of the stm32 and sunxi platforms.
>
> I would say this is the exception, not the rule and therefore should
> be handled in a machine desc function. Or it could be part of your
> timer setup. Or is the bootloader's problem (like arch timer setup).

The only valid way in my opinion would be to implement the init_time
callback (as your first proposal),
duplicating what performs the time_init() function.

Then, if other machines than sunxi and stm32 have some day the same need,
we could consider moving the call to reset_controller_of_init()
function to time_init().

>
> We just want to limit how much this mechanism gets used.

Could you elaborate the reason why we want to limit this mechanism please?
I am not sure to understand.

Thanks,
Maxime

>
> Rob
>
>>
>> This patch adds a call to reset_controller_of_init() to the default
>> .init_time callback when RESET_CONTROLLER is used by the platform.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> ---
>>  arch/arm/kernel/time.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
>> index 0cc7e58..4601b1e 100644
>> --- a/arch/arm/kernel/time.c
>> +++ b/arch/arm/kernel/time.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/kernel.h>
>>  #include <linux/profile.h>
>> +#include <linux/reset-controller.h>
>>  #include <linux/sched.h>
>>  #include <linux/sched_clock.h>
>>  #include <linux/smp.h>
>> @@ -117,6 +118,9 @@ void __init time_init(void)
>>         if (machine_desc->init_time) {
>>                 machine_desc->init_time();
>>         } else {
>> +#ifdef CONFIG_RESET_CONTROLLER
>> +               reset_controller_of_init();
>> +#endif
>>  #ifdef CONFIG_COMMON_CLK
>>                 of_clk_init(NULL);
>>  #endif
>> --
>> 1.9.1
>>

Re: [PATCH 03/14] clocksource: Add ARM System timer driver

[Rob Herring]

On Thu, Feb 12, 2015 at 11:45 AM, Maxime Coquelin
<mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This patch adds clocksource support for ARMv7-M's System timer,
> also known as SysTick.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/arm/system_timer.txt       | 15 +++++

Please include v7M in the name. System timer sounds very generic. This
is the only timer architecturally defined IIRC, so perhaps just
"armv7m_systick".

>  drivers/clocksource/Kconfig                        |  7 ++
>  drivers/clocksource/Makefile                       |  1 +
>  drivers/clocksource/arm_system_timer.c             | 74 ++++++++++++++++++++++

Same here.


>  4 files changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/system_timer.txt
>  create mode 100644 drivers/clocksource/arm_system_timer.c
>
> diff --git a/Documentation/devicetree/bindings/arm/system_timer.txt b/Documentation/devicetree/bindings/arm/system_timer.txt
> new file mode 100644
> index 0000000..35268b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/system_timer.txt
> @@ -0,0 +1,15 @@
> +* ARM System Timer
> +
> +ARMv7-M includes a system timer, known as SysTick. Current driver only
> +implements the clocksource feature.
> +
> +Required properties:
> +- compatible : Should be "arm,armv7m-systick"
> +- reg       : The address range of the timer
> +- clocks     : The input clock of the timer

You may want to consider supporting "clock-frequency" here too. In
more simple chips you may just have fixed clocks and may want to run a
kernel with COMMON_CLK disabled for size savings.

> +
> +systick: system-timer {

This should be "systick: timer@e000e010".

Same for your dts file.

> +       compatible = "arm,armv7m-systick";
> +       reg = <0xe000e010 0x10>;
> +       clocks = <&clk_systick>;
> +};
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index fc01ec2..f9fe4ac 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 ARM_SYSTEM_TIMER
> +       bool
> +       select CLKSRC_OF if OF
> +       select CLKSRC_MMIO
> +       help
> +         This options enables support for the ARM system timer unit
> +
>  config ATMEL_PIT
>         select CLKSRC_OF if OF
>         def_bool SOC_AT91SAM9 || SOC_SAMA5
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 94d90b2..194400b 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -42,6 +42,7 @@ obj-$(CONFIG_MTK_TIMER)               += mtk_timer.o
>
>  obj-$(CONFIG_ARM_ARCH_TIMER)           += arm_arch_timer.o
>  obj-$(CONFIG_ARM_GLOBAL_TIMER)         += arm_global_timer.o
> +obj-$(CONFIG_ARM_SYSTEM_TIMER)         += arm_system_timer.o
>  obj-$(CONFIG_CLKSRC_METAG_GENERIC)     += metag_generic.o
>  obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)  += dummy_timer.o
>  obj-$(CONFIG_ARCH_KEYSTONE)            += timer-keystone.o
> diff --git a/drivers/clocksource/arm_system_timer.c b/drivers/clocksource/arm_system_timer.c
> new file mode 100644
> index 0000000..69e6ef9
> --- /dev/null
> +++ b/drivers/clocksource/arm_system_timer.c
> @@ -0,0 +1,74 @@
> +/*
> + * Copyright (C) Maxime Coquelin 2015
> + * Author:  Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/clk.h>
> +#include <linux/bitops.h>
> +
> +#define SYST_CSR       0x00
> +#define SYST_RVR       0x04
> +#define SYST_CVR       0x08
> +#define SYST_CALIB     0x0c
> +
> +#define SYST_CSR_ENABLE BIT(0)
> +
> +#define SYSTICK_LOAD_RELOAD_MASK 0x00FFFFFF
> +
> +static void __init system_timer_of_register(struct device_node *np)
> +{
> +       struct clk *clk;
> +       void __iomem *base;
> +       unsigned long rate;
> +       int ret;
> +
> +       base = of_iomap(np, 0);
> +       if (!base) {
> +               pr_warn("system-timer: invalid base address\n");
> +               return;
> +       }
> +
> +       clk = of_clk_get(np, 0);
> +       if (IS_ERR(clk)) {
> +               pr_warn("system-timer: clk not found\n");
> +               ret = PTR_ERR(clk);
> +               goto out_unmap;
> +       }
> +
> +       ret = clk_prepare_enable(clk);
> +       if (ret)
> +               goto out_clk_put;
> +
> +       rate = clk_get_rate(clk);
> +
> +       writel_relaxed(SYSTICK_LOAD_RELOAD_MASK, base + SYST_RVR);
> +       writel_relaxed(SYST_CSR_ENABLE, base + SYST_CSR);
> +
> +       ret = clocksource_mmio_init(base + SYST_CVR, "arm_system_timer", rate,
> +                       200, 24, clocksource_mmio_readl_down);
> +       if (ret) {
> +               pr_err("failed to init clocksource (%d)\n", ret);
> +               goto out_clk_disable;
> +       }
> +
> +       pr_info("ARM System timer initialized as clocksource\n");
> +
> +       return;
> +
> +out_clk_disable:
> +       clk_disable_unprepare(clk);
> +out_clk_put:
> +       clk_put(clk);
> +out_unmap:
> +       iounmap(base);
> +       WARN(ret, "ARM System timer register failed (%d)\n", ret);
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(arm_systick, "arm,armv7m-systick",
> +                       system_timer_of_register);
> --
> 1.9.1
>

Re: [PATCH 03/14] clocksource: Add ARM System timer driver

[Maxime Coquelin]

2015-02-15 23:31 GMT+01:00 Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Thu, Feb 12, 2015 at 11:45 AM, Maxime Coquelin
> <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> This patch adds clocksource support for ARMv7-M's System timer,
>> also known as SysTick.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../devicetree/bindings/arm/system_timer.txt       | 15 +++++
>
> Please include v7M in the name. System timer sounds very generic. This
> is the only timer architecturally defined IIRC, so perhaps just
> "armv7m_systick".

Ok, let's go for "armv7m_systick".

>
>>  drivers/clocksource/Kconfig                        |  7 ++
>>  drivers/clocksource/Makefile                       |  1 +
>>  drivers/clocksource/arm_system_timer.c             | 74 ++++++++++++++++++++++
>
> Same here.

Agree, will be in the v2.

>
>
>>  4 files changed, 97 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/system_timer.txt
>>  create mode 100644 drivers/clocksource/arm_system_timer.c
>>
>> diff --git a/Documentation/devicetree/bindings/arm/system_timer.txt b/Documentation/devicetree/bindings/arm/system_timer.txt
>> new file mode 100644
>> index 0000000..35268b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/system_timer.txt
>> @@ -0,0 +1,15 @@
>> +* ARM System Timer
>> +
>> +ARMv7-M includes a system timer, known as SysTick. Current driver only
>> +implements the clocksource feature.
>> +
>> +Required properties:
>> +- compatible : Should be "arm,armv7m-systick"
>> +- reg       : The address range of the timer
>> +- clocks     : The input clock of the timer
>
> You may want to consider supporting "clock-frequency" here too. In
> more simple chips you may just have fixed clocks and may want to run a
> kernel with COMMON_CLK disabled for size savings.

Ok, I will add this option in the v2.

>
>> +
>> +systick: system-timer {
>
> This should be "systick: timer@e000e010".
>
> Same for your dts file.

Right, it will be fixed in the v2.

Thanks for the review,
Maxime
--
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 03/14] clocksource: Add ARM System timer driver

[Andreas Färber]

Am 12.02.2015 um 18:45 schrieb Maxime Coquelin:
> This patch adds clocksource support for ARMv7-M's System timer,
> also known as SysTick.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/arm/system_timer.txt       | 15 +++++
>  drivers/clocksource/Kconfig                        |  7 ++
>  drivers/clocksource/Makefile                       |  1 +
>  drivers/clocksource/arm_system_timer.c             | 74 ++++++++++++++++++++++
>  4 files changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/system_timer.txt
>  create mode 100644 drivers/clocksource/arm_system_timer.c
> 
> diff --git a/Documentation/devicetree/bindings/arm/system_timer.txt b/Documentation/devicetree/bindings/arm/system_timer.txt
> new file mode 100644
> index 0000000..35268b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/system_timer.txt
> @@ -0,0 +1,15 @@
> +* ARM System Timer
> +
> +ARMv7-M includes a system timer, known as SysTick. Current driver only
> +implements the clocksource feature.
> +
> +Required properties:
> +- compatible : Should be "arm,armv7m-systick"
> +- reg	     : The address range of the timer
> +- clocks     : The input clock of the timer
> +
> +systick: system-timer {
> +	compatible = "arm,armv7m-systick";
> +	reg = <0xe000e010 0x10>;
> +	clocks = <&clk_systick>;
> +};

Binding documentation is supposed to go into its own patch:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/submitting-patches.txt

> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index fc01ec2..f9fe4ac 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 ARM_SYSTEM_TIMER
> +	bool
> +	select CLKSRC_OF if OF
> +	select CLKSRC_MMIO
> +	help
> +	  This options enables support for the ARM system timer unit
> +
>  config ATMEL_PIT
>  	select CLKSRC_OF if OF
>  	def_bool SOC_AT91SAM9 || SOC_SAMA5
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 94d90b2..194400b 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -42,6 +42,7 @@ obj-$(CONFIG_MTK_TIMER)		+= mtk_timer.o
>  
>  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
>  obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
> +obj-$(CONFIG_ARM_SYSTEM_TIMER)		+= arm_system_timer.o
>  obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
>  obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)	+= dummy_timer.o
>  obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
> diff --git a/drivers/clocksource/arm_system_timer.c b/drivers/clocksource/arm_system_timer.c
> new file mode 100644
> index 0000000..69e6ef9
> --- /dev/null
> +++ b/drivers/clocksource/arm_system_timer.c
> @@ -0,0 +1,74 @@
> +/*
> + * Copyright (C) Maxime Coquelin 2015
> + * Author:  Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/clk.h>
> +#include <linux/bitops.h>
> +
> +#define SYST_CSR	0x00
> +#define SYST_RVR	0x04
> +#define SYST_CVR	0x08
> +#define SYST_CALIB	0x0c
> +
> +#define SYST_CSR_ENABLE BIT(0)
> +
> +#define SYSTICK_LOAD_RELOAD_MASK 0x00FFFFFF
> +
> +static void __init system_timer_of_register(struct device_node *np)
> +{
> +	struct clk *clk;
> +	void __iomem *base;
> +	unsigned long rate;
> +	int ret;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_warn("system-timer: invalid base address\n");
> +		return;
> +	}
> +
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk)) {
> +		pr_warn("system-timer: clk not found\n");
> +		ret = PTR_ERR(clk);
> +		goto out_unmap;
> +	}
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		goto out_clk_put;
> +
> +	rate = clk_get_rate(clk);
> +
> +	writel_relaxed(SYSTICK_LOAD_RELOAD_MASK, base + SYST_RVR);
> +	writel_relaxed(SYST_CSR_ENABLE, base + SYST_CSR);
> +
> +	ret = clocksource_mmio_init(base + SYST_CVR, "arm_system_timer", rate,
> +			200, 24, clocksource_mmio_readl_down);
> +	if (ret) {
> +		pr_err("failed to init clocksource (%d)\n", ret);
> +		goto out_clk_disable;
> +	}
> +
> +	pr_info("ARM System timer initialized as clocksource\n");
> +
> +	return;
> +
> +out_clk_disable:
> +	clk_disable_unprepare(clk);
> +out_clk_put:
> +	clk_put(clk);
> +out_unmap:
> +	iounmap(base);
> +	WARN(ret, "ARM System timer register failed (%d)\n", ret);
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(arm_systick, "arm,armv7m-systick",
> +			system_timer_of_register);

I've used a SysTick based implementation on my stm32 branch myself, but
looking at efm32 I got the impression that it would be better to use one
of the 32-bit TIM2/TIM5 as clocksource and the other as clockevents?

Still this implementation will be handy to have, also for other targets.

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 03/14] clocksource: Add ARM System timer driver

[Maxime Coquelin]

2015-02-16 0:43 GMT+01:00 Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>:
> Am 12.02.2015 um 18:45 schrieb Maxime Coquelin:
>> This patch adds clocksource support for ARMv7-M's System timer,
>> also known as SysTick.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../devicetree/bindings/arm/system_timer.txt       | 15 +++++
>>  drivers/clocksource/Kconfig                        |  7 ++
>>  drivers/clocksource/Makefile                       |  1 +
>>  drivers/clocksource/arm_system_timer.c             | 74 ++++++++++++++++++++++
>>  4 files changed, 97 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/system_timer.txt
>>  create mode 100644 drivers/clocksource/arm_system_timer.c
>>
>> diff --git a/Documentation/devicetree/bindings/arm/system_timer.txt b/Documentation/devicetree/bindings/arm/system_timer.txt
>> new file mode 100644
>> index 0000000..35268b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/system_timer.txt
>> @@ -0,0 +1,15 @@
>> +* ARM System Timer
>> +
>> +ARMv7-M includes a system timer, known as SysTick. Current driver only
>> +implements the clocksource feature.
>> +
>> +Required properties:
>> +- compatible : Should be "arm,armv7m-systick"
>> +- reg             : The address range of the timer
>> +- clocks     : The input clock of the timer
>> +
>> +systick: system-timer {
>> +     compatible = "arm,armv7m-systick";
>> +     reg = <0xe000e010 0x10>;
>> +     clocks = <&clk_systick>;
>> +};
>
> Binding documentation is supposed to go into its own patch:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/submitting-patches.txt
Ok, will change this in the v2.

>
...
>
> I've used a SysTick based implementation on my stm32 branch myself, but
> looking at efm32 I got the impression that it would be better to use one
> of the 32-bit TIM2/TIM5 as clocksource and the other as clockevents?
>
> Still this implementation will be handy to have, also for other targets.

My view is that we should use as much generic parts of the Cortex-M as possible.
Moreover, doing, that, we can keep one more IP instance under reset
with associated clock gated,
and so maybe reduce the power consumption a little (I haven't done any
measurements)

Do you see a case where it could be better to use the STM32 timers?


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 06/14] drivers: reset: Add STM32 reset driver

[Andreas Färber]

Am 12.02.2015 um 18:45 schrieb Maxime Coquelin:
> The STM32 MCUs family IP can be reset by accessing some shared registers.
> 
> The specificity is that some reset lines are used by the timers.
> At timer initialization time, the timer has to be reset, that's why
> we cannot use a regular driver.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  .../devicetree/bindings/reset/st,stm32-reset.txt   |  19 ++++
>  drivers/reset/Makefile                             |   1 +
>  drivers/reset/reset-stm32.c                        | 124 +++++++++++++++++++++
>  3 files changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/st,stm32-reset.txt
>  create mode 100644 drivers/reset/reset-stm32.c
> 
> diff --git a/Documentation/devicetree/bindings/reset/st,stm32-reset.txt b/Documentation/devicetree/bindings/reset/st,stm32-reset.txt
> new file mode 100644
> index 0000000..add1298
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/st,stm32-reset.txt
> @@ -0,0 +1,19 @@
> +STMicroelectronics STM32 Peripheral Reset Controller
> +====================================================
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +Required properties:
> +- compatible: Should be "st,stm32-reset"
> +- reg: should be register base and length as documented in the
> +  datasheet
> +- #reset-cells: 1, see below
> +
> +example:
> +
> +reset_ahb1: reset@40023810 {
> +	#reset-cells = <1>;
> +	compatible = "st,stm32-reset";
> +	reg = <0x40023810 0x4>;
> +};
[snip]

RM0090 has two different chapters on the RCC IP:
* Reset and clock control for STM32F42xxx and STM32F43xxx (RCC)
* Reset and clock control for STM32F405xx/07xx and STM32F415xx/17xx(RCC)

I therefore feel it is wrong to use "stm32-" here; instead I used
"st,stm32f429-rcc" (also relates to 12/14 discussion). This may apply to
other identifiers, too.

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)
--
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 06/14] drivers: reset: Add STM32 reset driver

[Maxime Coquelin]

2015-02-16 0:59 GMT+01:00 Andreas Färber <afaerber@suse.de>:
> Am 12.02.2015 um 18:45 schrieb Maxime Coquelin:
>> The STM32 MCUs family IP can be reset by accessing some shared registers.
>>
>> The specificity is that some reset lines are used by the timers.
>> At timer initialization time, the timer has to be reset, that's why
>> we cannot use a regular driver.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> ---
>>  .../devicetree/bindings/reset/st,stm32-reset.txt   |  19 ++++
>>  drivers/reset/Makefile                             |   1 +
>>  drivers/reset/reset-stm32.c                        | 124 +++++++++++++++++++++
>>  3 files changed, 144 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/st,stm32-reset.txt
>>  create mode 100644 drivers/reset/reset-stm32.c
>>
>> diff --git a/Documentation/devicetree/bindings/reset/st,stm32-reset.txt b/Documentation/devicetree/bindings/reset/st,stm32-reset.txt
>> new file mode 100644
>> index 0000000..add1298
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/st,stm32-reset.txt
>> @@ -0,0 +1,19 @@
>> +STMicroelectronics STM32 Peripheral Reset Controller
>> +====================================================
>> +
>> +Please also refer to reset.txt in this directory for common reset
>> +controller binding usage.
>> +
>> +Required properties:
>> +- compatible: Should be "st,stm32-reset"
>> +- reg: should be register base and length as documented in the
>> +  datasheet
>> +- #reset-cells: 1, see below
>> +
>> +example:
>> +
>> +reset_ahb1: reset@40023810 {
>> +     #reset-cells = <1>;
>> +     compatible = "st,stm32-reset";
>> +     reg = <0x40023810 0x4>;
>> +};
> [snip]
>
> RM0090 has two different chapters on the RCC IP:
> * Reset and clock control for STM32F42xxx and STM32F43xxx (RCC)
> * Reset and clock control for STM32F405xx/07xx and STM32F415xx/17xx(RCC)
>
> I therefore feel it is wrong to use "stm32-" here; instead I used
> "st,stm32f429-rcc" (also relates to 12/14 discussion). This may apply to
> other identifiers, too.

In this first version, the reset driver was really generic, and was
compatible with the STM32 family.
The only difference would have been in the device trees.

Now, from the discussion with Philipp, I will reconsider the
implementation to add some named constants,
so maybe I will reconsider the compatible, it will depend on how I
will implement it.

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 07/14] clockevent: Add STM32 Timer driver

[Linus Walleij]

On Thu, Feb 12, 2015 at 6:45 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> STM32 MCUs feature 16 and 32 bits general purpose timers with prescalers.
> The drivers detects whether the time is 16 or 32 bits, and applies a
> 1024 prescaler value if it is 16 bits.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>

This is a nice driver.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 14/14] MAINTAINERS: Add entry for STM32 MCUs

[Linus Walleij]

On Thu, Feb 12, 2015 at 6:46 PM, Maxime Coquelin
<mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Add a MAINTAINER entry covering all STM32 machine and drivers files.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

(...)
> +F:     drivers/clocksource/arm_system_timer.c

Is that all? And that is not even a STM32 specific driver.

Yours,
Linus Walleij

Re: [PATCH 14/14] MAINTAINERS: Add entry for STM32 MCUs

[Maxime Coquelin]

2015-03-06 10:03 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Feb 12, 2015 at 6:46 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>
>> Add a MAINTAINER entry covering all STM32 machine and drivers files.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>
> (...)
>> +F:     drivers/clocksource/arm_system_timer.c
>
> Is that all? And that is not even a STM32 specific driver.

For the ARM System Timer, I'm fine to add a new entry.
Or remove the line, and let the maintain-ship to clocksource maintainers.

All the STM32 files are covered by this line:
+N:     stm32

Thanks,
Maxime
>
> Yours,
> Linus Walleij

Re: [PATCH 14/14] MAINTAINERS: Add entry for STM32 MCUs

[Linus Walleij]

On Fri, Mar 6, 2015 at 10:55 AM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> 2015-03-06 10:03 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Thu, Feb 12, 2015 at 6:46 PM, Maxime Coquelin
>> <mcoquelin.stm32@gmail.com> wrote:
>>
>>> Add a MAINTAINER entry covering all STM32 machine and drivers files.
>>>
>>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>>
>> (...)
>>> +F:     drivers/clocksource/arm_system_timer.c
>>
>> Is that all? And that is not even a STM32 specific driver.
>
> For the ARM System Timer, I'm fine to add a new entry.
> Or remove the line, and let the maintain-ship to clocksource maintainers.
>
> All the STM32 files are covered by this line:
> +N:     stm32

Aha you're right, news2me, I'm old and stupid :/

Thanks,
Linus Walleij

Re: [PATCH 14/14] MAINTAINERS: Add entry for STM32 MCUs

[Maxime Coquelin]

2015-03-09 17:47 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Mar 6, 2015 at 10:55 AM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> 2015-03-06 10:03 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
>>> On Thu, Feb 12, 2015 at 6:46 PM, Maxime Coquelin
>>> <mcoquelin.stm32@gmail.com> wrote:
>>>
>>>> Add a MAINTAINER entry covering all STM32 machine and drivers files.
>>>>
>>>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>>>
>>> (...)
>>>> +F:     drivers/clocksource/arm_system_timer.c
>>>
>>> Is that all? And that is not even a STM32 specific driver.
>>
>> For the ARM System Timer, I'm fine to add a new entry.
>> Or remove the line, and let the maintain-ship to clocksource maintainers.
>>
>> All the STM32 files are covered by this line:
>> +N:     stm32
>
> Aha you're right, news2me, I'm old and stupid :/

This is new to me too. :)

Maxime

>
> Thanks,
> Linus Walleij