diff mbox

[v8,14/16] ARM: dts: Introduce STM32F429 MCU

Message ID 1431158038-3813-15-git-send-email-mcoquelin.stm32@gmail.com
State New
Headers show

Commit Message

Maxime Coquelin May 9, 2015, 7:53 a.m. UTC
The STMicrolectornics's STM32F429 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

Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 arch/arm/boot/dts/Makefile            |   1 +
 arch/arm/boot/dts/stm32f429-disco.dts |  71 +++++++++++
 arch/arm/boot/dts/stm32f429.dtsi      | 227 ++++++++++++++++++++++++++++++++++
 3 files changed, 299 insertions(+)
 create mode 100644 arch/arm/boot/dts/stm32f429-disco.dts
 create mode 100644 arch/arm/boot/dts/stm32f429.dtsi

Comments

Arnd Bergmann May 12, 2015, 9:21 p.m. UTC | #1
On Saturday 09 May 2015 09:53:56 Maxime Coquelin wrote:
> +#include  <dt-bindings/mfd/stm32f4-rcc.h>
> +
> 

Can you find a way to avoid this dependency?

Maybe you can change the bindings so that the numbers you pass as
arguments to the reset and clock specifiers reflect the numbers that
the hardware use?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin May 13, 2015, 11:45 a.m. UTC | #2
2015-05-12 23:21 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Saturday 09 May 2015 09:53:56 Maxime Coquelin wrote:
>> +#include  <dt-bindings/mfd/stm32f4-rcc.h>
>> +
>>
>
> Can you find a way to avoid this dependency?
>
> Maybe you can change the bindings so that the numbers you pass as
> arguments to the reset and clock specifiers reflect the numbers that
> the hardware use?

If I understand correctly, you prefer the way I did in v7 [0]?

I don't have a strong opinion on this. Either way is fine to me.

I changed the bindings in the v8 after discussions with Daniel
Thompson, who is implementing the clock driver part of the RCC IP.

He proposed we used common defines, because each peripheral has a
reset line and a clock gate.
Both reset and clock are represented as a single bit, with only the
base offset differing between clock and reset.
You can have a look at chapter 6 of the reference manual [1] if you
find some time.

Having common defines between clocks and reset make sense to me, but I
also understand your point of avoiding dependencies.

Maybe I can revert back to v7 bindings for now, and then we can
reconsider using common defines when Daniel will send the clock
patches.
Note that doing that won't break the DT binary compatibility, as the
raw reset values, or the ones from defines are the same.

Daniel, could you share an example of the bindings you would use for the clocks?

Kind regards,
Maxime

>
>         Arnd

[0]: http://lkml.iu.edu/hypermail/linux/kernel/1504.3/04523.html
[1]: http://www.st.com/web/en/resource/technical/document/reference_manual/DM00031020.pdf
--
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
Daniel Thompson May 13, 2015, 12:58 p.m. UTC | #3
On 13/05/15 12:45, Maxime Coquelin wrote:
> 2015-05-12 23:21 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> On Saturday 09 May 2015 09:53:56 Maxime Coquelin wrote:
>>> +#include  <dt-bindings/mfd/stm32f4-rcc.h>
>>> +
>>>
>>
>> Can you find a way to avoid this dependency?
>>
>> Maybe you can change the bindings so that the numbers you pass as
>> arguments to the reset and clock specifiers reflect the numbers that
>> the hardware use?
>
> If I understand correctly, you prefer the way I did in v7 [0]?
>
> I don't have a strong opinion on this. Either way is fine to me.
>
> I changed the bindings in the v8 after discussions with Daniel
> Thompson, who is implementing the clock driver part of the RCC IP.
>
> He proposed we used common defines, because each peripheral has a
> reset line and a clock gate.
> Both reset and clock are represented as a single bit, with only the
> base offset differing between clock and reset.
> You can have a look at chapter 6 of the reference manual [1] if you
> find some time.
>
> Having common defines between clocks and reset make sense to me, but I
> also understand your point of avoiding dependencies.
>
> Maybe I can revert back to v7 bindings for now, and then we can
> reconsider using common defines when Daniel will send the clock
> patches.
> Note that doing that won't break the DT binary compatibility, as the
> raw reset values, or the ones from defines are the same.
>
> Daniel, could you share an example of the bindings you would use for the clocks?

For the most cases, where there is a clock gate just before the 
peripheral it looks pretty much like the reset driver and I use the bit 
offset of the clock gating bit as the index.

However there are a couple of clocks without gating just before the 
clock reaches the peripheral:

1. A hard coded /8. I think this will have to be given a synthetic
    number.

2. Ungated dividers. For these I am using the bit offset of the LSB of
    the mux field.

So I think there is only one value that is completely unrelated to the 
hardware and will use a magic constant instead.

I had planned to macros similar to the STM32F4_AxB_RESET() family of 
macros in both clk driver and DT in order to reuse the bit layouts from 
dt-bindings/mfd/stm32f4-rcc.h .

Normal case would have looked like this:

		timer3: timer@40000000 {
			compatible = "st,stm32-timer";
			reg = <0x40000000 0x400>;
			interrupts = <28>;
			resets = <&rcc STM32F4_APB1_RESET(TIM3)>;
			clocks = <&rcc STM32F4_APB1_CLK(TIM3)>;
			status = "disabled";
		};

Without the macros it looks like this:

		timer3: timer@40000000 {
			compatible = "st,stm32-timer";
			reg = <0x40000000 0x400>;
			interrupts = <28>;
			resets = <&rcc 257>;
			clocks = <&rcc 513>;
			status = "disabled";
		};

However we could perhaps be more literate even if we don't use the macros?

		timer3: timer@40000000 {
			compatible = "st,stm32-timer";
			reg = <0x40000000 0x400>;
			interrupts = <28>;
			resets = <&rcc ((0x20*8) + 1)>;
			clocks = <&rcc ((0x40*8) + 1)>;
			status = "disabled";
		};


Daniel.


