Re: [PATCH 2/2] ARM: dts: enable init rate for clock

[Doug Anderson]

Hi,

On Tue, Oct 7, 2014 at 12:37 PM, Heiko StÃbner <heiko@xxxxxxxxx> wrote:
> Am Dienstag, 7. Oktober 2014, 12:20:38 schrieb Doug Anderson:
>> Heiko,
>>
>> On Tue, Oct 7, 2014 at 11:27 AM, Heiko StÃbner <heiko@xxxxxxxxx> wrote:
>> > Am Dienstag, 7. Oktober 2014, 10:03:19 schrieb Doug Anderson:
>> >> Kever,
>> >>
>> >> On Tue, Oct 7, 2014 at 2:33 AM, Kever Yang <kever.yang@xxxxxxxxxxxxxx>
>> >
>> > wrote:
>> >> > We need to initialize PLL rate and some of bus clock rate while
>> >> > kernel init, for there is no other module will do that.
>> >> >
>> >> > Signed-off-by: Kever Yang <kever.yang@xxxxxxxxxxxxxx>
>> >> > ---
>> >> >
>> >> >  arch/arm/boot/dts/rk3288.dtsi | 10 ++++++++++
>> >> >  1 file changed, 10 insertions(+)
>> >> >
>> >> > diff --git a/arch/arm/boot/dts/rk3288.dtsi
>> >> > b/arch/arm/boot/dts/rk3288.dtsi
>> >> > index 874e66d..2f4519b 100644
>> >> > --- a/arch/arm/boot/dts/rk3288.dtsi
>> >> > +++ b/arch/arm/boot/dts/rk3288.dtsi
>> >> > @@ -455,6 +455,16 @@
>> >> >
>> >> >                 rockchip,grf = <&grf>;
>> >> >                 #clock-cells = <1>;
>> >> >                 #reset-cells = <1>;
>> >> >
>> >> > +               assigned-clocks = <&cru PLL_GPLL>, <&cru PLL_CPLL>,
>> >> > +                                 <&cru PLL_NPLL>, <&cru ACLK_CPU>,
>> >> > +                                 <&cru HCLK_CPU>, <&cru PCLK_CPU>,
>> >> > +                                 <&cru ACLK_PERI>, <&cru HCLK_PERI>,
>> >> > +                                 <&cru PCLK_PERI>;
>> >> > +               assigned-clock-rates = <594000000>, <400000000>,
>> >> > +                                      <500000000>, <300000000>,
>> >>
>> >> When I boot up, I see that ACLK_CPU was 297000000.  You specified
>> >> 300000000.  Did you expect to get 300?  If you expected 297, I think
>> >> you should put 297.  If you expected 300 then we have some debugging
>> >> to do.  Note: I'm not quite sure how you'd expect to get 300 given
>> >> that none of the PLLs divide evenly to 300...
>> >
>> > I'd think 300 is simply the target value. I.e. take the closest rate <=
>> > 300
>> > MHz, same for 150 etc. I somehow like the approach of specifying what the
>> > rate _should_ ideally be :-) .
>> >
>> > Also reduces the amount of thougths necessary (and possible
>> > head-scratching) when adapting the pll rates to some other constraints
>> > (child-clocks already have the target rates and cannot drop to some even
>> > stranger value).
>> OK, fair enough.  I guess maybe I've overly paranoid and like to make
>> sure that someone has thought through exactly what various core clocks
>> should be and made sure that the voltage matched and that clocks were
>> never out of spec.  ...but if others really like specifying more ideal
>> values then I'm OK with it.
>
> Especially as clocks like ACLK_CPU can select from different plls (cpll and
> gpll on the rk3288) it would be even more difficult to predict which path the
> clock selection would take. So it makes sense to specifiy the value we wish for
> and let the common-clock-framework do its magic to select the best sources.
>
>
>>I will say that I'd really like the PLLs
>> to be exact, though.  I think those are now...
>
> agreed. PLL rates should be exact.

OK, fair enough.  Then you can give my:

Reviewed-by: Doug Anderson <dianders@xxxxxxxxxxxx>
Tested-by: Doug Anderson <dianders@xxxxxxxxxxxx>

...of course I'd still like comments addressed for part 1 of this series.  ;)

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/