| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMuHMdWSB4OvS5AeWqOBQPNG2J9VMYe9YUeXAp9kPjcJEQm3+g@mail.gmail.com>
Date: Mon, 10 Nov 2025 14:48:03 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Magnus Damm <magnus.damm@...il.com>, linux-renesas-soc@...r.kernel.org,
linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v2 5/5] clk: renesas: r9a09g077: Add xSPI core and module clocks
Hi Prabhakar,
On Tue, 28 Oct 2025 at 17:52, Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> 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@...renesas.com>
> ---
> 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@...ux-m68k.org
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
Powered by blists - more mailing lists