> Kind regards,
> Maxime
>
>>
>>          Arnd
>
> [0]: http://lkml.iu.edu/hypermail/linux/kernel/1504.3/04523.html
> [1]: http://www.st.com/web/en/resource/technical/document/reference_manual/DM00031020.pdf
>

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 13, 2015, 1:27 p.m. UTC | #4
On Wednesday 13 May 2015 13:58:05 Daniel Thompson wrote:
> On 13/05/15 12:45, Maxime Coquelin wrote:
> > 2015-05-12 23:21 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> >> On Saturday 09 May 2015 09:53:56 Maxime Coquelin wrote:
> >>> +#include  <dt-bindings/mfd/stm32f4-rcc.h>
> >>> +
> >>>
> >>
> >> Can you find a way to avoid this dependency?
> >>
> >> Maybe you can change the bindings so that the numbers you pass as
> >> arguments to the reset and clock specifiers reflect the numbers that
> >> the hardware use?
> >
> > If I understand correctly, you prefer the way I did in v7 [0]?

Yes, that looks better. I would probably not list all the possible
values in the binding though, when the intention is to use the
hardware specific values, and being able to reuse the binding
and driver for variations of the same chip.

> > Note that doing that won't break the DT binary compatibility, as the
> > raw reset values, or the ones from defines are the same.
> >
> > Daniel, could you share an example of the bindings you would use for the clocks?
> 
> For the most cases, where there is a clock gate just before the 
> peripheral it looks pretty much like the reset driver and I use the bit 
> offset of the clock gating bit as the index.

Is this bit always the same index as the one for the reset driver?

> However there are a couple of clocks without gating just before the 
> clock reaches the peripheral:
> 
> 1. A hard coded /8. I think this will have to be given a synthetic
>     number.

If this is just a divider, why not use a separate DT node for that,
like this:

        clock {
                compatible = "fixed-factor-clock";
                clocks = <&parentclk>;
                #clock-cells = <0>;
                clock-div = <8>;
                clock-mult = <1>;
        };

No need to assign a number for this.

> 2. Ungated dividers. For these I am using the bit offset of the LSB of
>     the mux field.

Do these ones also come with resets?

> So I think there is only one value that is completely unrelated to the 
> hardware and will use a magic constant instead.
> 
> I had planned to macros similar to the STM32F4_AxB_RESET() family of 
> macros in both clk driver and DT in order to reuse the bit layouts from 
> dt-bindings/mfd/stm32f4-rcc.h .
> 
> Normal case would have looked like this:
> 
>                 timer3: timer@40000000 {
>                         compatible = "st,stm32-timer";
>                         reg = <0x40000000 0x400>;
>                         interrupts = <28>;
>                         resets = <&rcc STM32F4_APB1_RESET(TIM3)>;
>                         clocks = <&rcc STM32F4_APB1_CLK(TIM3)>;
>                         status = "disabled";
>                 };
> 
> Without the macros it looks like this:
> 
>                 timer3: timer@40000000 {
>                         compatible = "st,stm32-timer";
>                         reg = <0x40000000 0x400>;
>                         interrupts = <28>;
>                         resets = <&rcc 257>;
>                         clocks = <&rcc 513>;
>                         status = "disabled";
>                 };
> 
> However we could perhaps be more literate even if we don't use the macros?
> 
>                 timer3: timer@40000000 {
>                         compatible = "st,stm32-timer";
>                         reg = <0x40000000 0x400>;
>                         interrupts = <28>;
>                         resets = <&rcc ((0x20*8) + 1)>;
>                         clocks = <&rcc ((0x40*8) + 1)>;
>                         status = "disabled";
>                 };

How about #address-cells = <2>, so you can do

		resets = <&rcc 8 1>;
		clocks = <&rcc 8 1>;

with the first cell being an index for the block and the second cell the
bit number within that block.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 13, 2015, 3:28 p.m. UTC | #5
On Wednesday 13 May 2015 16:20:34 Daniel Thompson wrote:
> For the all reset bits:
> 
>    clock idx = reset idx + 256
> 
> The opposite is not true; the clock bits are a superset of the reset 
> bits (the reset bits act on cells but some cells have >1 clock).

Ok, in that case, I would strongly recommend subtracting that 256
offset keeping the numbers the same, to remove the function-type
macros.

> >> However there are a couple of clocks without gating just before the
> >> clock reaches the peripheral:
> >>
> >> 1. A hard coded /8. I think this will have to be given a synthetic
> >>      number.
> >
> > If this is just a divider, why not use a separate DT node for that,
> > like this:
> >
> >          clock {
> >                  compatible = "fixed-factor-clock";
> >                  clocks = <&parentclk>;
> >                  #clock-cells = <0>;
> >                  clock-div = <8>;
> >                  clock-mult = <1>;
> >          };
> >
> > No need to assign a number for this.
> 
> I'd wondered about doing that.
> 
> It will certainly work but it seemed a bit odd to me to have one (really 
> tiny) part of the RCC cell included seperately in the platform 
> description whilst all the complicated bits end up aggregated into the 
> RCC cell.
> 
> Is there much prior art that uses this type of trick to avoid having 
> magic numbers into the bindings?

Are you sure that divider is actually part of the RCC?

> >> 2. Ungated dividers. For these I am using the bit offset of the LSB of
> >>      the mux field.
> >
> > Do these ones also come with resets?
> 
> No. They mostly run to the core and its intimate peripherals (i.e. only 
> reset line comes from WDT).

Ok.

> >> So I think there is only one value that is completely unrelated to the
> >> hardware and will use a magic constant instead.
> >>
> >> I had planned to macros similar to the STM32F4_AxB_RESET() family of
> >> macros in both clk driver and DT in order to reuse the bit layouts from
> >> dt-bindings/mfd/stm32f4-rcc.h .
> >>
> >> Normal case would have looked like this:
> >>
> >>                  timer3: timer@40000000 {
> >>                          compatible = "st,stm32-timer";
> >>                          reg = <0x40000000 0x400>;
> >>                          interrupts = <28>;
> >>                          resets = <&rcc STM32F4_APB1_RESET(TIM3)>;
> >>                          clocks = <&rcc STM32F4_APB1_CLK(TIM3)>;
> >>                          status = "disabled";
> >>                  };
> >>
> >> Without the macros it looks like this:
> >>
> >>                  timer3: timer@40000000 {
> >>                          compatible = "st,stm32-timer";
> >>                          reg = <0x40000000 0x400>;
> >>                          interrupts = <28>;
> >>                          resets = <&rcc 257>;
> >>                          clocks = <&rcc 513>;
> >>                          status = "disabled";
> >>                  };
> >>
> >> However we could perhaps be more literate even if we don't use the macros?
> >>
> >>                  timer3: timer@40000000 {
> >>                          compatible = "st,stm32-timer";
> >>                          reg = <0x40000000 0x400>;
> >>                          interrupts = <28>;
> >>                          resets = <&rcc ((0x20*8) + 1)>;
> >>                          clocks = <&rcc ((0x40*8) + 1)>;
> >>                          status = "disabled";
> >>                  };
> >
> > How about #address-cells = <2>, so you can do
> >
> > 		resets = <&rcc 8 1>;
> > 		clocks = <&rcc 8 1>;
> >
> > with the first cell being an index for the block and the second cell the
> > bit number within that block.
> 
> That would suit me very well (although is the 0x20/0x40 not the 8 that 
> we would need in the middle column).

We don't normally use register offsets in DT. The number 8 here instead
would indicate block 8, where each block is four bytes wide. Using the
same index here for reset and clock would also help readability.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin May 13, 2015, 4:29 p.m. UTC | #6
2015-05-13 17:28 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 13 May 2015 16:20:34 Daniel Thompson wrote:
>> For the all reset bits:
>>
>>    clock idx = reset idx + 256
>>
>> The opposite is not true; the clock bits are a superset of the reset
>> bits (the reset bits act on cells but some cells have >1 clock).
>
> Ok, in that case, I would strongly recommend subtracting that 256
> offset keeping the numbers the same, to remove the function-type
> macros.
>
>> >> However there are a couple of clocks without gating just before the
>> >> clock reaches the peripheral:
>> >>
>> >> 1. A hard coded /8. I think this will have to be given a synthetic
>> >>      number.
>> >
>> > If this is just a divider, why not use a separate DT node for that,
>> > like this:
>> >
>> >          clock {
>> >                  compatible = "fixed-factor-clock";
>> >                  clocks = <&parentclk>;
>> >                  #clock-cells = <0>;
>> >                  clock-div = <8>;
>> >                  clock-mult = <1>;
>> >          };
>> >
>> > No need to assign a number for this.
>>
>> I'd wondered about doing that.
>>
>> It will certainly work but it seemed a bit odd to me to have one (really
>> tiny) part of the RCC cell included seperately in the platform
>> description whilst all the complicated bits end up aggregated into the
>> RCC cell.
>>
>> Is there much prior art that uses this type of trick to avoid having
>> magic numbers into the bindings?
>
> Are you sure that divider is actually part of the RCC?
>
>> >> 2. Ungated dividers. For these I am using the bit offset of the LSB of
>> >>      the mux field.
>> >
>> > Do these ones also come with resets?
>>
>> No. They mostly run to the core and its intimate peripherals (i.e. only
>> reset line comes from WDT).
>
> Ok.
>
>> >> So I think there is only one value that is completely unrelated to the
>> >> hardware and will use a magic constant instead.
>> >>
>> >> I had planned to macros similar to the STM32F4_AxB_RESET() family of
>> >> macros in both clk driver and DT in order to reuse the bit layouts from
>> >> dt-bindings/mfd/stm32f4-rcc.h .
>> >>
>> >> Normal case would have looked like this:
>> >>
>> >>                  timer3: timer@40000000 {
>> >>                          compatible = "st,stm32-timer";
>> >>                          reg = <0x40000000 0x400>;
>> >>                          interrupts = <28>;
>> >>                          resets = <&rcc STM32F4_APB1_RESET(TIM3)>;
>> >>                          clocks = <&rcc STM32F4_APB1_CLK(TIM3)>;
>> >>                          status = "disabled";
>> >>                  };
>> >>
>> >> Without the macros it looks like this:
>> >>
>> >>                  timer3: timer@40000000 {
>> >>                          compatible = "st,stm32-timer";
>> >>                          reg = <0x40000000 0x400>;
>> >>                          interrupts = <28>;
>> >>                          resets = <&rcc 257>;
>> >>                          clocks = <&rcc 513>;
>> >>                          status = "disabled";
>> >>                  };
>> >>
>> >> However we could perhaps be more literate even if we don't use the macros?
>> >>
>> >>                  timer3: timer@40000000 {
>> >>                          compatible = "st,stm32-timer";
>> >>                          reg = <0x40000000 0x400>;
>> >>                          interrupts = <28>;
>> >>                          resets = <&rcc ((0x20*8) + 1)>;
>> >>                          clocks = <&rcc ((0x40*8) + 1)>;
>> >>                          status = "disabled";
>> >>                  };
>> >
>> > How about #address-cells = <2>, so you can do
>> >
>> >             resets = <&rcc 8 1>;
>> >             clocks = <&rcc 8 1>;
>> >
>> > with the first cell being an index for the block and the second cell the
>> > bit number within that block.
>>
>> That would suit me very well (although is the 0x20/0x40 not the 8 that
>> we would need in the middle column).
>
> We don't normally use register offsets in DT. The number 8 here instead
> would indicate block 8, where each block is four bytes wide. Using the
> same index here for reset and clock would also help readability.

My view is that it makes the bindings usage very complex.
Also, it implies we have a specific compatible for stm32f429, whereas
we didn't need with my earlier proposals.
Indeed, the reset driver will need to know the offset of every reset
registers, because:
  1. The AHB registers start at RCC offset 0x10 (up to 0x18)
  2. The APB registers start at RCC offset 0x20 (up to 0x24).
We have a gap between AHB and APB registers, so how do we map the
index for the block you propose?
Should the gap be considered as a block, or we should skip it?

I'm afraid it will not be straightforward for a reset user to
understand how to use this bindings.

Either my v7 or v8 versions would have made possible to use a single
compatible for STM32 series.
If we stick with one of these, we could even think to have a "generic"
reset driver, as it could be compatible with sunxi driver bindings.

What is your view?

Kind regards,
Maxime

>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 13, 2015, 4:37 p.m. UTC | #7
On Wednesday 13 May 2015 18:29:05 Maxime Coquelin wrote:
> 2015-05-13 17:28 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> > On Wednesday 13 May 2015 16:20:34 Daniel Thompson wrote:
> >>
> >> That would suit me very well (although is the 0x20/0x40 not the 8 that
> >> we would need in the middle column).
> >
> > We don't normally use register offsets in DT. The number 8 here instead
> > would indicate block 8, where each block is four bytes wide. Using the
> > same index here for reset and clock would also help readability.
> 
> My view is that it makes the bindings usage very complex.
> Also, it implies we have a specific compatible for stm32f429, whereas
> we didn't need with my earlier proposals.
> Indeed, the reset driver will need to know the offset of every reset
> registers, because:
>   1. The AHB registers start at RCC offset 0x10 (up to 0x18)
>   2. The APB registers start at RCC offset 0x20 (up to 0x24).
> We have a gap between AHB and APB registers, so how do we map the
> index for the block you propose?
> Should the gap be considered as a block, or we should skip it?
>
> I'm afraid it will not be straightforward for a reset user to
> understand how to use this bindings.
> 
> Either my v7 or v8 versions would have made possible to use a single
> compatible for STM32 series.
> If we stick with one of these, we could even think to have a "generic"
> reset driver, as it could be compatible with sunxi driver bindings.

