| Message ID | 1431158038-3813-15-git-send-email-mcoquelin.stm32@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 >
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
On 13/05/15 14:27, Arnd Bergmann wrote: > 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. Indeed. It was that long list that originally provoked me to comment in the first place. >>> 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? 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). >> 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? >> 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). >> 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). Maxime: Does that suit reset driver? Daniel.
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
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
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
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
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
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
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
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
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.
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
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
On Wed, May 20, 2015 at 06:17:34PM +0200, Maxime Coquelin wrote: > 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? I lack a bit of context here, but I'd be very happy to use a generic driver. As a matter of fact, the sunxi reset driver is already pretty much there (not that it's very difficult to do). The only thing we did that stands out a bit is that we actually need the reset driver much earlier than the default probe, since some of our timers are maintained in reset. We have some code to do just that already, we just need have something similar to be on board. Maxime (the other one)
2015-05-21 20:51 GMT+02:00 Maxime Ripard <maxime.ripard@free-electrons.com>: > On Wed, May 20, 2015 at 06:17:34PM +0200, Maxime Coquelin wrote: >> 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? > > I lack a bit of context here, but I'd be very happy to use a generic > driver. As a matter of fact, the sunxi reset driver is already pretty > much there (not that it's very difficult to do). Ok, good. The two drivers are almost the same, so squashing to a generic one would be trivial. > > The only thing we did that stands out a bit is that we actually need > the reset driver much earlier than the default probe, since some of > our timers are maintained in reset. We have some code to do just that > already, we just need have something similar to be on board. We have the same problem on stm32, as just discussed with Andreas [0]. I decided to patch the bootloader to deassert timers reset, after discussions with Rob Herring and Arnd. Moving to a common driver, stm32 could use the same early init method to initiliaze reset before timers. Regards, Maxime [0]: http://marc.info/?l=linux-arm-kernel&m=143223846802405&w=2#0
Am Mittwoch, den 20.05.2015, 18:17 +0200 schrieb Maxime Coquelin: > 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"; "reset" singular, "clock-gates" plural seems inconsistent to me. > }; > > 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). Adding support for providing the reset register range via of_device_id data and the possibility to invert set/clear ops would allow to also include the socfpga driver. > 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? I'm not a fan of describing the register layout in the device tree as detailed as the sunxi bindings do. I'd prefer the reg property to describe the device's register address space with one entry per contiguous block of registers. Unifying the mostly identical drivers is a good idea though, and reusing preexisting bindings is better than inventing new ones. I favor the socfpga binding, but I still like the sunxi bindings and this proposal better than encoding the register offset in the reset index. best regards Philipp
On Fri, May 22, 2015 at 11:06:28AM +0200, Philipp Zabel wrote: > > 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? > > I'm not a fan of describing the register layout in the device tree as > detailed as the sunxi bindings do. I'd prefer the reg property to > describe the device's register address space with one entry per > contiguous block of registers. That's exactly what we do. > Unifying the mostly identical drivers is a good idea though, and reusing > preexisting bindings is better than inventing new ones. I favor the > socfpga binding, but I still like the sunxi bindings and this proposal > better than encoding the register offset in the reset index. I don't really get the difference between the socfpga and our bindings actually. Would you mind to explain a bit further what you don't like about it ? Maxime
2015-05-22 11:06 GMT+02:00 Philipp Zabel <p.zabel@pengutronix.de>: > Am Mittwoch, den 20.05.2015, 18:17 +0200 schrieb Maxime Coquelin: >> 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"; > > "reset" singular, "clock-gates" plural seems inconsistent to me. Indeed. I will change to "resets". About the compatible string, any idea how could I name it? > >> }; >> >> 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). > > Adding support for providing the reset register range via of_device_id > data and the possibility to invert set/clear ops would allow to also > include the socfpga driver. Ok, it sounds like a good plan. Should I also add a property in the bindings for the reset polarity? It won't be used for now, as for socfpga we have to do it via of_device_id data, in order not to break DT backward compatibility. > >> 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? > > I'm not a fan of describing the register layout in the device tree as > detailed as the sunxi bindings do. I'd prefer the reg property to > describe the device's register address space with one entry per > contiguous block of registers. > Unifying the mostly identical drivers is a good idea though, and reusing > preexisting bindings is better than inventing new ones. I favor the > socfpga binding, but I still like the sunxi bindings and this proposal > better than encoding the register offset in the reset index. Ok, I will propose a RFC. Thanks, Maxime
Am Freitag, den 22.05.2015, 11:18 +0200 schrieb Maxime Ripard: > On Fri, May 22, 2015 at 11:06:28AM +0200, Philipp Zabel wrote: > > > 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? > > > > I'm not a fan of describing the register layout in the device tree as > > detailed as the sunxi bindings do. I'd prefer the reg property to > > describe the device's register address space with one entry per > > contiguous block of registers. > > That's exactly what we do. Sorry, what I mean is 'as detailed as reusing the sunxi bindings for stm32xx here would do'. I don't know enough about the Allwinner register layouts to form an opinion. The STM32F427xx/STM32F429xx manual, Table 13. "STM32F427xx and STM32F429xx register boundary addresses" contains this entry: Bus Boundary address Peripheral AHB1 0x40023800-0x400238bf RCC 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>; }; Instead of "reg = <0x40023810 0x20>" for the resets. Where in the address range the reset, clock gate and clock configuration registers reside could be derived from the compatible value. > > Unifying the mostly identical drivers is a good idea though, and reusing > > preexisting bindings is better than inventing new ones. I favor the > > socfpga binding, but I still like the sunxi bindings and this proposal > > better than encoding the register offset in the reset index. > > I don't really get the difference between the socfpga and our bindings > actually. Would you mind to explain a bit further what you don't like > about it ? The socfpga driver currently hardcodes the reset register offset (0x10) and number of banks (4), the sunxi driver has no register offset (0x0) and derives the number of banks from the resource size. I'd store the internal register offsets and number of banks in the driver, together with the compatible string. regards Philipp
2015-05-22 12:07 GMT+02:00 Philipp Zabel <p.zabel@pengutronix.de>: > The STM32F427xx/STM32F429xx manual, Table 13. "STM32F427xx and > STM32F429xx register boundary addresses" contains this entry: > Bus Boundary address Peripheral > AHB1 0x40023800-0x400238bf RCC > > 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. 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. Regards, Maxime
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.
Am 22.05.2015 um 14:32 schrieb Maxime Coquelin: > 2015-05-22 12:07 GMT+02:00 Philipp Zabel <p.zabel@pengutronix.de>: >> The STM32F427xx/STM32F429xx manual, Table 13. "STM32F427xx and >> STM32F429xx register boundary addresses" contains this entry: >> Bus Boundary address Peripheral >> AHB1 0x40023800-0x400238bf RCC >> >> 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. 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. Regards, Andreas
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
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
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.
Hi Philipp, On Fri, May 22, 2015 at 12:07:11PM +0200, Philipp Zabel wrote: > Am Freitag, den 22.05.2015, 11:18 +0200 schrieb Maxime Ripard: > > On Fri, May 22, 2015 at 11:06:28AM +0200, Philipp Zabel wrote: > > > > 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? > > > > > > I'm not a fan of describing the register layout in the device tree as > > > detailed as the sunxi bindings do. I'd prefer the reg property to > > > describe the device's register address space with one entry per > > > contiguous block of registers. > > > > That's exactly what we do. > > Sorry, what I mean is 'as detailed as reusing the sunxi bindings for > stm32xx here would do'. I don't know enough about the Allwinner register > layouts to form an opinion. > > The STM32F427xx/STM32F429xx manual, Table 13. "STM32F427xx and > STM32F429xx register boundary addresses" contains this entry: > Bus Boundary address Peripheral > AHB1 0x40023800-0x400238bf RCC > > 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>; > }; It's pretty much what we do already. The thing is that our reset controllers are intertwined with our clock ones, so our reset controllers are usually taking only a few registers, and we can't use more than that since we have clocks in the registers around. > Instead of "reg = <0x40023810 0x20>" for the resets. Where in the > address range the reset, clock gate and clock configuration registers > reside could be derived from the compatible value. I agree on that, and if a generic solution was to be made like this, we could definitely use it. Maxime
Hi Maxime, On Thu, May 21, 2015 at 10:10:35PM +0200, Maxime Coquelin wrote: > > The only thing we did that stands out a bit is that we actually need > > the reset driver much earlier than the default probe, since some of > > our timers are maintained in reset. We have some code to do just that > > already, we just need have something similar to be on board. > > We have the same problem on stm32, as just discussed with Andreas [0]. > I decided to patch the bootloader to deassert timers reset, after > discussions with Rob Herring and Arnd. That seems like the best solution, but we don't have control over the bootloader in most cases. We have an alternative implementation (ie mainline u-boot) that is required for a few things already, but I'd really like to still be able to at least boot the kernel on a "legacy" bootloader (that is the one flashed on the device). Maxime
Hi Maxime, 2015-05-23 10:28 GMT+02:00 Maxime Ripard <maxime.ripard@free-electrons.com>: > On Thu, May 21, 2015 at 10:10:35PM +0200, Maxime Coquelin wrote: >> We have the same problem on stm32, as just discussed with Andreas [0]. >> I decided to patch the bootloader to deassert timers reset, after >> discussions with Rob Herring and Arnd. > > That seems like the best solution, but we don't have control over the > bootloader in most cases. We have an alternative implementation (ie > mainline u-boot) that is required for a few things already, but I'd > really like to still be able to at least boot the kernel on a "legacy" > bootloader (that is the one flashed on the device). For sure it will have to remain compliant with the legacy bootloader. It is easier for STM32 since it is a newly platform. Regards, Maxime C.
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"; +};