| Message ID | 1448008952-1787-7-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20.11.2015 09:42, Jisheng Zhang wrote: > Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > index a4a1876..808a997 100644 > --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > @@ -42,6 +42,7 @@ > * OTHER DEALINGS IN THE SOFTWARE. > */ > > +#include <dt-bindings/clock/berlin4ct.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > > / { > @@ -135,6 +136,22 @@ > interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > }; > > + cpupll: cpupll { > + compatible = "marvell,berlin-pll"; > + reg = <0x922000 0x14>, <0xea0710 4>; > + #clock-cells = <0>; > + clocks = <&osc>, <&clk CLK_CPUFASTREF>; > + bypass-shift = /bits/ 8 <2>; > + }; > + > + mempll: mempll { > + compatible = "marvell,berlin-pll"; > + reg = <0x940034 0x14>, <0xea0710 4>; Whenever you see overlapping/repeating reg ranges, e.g. <0xea0710 4> you can be sure you are not representing HW structure but driver structure here. Please merge clocks/gates/plls to a single clock complex node and deal with the internals by using "simple-mfd" and "syscon" regmaps. > + #clock-cells = <0>; > + clocks = <&osc>, <&clk CLK_MEMFASTREF>; > + bypass-shift = /bits/ 8 <1>; > + }; > + > apb@e80000 { > compatible = "simple-bus"; > #address-cells = <1>; > @@ -225,6 +242,27 @@ > }; > }; > > + syspll: syspll { > + compatible = "marvell,berlin-pll"; > + reg = <0xea0200 0x14>, <0xea0710 4>; > + #clock-cells = <0>; > + clocks = <&osc>; > + bypass-shift = /bits/ 8 <0>; > + }; > + > + gateclk: gateclk { > + compatible = "marvell,berlin4ct-gateclk"; > + reg = <0xea0700 4>; > + #clock-cells = <1>; > + }; > + > + clk: clk { > + compatible = "marvell,berlin4ct-clk"; > + reg = <0xea0720 0x144>; Looking at the reg ranges, I'd say that they are all clock related and pretty close to each other: gateclk: reg = <0xea0700 4>; bypass: reg = <0xea0710 4>; clk: reg = <0xea0720 0x144>; So, please just follow the OF/driver structure we already have for Berlin2. Sebastian > + #clock-cells = <1>; > + clocks = <&syspll>; > + }; > + > soc_pinctrl: pin-controller@ea8000 { > compatible = "marvell,berlin4ct-soc-pinctrl"; > reg = <0xea8000 0x14>; >
Dear Sebastian, On Fri, 20 Nov 2015 22:06:59 +0100 Sebastian Hesselbarth wrote: > On 20.11.2015 09:42, Jisheng Zhang wrote: > > Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > --- > > arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > index a4a1876..808a997 100644 > > --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > @@ -42,6 +42,7 @@ > > * OTHER DEALINGS IN THE SOFTWARE. > > */ > > > > +#include <dt-bindings/clock/berlin4ct.h> > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > / { > > @@ -135,6 +136,22 @@ > > interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > > }; > > > > + cpupll: cpupll { > > + compatible = "marvell,berlin-pll"; > > + reg = <0x922000 0x14>, <0xea0710 4>; > > + #clock-cells = <0>; > > + clocks = <&osc>, <&clk CLK_CPUFASTREF>; > > + bypass-shift = /bits/ 8 <2>; > > + }; > > + > > + mempll: mempll { > > + compatible = "marvell,berlin-pll"; > > + reg = <0x940034 0x14>, <0xea0710 4>; > > Whenever you see overlapping/repeating reg ranges, e.g. <0xea0710 4> > you can be sure you are not representing HW structure but driver > structure here. > > Please merge clocks/gates/plls to a single clock complex node > and deal with the internals by using "simple-mfd" and "syscon" regmaps. > > > + #clock-cells = <0>; > > + clocks = <&osc>, <&clk CLK_MEMFASTREF>; > > + bypass-shift = /bits/ 8 <1>; > > + }; > > + > > apb@e80000 { > > compatible = "simple-bus"; > > #address-cells = <1>; > > @@ -225,6 +242,27 @@ > > }; > > }; > > > > + syspll: syspll { > > + compatible = "marvell,berlin-pll"; > > + reg = <0xea0200 0x14>, <0xea0710 4>; > > + #clock-cells = <0>; > > + clocks = <&osc>; > > + bypass-shift = /bits/ 8 <0>; > > + }; > > + > > + gateclk: gateclk { > > + compatible = "marvell,berlin4ct-gateclk"; > > + reg = <0xea0700 4>; > > + #clock-cells = <1>; > > + }; > > + > > + clk: clk { > > + compatible = "marvell,berlin4ct-clk"; > > + reg = <0xea0720 0x144>; > > Looking at the reg ranges, I'd say that they are all clock related > and pretty close to each other: > > gateclk: reg = <0xea0700 4>; > bypass: reg = <0xea0710 4>; > clk: reg = <0xea0720 0x144>; Although these ranges sit close, but we should represent HW structure as you said. First of all, let me describe the clks/plls in BG4CT. BG4CT contains: two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users together. For example: mempll pll registers <0xf7940034, 0x14> is put together with mem controller registers. AVPLL control registers are put with AV devices. You can also check mempll, cpupll and syspll ranges: cpupll: <0x922000 0x14> mempll: <0x940034 0x14> syspll: <0xea0200 0x14> We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use 25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed, its corresponding fastrefclk is directly output to ddrphyclk/cpuclk: ---25MHZ osc----------|\ ________ | |-- syspllclk ---| SYSPLL |---------|/ ---cpufastrefclk------|\ ________ | |-- cpuclk ---| CPUPLL |---------|/ ---memfastrefclk------|\ ________ | |-- ddrphyclk ---| MEMPLL |---------|/ NOTE: the fastrefclk is the so called normal clk below. two kinds of clk: normal clk and gate clk. The normal clk supports changing divider, selecting clock source, disabling/enabling etc. The gate clk only supports disabling/enabling. normal clks use syspllclk as clocksource, while gate clks use perifsysclk as clocksource. So what's the representing HW structure in fact? Here is my proposal: 1. have mempll, cpupll and syspll node in dts 2. one gateclk node in dts for gateclks 3. one normalclk node in dts for normal clks 4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk. what do you think? From another side, let's have a look at driver/clk/mvebu. As can be seen, different clks register are close each other, for example, gateclk and coreclk in arch/arm/boot/dts/armada-xp.dtsi. And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk and ahb*, apb* etc... why these SoCs don't merge clocks/gates/plls to a single clock complex node? I think that's because they are representing real HW structure. Thanks, Jisheng > > So, please just follow the OF/driver structure we already > have for Berlin2. > > Sebastian > > > + #clock-cells = <1>; > > + clocks = <&syspll>; > > + }; > > + > > soc_pinctrl: pin-controller@ea8000 { > > compatible = "marvell,berlin4ct-soc-pinctrl"; > > reg = <0xea8000 0x14>; > > >
Dear all, On Mon, 23 Nov 2015 15:21:58 +0800 Jisheng Zhang wrote: > Dear Sebastian, > > On Fri, 20 Nov 2015 22:06:59 +0100 > Sebastian Hesselbarth wrote: > > > On 20.11.2015 09:42, Jisheng Zhang wrote: > > > Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > > > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > > --- > > > arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++ > > > 1 file changed, 38 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > > index a4a1876..808a997 100644 > > > --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > > +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > > @@ -42,6 +42,7 @@ > > > * OTHER DEALINGS IN THE SOFTWARE. > > > */ > > > > > > +#include <dt-bindings/clock/berlin4ct.h> > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > > / { > > > @@ -135,6 +136,22 @@ > > > interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > > > }; > > > > > > + cpupll: cpupll { > > > + compatible = "marvell,berlin-pll"; > > > + reg = <0x922000 0x14>, <0xea0710 4>; > > > + #clock-cells = <0>; > > > + clocks = <&osc>, <&clk CLK_CPUFASTREF>; > > > + bypass-shift = /bits/ 8 <2>; > > > + }; > > > + > > > + mempll: mempll { > > > + compatible = "marvell,berlin-pll"; > > > + reg = <0x940034 0x14>, <0xea0710 4>; > > > > Whenever you see overlapping/repeating reg ranges, e.g. <0xea0710 4> > > you can be sure you are not representing HW structure but driver > > structure here. > > > > Please merge clocks/gates/plls to a single clock complex node > > and deal with the internals by using "simple-mfd" and "syscon" regmaps. > > > > > + #clock-cells = <0>; > > > + clocks = <&osc>, <&clk CLK_MEMFASTREF>; > > > + bypass-shift = /bits/ 8 <1>; > > > + }; > > > + > > > apb@e80000 { > > > compatible = "simple-bus"; > > > #address-cells = <1>; > > > @@ -225,6 +242,27 @@ > > > }; > > > }; > > > > > > + syspll: syspll { > > > + compatible = "marvell,berlin-pll"; > > > + reg = <0xea0200 0x14>, <0xea0710 4>; > > > + #clock-cells = <0>; > > > + clocks = <&osc>; > > > + bypass-shift = /bits/ 8 <0>; > > > + }; > > > + > > > + gateclk: gateclk { > > > + compatible = "marvell,berlin4ct-gateclk"; > > > + reg = <0xea0700 4>; > > > + #clock-cells = <1>; > > > + }; > > > + > > > + clk: clk { > > > + compatible = "marvell,berlin4ct-clk"; > > > + reg = <0xea0720 0x144>; > > > > Looking at the reg ranges, I'd say that they are all clock related > > and pretty close to each other: > > > > gateclk: reg = <0xea0700 4>; > > bypass: reg = <0xea0710 4>; > > clk: reg = <0xea0720 0x144>; > > Although these ranges sit close, but we should represent HW structure as you > said. > > First of all, let me describe the clks/plls in BG4CT. BG4CT contains: > > two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users > together. For example: mempll pll registers <0xf7940034, 0x14> is put together > with mem controller registers. AVPLL control registers are put with AV devices. > You can also check mempll, cpupll and syspll ranges: > > cpupll: <0x922000 0x14> > > mempll: <0x940034 0x14> > > syspll: <0xea0200 0x14> > > > We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use > 25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed > the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed, > its corresponding fastrefclk is directly output to ddrphyclk/cpuclk: > > > ---25MHZ osc----------|\ > ________ | |-- syspllclk > ---| SYSPLL |---------|/ > > > > ---cpufastrefclk------|\ > ________ | |-- cpuclk > ---| CPUPLL |---------|/ > > > ---memfastrefclk------|\ > ________ | |-- ddrphyclk > ---| MEMPLL |---------|/ > > NOTE: the fastrefclk is the so called normal clk below. > > > > two kinds of clk: normal clk and gate clk. The normal clk supports changing > divider, selecting clock source, disabling/enabling etc. The gate clk only > supports disabling/enabling. normal clks use syspllclk as clocksource, while > gate clks use perifsysclk as clocksource. > I hope I have described the BG4CT HW clk/pll clearly, I really need your advice about my proposal. The clk nodes in my proposal will finally look like: cpupll: cpupll { compatible = "marvell,berlin-pll"; reg = <0x922000 0x14> #clock-cells = <0>; clocks = <&osc>; }; mempll: mempll { compatible = "marvell,berlin-pll"; reg = <0x940034 0x14> #clock-cells = <0>; clocks = <&osc>; }; syspll: syspll { compatible = "marvell,berlin-pll"; reg = <0xea0200 0x14> #clock-cells = <0>; clocks = <&osc>; }; pllclk: pllclk { compatible = "marvell,berlin4ct-pllclk"; reg = <0xea0710 4> #clock-cells = <1>; }; gateclk: gateclk { compatible = "marvell,berlin4ct-gateclk"; reg = <0xea0700 4>; #clock-cells = <1>; }; clk: clk { compatible = "marvell,berlin4ct-clk"; reg = <0xea0720 0x144>; #clock-cells = <1>; clocks = <&pllclk SYSPLLCLK>; }; Using the ccf clk-mux, there's no overlapping/repeating reg ranges any more. If you want more BG4CT clk/pll details, please let me know. Thanks, Jisheng > > So what's the representing HW structure in fact? Here is my proposal: > > 1. have mempll, cpupll and syspll node in dts > > 2. one gateclk node in dts for gateclks > > 3. one normalclk node in dts for normal clks > > 4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk. > > what do you think? > > > From another side, let's have a look at driver/clk/mvebu. As can be seen, > different clks register are close each other, for example, gateclk and coreclk > in arch/arm/boot/dts/armada-xp.dtsi. > > And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk > and ahb*, apb* etc... > > why these SoCs don't merge clocks/gates/plls to a single clock complex node? > I think that's because they are representing real HW structure. > > Thanks, > Jisheng > > > > > > So, please just follow the OF/driver structure we already > > have for Berlin2. > > > > Sebastian > > > > > + #clock-cells = <1>; > > > + clocks = <&syspll>; > > > + }; > > > + > > > soc_pinctrl: pin-controller@ea8000 { > > > compatible = "marvell,berlin4ct-soc-pinctrl"; > > > reg = <0xea8000 0x14>; > > > > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 23.11.2015 08:21, Jisheng Zhang wrote: > On Fri, 20 Nov 2015 22:06:59 +0100 > Sebastian Hesselbarth wrote: >> On 20.11.2015 09:42, Jisheng Zhang wrote: >>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. >>> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> >>> --- [...] >>> + syspll: syspll { >>> + compatible = "marvell,berlin-pll"; >>> + reg = <0xea0200 0x14>, <0xea0710 4>; >>> + #clock-cells = <0>; >>> + clocks = <&osc>; >>> + bypass-shift = /bits/ 8 <0>; >>> + }; >>> + >>> + gateclk: gateclk { >>> + compatible = "marvell,berlin4ct-gateclk"; >>> + reg = <0xea0700 4>; >>> + #clock-cells = <1>; >>> + }; >>> + >>> + clk: clk { >>> + compatible = "marvell,berlin4ct-clk"; >>> + reg = <0xea0720 0x144>; >> >> Looking at the reg ranges, I'd say that they are all clock related >> and pretty close to each other: >> >> gateclk: reg = <0xea0700 4>; >> bypass: reg = <0xea0710 4>; >> clk: reg = <0xea0720 0x144>; > > Although these ranges sit close, but we should represent HW structure as you > said. Then how do you argue that you have to share the gate clock register with every PLL? The answer is quite simple: You are not separating by HW either but existing SW API. If you would really want to just describe the HW, then you'd have to have a single node for _all_ clocks that get controlled by 0xea0700/0x4, feed some 32+ clocks into the node, and out again. Obviously, this isn't what we want, right? So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some SoCs just dump some functions into a bunch of registers with no particular order. We'd never find a good representation for that in DT, so we don't bother to try but let the driver implementation deal with the mess. Using "simple-mfd" is a nice solution to scattered registers please have a look at it and come up with a cleaner separation for bg4 clock. > First of all, let me describe the clks/plls in BG4CT. BG4CT contains: > > two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users > together. For example: mempll pll registers <0xf7940034, 0x14> is put together > with mem controller registers. AVPLL control registers are put with AV devices. Why didn't you choose to have a memory-controller node that provides mempll clock then? I am open to having multiple nodes providing clocks but having a node for every clock in any subsystem is something I'll not even think about. > You can also check mempll, cpupll and syspll ranges: > > cpupll: <0x922000 0x14> > > mempll: <0x940034 0x14> > > syspll: <0xea0200 0x14> > > > We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use > 25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed > the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed, > its corresponding fastrefclk is directly output to ddrphyclk/cpuclk: > > > ---25MHZ osc----------|\ > ________ | |-- syspllclk > ---| SYSPLL |---------|/ > > > > ---cpufastrefclk------|\ > ________ | |-- cpuclk > ---| CPUPLL |---------|/ > > > ---memfastrefclk------|\ > ________ | |-- ddrphyclk > ---| MEMPLL |---------|/ > > NOTE: the fastrefclk is the so called normal clk below. > > > > two kinds of clk: normal clk and gate clk. The normal clk supports changing > divider, selecting clock source, disabling/enabling etc. The gate clk only > supports disabling/enabling. normal clks use syspllclk as clocksource, while > gate clks use perifsysclk as clocksource. > > > So what's the representing HW structure in fact? Here is my proposal: > 1. have mempll, cpupll and syspll node in dts No. > 2. one gateclk node in dts for gateclks No. > 3. one normalclk node in dts for normal clks No. > 4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk. No. > what do you think? I think that the current separation is not a good one. I am open for suggestions but I am not accepting single PLL/clock nodes. > From another side, let's have a look at driver/clk/mvebu. As can be seen, > different clks register are close each other, for example, gateclk and coreclk > in arch/arm/boot/dts/armada-xp.dtsi. > > And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk > and ahb*, apb* etc... > > why these SoCs don't merge clocks/gates/plls to a single clock complex node? > I think that's because they are representing real HW structure. These SoC (at least mvebu) didn't merge them into a single clock complex node because nobody had a better idea or an impression of the consequences. Looking back with the idea of syscon/simple-mfd we probably would have chosen to separate differently. >> So, please just follow the OF/driver structure we already >> have for Berlin2. To repeat: "please just follow the OF/driver structure we already have for Berlin2" Sebastian >>> + #clock-cells = <1>; >>> + clocks = <&syspll>; >>> + }; >>> + >>> soc_pinctrl: pin-controller@ea8000 { >>> compatible = "marvell,berlin4ct-soc-pinctrl"; >>> reg = <0xea8000 0x14>; >>> >> >
On Mon, 23 Nov 2015 09:30:42 +0100 Sebastian Hesselbarth wrote: > On 23.11.2015 08:21, Jisheng Zhang wrote: > > On Fri, 20 Nov 2015 22:06:59 +0100 > > Sebastian Hesselbarth wrote: > >> On 20.11.2015 09:42, Jisheng Zhang wrote: > >>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > >>> > >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > >>> --- > [...] > >>> + syspll: syspll { > >>> + compatible = "marvell,berlin-pll"; > >>> + reg = <0xea0200 0x14>, <0xea0710 4>; > >>> + #clock-cells = <0>; > >>> + clocks = <&osc>; > >>> + bypass-shift = /bits/ 8 <0>; > >>> + }; > >>> + > >>> + gateclk: gateclk { > >>> + compatible = "marvell,berlin4ct-gateclk"; > >>> + reg = <0xea0700 4>; > >>> + #clock-cells = <1>; > >>> + }; > >>> + > >>> + clk: clk { > >>> + compatible = "marvell,berlin4ct-clk"; > >>> + reg = <0xea0720 0x144>; > >> > >> Looking at the reg ranges, I'd say that they are all clock related > >> and pretty close to each other: > >> > >> gateclk: reg = <0xea0700 4>; > >> bypass: reg = <0xea0710 4>; > >> clk: reg = <0xea0720 0x144>; > > > > Although these ranges sit close, but we should represent HW structure as you > > said. > > Then how do you argue that you have to share the gate clock register > with every PLL? The answer is quite simple: You are not separating by > HW either but existing SW API. No, PLLs don't share register any more. You can find what all clock nodes will look like in: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/387322.html > > If you would really want to just describe the HW, then you'd have to > have a single node for _all_ clocks that get controlled by 0xea0700/0x4, > feed some 32+ clocks into the node, and out again. Obviously, this > isn't what we want, right? I represented the HW by "kind", for example, gateclks, common-clks, pllclk. And let common PLLs follow this rule as well: one node for all common plls reg <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> > > So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some > SoCs just dump some functions into a bunch of registers with no > particular order. We'd never find a good representation for that in DT, > so we don't bother to try but let the driver implementation deal with > the mess. Using "simple-mfd" is a nice solution to scattered registers > please have a look at it and come up with a cleaner separation for bg4 > clock. > > > First of all, let me describe the clks/plls in BG4CT. BG4CT contains: > > > > two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users > > together. For example: mempll pll registers <0xf7940034, 0x14> is put together > > with mem controller registers. AVPLL control registers are put with AV devices. > > Why didn't you choose to have a memory-controller node that provides > mempll clock then? I am open to having multiple nodes providing clocks > but having a node for every clock in any subsystem is something I'll > not even think about. OK. As you said, "SoCs just dump some functions into a bunch of registers with no particular order", BG4CT is now cleaner, all common-clks are put together, gate-clks are put together, pllclks are put together, only the common PLLs are dumped here and there. So how about representing the HW by "kind", one node for common plls, one node for gateclks, one node for common clks and one node for pllclks? pll: pll { compatible = "marvell,berlin4ct-pll"; reg = <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> #clock-cells = <0>; clocks = <&osc>; }; pllclk: pllclk { compatible = "marvell,berlin4ct-pllclk"; reg = <0xea0710 4> #clock-cells = <1>; }; gateclk: gateclk { compatible = "marvell,berlin4ct-gateclk"; reg = <0xea0700 4>; #clock-cells = <1>; }; clk: clk { compatible = "marvell,berlin4ct-clk"; reg = <0xea0720 0x144>; #clock-cells = <1>; clocks = <&pllclk SYSPLLCLK>; }; thanks > > > You can also check mempll, cpupll and syspll ranges: > > > > cpupll: <0x922000 0x14> > > > > mempll: <0x940034 0x14> > > > > syspll: <0xea0200 0x14> > > > > > > We have three normal PLLS: cpupll, mempll and syspll. All these three PLLs use > > 25MHZ osc as clocksource. These plls can be bypassed. when syspll is bypassed > > the 25MHZ osc is directly output to syspllclk. When mempll/cpupll is bypassed, > > its corresponding fastrefclk is directly output to ddrphyclk/cpuclk: > > > > > > ---25MHZ osc----------|\ > > ________ | |-- syspllclk > > ---| SYSPLL |---------|/ > > > > > > > > ---cpufastrefclk------|\ > > ________ | |-- cpuclk > > ---| CPUPLL |---------|/ > > > > > > ---memfastrefclk------|\ > > ________ | |-- ddrphyclk > > ---| MEMPLL |---------|/ > > > > NOTE: the fastrefclk is the so called normal clk below. > > > > > > > > two kinds of clk: normal clk and gate clk. The normal clk supports changing > > divider, selecting clock source, disabling/enabling etc. The gate clk only > > supports disabling/enabling. normal clks use syspllclk as clocksource, while > > gate clks use perifsysclk as clocksource. > > > > > > So what's the representing HW structure in fact? Here is my proposal: > > 1. have mempll, cpupll and syspll node in dts > > No. > > > 2. one gateclk node in dts for gateclks > > No. > > > 3. one normalclk node in dts for normal clks > > No. > > > 4. one ccf clock-mux for cpuclk, ddrphyclk and syspllclk. > > No. > > > what do you think? > > I think that the current separation is not a good one. I am open for > suggestions but I am not accepting single PLL/clock nodes. > > > From another side, let's have a look at driver/clk/mvebu. As can be seen, > > different clks register are close each other, for example, gateclk and coreclk > > in arch/arm/boot/dts/armada-xp.dtsi. > > > > And drivers/clk/sunxi, arch/arm/boot/dts/sun7i-a20.dtsi, the pll4, pll12, gt_clk > > and ahb*, apb* etc... > > > > why these SoCs don't merge clocks/gates/plls to a single clock complex node? > > I think that's because they are representing real HW structure. > > These SoC (at least mvebu) didn't merge them into a single clock > complex node because nobody had a better idea or an impression of > the consequences. Looking back with the idea of syscon/simple-mfd > we probably would have chosen to separate differently. > > >> So, please just follow the OF/driver structure we already > >> have for Berlin2. > > To repeat: "please just follow the OF/driver structure we already > have for Berlin2" > > Sebastian > > >>> + #clock-cells = <1>; > >>> + clocks = <&syspll>; > >>> + }; > >>> + > >>> soc_pinctrl: pin-controller@ea8000 { > >>> compatible = "marvell,berlin4ct-soc-pinctrl"; > >>> reg = <0xea8000 0x14>; > >>> > >> > > >
Dear Sebastian, On Mon, 23 Nov 2015 16:54:44 +0800 Jisheng Zhang wrote: > On Mon, 23 Nov 2015 09:30:42 +0100 > Sebastian Hesselbarth wrote: > > > On 23.11.2015 08:21, Jisheng Zhang wrote: > > > On Fri, 20 Nov 2015 22:06:59 +0100 > > > Sebastian Hesselbarth wrote: > > >> On 20.11.2015 09:42, Jisheng Zhang wrote: > > >>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > > >>> > > >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > >>> --- > > [...] > > >>> + syspll: syspll { > > >>> + compatible = "marvell,berlin-pll"; > > >>> + reg = <0xea0200 0x14>, <0xea0710 4>; > > >>> + #clock-cells = <0>; > > >>> + clocks = <&osc>; > > >>> + bypass-shift = /bits/ 8 <0>; > > >>> + }; > > >>> + > > >>> + gateclk: gateclk { > > >>> + compatible = "marvell,berlin4ct-gateclk"; > > >>> + reg = <0xea0700 4>; > > >>> + #clock-cells = <1>; > > >>> + }; > > >>> + > > >>> + clk: clk { > > >>> + compatible = "marvell,berlin4ct-clk"; > > >>> + reg = <0xea0720 0x144>; > > >> > > >> Looking at the reg ranges, I'd say that they are all clock related > > >> and pretty close to each other: > > >> > > >> gateclk: reg = <0xea0700 4>; > > >> bypass: reg = <0xea0710 4>; > > >> clk: reg = <0xea0720 0x144>; > > > > > > Although these ranges sit close, but we should represent HW structure as you > > > said. > > > > Then how do you argue that you have to share the gate clock register > > with every PLL? The answer is quite simple: You are not separating by > > HW either but existing SW API. > > No, PLLs don't share register any more. You can find what all clock nodes will > look like in: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/387322.html > > > > > If you would really want to just describe the HW, then you'd have to > > have a single node for _all_ clocks that get controlled by 0xea0700/0x4, > > feed some 32+ clocks into the node, and out again. Obviously, this > > isn't what we want, right? > > I represented the HW by "kind", for example, gateclks, common-clks, pllclk. > And let common PLLs follow this rule as well: > > one node for all common plls > > reg <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> > > > > > So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some > > SoCs just dump some functions into a bunch of registers with no > > particular order. We'd never find a good representation for that in DT, > > so we don't bother to try but let the driver implementation deal with > > the mess. Using "simple-mfd" is a nice solution to scattered registers > > please have a look at it and come up with a cleaner separation for bg4 > > clock. > > > > > First of all, let me describe the clks/plls in BG4CT. BG4CT contains: > > > > > > two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users > > > together. For example: mempll pll registers <0xf7940034, 0x14> is put together > > > with mem controller registers. AVPLL control registers are put with AV devices. > > > > Why didn't you choose to have a memory-controller node that provides > > mempll clock then? I am open to having multiple nodes providing clocks > > but having a node for every clock in any subsystem is something I'll > > not even think about. > > OK. As you said, "SoCs just dump some functions into a bunch of registers with > no particular order", BG4CT is now cleaner, all common-clks are put together, > gate-clks are put together, pllclks are put together, only the common PLLs > are dumped here and there. So how about representing the HW by "kind", one > node for common plls, one node for gateclks, one node for common clks and > one node for pllclks? > > pll: pll { > compatible = "marvell,berlin4ct-pll"; > reg = <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> > #clock-cells = <0>; should be "#clock-cells = <1>;" > clocks = <&osc>; > }; > > pllclk: pllclk { > compatible = "marvell,berlin4ct-pllclk"; > reg = <0xea0710 4> > #clock-cells = <1>; > }; > > gateclk: gateclk { > compatible = "marvell,berlin4ct-gateclk"; > reg = <0xea0700 4>; > #clock-cells = <1>; > }; > > clk: clk { > compatible = "marvell,berlin4ct-clk"; > reg = <0xea0720 0x144>; > #clock-cells = <1>; > clocks = <&pllclk SYSPLLCLK>; > }; > there's no a node for every clock with this proposal, all clks/plls are separated by "kind". Is this OK for you? thanks
On 24.11.2015 03:35, Jisheng Zhang wrote: > On Mon, 23 Nov 2015 16:54:44 +0800 > Jisheng Zhang wrote: >> On Mon, 23 Nov 2015 09:30:42 +0100 >> Sebastian Hesselbarth wrote: >>> On 23.11.2015 08:21, Jisheng Zhang wrote: >>>> On Fri, 20 Nov 2015 22:06:59 +0100 >>>> Sebastian Hesselbarth wrote: >>>>> On 20.11.2015 09:42, Jisheng Zhang wrote: >>>>>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. >>>>>> >>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> >>>>>> --- >>> [...] >>>>>> + syspll: syspll { >>>>>> + compatible = "marvell,berlin-pll"; >>>>>> + reg = <0xea0200 0x14>, <0xea0710 4>; >>>>>> + #clock-cells = <0>; >>>>>> + clocks = <&osc>; >>>>>> + bypass-shift = /bits/ 8 <0>; >>>>>> + }; >>>>>> + >>>>>> + gateclk: gateclk { >>>>>> + compatible = "marvell,berlin4ct-gateclk"; >>>>>> + reg = <0xea0700 4>; >>>>>> + #clock-cells = <1>; >>>>>> + }; >>>>>> + >>>>>> + clk: clk { >>>>>> + compatible = "marvell,berlin4ct-clk"; >>>>>> + reg = <0xea0720 0x144>; >>>>> >>>>> Looking at the reg ranges, I'd say that they are all clock related >>>>> and pretty close to each other: >>>>> >>>>> gateclk: reg = <0xea0700 4>; >>>>> bypass: reg = <0xea0710 4>; >>>>> clk: reg = <0xea0720 0x144>; >>>> >>>> Although these ranges sit close, but we should represent HW structure as you >>>> said. >>> >>> Then how do you argue that you have to share the gate clock register >>> with every PLL? The answer is quite simple: You are not separating by >>> HW either but existing SW API. >> >> No, PLLs don't share register any more. You can find what all clock nodes will >> look like in: Jisheng, referring to the sunxi clock related thread, I am glad you finally picked up the idea of merging clock nodes. Before you start reworking bg4 clocks, I think I should be a little bit more clear about what I expect to be the outcome. When I said "one single clock complex node", I was referring to the clocks located within the system-controller reg region, i.e. those at 0xea0000. With bg2x we came to the conclusion that those registers cannot be not cleanly separated by functions, so we decided to have a single system-controller simple-mfd node with sub-nodes for pinctrl, clock, reset, and whatever we will find there in the future. Please also follow this scheme for bg4 because when you start looking at reset, you'll likely see the same issues we were facing: Either you have a reset controller node with a plethora of reg property entries or you constantly undermine the concept of requested resources by using of_iomap(). Using simple-mfd is a compromise between detailed HW description and usability. It will also automatically deal with concurrent accesses to the same register for e.g. clock and reset because simple-mfd and syscon install an access lock for the reg region. Even though there may be no real concurrent accesses to the same register, it does no real harm because we are locking resisters that aren't supposed to be used in high-speed applications. Some of them are touched once at boot, most of them are never touched by Linux at all. For the other PLLs at <0x922000 0x14> and <0x940034 0x14>, I do accept that they are not part of the system-controller sub-node. For the short run, I would accept separate nodes for the PLLs alone, but on the long run they should be hidden within the functional node they belong to, i.e. mempll should be a clock provided by some memory-controller node. As soon as you look at power saving modes for the memory-controller, you would need access to memory-controller register _and_ mempll anyway. We do have our DT tagged unstable, so we still can easily revert our limited view of some nodes later. BTW, if the clock provided by mempll is used to generate peripheral clocks that are dealt with in the sysctrl clock complex, you should, of course, expose that relation in DT, e.g.: sysctrl: system-controller { reg = <0xea0700 0xfoo>; sysclk: clock { #clock-cells = <1>; clocks = <&osc>, <&memctrl 0>; clock-names = "osc", "mempll"; }; }; memctrl: memory-controller { reg = <0x920000 0xbar>; /* * clock-cells can also be 0 * if there is no other clock provided */ #clock-cells = <1>; clocks = <&osc>; clock-names = "osc"; }; some-peripheral: bla { clocks = <&sysclk SOME_PERIPHERAL_CLOCK_ID>; }; Sebastian >>> If you would really want to just describe the HW, then you'd have to >>> have a single node for _all_ clocks that get controlled by 0xea0700/0x4, >>> feed some 32+ clocks into the node, and out again. Obviously, this >>> isn't what we want, right? >> >> I represented the HW by "kind", for example, gateclks, common-clks, pllclk. >> And let common PLLs follow this rule as well: >> >> one node for all common plls >> >> reg <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> >> >>> >>> So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some >>> SoCs just dump some functions into a bunch of registers with no >>> particular order. We'd never find a good representation for that in DT, >>> so we don't bother to try but let the driver implementation deal with >>> the mess. Using "simple-mfd" is a nice solution to scattered registers >>> please have a look at it and come up with a cleaner separation for bg4 >>> clock. >>> >>>> First of all, let me describe the clks/plls in BG4CT. BG4CT contains: >>>> >>>> two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users >>>> together. For example: mempll pll registers <0xf7940034, 0x14> is put together >>>> with mem controller registers. AVPLL control registers are put with AV devices. >>> >>> Why didn't you choose to have a memory-controller node that provides >>> mempll clock then? I am open to having multiple nodes providing clocks >>> but having a node for every clock in any subsystem is something I'll >>> not even think about. >> >> OK. As you said, "SoCs just dump some functions into a bunch of registers with >> no particular order", BG4CT is now cleaner, all common-clks are put together, >> gate-clks are put together, pllclks are put together, only the common PLLs >> are dumped here and there. So how about representing the HW by "kind", one >> node for common plls, one node for gateclks, one node for common clks and >> one node for pllclks? >> >> pll: pll { >> compatible = "marvell,berlin4ct-pll"; >> reg = <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> >> #clock-cells = <0>; > > should be "#clock-cells = <1>;" > >> clocks = <&osc>; >> }; >> >> pllclk: pllclk { >> compatible = "marvell,berlin4ct-pllclk"; >> reg = <0xea0710 4> >> #clock-cells = <1>; >> }; >> >> gateclk: gateclk { >> compatible = "marvell,berlin4ct-gateclk"; >> reg = <0xea0700 4>; >> #clock-cells = <1>; >> }; >> >> clk: clk { >> compatible = "marvell,berlin4ct-clk"; >> reg = <0xea0720 0x144>; >> #clock-cells = <1>; >> clocks = <&pllclk SYSPLLCLK>; >> }; >> > > there's no a node for every clock with this proposal, all clks/plls are separated > by "kind". Is this OK for you? > > thanks >
On Fri, 27 Nov 2015 08:51:27 +0100 Sebastian Hesselbarth wrote: > On 24.11.2015 03:35, Jisheng Zhang wrote: > > On Mon, 23 Nov 2015 16:54:44 +0800 > > Jisheng Zhang wrote: > >> On Mon, 23 Nov 2015 09:30:42 +0100 > >> Sebastian Hesselbarth wrote: > >>> On 23.11.2015 08:21, Jisheng Zhang wrote: > >>>> On Fri, 20 Nov 2015 22:06:59 +0100 > >>>> Sebastian Hesselbarth wrote: > >>>>> On 20.11.2015 09:42, Jisheng Zhang wrote: > >>>>>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > >>>>>> > >>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > >>>>>> --- > >>> [...] > >>>>>> + syspll: syspll { > >>>>>> + compatible = "marvell,berlin-pll"; > >>>>>> + reg = <0xea0200 0x14>, <0xea0710 4>; > >>>>>> + #clock-cells = <0>; > >>>>>> + clocks = <&osc>; > >>>>>> + bypass-shift = /bits/ 8 <0>; > >>>>>> + }; > >>>>>> + > >>>>>> + gateclk: gateclk { > >>>>>> + compatible = "marvell,berlin4ct-gateclk"; > >>>>>> + reg = <0xea0700 4>; > >>>>>> + #clock-cells = <1>; > >>>>>> + }; > >>>>>> + > >>>>>> + clk: clk { > >>>>>> + compatible = "marvell,berlin4ct-clk"; > >>>>>> + reg = <0xea0720 0x144>; > >>>>> > >>>>> Looking at the reg ranges, I'd say that they are all clock related > >>>>> and pretty close to each other: > >>>>> > >>>>> gateclk: reg = <0xea0700 4>; > >>>>> bypass: reg = <0xea0710 4>; > >>>>> clk: reg = <0xea0720 0x144>; > >>>> > >>>> Although these ranges sit close, but we should represent HW structure as you > >>>> said. > >>> > >>> Then how do you argue that you have to share the gate clock register > >>> with every PLL? The answer is quite simple: You are not separating by > >>> HW either but existing SW API. > >> > >> No, PLLs don't share register any more. You can find what all clock nodes will > >> look like in: > > Jisheng, > > referring to the sunxi clock related thread, I am glad you finally > picked up the idea of merging clock nodes. Before you start reworking > bg4 clocks, I think I should be a little bit more clear about what I > expect to be the outcome. > > When I said "one single clock complex node", I was referring to the > clocks located within the system-controller reg region, i.e. those at > 0xea0000. With bg2x we came to the conclusion that those registers > cannot be not cleanly separated by functions, so we decided to have > a single system-controller simple-mfd node with sub-nodes for > pinctrl, clock, reset, and whatever we will find there in the future. > > Please also follow this scheme for bg4 because when you start looking > at reset, you'll likely see the same issues we were facing: Either you > have a reset controller node with a plethora of reg property entries > or you constantly undermine the concept of requested resources by using > of_iomap(). > > Using simple-mfd is a compromise between detailed HW description and > usability. It will also automatically deal with concurrent accesses to > the same register for e.g. clock and reset because simple-mfd and syscon > install an access lock for the reg region. Even though there may be no > real concurrent accesses to the same register, it does no real harm > because we are locking resisters that aren't supposed to be used in > high-speed applications. Some of them are touched once at boot, most > of them are never touched by Linux at all. Thank you so much for the detailed information. It do help me to have a better understanding why. > > For the other PLLs at <0x922000 0x14> and <0x940034 0x14>, I do accept > that they are not part of the system-controller sub-node. For the short > run, I would accept separate nodes for the PLLs alone, but on the long > run they should be hidden within the functional node they belong to, > i.e. mempll should be a clock provided by some memory-controller node. > As soon as you look at power saving modes for the memory-controller, > you would need access to memory-controller register _and_ mempll anyway. > > We do have our DT tagged unstable, so we still can easily revert our > limited view of some nodes later. > > BTW, if the clock provided by mempll is used to generate peripheral > clocks that are dealt with in the sysctrl clock complex, you should, mempll is only for ddrphy clk. So we are lucky ;) Thanks, Jisheng > of course, expose that relation in DT, e.g.: > > sysctrl: system-controller { > reg = <0xea0700 0xfoo>; > > sysclk: clock { > #clock-cells = <1>; > clocks = <&osc>, <&memctrl 0>; > clock-names = "osc", "mempll"; > }; > }; > > memctrl: memory-controller { > reg = <0x920000 0xbar>; > /* > * clock-cells can also be 0 > * if there is no other clock provided > */ > #clock-cells = <1>; > > clocks = <&osc>; > clock-names = "osc"; > }; > > some-peripheral: bla { > clocks = <&sysclk SOME_PERIPHERAL_CLOCK_ID>; > }; > > Sebastian >
On Fri, 27 Nov 2015 16:39:37 +0800 Jisheng Zhang wrote: > On Fri, 27 Nov 2015 08:51:27 +0100 > Sebastian Hesselbarth wrote: > > > On 24.11.2015 03:35, Jisheng Zhang wrote: > > > On Mon, 23 Nov 2015 16:54:44 +0800 > > > Jisheng Zhang wrote: > > >> On Mon, 23 Nov 2015 09:30:42 +0100 > > >> Sebastian Hesselbarth wrote: > > >>> On 23.11.2015 08:21, Jisheng Zhang wrote: > > >>>> On Fri, 20 Nov 2015 22:06:59 +0100 > > >>>> Sebastian Hesselbarth wrote: > > >>>>> On 20.11.2015 09:42, Jisheng Zhang wrote: > > >>>>>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > > >>>>>> > > >>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > >>>>>> --- > > >>> [...] > > >>>>>> + syspll: syspll { > > >>>>>> + compatible = "marvell,berlin-pll"; > > >>>>>> + reg = <0xea0200 0x14>, <0xea0710 4>; > > >>>>>> + #clock-cells = <0>; > > >>>>>> + clocks = <&osc>; > > >>>>>> + bypass-shift = /bits/ 8 <0>; > > >>>>>> + }; > > >>>>>> + > > >>>>>> + gateclk: gateclk { > > >>>>>> + compatible = "marvell,berlin4ct-gateclk"; > > >>>>>> + reg = <0xea0700 4>; > > >>>>>> + #clock-cells = <1>; > > >>>>>> + }; > > >>>>>> + > > >>>>>> + clk: clk { > > >>>>>> + compatible = "marvell,berlin4ct-clk"; > > >>>>>> + reg = <0xea0720 0x144>; > > >>>>> > > >>>>> Looking at the reg ranges, I'd say that they are all clock related > > >>>>> and pretty close to each other: > > >>>>> > > >>>>> gateclk: reg = <0xea0700 4>; > > >>>>> bypass: reg = <0xea0710 4>; > > >>>>> clk: reg = <0xea0720 0x144>; > > >>>> > > >>>> Although these ranges sit close, but we should represent HW structure as you > > >>>> said. > > >>> > > >>> Then how do you argue that you have to share the gate clock register > > >>> with every PLL? The answer is quite simple: You are not separating by > > >>> HW either but existing SW API. > > >> > > >> No, PLLs don't share register any more. You can find what all clock nodes will > > >> look like in: > > > > Jisheng, > > > > referring to the sunxi clock related thread, I am glad you finally > > picked up the idea of merging clock nodes. Before you start reworking > > bg4 clocks, I think I should be a little bit more clear about what I > > expect to be the outcome. > > > > When I said "one single clock complex node", I was referring to the > > clocks located within the system-controller reg region, i.e. those at > > 0xea0000. With bg2x we came to the conclusion that those registers > > cannot be not cleanly separated by functions, so we decided to have > > a single system-controller simple-mfd node with sub-nodes for > > pinctrl, clock, reset, and whatever we will find there in the future. > > > > Please also follow this scheme for bg4 because when you start looking > > at reset, you'll likely see the same issues we were facing: Either you > > have a reset controller node with a plethora of reg property entries > > or you constantly undermine the concept of requested resources by using > > of_iomap(). > > > > Using simple-mfd is a compromise between detailed HW description and > > usability. It will also automatically deal with concurrent accesses to > > the same register for e.g. clock and reset because simple-mfd and syscon > > install an access lock for the reg region. Even though there may be no > > real concurrent accesses to the same register, it does no real harm > > because we are locking resisters that aren't supposed to be used in > > high-speed applications. Some of them are touched once at boot, most > > of them are never touched by Linux at all. > > Thank you so much for the detailed information. It do help me to have > a better understanding why. > > > > > For the other PLLs at <0x922000 0x14> and <0x940034 0x14>, I do accept > > that they are not part of the system-controller sub-node. For the short > > run, I would accept separate nodes for the PLLs alone, but on the long > > run they should be hidden within the functional node they belong to, > > i.e. mempll should be a clock provided by some memory-controller node. > > As soon as you look at power saving modes for the memory-controller, > > you would need access to memory-controller register _and_ mempll anyway. > > > > We do have our DT tagged unstable, so we still can easily revert our > > limited view of some nodes later. > > > > BTW, if the clock provided by mempll is used to generate peripheral > > clocks that are dealt with in the sysctrl clock complex, you should, > > mempll is only for ddrphy clk. So we are lucky ;) oops, no, we are not lucky. mempll and memfastrefclk are clk-muxed to ddrphy clk which is dealt within the complex big clock block. So is for cpupll, cpufastrefclk But it's not that hard to add this support in the code. Thanks for the example, I do need that.... > > Thanks, > Jisheng > > > of course, expose that relation in DT, e.g.: > > > > sysctrl: system-controller { > > reg = <0xea0700 0xfoo>; > > > > sysclk: clock { > > #clock-cells = <1>; > > clocks = <&osc>, <&memctrl 0>; > > clock-names = "osc", "mempll"; > > }; > > }; > > > > memctrl: memory-controller { > > reg = <0x920000 0xbar>; > > /* > > * clock-cells can also be 0 > > * if there is no other clock provided > > */ > > #clock-cells = <1>; > > > > clocks = <&osc>; > > clock-names = "osc"; > > }; > > > > some-peripheral: bla { > > clocks = <&sysclk SOME_PERIPHERAL_CLOCK_ID>; > > }; > > > > Sebastian > >
diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi index a4a1876..808a997 100644 --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi @@ -42,6 +42,7 @@ * OTHER DEALINGS IN THE SOFTWARE. */ +#include <dt-bindings/clock/berlin4ct.h> #include <dt-bindings/interrupt-controller/arm-gic.h> / { @@ -135,6 +136,22 @@ interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; }; + cpupll: cpupll { + compatible = "marvell,berlin-pll"; + reg = <0x922000 0x14>, <0xea0710 4>; + #clock-cells = <0>; + clocks = <&osc>, <&clk CLK_CPUFASTREF>; + bypass-shift = /bits/ 8 <2>; + }; + + mempll: mempll { + compatible = "marvell,berlin-pll"; + reg = <0x940034 0x14>, <0xea0710 4>; + #clock-cells = <0>; + clocks = <&osc>, <&clk CLK_MEMFASTREF>; + bypass-shift = /bits/ 8 <1>; + }; + apb@e80000 { compatible = "simple-bus"; #address-cells = <1>; @@ -225,6 +242,27 @@ }; }; + syspll: syspll { + compatible = "marvell,berlin-pll"; + reg = <0xea0200 0x14>, <0xea0710 4>; + #clock-cells = <0>; + clocks = <&osc>; + bypass-shift = /bits/ 8 <0>; + }; + + gateclk: gateclk { + compatible = "marvell,berlin4ct-gateclk"; + reg = <0xea0700 4>; + #clock-cells = <1>; + }; + + clk: clk { + compatible = "marvell,berlin4ct-clk"; + reg = <0xea0720 0x144>; + #clock-cells = <1>; + clocks = <&syspll>; + }; + soc_pinctrl: pin-controller@ea8000 { compatible = "marvell,berlin4ct-soc-pinctrl"; reg = <0xea8000 0x14>;
Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 38 ++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)