We should definitely try to use the same compatible string for all of
them, and make a binding that is easy to use.

I haven't fully understood the requirements for the various parts that
are involved here. My understanding so far was that the driver could
use the index from the first cell and compute

	void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
	void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;

Are there parts that need something else? If the 0x10 offset is
different, we probably want a different compatible string, and I'd
consider it a different part at that point. If there are chips
that do not spread the clock from the reset by exactly 256 bits,
we could add a DT property in the rcc node for that.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin May 13, 2015, 4:54 p.m. UTC | #8
2015-05-13 18:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 13 May 2015 18:29:05 Maxime Coquelin wrote:
>> 2015-05-13 17:28 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> > On Wednesday 13 May 2015 16:20:34 Daniel Thompson wrote:
>> >>
>> >> That would suit me very well (although is the 0x20/0x40 not the 8 that
>> >> we would need in the middle column).
>> >
>> > We don't normally use register offsets in DT. The number 8 here instead
>> > would indicate block 8, where each block is four bytes wide. Using the
>> > same index here for reset and clock would also help readability.
>>
>> My view is that it makes the bindings usage very complex.
>> Also, it implies we have a specific compatible for stm32f429, whereas
>> we didn't need with my earlier proposals.
>> Indeed, the reset driver will need to know the offset of every reset
>> registers, because:
>>   1. The AHB registers start at RCC offset 0x10 (up to 0x18)
>>   2. The APB registers start at RCC offset 0x20 (up to 0x24).
>> We have a gap between AHB and APB registers, so how do we map the
>> index for the block you propose?
>> Should the gap be considered as a block, or we should skip it?
>>
>> I'm afraid it will not be straightforward for a reset user to
>> understand how to use this bindings.
>>
>> Either my v7 or v8 versions would have made possible to use a single
>> compatible for STM32 series.
>> If we stick with one of these, we could even think to have a "generic"
>> reset driver, as it could be compatible with sunxi driver bindings.
>
> We should definitely try to use the same compatible string for all of
> them, and make a binding that is easy to use.
>
> I haven't fully understood the requirements for the various parts that
> are involved here. My understanding so far was that the driver could
> use the index from the first cell and compute
>
>         void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
>         void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;

This calculation is true, but we have to take into account there is a
hole in the middle, between AHB3, and APB1 register:

AHB1RSTR : offset = 0x10, index = 0
AHB2RSTR : offset = 0x14, index = 1
AHB3RSTR : offset = 0x18, index = 2
<HOLE >     : offset = 0x1c, index = 3
APB1RSTR : offset = 0x20, index = 4
APB2RSTR : offset = 0x24, index = 5

So we have to carefully document this hole in the bindings, maybe by
listing indexes in the documentation?

>
> Are there parts that need something else? If the 0x10 offset is
> different, we probably want a different compatible string, and I'd
> consider it a different part at that point. If there are chips
> that do not spread the clock from the reset by exactly 256 bits,
> we could add a DT property in the rcc node for that.

I will check other chips, to see if this is valid generally.

Thanks for your feedback,
Maxime

>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 13, 2015, 7:11 p.m. UTC | #9
On Wednesday 13 May 2015 18:54:10 Maxime Coquelin wrote:
> This calculation is true, but we have to take into account there is a
> hole in the middle, between AHB3, and APB1 register:
> 
> AHB1RSTR : offset = 0x10, index = 0
> AHB2RSTR : offset = 0x14, index = 1
> AHB3RSTR : offset = 0x18, index = 2
> <HOLE >     : offset = 0x1c, index = 3
> APB1RSTR : offset = 0x20, index = 4
> APB2RSTR : offset = 0x24, index = 5
> 
> So we have to carefully document this hole in the bindings, maybe by
> listing indexes in the documentation?

I would only list the index definitions in the binding if they
are common across all chips using that binding.

From what I see above, this is really regular, so it's possible
that others follow it as well, but it's also possible that another
chip completely screwed up that system because it didn't fit
otherwise.

Ideally the binding should follow closely what is documented
in the data sheet.

> > Are there parts that need something else? If the 0x10 offset is
> > different, we probably want a different compatible string, and I'd
> > consider it a different part at that point. If there are chips
> > that do not spread the clock from the reset by exactly 256 bits,
> > we could add a DT property in the rcc node for that.
> 
> I will check other chips, to see if this is valid generally.

Ok.

	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
Daniel Thompson May 13, 2015, 7:29 p.m. UTC | #10
On 13/05/15 17:54, Maxime Coquelin wrote:
> 2015-05-13 18:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wednesday 13 May 2015 18:29:05 Maxime Coquelin wrote:
>>> 2015-05-13 17:28 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>>> On Wednesday 13 May 2015 16:20:34 Daniel Thompson wrote:
>>>>>
>>>>> That would suit me very well (although is the 0x20/0x40 not the 8 that
>>>>> we would need in the middle column).
>>>>
>>>> We don't normally use register offsets in DT. The number 8 here instead
>>>> would indicate block 8, where each block is four bytes wide. Using the
>>>> same index here for reset and clock would also help readability.
>>>
>>> My view is that it makes the bindings usage very complex.
>>> Also, it implies we have a specific compatible for stm32f429, whereas
>>> we didn't need with my earlier proposals.
>>> Indeed, the reset driver will need to know the offset of every reset
>>> registers, because:
>>>    1. The AHB registers start at RCC offset 0x10 (up to 0x18)
>>>    2. The APB registers start at RCC offset 0x20 (up to 0x24).
>>> We have a gap between AHB and APB registers, so how do we map the
>>> index for the block you propose?
>>> Should the gap be considered as a block, or we should skip it?
>>>
>>> I'm afraid it will not be straightforward for a reset user to
>>> understand how to use this bindings.
>>>
>>> Either my v7 or v8 versions would have made possible to use a single
>>> compatible for STM32 series.
>>> If we stick with one of these, we could even think to have a "generic"
>>> reset driver, as it could be compatible with sunxi driver bindings.
>>
>> We should definitely try to use the same compatible string for all of
>> them, and make a binding that is easy to use.
>>
>> I haven't fully understood the requirements for the various parts that
>> are involved here. My understanding so far was that the driver could
>> use the index from the first cell and compute
>>
>>          void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
>>          void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
>
> This calculation is true, but we have to take into account there is a
> hole in the middle, between AHB3, and APB1 register:

