Re: [PATCH v2 5/5] clk: renesas: r9a09g077: Add xSPI core and module clocks

[Geert Uytterhoeven]

Hi Prabhakar,

On Tue, 28 Oct 2025 at 17:52, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Add core clocks and module clock definitions required by the xSPI
> (Expanded SPI) IP on the R9A09G077 SoC.
>
> Define the new SCKCR fields FSELXSPI0/FSELXSPI1 and DIVSEL_XSPI0/1 and
> add two new core clocks XSPI_CLK0 and XSPI_CLK1. The xSPI block uses
> PCLKH as its bus clock (use as module clock parent) while the operation
> clock (XSPI_CLKn) is derived from PLL4. To support this arrangement
> provide mux/div selectors and divider tables for the supported
> XSPI operating rates.
>
> Add CLK_TYPE_RZT2H_FSELXSPI to implement a custom divider/mux clock
> where the determine_rate() callback enforces the hardware constraint:
> when the parent output is 600MHz only dividers 8 and 16 are valid,
> whereas for 800MHz operation the full divider set (6,8,16,32,64) may
> be used. The custom determine_rate() picks the best parent/divider pair
> to match the requested rate and programs the appropriate SCKCR fields.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> v1->v2:
> - Added custom divider clock type for XSPI clocks to enforce hardware
>   constraints on supported operating rates.

Thanks for the update!

> --- a/drivers/clk/renesas/r9a09g077-cpg.c
> +++ b/drivers/clk/renesas/r9a09g077-cpg.c

> @@ -54,12 +56,19 @@
>  #define DIVSCI3ASYNC   CONF_PACK(SCKCR3, 12, 2)
>  #define DIVSCI4ASYNC   CONF_PACK(SCKCR3, 14, 2)
>
> +#define FSELXSPI0      CONF_PACK(SCKCR, 0, 3)
> +#define FSELXSPI1      CONF_PACK(SCKCR, 8, 3)
> +#define DIVSEL_XSPI0   CONF_PACK(SCKCR, 6, 1)
> +#define DIVSEL_XSPI1   CONF_PACK(SCKCR, 14, 1)
>  #define SEL_PLL                CONF_PACK(SCKCR, 22, 1)
>
> +#define DIVSELXSPI_RATE_600MHZ         600000000UL
> +#define DIVSELXSPI_RATE_800MHZ         800000000UL

I find it a bit weird that the name of the define includes its value.
Perhaps just use "600 * MEGA" resp. "800 * MEGA" in the code instead?
But see below...

> @@ -154,6 +180,15 @@ static const struct cpg_core_clk r9a09g077_core_clks[] __initconst = {
>         DEF_DIV(".sci5async", CLK_SCI5ASYNC, CLK_PLL4D1, DIVSCI5ASYNC,
>                 dtable_24_25_30_32),
>
> +       DEF_FIXED(".pll4d1_div3", CLK_PLL4D1_DIV3, CLK_PLL4D1, 3, 1),
> +       DEF_FIXED(".pll4d1_div4", CLK_PLL4D1_DIV4, CLK_PLL4D1, 4, 1),

Please move these two just below the existing entry for ".pll4d1".

> +       DEF_MUX(".divselxspi0", CLK_DIVSELXSPI0_SCKCR, DIVSEL_XSPI0,
> +               sel_clk_pll4d1_div3_div4,
> +               ARRAY_SIZE(sel_clk_pll4d1_div3_div4), 0),
> +       DEF_MUX(".divselxspi1", CLK_DIVSELXSPI1_SCKCR, DIVSEL_XSPI1,
> +               sel_clk_pll4d1_div3_div4,
> +               ARRAY_SIZE(sel_clk_pll4d1_div3_div4), 0),
> +
>         /* Core output clk */
>         DEF_DIV("CA55C0", R9A09G077_CLK_CA55C0, CLK_SEL_CLK_PLL0, DIVCA55C0,
>                 dtable_1_2),

> @@ -264,6 +305,116 @@ r9a09g077_cpg_mux_clk_register(struct device *dev,
>         return clk_hw->clk;
>  }
>
> +static int r9a09g077_cpg_fselxspi_determine_rate(struct clk_hw *hw,
> +                                                struct clk_rate_request *req)
> +{
> +       struct clk_divider *divider = to_clk_divider(hw);
> +       unsigned long parent_rate, best = 0, now;
> +       const struct clk_div_table *clkt;
> +       unsigned long rate = req->rate;
> +       int div = 0;

unsigned int

> +
> +       if (!rate)
> +               rate = 1;
> +
> +       for (clkt = divider->table; clkt->div; clkt++) {
> +               parent_rate = clk_hw_round_rate(req->best_parent_hw, rate * clkt->div);

I had expected the use of some *_determinate_rate_*() helper, as the
parent can be changed to find a better clock rate?
Perhaps you should use a composite clock for that?

> +               /*
> +                * DIVSELXSPIx supports 800MHz and 600MHz operation.
> +                * When the parent_rate is 600MHz, only dividers of 8 and 16
> +                * are supported otherwise dividers of 6, 8, 16, 32, 64 are supported.
> +                * This check ensures that FSELXSPIx is set correctly.
> +                */
> +               if (parent_rate == DIVSELXSPI_RATE_600MHZ &&

Does this actually work as expected? I doubt parent_rate is guaranteed
to be exactly 600 or 800 MHz, and expect it can differ slightly due
to rounding.  Hence I would look at clk_fixed_factor.div instead.

> +                   (clkt->div != 8 && clkt->div != 16))
> +                       continue;
> +               now = DIV_ROUND_UP_ULL((u64)parent_rate, clkt->div);

No need to cast to u64 (DIV_ROUND_*_ULL() handle this internally).

> +               if (abs(rate - now) < abs(rate - best)) {
> +                       div = clkt->div;
> +                       best = now;
> +                       req->best_parent_rate = parent_rate;
> +               }
> +       }
> +
> +       if (!div) {
> +               u8 maxdiv = 0;
> +
> +               req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw, 1);
> +               /*
> +                * If DIVSELXSPIx is set to 800MHz set the maximum divider
> +                * or else fall back to divider of 16 which is a maximum
> +                * supported divider for 600MHz operation.
> +                */
> +               if (req->best_parent_rate == DIVSELXSPI_RATE_800MHZ) {
> +                       for (clkt = divider->table; clkt->div; clkt++) {
> +                               if (clkt->div > maxdiv)
> +                                       maxdiv = clkt->div;
> +                       }
> +                       div = maxdiv;

Why not hardcode the divider, like in the else branch?

> +               } else {
> +                       div = 16;
> +               }
> +       }
> +
> +       req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);

No need to cast to u64.


> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds