* Re: [PATCH v4 6/9] clk: sunxi-ng: div: add read-only operation support
From: Chen-Yu Tsai @ 2026-07-06 13:09 UTC (permalink / raw)
To: Jerome Brunet
Cc: sashiko-bot, sashiko-reviews, conor+dt, robh, linux-rtc,
Alexandre Belloni, devicetree, linux-sunxi
In-Reply-To: <1jv7asguqu.fsf@starbuckisacylon.baylibre.com>
On Mon, Jul 6, 2026 at 8:36 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On lun. 06 juil. 2026 at 10:06, sashiko-bot@kernel.org wrote:
>
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] Inconsistent rate calculation between `determine_rate` and `recalc_rate` due to division order.
> > --
> >
> > commit 39f872462c04ad724c3624b152234160de56a47e
> > Author: Jerome Brunet <jbrunet@baylibre.com>
> >
> > clk: sunxi-ng: div: add read-only operation support
> >
> > Add support for sunxi-ng read-only dividers. This will be
> > useful to the a733 oscillator detection logic.
> >
> >> diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
> >> index 62d680ccb524b..d1c8c7baa12dc 100644
> >> --- a/drivers/clk/sunxi-ng/ccu_div.c
> >> +++ b/drivers/clk/sunxi-ng/ccu_div.c
> > [ ... ]
> >> @@ -84,6 +84,36 @@ static int ccu_div_determine_rate(struct clk_hw *hw,
> >> req, ccu_div_determine_rate_helper, cd);
> >> }
> >>
> >> +static int ccu_rodiv_determine_rate(struct clk_hw *hw,
> >> + struct clk_rate_request *req)
> >> +{
> >> + struct ccu_div *cd = hw_to_ccu_div(hw);
> >> + unsigned long val;
> >> + u32 reg;
> >> + int ret;
> >> +
> >> + reg = readl(cd->common.base + cd->common.reg);
> >> + val = reg >> cd->div.shift;
> >> + val &= (1 << cd->div.width) - 1;
> >> +
> >> + req->rate = ccu_mux_helper_unapply_prediv(&cd->common, &cd->mux, -1,
> >> + req->rate);
> >> +
> >> + if (cd->common.features & CCU_FEATURE_FIXED_POSTDIV)
> >> + req->rate *= cd->fixed_post_div;
> >> +
> >> + ret = divider_ro_determine_rate(hw, req, cd->div.table,
> >> + cd->div.width, cd->div.flags, val);
> >> +
> >> + if (cd->common.features & CCU_FEATURE_FIXED_POSTDIV)
> >> + req->rate /= cd->fixed_post_div;
> >> +
> >> + req->rate = ccu_mux_helper_apply_prediv(&cd->common, &cd->mux, -1,
> >> + req->rate);
> >
> > [Severity: Medium]
> > Will this division order cause inconsistent rate calculations compared to
> > ccu_div_recalc_rate()?
> >
> > The divider_ro_determine_rate() helper applies DIV_ROUND_UP() to the unadjusted
> > parent rate, and then this function divides that result by fixed_post_div and
> > prediv.
> >
> > However, ccu_div_recalc_rate() applies prediv to the parent rate first, then
> > calculates the divider rate, and finally divides by fixed_post_div.
> >
> > Since the Common Clock Framework requires determine_rate and recalc_rate to be
> > mathematically consistent, could this discrepancy cause rate mismatches or
> > warnings for clocks using a pre-divider or fixed post-divider?
>
> I think the RO side is correct and RW one is not.
>
> The RO part correctly walk back the tree by un-applying the predivider
> so, whatever CCF does with the parent rate value will be correct.
>
> The RW part query the parent rate then applies the prediv. This
> will be thrown away when/if clk_divider_bestdiv() calls
> clk_hw_round_rate() on the parent. If that happens, the subsequent
> un-apply of the prediv would make the matter even worse. So (if I
> understand all this correctly) this case does not happen yet, otherwise
> we would have noticed, and it can be fixed separately.
Looking at the history, ccu_div_determine_rate_helper() used to be
ccu_div_round_rate(), so it wouldn't have tried all the parents. Looks
like I missed this when looking at the conversion patches back in
January.
The fix is easy though: just reimplement divider_round_rate_parent().
Somehow we did this already for the multiplier but not the divider.
BTW, would it make sense to implement ccu_rodiv_determine_rate() using
ccu_mux_helper_determine_rate() as well? That would make both RW and RW
consistent, but it would mean another rewrite for you ...
> Side Note: I've not followed the sunxi history behind all those
> prediv/postdiv but I find them a bit confusing. It seems like inserting
> some fixed factor clocks in the clock trees would accomplish the same
> thing while simplifying mux/divider quite a lot by droping these quirks ?
I think the whole idea was that we didn't want all those extra intermediate
clocks. If you look at drivers/clk/sunxi/ you can see that we came from a
one device node per clock design, which was messy and bloated. The second
try ended up being the "one clock per useful hardware clock unit" design
you see now..
And we somehow tied the list of clks to the exported list of clks,
which probably made it worse for adding intermediates.
> Side Note #2: I feel that while sashiko is pointing out valid things but we
> are slowly getting off topic and down the rabbit hole with it ...
Yeah. That's the downside of Sashiko.
Thanks
ChenYu
^ permalink raw reply
* Re: (subset) [PATCH v4 0/9] clk: sun6i-rtc: Add support for Allwinner A733 SoC
From: Chen-Yu Tsai @ 2026-07-06 15:38 UTC (permalink / raw)
To: Junhui Liu, Alexandre Belloni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, Michael Turquette,
Stephen Boyd, Maxime Ripard, Jerome Brunet
Cc: linux-rtc, devicetree, linux-arm-kernel, linux-sunxi,
linux-kernel, linux-clk, Sashiko, Conor Dooley
In-Reply-To: <20260706-a733-rtc-v4-0-f330728db3d3@baylibre.com>
On Mon, 06 Jul 2026 11:32:08 +0200, Jerome Brunet wrote:
> Add support for the Allwinner A733 RTC and its internal Clock Control
> Unit (CCU). Reuse the rtc-sun6i rtc driver while introducing a new
> SoC-specific RTC CCU driver to handle the hardware's evolved clock
> structure.
>
> The A733 implementation supports hardware detection of three external
> crystal frequencies (19.2MHz, 24MHz and 26MHz), which is represented in
> the driver via read-only divider operations. Implement logic to derive a
> normalized 32kHz reference from these DCXO sources using fixed
> pre-dividers. Additionally, provide several new DCXO gate clocks for
> peripherals, including SerDes, HDMI, and UFS.
>
> [...]
Applied to sunxi/clk-for-7.3 in sunxi, thanks!
[1/9] dt-bindings: rtc: sun6i: no clock-output-names on h616/r329
https://git.kernel.org/sunxi/linux/c/7b9f8fe9c9e0
[2/9] dt-bindings: rtc: sun6i: add sun60i-a733 support
https://git.kernel.org/sunxi/linux/c/38d6b194a21c
[3/9] clk: sunxi-ng: fix ccu probe clock unregister on error
https://git.kernel.org/sunxi/linux/c/81f4ddc9f7e0
[4/9] clk: sunxi-ng: sun6i-rtc: clean up DT usage
https://git.kernel.org/sunxi/linux/c/192c6220e14d
[5/9] clk: sunxi-ng: sun6i-rtc: Add feature bit for IOSC calibration
https://git.kernel.org/sunxi/linux/c/d2fcd82ca07d
[7/9] clk: sunxi-ng: mux: remove unneeded export
https://git.kernel.org/sunxi/linux/c/c52b5090bf1e
Best regards,
--
Chen-Yu Tsai <wens@kernel.org>
^ permalink raw reply
* Re: [PATCH v4 8/9] clk: sunxi-ng: sun6i-rtc: split main oscillator div and gate.
From: Chen-Yu Tsai @ 2026-07-06 15:49 UTC (permalink / raw)
To: Jerome Brunet
Cc: Junhui Liu, Alexandre Belloni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland, Michael Turquette,
Stephen Boyd, Maxime Ripard, linux-rtc, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel, linux-clk
In-Reply-To: <20260706-a733-rtc-v4-8-f330728db3d3@baylibre.com>
On Mon, Jul 6, 2026 at 5:32 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On the a733 the "osc24M-32k" clock has the same gate bits as the previously
> supported SoC but a different divider implementation.
>
> Instead of a fixed 750 divider, the divider is selected based on the
> rate of the oscillator. It can be seen as a simple read-only divider.
>
> To easily replace the divider part depending the SoC, split the divider
> and gate into two separate clock entities.
Actually, an even bigger reason to do this is because the read-only divider
and gate are in different registers, something that our combined clocks
don't support. So they need to be separate clock entities.
I think this should be mentioned. Otherwise,
Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> drivers/clk/sunxi-ng/ccu-sun6i-rtc.c | 20 +++++++++++---------
> drivers/clk/sunxi-ng/ccu-sun6i-rtc.h | 3 ++-
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
> index b24c8b196e66..25dd87e78eb7 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
> @@ -218,17 +218,18 @@ static const struct clk_parent_data osc24M[] = {
> { .fw_name = "hosc", .name = "osc24M" }
> };
>
> -static struct ccu_gate osc24M_32k_clk = {
> - .enable = BIT(16),
> - .common = {
> - .reg = LOSC_OUT_GATING_REG,
> - .prediv = 750,
> - .features = CCU_FEATURE_ALL_PREDIV,
> - .hw.init = CLK_HW_INIT_PARENTS_DATA("osc24M-32k", osc24M,
> - &ccu_gate_ops, 0),
> - },
> +static struct clk_fixed_factor osc24M_32k_div_clk = {
> + .mult = 1,
> + .div = 750,
> + .hw.init = CLK_HW_INIT_PARENTS_DATA("osc24M-32k-div",
> + osc24M,
> + &clk_fixed_factor_ops,
> + 0),
> };
>
> +static SUNXI_CCU_GATE_HW(osc24M_32k_clk, "osc24M-32k", &osc24M_32k_div_clk.hw,
> + LOSC_OUT_GATING_REG, BIT(16), 0);
> +
> static const struct clk_hw *rtc_32k_parents[] = {
> &osc32k_clk.common.hw,
> &osc24M_32k_clk.common.hw
> @@ -286,6 +287,7 @@ static struct clk_hw_onecell_data sun6i_rtc_ccu_hw_clks = {
> [CLK_EXT_OSC32K_GATE] = &ext_osc32k_gate_clk.common.hw,
> [CLK_OSC24M_32K] = &osc24M_32k_clk.common.hw,
> [CLK_RTC_32K] = &rtc_32k_clk.common.hw,
> + [CLK_OSC24M_32K_DIV] = &osc24M_32k_div_clk.hw,
> },
> };
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h
> index 9ae821fc2599..ab7b92b47f59 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h
> +++ b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h
> @@ -9,7 +9,8 @@
> #define CLK_EXT_OSC32K_GATE 4
> #define CLK_OSC24M_32K 5
> #define CLK_RTC_32K 6
> +#define CLK_OSC24M_32K_DIV 7
>
> -#define CLK_NUMBER (CLK_RTC_32K + 1)
> +#define CLK_NUMBER (CLK_OSC24M_32K_DIV + 1)
>
> #endif /* _CCU_SUN6I_RTC_H */
>
> --
> 2.47.3
>
^ permalink raw reply
page: | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox