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

[Dmitry Rokosov]

Hello Martin,

On Mon, May 01, 2023 at 09:06:24PM +0200, Martin Blumenstingl wrote:
> Hi Dmitry,
> 
> overall this looks pretty good.

Thank you! Please find my thoughts about RTC clock below.

> 
> +Cc Heiner
> 
> On Wed, Apr 26, 2023 at 11:58 AM Dmitry Rokosov
> <ddrokosov@xxxxxxxxxxxxxx> wrote:
> [...]
> > +static struct clk_regmap pwm_a_sel = {
> > +       .data = &(struct clk_regmap_mux_data){
> > +               .offset = PWM_CLK_AB_CTRL,
> > +               .mask = 0x1,
> > +               .shift = 9,
> > +       },
> > +       .hw.init = &(struct clk_init_data){
> > +               .name = "pwm_a_sel",
> > +               .ops = &clk_regmap_mux_ops,
> > +               .parent_data = pwm_abcd_parents,
> > +               .num_parents = ARRAY_SIZE(pwm_abcd_parents),
> > +               /* For more information, please refer to rtc clock */
> > +               .flags = CLK_SET_RATE_NO_REPARENT,
> Heiner is working on a series that adds common clock support to the
> PWM driver [0].
> I think his plans for a next step are adding support for SoCs where
> the PWM clocks are part of the peripheral clock controller (instead of
> being part of the PWM controller registers).
> 

Yes, I'm keeping up with this review and staying informed. It's worth
noting that the peripheral clock driver already includes PWM clocks,
with an important remark about reparenting being switched off. It's
described below.

> Have you considered removing CLK_SET_RATE_PARENT from the &rtc clock
> so downstream clocks won't change the rtc clock rate by accident?
> Then we could drop the CLK_SET_RATE_NO_REPARENT flag from the PWM
> clocks to allow them to pick the best available parent (whether that's
> the rtc clock, xtal or sys_pll).
> That said, it would require managing the CLKID_RTC_32K_SEL clock (or
> it's parents) using assigned-clocks instead of doing so with the PWM
> (and other) clocks. Whether this would cause problems: I'm not sure,
> so I'm hoping that you can share some insights.
> 
> 

Allow me to share my thoughts on this matter. From my understanding,
Amlogic provides an RTC clock that is both accurate and power-effective
in achieving a 32KHz signal from an internal xtal of 24MHz. However,
this requires a complex RTC divider with four parameters (m1, m2, n1,
n2), as it cannot be accomplished with a single divider. Our team has
measured the RTC clock using an oscilloscope on the GEN CLK pin and
found that it provides a stable 32KHz signal with acceptable jitter. On
the other hand, other approaches, such as the PWM way, yield less stable
and less accurate 32KHz signals with greater jitter.

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.

Given the advantages of the RTC clock, we wish to be able to control the
RTC as a parent for specific leaf clocks. This is achievable with the
'assigned-clocks' feature of CCF OF, but it poses a significant
architectural problem. The 'assigned-clocks' node does not lock/pin the
parent, and a simple clk_set_rate() call can change the parent during
rate propagation. In my opinion, an ideal solution to this problem would
be an additional patch to the CCF core that provides this locking
capability. As a board DTS developer, I know which clock I want to use
as the parent and have a strong reason for doing so, and I do not wish
to open up my parent muxing to other drivers. But until the behavior of
'assigned-clocks' is not available, we will simply label all RTC
children with the CLK_SET_RATE_NO_REPARENT flag.

> Best regards,
> Martin
> 
> 
> [0] https://lore.kernel.org/linux-amlogic/9faca2e6-b7a1-4748-7eb0-48f8064e323e@xxxxxxxxx/

-- 
Thank you,
Dmitry