... and equally importantly, only allows us to use hardware mappings for 
the gated clocks.

> AHB1RSTR : offset = 0x10, index = 0
> AHB2RSTR : offset = 0x14, index = 1
> AHB3RSTR : offset = 0x18, index = 2
> <HOLE >     : offset = 0x1c, index = 3
> APB1RSTR : offset = 0x20, index = 4
> APB2RSTR : offset = 0x24, index = 5
>
> So we have to carefully document this hole in the bindings, maybe by
> listing indexes in the documentation?

The register set has PLL, mux and dividers in the registers at 0x00, 
0x04 and 0x08.

Many of these clocks can be kept out of DT entirely because they are 
only there to feed other parts of the clock tree. However some of the 
dividers flow directly into cells that appear in device tree (such as 
the systick) and so we need to be able to reference them.

In other words the proposed mapping cannot allow us to express the 
dividers properly (because the index would have to be negative):
   void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;

Thus I'd favour using different indexes for reset and clock bindings, 
both using the naive mapping function:
   void __iomem *reg =  rcc_base + 4 * index

I think that its so much easier to check against the datasheet like 
that. Admittedly is we follow the block-of-4-bytes idiom we have to 
divide a hex number by four but thats not so hard and we end up with:

		resets = <&rcc  8 0>;
		clocks = <&rcc 16 0>;

At the end of the day if we say we want to follow the datasheet, lets be 
do it in the most direct way properly.


Daniel.


PS
I've written a custom lookup function to to get from the DT index to an 
offset into the struct clk *array I'm using. That means I don't care 
much about any big holes in the register space.

>> Are there parts that need something else? If the 0x10 offset is
>> different, we probably want a different compatible string, and I'd
>> consider it a different part at that point. If there are chips
>> that do not spread the clock from the reset by exactly 256 bits,
>> we could add a DT property in the rcc node for that.
>
> I will check other chips, to see if this is valid generally.
>
> Thanks for your feedback,
> Maxime
>
>>
>>          Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 13, 2015, 7:37 p.m. UTC | #11
On Wednesday 13 May 2015 20:29:12 Daniel Thompson wrote:
> On 13/05/15 17:54, Maxime Coquelin wrote:
> > 2015-05-13 18:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> >>
> >> We should definitely try to use the same compatible string for all of
> >> them, and make a binding that is easy to use.
> >>
> >> I haven't fully understood the requirements for the various parts that
> >> are involved here. My understanding so far was that the driver could
> >> use the index from the first cell and compute
> >>
> >>          void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
> >>          void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
> >
> > This calculation is true, but we have to take into account there is a
> > hole in the middle, between AHB3, and APB1 register:
> 
> ... and equally importantly, only allows us to use hardware mappings for 
> the gated clocks.
> 
> > AHB1RSTR : offset = 0x10, index = 0
> > AHB2RSTR : offset = 0x14, index = 1
> > AHB3RSTR : offset = 0x18, index = 2
> > <HOLE >     : offset = 0x1c, index = 3
> > APB1RSTR : offset = 0x20, index = 4
> > APB2RSTR : offset = 0x24, index = 5
> >
> > So we have to carefully document this hole in the bindings, maybe by
> > listing indexes in the documentation?
> 
> The register set has PLL, mux and dividers in the registers at 0x00, 
> 0x04 and 0x08.
> 
> Many of these clocks can be kept out of DT entirely because they are 
> only there to feed other parts of the clock tree. However some of the 
> dividers flow directly into cells that appear in device tree (such as 
> the systick) and so we need to be able to reference them.
> 
> In other words the proposed mapping cannot allow us to express the 
> dividers properly (because the index would have to be negative):
>    void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
> 
> Thus I'd favour using different indexes for reset and clock bindings, 
> both using the naive mapping function:
>    void __iomem *reg =  rcc_base + 4 * index
> 
> I think that its so much easier to check against the datasheet like 
> that. Admittedly is we follow the block-of-4-bytes idiom we have to 
> divide a hex number by four but thats not so hard and we end up with:
> 
> 		resets = <&rcc  8 0>;
> 		clocks = <&rcc 16 0>;
> 
> At the end of the day if we say we want to follow the datasheet, lets be 
> do it in the most direct way properly.
> 
> 
> PS
> I've written a custom lookup function to to get from the DT index to an 
> offset into the struct clk *array I'm using. That means I don't care 
> much about any big holes in the register space.

How about using the first cell to indicate the type (pll, mux, div, gate)
and the second cell for the number (between 0 and 256)? That way, the
gates numbers would match the reset numbers, and your internal mapping
function would look a bit nicer.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin May 14, 2015, 4:34 p.m. UTC | #12
2015-05-13 21:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 13 May 2015 20:29:12 Daniel Thompson wrote:
>> On 13/05/15 17:54, Maxime Coquelin wrote:
>> > 2015-05-13 18:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> >>
>> >> We should definitely try to use the same compatible string for all of
>> >> them, and make a binding that is easy to use.
>> >>
>> >> I haven't fully understood the requirements for the various parts that
>> >> are involved here. My understanding so far was that the driver could
>> >> use the index from the first cell and compute
>> >>
>> >>          void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
>> >>          void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
>> >
>> > This calculation is true, but we have to take into account there is a
>> > hole in the middle, between AHB3, and APB1 register:
>>
>> ... and equally importantly, only allows us to use hardware mappings for
>> the gated clocks.
>>
>> > AHB1RSTR : offset = 0x10, index = 0
>> > AHB2RSTR : offset = 0x14, index = 1
>> > AHB3RSTR : offset = 0x18, index = 2
>> > <HOLE >     : offset = 0x1c, index = 3
>> > APB1RSTR : offset = 0x20, index = 4
>> > APB2RSTR : offset = 0x24, index = 5
>> >
>> > So we have to carefully document this hole in the bindings, maybe by
>> > listing indexes in the documentation?
>>
>> The register set has PLL, mux and dividers in the registers at 0x00,
>> 0x04 and 0x08.
>>
>> Many of these clocks can be kept out of DT entirely because they are
>> only there to feed other parts of the clock tree. However some of the
>> dividers flow directly into cells that appear in device tree (such as
>> the systick) and so we need to be able to reference them.
>>
>> In other words the proposed mapping cannot allow us to express the
>> dividers properly (because the index would have to be negative):
>>    void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
>>
>> Thus I'd favour using different indexes for reset and clock bindings,
>> both using the naive mapping function:
>>    void __iomem *reg =  rcc_base + 4 * index
>>
>> I think that its so much easier to check against the datasheet like
>> that. Admittedly is we follow the block-of-4-bytes idiom we have to
>> divide a hex number by four but thats not so hard and we end up with:
>>
>>               resets = <&rcc  8 0>;
>>               clocks = <&rcc 16 0>;
>>
>> At the end of the day if we say we want to follow the datasheet, lets be
>> do it in the most direct way properly.

