Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1472036 > unrolled thread

[PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399

Back to article view | Back to linux.kernel

Contents

  [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 Douglas Anderson <dianders@chromium.org> - 2016-08-29 20:20 +0200
    Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on  rk3399 Brian Norris <briannorris@chromium.org> - 2016-08-29 20:20 +0200
      Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on  rk3399 Elaine Zhang <zhangqing@rock-chips.com> - 2016-08-30 03:10 +0200
        Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 Heiko Stübner <heiko@sntech.de> - 2016-08-30 09:10 +0200
          Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on  rk3399 Brian Norris <briannorris@chromium.org> - 2016-08-30 19:10 +0200
            Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399 Doug Anderson <dianders@chromium.org> - 2016-08-30 22:20 +0200

#1472036 — [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399

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

[toc] | [next] | [standalone]

#1472040 — Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399

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
> 

[toc] | [prev] | [next] | [standalone]

#1472186 — Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399

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

[toc] | [prev] | [next] | [standalone]

#1472266

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

[toc] | [prev] | [next] | [standalone]

#1472659 — Re: [PATCH] arm64: dts: rockchip: Explicitly set pclk_pmu_src on rk3399

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

[toc] | [prev] | [next] | [standalone]

#1472837

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web