Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230426095805.15338-1-ddrokosov@sberdevices.ru>
 <20230426095805.15338-7-ddrokosov@sberdevices.ru> <CAFBinCA2OhtVaCJDi8ZfAFLSE4oUgxYBDScaP_WW63curEK8Mg@mail.gmail.com>
 <20230512140630.qd33rwzaalmadpmk@CAB-WSD-L081021>
In-Reply-To: <20230512140630.qd33rwzaalmadpmk@CAB-WSD-L081021>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Tue, 16 May 2023 23:10:19 +0200
Message-ID: <CAFBinCA8e9evk+9hTEgoNOD_+3DBst6vYDcradmr2c996jdUmw@mail.gmail.com>
Subject: Re: [PATCH v14 6/6] clk: meson: a1: add Amlogic A1 Peripherals clock
 controller driver
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     neil.armstrong@linaro.org, jbrunet@baylibre.com,
        mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.com,
        jian.hu@amlogic.com, kernel@sberdevices.ru, rockosov@gmail.com,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Heiner Kallweit <hkallweit1@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello Dmitry,

On Fri, May 12, 2023 at 4:06=E2=80=AFPM Dmitry Rokosov <ddrokosov@sberdevic=
es.ru> wrote:
[...]
> > > +static struct clk_regmap pwm_a_sel =3D {
> > > +       .data =3D &(struct clk_regmap_mux_data){
> > > +               .offset =3D PWM_CLK_AB_CTRL,
> > > +               .mask =3D 0x1,
> > > +               .shift =3D 9,
> > > +       },
> > > +       .hw.init =3D &(struct clk_init_data){
> > > +               .name =3D "pwm_a_sel",
> > > +               .ops =3D &clk_regmap_mux_ops,
> > > +               .parent_data =3D pwm_abcd_parents,
> > > +               .num_parents =3D ARRAY_SIZE(pwm_abcd_parents),
> > > +               /* For more information, please refer to rtc clock */
> > > +               .flags =3D 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.
Indeed, this is why this question came to my mind

> > 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.
This part is clear to me (we may have even chatted on IRC how to use
the GEN CLK output previously)

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

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

> 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.
Are you aware of clk_set_rate_exclusive() and clk_rate_exclusive_{get,put}(=
)?
It locks a clock and all of its parents to a certain rate. Other
consumers are unable to change the rate unless the lock is released
again.

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

PS: while writing this reply I found
drivers/clk/sunxi-ng/ccu-sun6i-rtc.c which implements
clk_ops.recalc_accuracy
I'm not sure I understand this correctly but it seems that CCF is not
using that information when making the decision which parent to use.


Best regards,
Martin