Re: [PATCH v2 01/31] arm64: dts: qcom: msm8916: correct sleep clock frequency

[Stephan Gerhold]

On Sat, Nov 30, 2024 at 12:42:24PM +0200, Dmitry Baryshkov wrote:
> On Sat, Nov 30, 2024 at 11:21:56AM +0100, Stephan Gerhold wrote:
> > On Sat, Nov 30, 2024 at 03:44:13AM +0200, Dmitry Baryshkov wrote:
> > > The MSM8916 platform uses PM8916 to provide sleep clock. According to the
> > > documentation, that clock has 32.7645 kHz frequency. Correct the sleep
> > > clock definition.
> > >
> > > Fixes: f4fb6aeafaaa ("arm64: dts: qcom: msm8916: Add fixed rate on-board oscillators")
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >
> > Thanks for spotting this! This fix looks good independent of the more
> > controversial "arm64: dts: qcom: move board clocks to SoC DTSI files"
> > changes. Maybe move these to a separate series?
> >
> > > ---
> > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > index 5e558bcc9d87893486352e5e211f131d4a1f67e5..8f35c9af18782aa1da7089988692e6588c4b7c5d 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > @@ -125,7 +125,7 @@ xo_board: xo-board {
> > > 		sleep_clk: sleep-clk {
> > > 			compatible = "fixed-clock";
> > > 			#clock-cells = <0>;
> > > -			clock-frequency = <32768>;
> > > +			clock-frequency = <32764>;
> >
> > To be precise the PM8916 specification says the sleep clock is "The 19.2
> > MHz XO divided by 586". Maybe we can actually describe it that way with
> > a fixed-factor-clock?
> >
> > 		sleep_clk: sleep-clk {
> > 			compatible = "fixed-factor-clock";
> > 			clocks = <&xo_board>;
> > 			#clock-cells = <0>;
> > 			clock-div = <586>;
> > 			clock-mult = <1>;
> > 		};
>
> I thought about it, but then it's also not complete truth (at least for
> some of PMICs, don't remember if that's the case for PM8916): there is
> an external XO and also there is an on-PMIC RC, which is further
> divided with PMIC actually selecting which source to use as a source for
> sleep_clk.

This exists on PM8916 as well, but I'm not sure it's worth taking it
into consideration here. The PM8916 specification says XO "is the source
of sleep clock when the device is in active and sleep mode". The RC is
used "during PMIC power-up" and "in active or sleep mode only if other
sources are unavailable".

> > If we keep the fixed-clock with the hardcoded frequency I wonder if we
> > should put 32765 instead of 32764. If you calculate it exactly it's
> > slightly closer to 32765 than 32764. :-)
> >
> > 	19200000/586 = 32764.505119453926 = ~32765
>
> Well, I think according to the most typical rounding rules it is 32764.

I think typically you round up on .5? But it's not even exactly half-way
at .500 - given that it's .505, I think any rounding function other than
floor() should round that up to 32765. :-)

Thanks,
Stephan