Daniel, I'm fine with your proposal.
Doing that, we can have a single compatible string for stm32 family,
even if the reset start offset change between two chips.

>>
>>
>> PS
>> I've written a custom lookup function to to get from the DT index to an
>> offset into the struct clk *array I'm using. That means I don't care
>> much about any big holes in the register space.
>
> How about using the first cell to indicate the type (pll, mux, div, gate)
> and the second cell for the number (between 0 and 256)? That way, the
> gates numbers would match the reset numbers, and your internal mapping
> function would look a bit nicer.

That's another option.
In this case, for reset, we will only need one cell, right?

Regards,
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
Daniel Thompson May 14, 2015, 7:38 p.m. UTC | #13
On 14/05/15 17:34, Maxime Coquelin wrote:
> 2015-05-13 21:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wednesday 13 May 2015 20:29:12 Daniel Thompson wrote:
>>> On 13/05/15 17:54, Maxime Coquelin wrote:
>>>> 2015-05-13 18:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>>>>
>>>>> We should definitely try to use the same compatible string for all of
>>>>> them, and make a binding that is easy to use.
>>>>>
>>>>> I haven't fully understood the requirements for the various parts that
>>>>> are involved here. My understanding so far was that the driver could
>>>>> use the index from the first cell and compute
>>>>>
>>>>>           void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
>>>>>           void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
>>>>
>>>> This calculation is true, but we have to take into account there is a
>>>> hole in the middle, between AHB3, and APB1 register:
>>>
>>> ... and equally importantly, only allows us to use hardware mappings for
>>> the gated clocks.
>>>
>>>> AHB1RSTR : offset = 0x10, index = 0
>>>> AHB2RSTR : offset = 0x14, index = 1
>>>> AHB3RSTR : offset = 0x18, index = 2
>>>> <HOLE >     : offset = 0x1c, index = 3
>>>> APB1RSTR : offset = 0x20, index = 4
>>>> APB2RSTR : offset = 0x24, index = 5
>>>>
>>>> So we have to carefully document this hole in the bindings, maybe by
>>>> listing indexes in the documentation?
>>>
>>> The register set has PLL, mux and dividers in the registers at 0x00,
>>> 0x04 and 0x08.
>>>
>>> Many of these clocks can be kept out of DT entirely because they are
>>> only there to feed other parts of the clock tree. However some of the
>>> dividers flow directly into cells that appear in device tree (such as
>>> the systick) and so we need to be able to reference them.
>>>
>>> In other words the proposed mapping cannot allow us to express the
>>> dividers properly (because the index would have to be negative):
>>>     void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
>>>
>>> Thus I'd favour using different indexes for reset and clock bindings,
>>> both using the naive mapping function:
>>>     void __iomem *reg =  rcc_base + 4 * index
>>>
>>> I think that its so much easier to check against the datasheet like
>>> that. Admittedly is we follow the block-of-4-bytes idiom we have to
>>> divide a hex number by four but thats not so hard and we end up with:
>>>
>>>                resets = <&rcc  8 0>;
>>>                clocks = <&rcc 16 0>;
>>>
>>> At the end of the day if we say we want to follow the datasheet, lets be
>>> do it in the most direct way properly.
>
> Daniel, I'm fine with your proposal.
> Doing that, we can have a single compatible string for stm32 family,
> even if the reset start offset change between two chips.
>
>>> PS
>>> I've written a custom lookup function to to get from the DT index to an
>>> offset into the struct clk *array I'm using. That means I don't care
>>> much about any big holes in the register space.
>>
>> How about using the first cell to indicate the type (pll, mux, div, gate)
>> and the second cell for the number (between 0 and 256)? That way, the
>> gates numbers would match the reset numbers, and your internal mapping
>> function would look a bit nicer.
>
> That's another option.
> In this case, for reset, we will only need one cell, right?

I think so. For the reset, this is essentially no change versus v7 
except for applying an offset of 0x10 when calculating the base address.

I won't get time to polish up the clk driver this weekend but I will try 
to write a documentation file proposing some clock bindings for you to 
look at.


Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin May 18, 2015, 12:21 p.m. UTC | #14
Hi Arnd, Daniel,

