| Message ID | 1400029876-5830-5-git-send-email-thomas.ab@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thomas, Please see my comments inline. On 14.05.2014 03:11, Thomas Abraham wrote: > From; Thomas Abraham <thomas.ab@samsung.com> > > The clock blocks within the CMU_CPU clock domain are put together into a > new composite clock type called the cpu clock. This clock type requires > configuration data that will be atomically programmed in the multiple > clock blocks encapsulated within the cpu clock type when the parent clock > frequency is changed. This configuration data is held in the clock controller > node. Update clock binding documentation about this configuration data format > for Samsung Exynos4 and Exynos5 platforms. > > Cc: Tomasz Figa <t.figa@samsung.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > Cc: Kumar Gala <galak@codeaurora.org> > Cc: <devicetree@vger.kernel.org> > Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> > --- > .../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++ > .../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++ > 2 files changed, 73 insertions(+), 0 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt b/Documentation/devicetree/bindings/clock/exynos4-clock.txt > index f5a5b19..0934e02 100644 > --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt > +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt > @@ -15,6 +15,35 @@ Required Properties: > > - #clock-cells: should be 1. > > +- samsung,armclk-divider-table: when the frequency of the APLL is changed > + the divider clocks in CMU_CPU clock domain also need to be updated. These > + divider clocks have SoC specific divider clock output requirements for a > + specific APLL clock speeds. When APLL clock rate is changed, these divider > + clocks are reprogrammed with pre-determined values in order to maintain the > + SoC specific divider clock outputs. This property lists the divider values > + for divider clocks in the CMU_CPU block for supported APLL clock speeds. > + The format of each entry included in the arm-frequency-table should be > + as defined below As far as I understand, the relation is not between the APLL frequency and particular clocks in CPU domain, but rather between the latter and input clock to CPU domain, which is _after_ the two dividers (called DIV_CORE and DIV_CORE2 or ARM_DIV1 and ARM_DIV2), which is also exactly the output frequency of ARMCLK. > + > + - for Exynos4210 and Exynos4212 based platforms: > + cell #1: arm clock parent frequency Considering my comment above, this should be rather ARMCLK frequency. > + cell #2 ~ cell 9#: value of clock divider in the following order > + corem0_ratio, corem1_ratio, periph_ratio, atb_ratio, > + pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio. > + > + - for Exynos4412 based platforms: > + cell #1: expected arm clock parent frequency Ditto. > + cell #2 ~ cell #10: value of clock divider in the following order > + corem0_ratio, corem1_ratio, periph_ratio, atb_ratio, > + pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio, cores_ratio > + > +- samsung,armclk-cells: defines the number of cells in > + samsung,armclk-divider-table property. The value of this property depends on > + the SoC type. To follow conventions used by all other bindings with variable number of cells, the property should be called "#samsung,armclk-cells". AFAIK the "#" should be interpreted as "number of" and so accents the meaning of the property. > + > + - for Exynos4210 and Exynos4212: the value should be 9. > + - for Exynos4412: the value should be 10. > + > Each clock is assigned an identifier and client nodes can use this identifier > to specify the clock which they consume. > > @@ -28,6 +57,14 @@ Example 1: An example of a clock controller node is listed below. > compatible = "samsung,exynos4210-clock"; > reg = <0x10030000 0x20000>; > #clock-cells = <1>; > + > + samsung,armclk-cells = <9>; > + samsung,armclk-divider-table = <1200000 3 7 3 4 1 7 5 0>, > + <1000000 3 7 3 4 1 7 4 0>, > + < 800000 3 7 3 3 1 7 3 0>, > + < 500000 3 7 3 3 1 7 3 0>, > + < 400000 3 7 3 3 1 7 3 0>, > + < 200000 1 3 1 1 1 0 3 0>; > }; > > Example 2: UART controller node that consumes the clock generated by the clock > diff --git a/Documentation/devicetree/bindings/clock/exynos5250-clock.txt b/Documentation/devicetree/bindings/clock/exynos5250-clock.txt > index 536eacd..3d63d09 100644 > --- a/Documentation/devicetree/bindings/clock/exynos5250-clock.txt > +++ b/Documentation/devicetree/bindings/clock/exynos5250-clock.txt > @@ -13,6 +13,24 @@ Required Properties: Same comments apply to this file as well. Also, shouldn't you also extend exynos5420-clock.txt in the same way? Best regards, Tomasz
On 17.05.2014 01:24, Tomasz Figa wrote:
> Also, shouldn't you also extend exynos5420-clock.txt in the same way?
Please ignore this comment. Somehow I thought other patches of this
series also had support for Exynos5420.
Best regards,
Tomasz
Hi Tomasz, Thanks for your comments. Please see inline reply. On Sat, May 17, 2014 at 4:54 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Hi Thomas, > > Please see my comments inline. > > On 14.05.2014 03:11, Thomas Abraham wrote: >> From; Thomas Abraham <thomas.ab@samsung.com> >> >> The clock blocks within the CMU_CPU clock domain are put together into a >> new composite clock type called the cpu clock. This clock type requires >> configuration data that will be atomically programmed in the multiple >> clock blocks encapsulated within the cpu clock type when the parent clock >> frequency is changed. This configuration data is held in the clock controller >> node. Update clock binding documentation about this configuration data format >> for Samsung Exynos4 and Exynos5 platforms. >> >> Cc: Tomasz Figa <t.figa@samsung.com> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Pawel Moll <pawel.moll@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> >> Cc: Kumar Gala <galak@codeaurora.org> >> Cc: <devicetree@vger.kernel.org> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> >> --- >> .../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++ >> .../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++ >> 2 files changed, 73 insertions(+), 0 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt b/Documentation/devicetree/bindings/clock/exynos4-clock.txt >> index f5a5b19..0934e02 100644 >> --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt >> +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt >> @@ -15,6 +15,35 @@ Required Properties: >> >> - #clock-cells: should be 1. >> >> +- samsung,armclk-divider-table: when the frequency of the APLL is changed >> + the divider clocks in CMU_CPU clock domain also need to be updated. These >> + divider clocks have SoC specific divider clock output requirements for a >> + specific APLL clock speeds. When APLL clock rate is changed, these divider >> + clocks are reprogrammed with pre-determined values in order to maintain the >> + SoC specific divider clock outputs. This property lists the divider values >> + for divider clocks in the CMU_CPU block for supported APLL clock speeds. >> + The format of each entry included in the arm-frequency-table should be >> + as defined below > > As far as I understand, the relation is not between the APLL frequency > and particular clocks in CPU domain, but rather between the latter and > input clock to CPU domain, which is _after_ the two dividers (called > DIV_CORE and DIV_CORE2 or ARM_DIV1 and ARM_DIV2), which is also exactly > the output frequency of ARMCLK. > >> + >> + - for Exynos4210 and Exynos4212 based platforms: >> + cell #1: arm clock parent frequency > > Considering my comment above, this should be rather ARMCLK frequency. The clocks SCLK_APLL, SCLK_HPM, ATCLK and PCLK_DBG have no relation to the ARMCLK frequency. These clocks are directly derived from the PLL clock and so it would not be correct to have them related to ARMCLK. So, I see two solutions to this, first being preferred solution. [A] Cell #1 should define PLL (parent of armclk) clock speed. Cell #2 and Cell #3 should define divider values for ARMCLK clock speed. The hardware does support PLL frequency != ARMCLK frequency and so DT binding should allow that (even though implementation in the linux kernel does not use this feature). BTW, this was what was done in v2 of this series. [B] Embedded this data with the code and don't get this from DT. The reason for doing this is, these are SoC specific values and not board specific. And when we are clear about what we want to put in DT, have a provision to lookup DT first and if DT values are not found, fallback on data embedded with the code. Thanks, Thomas. > >> + cell #2 ~ cell 9#: value of clock divider in the following order >> + corem0_ratio, corem1_ratio, periph_ratio, atb_ratio, >> + pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio. >> + >> + - for Exynos4412 based platforms: >> + cell #1: expected arm clock parent frequency > > Ditto. > >> + cell #2 ~ cell #10: value of clock divider in the following order >> + corem0_ratio, corem1_ratio, periph_ratio, atb_ratio, >> + pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio, cores_ratio >> + >> +- samsung,armclk-cells: defines the number of cells in >> + samsung,armclk-divider-table property. The value of this property depends on >> + the SoC type. > > To follow conventions used by all other bindings with variable number of > cells, the property should be called "#samsung,armclk-cells". AFAIK the > "#" should be interpreted as "number of" and so accents the meaning of > the property. > >> + >> + - for Exynos4210 and Exynos4212: the value should be 9. >> + - for Exynos4412: the value should be 10. >> + >> Each clock is assigned an identifier and client nodes can use this identifier >> to specify the clock which they consume. >> >> @@ -28,6 +57,14 @@ Example 1: An example of a clock controller node is listed below. >> compatible = "samsung,exynos4210-clock"; >> reg = <0x10030000 0x20000>; >> #clock-cells = <1>; >> + >> + samsung,armclk-cells = <9>; >> + samsung,armclk-divider-table = <1200000 3 7 3 4 1 7 5 0>, >> + <1000000 3 7 3 4 1 7 4 0>, >> + < 800000 3 7 3 3 1 7 3 0>, >> + < 500000 3 7 3 3 1 7 3 0>, >> + < 400000 3 7 3 3 1 7 3 0>, >> + < 200000 1 3 1 1 1 0 3 0>; >> }; >> >> Example 2: UART controller node that consumes the clock generated by the clock >> diff --git a/Documentation/devicetree/bindings/clock/exynos5250-clock.txt b/Documentation/devicetree/bindings/clock/exynos5250-clock.txt >> index 536eacd..3d63d09 100644 >> --- a/Documentation/devicetree/bindings/clock/exynos5250-clock.txt >> +++ b/Documentation/devicetree/bindings/clock/exynos5250-clock.txt >> @@ -13,6 +13,24 @@ Required Properties: > > Same comments apply to this file as well. > > Also, shouldn't you also extend exynos5420-clock.txt in the same way? > > Best regards, > Tomasz
Thomas, On 26.05.2014 08:05, Thomas Abraham wrote: > Hi Tomasz, > > Thanks for your comments. Please see inline reply. > > On Sat, May 17, 2014 at 4:54 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> Hi Thomas, >> >> Please see my comments inline. >> >> On 14.05.2014 03:11, Thomas Abraham wrote: >>> From; Thomas Abraham <thomas.ab@samsung.com> >>> >>> The clock blocks within the CMU_CPU clock domain are put together into a >>> new composite clock type called the cpu clock. This clock type requires >>> configuration data that will be atomically programmed in the multiple >>> clock blocks encapsulated within the cpu clock type when the parent clock >>> frequency is changed. This configuration data is held in the clock controller >>> node. Update clock binding documentation about this configuration data format >>> for Samsung Exynos4 and Exynos5 platforms. >>> >>> Cc: Tomasz Figa <t.figa@samsung.com> >>> Cc: Rob Herring <robh+dt@kernel.org> >>> Cc: Pawel Moll <pawel.moll@arm.com> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> >>> Cc: Kumar Gala <galak@codeaurora.org> >>> Cc: <devicetree@vger.kernel.org> >>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> >>> --- >>> .../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++ >>> .../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++ >>> 2 files changed, 73 insertions(+), 0 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt b/Documentation/devicetree/bindings/clock/exynos4-clock.txt >>> index f5a5b19..0934e02 100644 >>> --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt >>> +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt >>> @@ -15,6 +15,35 @@ Required Properties: >>> >>> - #clock-cells: should be 1. >>> >>> +- samsung,armclk-divider-table: when the frequency of the APLL is changed >>> + the divider clocks in CMU_CPU clock domain also need to be updated. These >>> + divider clocks have SoC specific divider clock output requirements for a >>> + specific APLL clock speeds. When APLL clock rate is changed, these divider >>> + clocks are reprogrammed with pre-determined values in order to maintain the >>> + SoC specific divider clock outputs. This property lists the divider values >>> + for divider clocks in the CMU_CPU block for supported APLL clock speeds. >>> + The format of each entry included in the arm-frequency-table should be >>> + as defined below >> >> As far as I understand, the relation is not between the APLL frequency >> and particular clocks in CPU domain, but rather between the latter and >> input clock to CPU domain, which is _after_ the two dividers (called >> DIV_CORE and DIV_CORE2 or ARM_DIV1 and ARM_DIV2), which is also exactly >> the output frequency of ARMCLK. >> >>> + >>> + - for Exynos4210 and Exynos4212 based platforms: >>> + cell #1: arm clock parent frequency >> >> Considering my comment above, this should be rather ARMCLK frequency. > > The clocks SCLK_APLL, SCLK_HPM, ATCLK and PCLK_DBG have no relation to > the ARMCLK frequency. These clocks are directly derived from the PLL > clock and so it would not be correct to have them related to ARMCLK. Oh, right, the old driver was changing DIV_APLL, DIV_ATB and DIV_PCLK_DBG as well. Somehow I was under an impression that we need to care only about those dividers on the path after DIV_CORE and DIV_CORE2. In this case the parent rate is the key here, although I'd call it "CPU block parent rate (usually APLL)". However this means that the trick with using DIV_CORE and DIV_CORE2 to divide the rate of temporary parent clock is not enough, because DIV_ATB is sourced directly from MOUT_CORE. > > So, I see two solutions to this, first being preferred solution. > > [A] Cell #1 should define PLL (parent of armclk) clock speed. Cell #2 > and Cell #3 should define divider values for ARMCLK clock speed. The > hardware does support PLL frequency != ARMCLK frequency and so DT > binding should allow that (even though implementation in the linux > kernel does not use this feature). BTW, this was what was done in v2 > of this series. > > [B] Embedded this data with the code and don't get this from DT. The > reason for doing this is, these are SoC specific values and not board > specific. And when we are clear about what we want to put in DT, have > a provision to lookup DT first and if DT values are not found, > fallback on data embedded with the code. Well, they are not that generic as they might appear. I've seen different values for the same SoC in different vendor kernels, depending on device the kernel was targeted for. Also they will likely differ between SoC revisions. However it might be a good idea indeed to keep the table in the code as a first step to get the driver running without creating new DT bindings. By the way, I'm not fully convinced if there is really a need for such hardcoded look-up tables at all. Those divisors certainly look like they are calculated based on some upper bounds for certain clocks and the driver could simply find them out itself if those limits were provided. Best regards, Tomasz
diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt b/Documentation/devicetree/bindings/clock/exynos4-clock.txt index f5a5b19..0934e02 100644 --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt @@ -15,6 +15,35 @@ Required Properties: - #clock-cells: should be 1. +- samsung,armclk-divider-table: when the frequency of the APLL is changed + the divider clocks in CMU_CPU clock domain also need to be updated. These + divider clocks have SoC specific divider clock output requirements for a + specific APLL clock speeds. When APLL clock rate is changed, these divider + clocks are reprogrammed with pre-determined values in order to maintain the + SoC specific divider clock outputs. This property lists the divider values + for divider clocks in the CMU_CPU block for supported APLL clock speeds. + The format of each entry included in the arm-frequency-table should be + as defined below + + - for Exynos4210 and Exynos4212 based platforms: + cell #1: arm clock parent frequency + cell #2 ~ cell 9#: value of clock divider in the following order + corem0_ratio, corem1_ratio, periph_ratio, atb_ratio, + pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio. + + - for Exynos4412 based platforms: + cell #1: expected arm clock parent frequency + cell #2 ~ cell #10: value of clock divider in the following order + corem0_ratio, corem1_ratio, periph_ratio, atb_ratio, + pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio, cores_ratio + +- samsung,armclk-cells: defines the number of cells in + samsung,armclk-divider-table property. The value of this property depends on + the SoC type. + + - for Exynos4210 and Exynos4212: the value should be 9. + - for Exynos4412: the value should be 10. + Each clock is assigned an identifier and client nodes can use this identifier to specify the clock which they consume. @@ -28,6 +57,14 @@ Example 1: An example of a clock controller node is listed below. compatible = "samsung,exynos4210-clock"; reg = <0x10030000 0x20000>; #clock-cells = <1>; + + samsung,armclk-cells = <9>; + samsung,armclk-divider-table = <1200000 3 7 3 4 1 7 5 0>, + <1000000 3 7 3 4 1 7 4 0>, + < 800000 3 7 3 3 1 7 3 0>, + < 500000 3 7 3 3 1 7 3 0>, + < 400000 3 7 3 3 1 7 3 0>, + < 200000 1 3 1 1 1 0 3 0>; }; Example 2: UART controller node that consumes the clock generated by the clock diff --git a/Documentation/devicetree/bindings/clock/exynos5250-clock.txt b/Documentation/devicetree/bindings/clock/exynos5250-clock.txt index 536eacd..3d63d09 100644 --- a/Documentation/devicetree/bindings/clock/exynos5250-clock.txt +++ b/Documentation/devicetree/bindings/clock/exynos5250-clock.txt @@ -13,6 +13,24 @@ Required Properties: - #clock-cells: should be 1. +- samsung,armclk-divider-table: when the frequency of the APLL is changed + the divider clocks in CMU_CPU clock domain also need to be updated. These + divider clocks have SoC specific divider clock output requirements for a + specific APLL clock speeds. When APLL clock rate is changed, these divider + clocks are reprogrammed with pre-determined values in order to maintain the + SoC specific divider clock outputs. This property lists the divider values + for divider clocks in the CMU_CPU block for supported APLL clock speeds. + The format of each entry included in the arm-frequency-table should be + as defined below + + cell #1: expected arm clock parent frequency + cell #2 ~ cell #9: value of clock divider in the following order + cpud_ratio, acp_ratio, periph_ratio, atb_ratio, + pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio + +- samsung,armclk-cells: defines the number of cells in + samsung,armclk-divider-table property. The value of this property should be 9. + Each clock is assigned an identifier and client nodes can use this identifier to specify the clock which they consume. @@ -26,6 +44,24 @@ Example 1: An example of a clock controller node is listed below. compatible = "samsung,exynos5250-clock"; reg = <0x10010000 0x30000>; #clock-cells = <1>; + + samsung,armclk-cells = <9>; + samsung,armclk-divider-table = <1700000 3 7 7 7 3 5 0 2>, + <1600000 3 7 7 7 1 4 0 2>, + <1500000 2 7 7 7 1 4 0 2>, + <1400000 2 7 7 6 1 4 0 2>, + <1300000 2 7 7 6 1 3 0 2>, + <1200000 2 7 7 5 1 3 0 2>, + <1100000 3 7 7 5 1 3 0 2>, + <1000000 1 7 7 4 1 2 0 2>, + < 900000 1 7 7 4 1 2 0 2>, + < 800000 1 7 7 4 1 2 0 2>, + < 700000 1 7 7 3 1 1 0 2>, + < 600000 1 7 7 3 1 1 0 2>, + < 500000 1 7 7 2 1 1 0 2>, + < 400000 1 7 7 2 1 1 0 2>, + < 300000 1 7 7 1 1 1 0 2>, + < 200000 1 7 7 1 1 1 0 2>; }; Example 2: UART controller node that consumes the clock generated by the clock
From; Thomas Abraham <thomas.ab@samsung.com> The clock blocks within the CMU_CPU clock domain are put together into a new composite clock type called the cpu clock. This clock type requires configuration data that will be atomically programmed in the multiple clock blocks encapsulated within the cpu clock type when the parent clock frequency is changed. This configuration data is held in the clock controller node. Update clock binding documentation about this configuration data format for Samsung Exynos4 and Exynos5 platforms. Cc: Tomasz Figa <t.figa@samsung.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> Cc: Kumar Gala <galak@codeaurora.org> Cc: <devicetree@vger.kernel.org> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> --- .../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++ .../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++ 2 files changed, 73 insertions(+), 0 deletions(-)