Re: [PATCH v14 6/6] clk: meson: a1: add Amlogic A1 Peripherals clock controller driver

[Dmitry Rokosov]

On Thu, May 18, 2023 at 10:04:41PM +0200, Martin Blumenstingl wrote:
> Hi Dmitry,
> 
> On Wed, May 17, 2023 at 12:34 PM Dmitry Rokosov
> <ddrokosov@xxxxxxxxxxxxxx> wrote:
> [...]
> > > > Additionally, the CCF determines the best ancestor based on how close
> > > > its rate is to the given one, based on arithmetic calculations. However,
> > > > we have independent knowledge that a certain clock would be better, with
> > > > less jitter and fewer intermediaries, which will likely improve energy
> > > > efficiency. Sadly, the CCF cannot take this into account.
> > > I agree that the implementation in CCF is fairly simple. There's ways
> > > to trick it though: IIRC if there are multiple equally suitable clocks
> > > it picks the first one. For me all of this has worked so far which is
> > > what makes me curious in this case (not saying that anything is wrong
> > > with your approach).
> > >
> > > Do you have a (real world) example where the RTC clock should be
> > > preferred over another clock?
> > >
> >
> > Yes, a real-life example is the need for a 32Khz clock for an external
> > wifi chip. There is one option to provide this clock with high
> > precision, which is RTC + GENCLK.
> >
> > > I'm thinking about the following scenario.
> > > PWM parents:
> > > - XTAL: 24MHz
> > > - sys: not sure - let's say 166.67MHz
> > > - RTC: 32kHz
> > >
> > > Then after that there's a divider and a gate.
> > >
> > > Let's say the PWM controller needs a 1MHz clock: it can take that from
> > > XTAL or sys. Since XTAL is evenly divisible to 1MHz CCF will pick that
> > > and use the divider.
> > > But let's say the PWM controller needs a 32kHz clock: CCF would
> > > automatically pick the RTC clock.
> > > So is your implementation there to cover let's say 1kHz where
> > > mathematically 24MHz can be divided evenly to 1kHz (and thus should
> > > not result in any jitter) but RTC gives better precision in the real
> > > world (even though it's off by 24Hz)?
> > >
> >
> > I don't think so. The highest precision that RTC can provide is from a
> > 32KHz rate only. However, I believe that a 1kHz frequency can also be
> > achieved by using xtal 24MHz with a divider, which can provide high
> > precision as well.
> Thank you again for the great discussion on IRC today.
> Here's my short summary so I don't forget before you'll follow up on this.
> 
> In general there's two known cases where the RTC clock needs to be used:
> a) When using the GENCLK output of the SoC to output the 32kHz RTC
> clock and connect that to an SDIO WiFi chip clock input (this seems
> useful in my understanding because the RTC clock provides high
> precision)
> b) When using the PWM controller to output a 32kHz clock signal. In
> this case my understanding is that using the RTC clock as input to the
> PWM controller results in the best possible signal
> 
> The second case won't be supported with Heiner's patches [0] that use
> CCF (common clock framework) in the PWM controller driver.
> In this series the parent clock is calculated using:
>   freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period);
> 
> A 32kHz clock means a PWM period of 30518ns. So with the above
> calculation the PWM driver is asking for a clock rate of >=2GHz.
> We concluded that letting the common clock framework choose the best
> possible parent (meaning: removing CLK_SET_RATE_NO_REPARENT here) can
> be a way forward.
> But this means that the PWM controller driver must try to find the
> best possible parent somehow. The easiest way we came up with
> (pseudo-code):
>   freq = NSEC_PER_SEC / period;
>   fin_freq = clk_round_rate(channel->clk, freq);
>   if (fin_freq != freq) {
>     freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period);
>     fin_freq = clk_round_rate(channel->clk, freq);
>   }
> 
> The idea is: for a requested 32kHz signal the PWM period is 30518ns.
> The updated logic would find that there's a matching clock input and
> use that directly. If not: use the original logic as suggested by
> Heiner.
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://lore.kernel.org/linux-amlogic/9faca2e6-b7a1-4748-7eb0-48f8064e323e@xxxxxxxxx/

Thank you for the excellent follow-up! I will reply to Heiner's thread
with these comments. Let's continue this discussion further in the
Heiner patch series.

-- 
Thank you,
Dmitry