2015-05-13 18:54 GMT+02:00 Maxime Coquelin <mcoquelin.stm32@gmail.com>:
> 2015-05-13 18:37 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wednesday 13 May 2015 18:29:05 Maxime Coquelin wrote:
>>> 2015-05-13 17:28 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>>> > On Wednesday 13 May 2015 16:20:34 Daniel Thompson wrote:
>>> >>
>>> >> That would suit me very well (although is the 0x20/0x40 not the 8 that
>>> >> we would need in the middle column).
>>> >
>>> > We don't normally use register offsets in DT. The number 8 here instead
>>> > would indicate block 8, where each block is four bytes wide. Using the
>>> > same index here for reset and clock would also help readability.
>>>
>>> My view is that it makes the bindings usage very complex.
>>> Also, it implies we have a specific compatible for stm32f429, whereas
>>> we didn't need with my earlier proposals.
>>> Indeed, the reset driver will need to know the offset of every reset
>>> registers, because:
>>>   1. The AHB registers start at RCC offset 0x10 (up to 0x18)
>>>   2. The APB registers start at RCC offset 0x20 (up to 0x24).
>>> We have a gap between AHB and APB registers, so how do we map the
>>> index for the block you propose?
>>> Should the gap be considered as a block, or we should skip it?
>>>
>>> I'm afraid it will not be straightforward for a reset user to
>>> understand how to use this bindings.
>>>
>>> Either my v7 or v8 versions would have made possible to use a single
>>> compatible for STM32 series.
>>> If we stick with one of these, we could even think to have a "generic"
>>> reset driver, as it could be compatible with sunxi driver bindings.
>>
>> We should definitely try to use the same compatible string for all of
>> them, and make a binding that is easy to use.
>>
>> I haven't fully understood the requirements for the various parts that
>> are involved here. My understanding so far was that the driver could
>> use the index from the first cell and compute
>>
>>         void __iomem *reset_reg = rcc_base + 0x10 + 4 * index;
>>         void __iomem *clock_reg = rcc_base + 0x30 + 4 * index;
>
> This calculation is true, but we have to take into account there is a
> hole in the middle, between AHB3, and APB1 register:
>
> AHB1RSTR : offset = 0x10, index = 0
> AHB2RSTR : offset = 0x14, index = 1
> AHB3RSTR : offset = 0x18, index = 2
> <HOLE >     : offset = 0x1c, index = 3
> APB1RSTR : offset = 0x20, index = 4
> APB2RSTR : offset = 0x24, index = 5
>
> So we have to carefully document this hole in the bindings, maybe by
> listing indexes in the documentation?
>
>>
>> Are there parts that need something else? If the 0x10 offset is
>> different, we probably want a different compatible string, and I'd
>> consider it a different part at that point. If there are chips
>> that do not spread the clock from the reset by exactly 256 bits,
>> we could add a DT property in the rcc node for that.
>
> I will check other chips, to see if this is valid generally.

I checked on other chips, and the assumption looks valid generally.

Kind regards,
Maxime

>
> Thanks for your feedback,
> 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
Maxime Coquelin May 20, 2015, 4:17 p.m. UTC | #15
Hi Arnd, Philipp,

2015-05-13 21:11 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> Ideally the binding should follow closely what is documented
> in the data sheet.
>

Daniel and myself would like your opinion about this binding:

rcc: rcc@40023800 {
    #reset-cells = <1>;
    #clock-cells = <2>;
    compatible = "st,stm32-rcc";
    reg = <0x40023800 0x10>, <0x40023810 0x20>, <0x40023830 0x20>;
    reg-names = "clock-cfg", "reset", "clock-gates";
};

It would solve a problem Daniel is facing due to conflicting mem
region when clock and reset drivers are enabled, as both would reserve
the same region.

Also, it would make the reset driver very generic.
Doing that, we could even create a generic-reset.c driver that would
be used by STM32 and Sunxi (at least).
In the probe function, it would check the number of reg resources.
If a single resource is passed, it would take it, else it would look
the one named "reset".
The driver and bindings would be the same for the two families, and
the bindings would be backward compatible with sunxi ones.

Philip, Arnd, what do you think?

Kind regards,
Maxime
--
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
Daniel Thompson May 22, 2015, 12:43 p.m. UTC | #16
On 22/05/15 13:32, Maxime Coquelin wrote:
> 2015-05-22 12:07 GMT+02:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> And that's how I'd expect it to be described by the device tree:
>>
>>      rcc: rcc@40023800 {
>>          compatible = "st,stm32-rcc";
>>          reg = <0x40023800 0xc0>;
>>      };
>>
>
> Doing that, since this register bank contains both reset and clock
> registers, the reset device cannot get the IO resource at probe time
> because the clock driver has already reserved it.
> Daniel, who has started to work on the clock driver is facing this issue.
> This is why I proposed this binding for "reg" property.

Temporarily I've kept things working for me by avoiding 
of_io_request_and_map() in the clock driver (I am using raw of_iomap() 
instead).

I view this as a hack rather than a solution!

Note that, with or without the hack, I do hope to be able to post the 
clock driver this weekend. I am acutely aware that at the moment we are 
discussing code I haven't posted yet.


> We could think of creating a MFD driver, but the problem is that clock
> need to be intialized before a MFD device can be probed.
>
> Maybe there is a way to have you binding working properly, but I
> haven't found one for now.

I guess a driver doesn't *have* to reserve all the register space 
described in DT. The driver could replace of_io_request_and_map() with 
lower level calls.

Thus if we wanted to keep this detail out of DT then I guess the two 
drivers could simply make an agreement not to request registers they do 
not use.


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin May 22, 2015, 1:57 p.m. UTC | #17
2015-05-22 15:09 GMT+02:00 Andreas Färber <afaerber@suse.de>:
> As you should know, I did have an RCC clk driver, and there is no such
> issue. The two drivers use different mechanisms for initialization. And
> I'm pretty sure that I've already remarked that on the list, too.

Yes, you use of_iomap in your clock driver [0].
Daniel, would you accept to do the same?
That would remove one difference between stm32/sunxi/socfpga reset drivers.

Regards,
Maxime

[0]: https://github.com/afaerber/linux/blob/stm32/drivers/clk/clk-stm32f42xxx-rcc.c
--
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
Andreas Färber May 22, 2015, 2:06 p.m. UTC | #18
Am 22.05.2015 um 15:57 schrieb Maxime Coquelin:
> 2015-05-22 15:09 GMT+02:00 Andreas Färber <afaerber@suse.de>:
>> As you should know, I did have an RCC clk driver, and there is no such
>> issue. The two drivers use different mechanisms for initialization. And
>> I'm pretty sure that I've already remarked that on the list, too.
> 
> Yes, you use of_iomap in your clock driver [0].

Which was inspired by the efm32 driver iirc.

> Daniel, would you accept to do the same?
> That would remove one difference between stm32/sunxi/socfpga reset drivers.
> 
> Regards,
> Maxime
> 
> [0]: https://github.com/afaerber/linux/blob/stm32/drivers/clk/clk-stm32f42xxx-rcc.c

For the record, that still has some internal clocks that shouldn't be
exposed.

Andreas
Daniel Thompson May 22, 2015, 2:14 p.m. UTC | #19
On 22/05/15 14:57, Maxime Coquelin wrote:
> 2015-05-22 15:09 GMT+02:00 Andreas Färber <afaerber@suse.de>:
>> As you should know, I did have an RCC clk driver, and there is no such
>> issue. The two drivers use different mechanisms for initialization. And
>> I'm pretty sure that I've already remarked that on the list, too.
>
> Yes, you use of_iomap in your clock driver [0].
> Daniel, would you accept to do the same?
> That would remove one difference between stm32/sunxi/socfpga reset drivers.

