* [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 @ 2016-08-29 18:11 Douglas Anderson 2016-08-29 18:18 ` Brian Norris 0 siblings, 1 reply; 6+ messages in thread From: Douglas Anderson @ 2016-08-29 18:11 UTC (permalink / raw) To: Heiko Stuebner Cc: briannorris, linux-rockchip, zhengxing, Douglas Anderson, robh+dt, mark.rutland, catalin.marinas, will.deacon, wxt, jay.xu, zhangqing, david.wu, yamada.masahiro, devicetree, linux-arm-kernel, linux-kernel On rk3399 we explicitly set ppll in the device tree to 676000000. The ppll has one major child, pclk_pmu_src, that is the parent of lots of other clocks. Right now nobody is setting that clock rate and we're relying on the divider to just happen to be something sane. Let's be explicit in our request so we're not relying on the firmware. With the current firmware I tested with this patch has no expected impact but it's probably good to do anyway. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- arch/arm64/boot/dts/rockchip/rk3399.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index 62d450935a57..ffb3faa8c176 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi @@ -908,8 +908,8 @@ reg = <0x0 0xff750000 0x0 0x1000>; #clock-cells = <1>; #reset-cells = <1>; - assigned-clocks = <&pmucru PLL_PPLL>; - assigned-clock-rates = <676000000>; + assigned-clocks = <&pmucru PLL_PPLL>, <&pmucru PCLK_SRC_PMU>; + assigned-clock-rates = <676000000>, <112666667>; }; cru: clock-controller@ff760000 { -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 2016-08-29 18:11 [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 Douglas Anderson @ 2016-08-29 18:18 ` Brian Norris 2016-08-30 0:59 ` Elaine Zhang 0 siblings, 1 reply; 6+ messages in thread From: Brian Norris @ 2016-08-29 18:18 UTC (permalink / raw) To: Douglas Anderson Cc: Heiko Stuebner, linux-rockchip, zhengxing, robh+dt, mark.rutland, catalin.marinas, will.deacon, wxt, jay.xu, zhangqing, david.wu, yamada.masahiro, devicetree, linux-arm-kernel, linux-kernel On Mon, Aug 29, 2016 at 11:11:24AM -0700, Doug Anderson wrote: > On rk3399 we explicitly set ppll in the device tree to 676000000. The > ppll has one major child, pclk_pmu_src, that is the parent of lots of > other clocks. Right now nobody is setting that clock rate and we're > relying on the divider to just happen to be something sane. Let's be > explicit in our request so we're not relying on the firmware. > > With the current firmware I tested with this patch has no expected > impact but it's probably good to do anyway. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > arch/arm64/boot/dts/rockchip/rk3399.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > index 62d450935a57..ffb3faa8c176 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > @@ -908,8 +908,8 @@ > reg = <0x0 0xff750000 0x0 0x1000>; > #clock-cells = <1>; > #reset-cells = <1>; > - assigned-clocks = <&pmucru PLL_PPLL>; > - assigned-clock-rates = <676000000>; > + assigned-clocks = <&pmucru PLL_PPLL>, <&pmucru PCLK_SRC_PMU>; > + assigned-clock-rates = <676000000>, <112666667>; I think this makes sense and is a good idea. One alternative would be to have the various children actually set a rate that they expect, but several of them don't have a separate driver at all, and that would be of dubious value anyway I think. Reviewed-by: Brian Norris <briannorris@chromium.org> > }; > > cru: clock-controller@ff760000 { > -- > 2.8.0.rc3.226.g39d4020 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 2016-08-29 18:18 ` Brian Norris @ 2016-08-30 0:59 ` Elaine Zhang 2016-08-30 7:05 ` Heiko Stübner 0 siblings, 1 reply; 6+ messages in thread From: Elaine Zhang @ 2016-08-30 0:59 UTC (permalink / raw) To: Brian Norris, Douglas Anderson Cc: Heiko Stuebner, linux-rockchip, zhengxing, robh+dt, mark.rutland, catalin.marinas, will.deacon, wxt, jay.xu, david.wu, yamada.masahiro, devicetree, linux-arm-kernel, linux-kernel On 08/30/2016 02:18 AM, Brian Norris wrote: > On Mon, Aug 29, 2016 at 11:11:24AM -0700, Doug Anderson wrote: >> On rk3399 we explicitly set ppll in the device tree to 676000000. The >> ppll has one major child, pclk_pmu_src, that is the parent of lots of >> other clocks. Right now nobody is setting that clock rate and we're >> relying on the divider to just happen to be something sane. Let's be >> explicit in our request so we're not relying on the firmware. >> >> With the current firmware I tested with this patch has no expected >> impact but it's probably good to do anyway. >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- >> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> index 62d450935a57..ffb3faa8c176 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> @@ -908,8 +908,8 @@ >> reg = <0x0 0xff750000 0x0 0x1000>; >> #clock-cells = <1>; >> #reset-cells = <1>; >> - assigned-clocks = <&pmucru PLL_PPLL>; >> - assigned-clock-rates = <676000000>; >> + assigned-clocks = <&pmucru PLL_PPLL>, <&pmucru PCLK_SRC_PMU>; >> + assigned-clock-rates = <676000000>, <112666667>; > > I think this makes sense and is a good idea. One alternative would be to > have the various children actually set a rate that they expect, but > several of them don't have a separate driver at all, and that would be > of dubious value anyway I think. I agree with you. This clk default div is set in the uboot or coreboot. And if is need to set in kernel ,I hope the freq is 50M(<48285714>). This freq can meet the performance,and the power consumption is not too much. > > Reviewed-by: Brian Norris <briannorris@chromium.org> > >> }; >> >> cru: clock-controller@ff760000 { >> -- >> 2.8.0.rc3.226.g39d4020 >> > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 2016-08-30 0:59 ` Elaine Zhang @ 2016-08-30 7:05 ` Heiko Stübner 2016-08-30 17:06 ` Brian Norris 0 siblings, 1 reply; 6+ messages in thread From: Heiko Stübner @ 2016-08-30 7:05 UTC (permalink / raw) To: Elaine Zhang Cc: Brian Norris, Douglas Anderson, linux-rockchip, zhengxing, robh+dt, mark.rutland, catalin.marinas, will.deacon, wxt, jay.xu, david.wu, yamada.masahiro, devicetree, linux-arm-kernel, linux-kernel Hi Elaine, Am Dienstag, 30. August 2016, 08:59:31 schrieb Elaine Zhang: > On 08/30/2016 02:18 AM, Brian Norris wrote: > > On Mon, Aug 29, 2016 at 11:11:24AM -0700, Doug Anderson wrote: > >> On rk3399 we explicitly set ppll in the device tree to 676000000. The > >> ppll has one major child, pclk_pmu_src, that is the parent of lots of > >> other clocks. Right now nobody is setting that clock rate and we're > >> relying on the divider to just happen to be something sane. Let's be > >> explicit in our request so we're not relying on the firmware. > >> > >> With the current firmware I tested with this patch has no expected > >> impact but it's probably good to do anyway. > >> > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > >> --- > >> > >> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index > >> 62d450935a57..ffb3faa8c176 100644 > >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> @@ -908,8 +908,8 @@ > >> > >> reg = <0x0 0xff750000 0x0 0x1000>; > >> #clock-cells = <1>; > >> #reset-cells = <1>; > >> > >> - assigned-clocks = <&pmucru PLL_PPLL>; > >> - assigned-clock-rates = <676000000>; > >> + assigned-clocks = <&pmucru PLL_PPLL>, <&pmucru PCLK_SRC_PMU>; > >> + assigned-clock-rates = <676000000>, <112666667>; > > > > I think this makes sense and is a good idea. One alternative would be to > > have the various children actually set a rate that they expect, but > > several of them don't have a separate driver at all, and that would be > > of dubious value anyway I think. > > I agree with you. This clk default div is set in the uboot or coreboot. > And if is need to set in kernel ,I hope the freq is 50M(<48285714>). > This freq can meet the performance,and the power consumption is not too > much. can you maybe also provide a tag like the one Brian did below. Your sentence above indicates that you reviewed and approve, but it's helpful to also state that explicitly :-) > > Reviewed-by: Brian Norris <briannorris@chromium.org> Thanks Heiko > > > >> }; > >> > >> cru: clock-controller@ff760000 { > >> > >> -- > >> 2.8.0.rc3.226.g39d4020 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 2016-08-30 7:05 ` Heiko Stübner @ 2016-08-30 17:06 ` Brian Norris 2016-08-30 20:17 ` Doug Anderson 0 siblings, 1 reply; 6+ messages in thread From: Brian Norris @ 2016-08-30 17:06 UTC (permalink / raw) To: Heiko Stübner Cc: Elaine Zhang, Douglas Anderson, linux-rockchip, zhengxing, robh+dt, mark.rutland, catalin.marinas, will.deacon, wxt, jay.xu, david.wu, yamada.masahiro, devicetree, linux-arm-kernel, linux-kernel On Tue, Aug 30, 2016 at 09:05:06AM +0200, Heiko Stuebner wrote: > Am Dienstag, 30. August 2016, 08:59:31 schrieb Elaine Zhang: > > On 08/30/2016 02:18 AM, Brian Norris wrote: > > > On Mon, Aug 29, 2016 at 11:11:24AM -0700, Doug Anderson wrote: > > >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > > >> @@ -908,8 +908,8 @@ > > >> > > >> reg = <0x0 0xff750000 0x0 0x1000>; > > >> #clock-cells = <1>; > > >> #reset-cells = <1>; > > >> > > >> - assigned-clocks = <&pmucru PLL_PPLL>; > > >> - assigned-clock-rates = <676000000>; > > >> + assigned-clocks = <&pmucru PLL_PPLL>, <&pmucru PCLK_SRC_PMU>; > > >> + assigned-clock-rates = <676000000>, <112666667>; > > > > > > I think this makes sense and is a good idea. One alternative would be to > > > have the various children actually set a rate that they expect, but > > > several of them don't have a separate driver at all, and that would be > > > of dubious value anyway I think. > > > > I agree with you. This clk default div is set in the uboot or coreboot. > > And if is need to set in kernel ,I hope the freq is 50M(<48285714>). > > This freq can meet the performance,and the power consumption is not too > > much. > > can you maybe also provide a tag like the one Brian did below. Your sentence > above indicates that you reviewed and approve, but it's helpful to also state > that explicitly :-) If I understand Elaine correctly, that's not actually a full agreement; it looks like a suggestion to change that from 112 MHz to 48.2 MHz. I haven't tested that out personally yet, but if that's a formal recommendation from Rockchip, we'd like to know more about it :) Brian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 2016-08-30 17:06 ` Brian Norris @ 2016-08-30 20:17 ` Doug Anderson 0 siblings, 0 replies; 6+ messages in thread From: Doug Anderson @ 2016-08-30 20:17 UTC (permalink / raw) To: Brian Norris Cc: Heiko Stübner, Elaine Zhang, open list:ARM/Rockchip SoC..., Xing Zheng, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, Caesar Wang, Xu Jianqun, David Wu, yamada.masahiro, devicetree, linux-arm-kernel, linux-kernel Hi, On Tue, Aug 30, 2016 at 10:06 AM, Brian Norris <briannorris@chromium.org> wrote: > On Tue, Aug 30, 2016 at 09:05:06AM +0200, Heiko Stuebner wrote: >> Am Dienstag, 30. August 2016, 08:59:31 schrieb Elaine Zhang: >> > On 08/30/2016 02:18 AM, Brian Norris wrote: >> > > On Mon, Aug 29, 2016 at 11:11:24AM -0700, Doug Anderson wrote: >> > >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> > >> @@ -908,8 +908,8 @@ >> > >> >> > >> reg = <0x0 0xff750000 0x0 0x1000>; >> > >> #clock-cells = <1>; >> > >> #reset-cells = <1>; >> > >> >> > >> - assigned-clocks = <&pmucru PLL_PPLL>; >> > >> - assigned-clock-rates = <676000000>; >> > >> + assigned-clocks = <&pmucru PLL_PPLL>, <&pmucru PCLK_SRC_PMU>; >> > >> + assigned-clock-rates = <676000000>, <112666667>; >> > > >> > > I think this makes sense and is a good idea. One alternative would be to >> > > have the various children actually set a rate that they expect, but >> > > several of them don't have a separate driver at all, and that would be >> > > of dubious value anyway I think. >> > >> > I agree with you. This clk default div is set in the uboot or coreboot. >> > And if is need to set in kernel ,I hope the freq is 50M(<48285714>). >> > This freq can meet the performance,and the power consumption is not too >> > much. >> >> can you maybe also provide a tag like the one Brian did below. Your sentence >> above indicates that you reviewed and approve, but it's helpful to also state >> that explicitly :-) Also, I actually realized that I may want to NAK my own patch and possibly want to suggest doing the opposite: removing the set of "PPLL" from the dtsi and moving it into the board .dts files for any boards that need it. Specifically on out rk3399 boards we use PWM regulators for all the major rails in the system. The firmware inits the PWMs to give the system a good voltage and then boots the kernel. The kernel reads the PWMs to find out what the boot voltage was. That whole scheme only works if the PWM's clock doesn't get mucked with at bootup. ...and the PWM's clock is derived from "pclk_pmu_src" which comes from PPLL. So it's actually very important that we _don't_ mess with whatever the firmware set here and that the firmware set something sane. > If I understand Elaine correctly, that's not actually a full agreement; > it looks like a suggestion to change that from 112 MHz to 48.2 MHz. I > haven't tested that out personally yet, but if that's a formal > recommendation from Rockchip, we'd like to know more about it :) Yes, if this is a good idea we should update our firmware to do it. -Doug ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-30 20:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-29 18:11 [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 Douglas Anderson 2016-08-29 18:18 ` Brian Norris 2016-08-30 0:59 ` Elaine Zhang 2016-08-30 7:05 ` Heiko Stübner 2016-08-30 17:06 ` Brian Norris 2016-08-30 20:17 ` Doug Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).