In fact, that is exactly what I am currently doing, though I was 
planning for it to be temporary.

It seems a bit weird to me that one driver (which requests too much 
register space) only works because another driver chooses not to request 
any.

BTW in drivers/clk there are ~110 of_iomaps and only 10 
of_io_request_and_maps... so I don't think anyone will yell at me for 
using of_iomap().


Daniel.

--
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
diff mbox

Patch

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 86217db..db1d8c6 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -520,6 +520,7 @@  dtb-$(CONFIG_ARCH_STI) += \
 	stih416-b2020.dtb \
 	stih416-b2020e.dtb \
 	stih418-b2199.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..6b9aa59
--- /dev/null
+++ b/arch/arm/boot/dts/stm32f429-disco.dts
@@ -0,0 +1,71 @@ 
+/*
+ * Copyright 2015 - Maxime Coquelin <mcoquelin.stm32@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ *     You should have received a copy of the GNU General Public
+ *     License along with this file; if not, write to the Free
+ *     Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+ *     MA 02110-1301 USA
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+#include "stm32f429.dtsi"
+
+/ {
+	model = "STMicroelectronics STM32F429i-DISCO board";
+	compatible = "st,stm32f429i-disco", "st,stm32f429";
+
+	chosen {
+		bootargs = "console=ttyS0,115200 root=/dev/ram rdinit=/linuxrc";
+		linux,stdout-path = &usart1;
+	};
+
+	memory {
+		reg = <0x90000000 0x800000>;
+	};
+
+	aliases {
+		serial0 = &usart1;
+	};
+};
+
+&usart1 {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
new file mode 100644
index 0000000..2719f3a
--- /dev/null
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -0,0 +1,227 @@ 
+/*
+ * Copyright 2015 - Maxime Coquelin <mcoquelin.stm32@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ *     You should have received a copy of the GNU General Public
+ *     License along with this file; if not, write to the Free
+ *     Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+ *     MA 02110-1301 USA
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include "armv7-m.dtsi"
+#include  <dt-bindings/mfd/stm32f4-rcc.h>
+
+/ {
+	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>;
+		};
+	};
+
+	soc {
+		timer2: timer@40000000 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000000 0x400>;
+			interrupts = <28>;
+			resets = <&rcc STM32F4_APB1_RESET(TIM2)>;
+			clocks = <&clk_pmtr1>;
+			status = "disabled";
+		};
+
+		timer3: timer@40000400 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000400 0x400>;
+			interrupts = <29>;
+			resets = <&rcc STM32F4_APB1_RESET(TIM3)>;
+			clocks = <&clk_pmtr1>;
+			status = "disabled";
+		};
+
+		timer4: timer@40000800 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000800 0x400>;
+			interrupts = <30>;
+			resets = <&rcc STM32F4_APB1_RESET(TIM4)>;
+			clocks = <&clk_pmtr1>;
+			status = "disabled";
+		};
+
+		timer5: timer@40000c00 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000c00 0x400>;
+			interrupts = <50>;
+			resets = <&rcc STM32F4_APB1_RESET(TIM5)>;
+			clocks = <&clk_pmtr1>;
+		};
+
+		timer6: timer@40001000 {
+			compatible = "st,stm32-timer";
+			reg = <0x40001000 0x400>;
+			interrupts = <54>;
+			resets = <&rcc STM32F4_APB1_RESET(TIM6)>;
+			clocks = <&clk_pmtr1>;
+			status = "disabled";
+		};
+
+		timer7: timer@40001400 {
+			compatible = "st,stm32-timer";
+			reg = <0x40001400 0x400>;
+			interrupts = <55>;
+			resets = <&rcc STM32F4_APB1_RESET(TIM7)>;
+			clocks = <&clk_pmtr1>;
+			status = "disabled";
+		};
+
+		usart2: serial@40004400 {
+			compatible = "st,stm32-usart", "st,stm32-uart";
+			reg = <0x40004400 0x400>;
+			interrupts = <38>;
+			clocks = <&clk_pclk1>;
+			status = "disabled";
+		};
+
+		usart3: serial@40004800 {
+			compatible = "st,stm32-usart", "st,stm32-uart";
+			reg = <0x40004800 0x400>;
+			interrupts = <39>;
+			clocks = <&clk_pclk1>;
+			status = "disabled";
+		};
+
+		usart4: serial@40004c00 {
+			compatible = "st,stm32-uart";
+			reg = <0x40004c00 0x400>;
+			interrupts = <52>;
+			clocks = <&clk_pclk1>;
+			status = "disabled";
+		};
+
+		usart5: serial@40005000 {
+			compatible = "st,stm32-uart";
+			reg = <0x40005000 0x400>;
+			interrupts = <53>;
+			clocks = <&clk_pclk1>;
+			status = "disabled";
+		};
+
+		usart7: serial@40007800 {
+			compatible = "st,stm32-usart", "st,stm32-uart";
+			reg = <0x40007800 0x400>;
+			interrupts = <82>;
+			clocks = <&clk_pclk1>;
+			status = "disabled";
+		};
+
+		usart8: serial@40007c00 {
+			compatible = "st,stm32-usart", "st,stm32-uart";
+			reg = <0x40007c00 0x400>;
+			interrupts = <83>;
+			clocks = <&clk_pclk1>;
+			status = "disabled";
+		};
+
+		usart1: serial@40011000 {
+			compatible = "st,stm32-usart", "st,stm32-uart";
+			reg = <0x40011000 0x400>;
+			interrupts = <37>;
+			clocks = <&clk_pclk2>;
+			status = "disabled";
+		};
+
+		usart6: serial@40011400 {
+			compatible = "st,stm32-usart", "st,stm32-uart";
+			reg = <0x40011400 0x400>;
+			interrupts = <71>;
+			clocks = <&clk_pclk2>;
+			status = "disabled";
+		};
+
+		rcc: rcc@40023810 {
+			#reset-cells = <1>;
+			compatible = "st,stm32-rcc";
+			reg = <0x40023800 0x400>;
+		};
+	};
+};
+
+&systick {
+	clocks = <&clk_systick>;
+	status = "okay";
+};