| 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: <CA+V-a8uCqSdju3TKPDgmNw8F6_Qu1An5mpBePWzMc02Z8iDUZg@mail.gmail.com>
Date: Mon, 17 Nov 2025 10:52:30 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
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 Geert,
On Mon, Nov 10, 2025 at 9:38 PM Lad, Prabhakar
<prabhakar.csengg@...il.com> wrote:
>
> Hi Geert,
>
> Thank you for the review.
>
> On Mon, Nov 10, 2025 at 1:48 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> >
> > 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?
> OK.
>
> > 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".
> >
> Ok, I will move it below .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
> >
> Ok.
>
> > > +
> > > + 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.
> >
> With below diff, Ive got the below results for the various freqs
> requested where appropriate parent and divider clocks are picked.
>
> @@ -317,6 +317,7 @@ static int
> r9a09g077_cpg_fselxspi_determine_rate(struct clk_hw *hw,
>
> for (clkt = divider->table; clkt->div; clkt++) {
> parent_rate = clk_hw_round_rate(req->best_parent_hw,
> rate * clkt->div);
> + pr_err("parent_rate=%lu, req-rate=%lu div=%u\n",
> parent_rate, rate, clkt->div);
> /*
> * DIVSELXSPIx supports 800MHz and 600MHz operation.
> * When the parent_rate is 600MHz, only dividers of 8 and 16
>
> Logs:
> ---------
>
> Case 0# assigned-clock-rates = <133333334>;
> [ 15.419300] parent_rate=800000000, req-rate=133333334 div=64
> [ 15.437698] parent_rate=800000000, req-rate=133333334 div=32
> [ 15.455224] parent_rate=800000000, req-rate=133333334 div=16
> [ 15.501291] parent_rate=800000000, req-rate=133333334 div=8
> [ 15.507801] parent_rate=800000000, req-rate=133333334 div=6
> [ 15.519221] parent_rate=800000000, req-rate=133333334 div=64
> [ 15.525789] parent_rate=800000000, req-rate=133333334 div=32
> [ 15.549625] parent_rate=800000000, req-rate=133333334 div=16
> [ 15.556120] parent_rate=800000000, req-rate=133333334 div=8
> [ 15.564110] parent_rate=800000000, req-rate=133333334 div=6
>
> root@...2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep -e
> "xspi0" -e "XSPI_CLK0" -e "divselxspi0"
> .divselxspi0 1 1 0
> 800000000 0 0 50000 Y deviceless
> no_connection_id
> XSPI_CLK0 1 1 0
> 133333334 0 0 50000 Y
> 801c0000.spi spi
> xspi0 0 1 0
> 250000000 0 0 50000 N deviceless
> of_clk_get_from_provider
> root@...2h-evk:~#
>
> Case 1# assigned-clock-rates = <100000000>;
> [ 15.496291] parent_rate=800000000, req-rate=100000000 div=64
> [ 15.510068] parent_rate=800000000, req-rate=100000000 div=32
> [ 15.517142] parent_rate=800000000, req-rate=100000000 div=16
> [ 15.524047] parent_rate=800000000, req-rate=100000000 div=8
> [ 15.533174] parent_rate=600000000, req-rate=100000000 div=6
> [ 15.540096] parent_rate=800000000, req-rate=100000000 div=64
> [ 15.548135] parent_rate=800000000, req-rate=100000000 div=32
> [ 15.555119] parent_rate=800000000, req-rate=100000000 div=16
> [ 15.562395] parent_rate=800000000, req-rate=100000000 div=8
> [ 15.573521] parent_rate=600000000, req-rate=100000000 div=6
>
> root@...2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep -e
> "xspi0" -e "XSPI_CLK0" -e "divselxspi0"
> .divselxspi0 1 1 0
> 800000000 0 0 50000 Y deviceless
> no_connection_id
> XSPI_CLK0 1 1 0
> 100000000 0 0 50000 Y
> 801c0000.spi spi
> xspi0 0 1 0
> 250000000 0 0 50000 N deviceless
> of_clk_get_from_provider
> root@...2h-evk:~#
>
>
> Case 2# assigned-clock-rates = <75000000>;
> [ 12.288507] parent_rate=800000000, req-rate=75000000 div=64
> [ 12.310528] parent_rate=800000000, req-rate=75000000 div=32
> [ 12.318426] parent_rate=800000000, req-rate=75000000 div=16
> [ 12.326361] parent_rate=600000000, req-rate=75000000 div=8
> [ 12.341540] parent_rate=0, req-rate=75000000 div=6
> [ 12.347546] parent_rate=800000000, req-rate=75000000 div=64
> [ 12.357593] parent_rate=800000000, req-rate=75000000 div=32
> [ 12.367148] parent_rate=800000000, req-rate=75000000 div=16
> [ 12.418871] parent_rate=600000000, req-rate=75000000 div=8
> [ 12.433560] parent_rate=0, req-rate=75000000 div=6
>
> root@...2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep -e
> "xspi0" -e "XSPI_CLK0" -e "divselxspi0"
> .divselxspi0 1 1 0
> 600000000 0 0 50000 Y deviceless
> no_connection_id
> XSPI_CLK0 1 1 0
> 75000000 0 0 50000 Y
> 801c0000.spi spi
> xspi0 0 1 0
> 250000000 0 0 50000 N deviceless
> of_clk_get_from_provider
> root@...2h-evk:~#
>
> Case 3# assigned-clock-rates = <50000000>;
> [ 15.240214] parent_rate=800000000, req-rate=50000000 div=64
> [ 15.253498] parent_rate=800000000, req-rate=50000000 div=32
> [ 15.261521] parent_rate=800000000, req-rate=50000000 div=16
> [ 15.272941] parent_rate=0, req-rate=50000000 div=8
> [ 15.280532] parent_rate=0, req-rate=50000000 div=6
> [ 15.289979] parent_rate=800000000, req-rate=50000000 div=64
> [ 15.298745] parent_rate=800000000, req-rate=50000000 div=32
> [ 15.309879] parent_rate=800000000, req-rate=50000000 div=16
> [ 15.319881] parent_rate=0, req-rate=50000000 div=8
> [ 15.327977] parent_rate=0, req-rate=50000000 div=6
>
> root@...2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep -e
> "xspi0" -e "XSPI_CLK0" -e "divselxspi0"
> .divselxspi0 1 1 0
> 800000000 0 0 50000 Y deviceless
> no_connection_id
> XSPI_CLK0 1 1 0
> 50000000 0 0 50000 Y
> 801c0000.spi spi
> xspi0 0 1 0
> 250000000 0 0 50000 N deviceless
> of_clk_get_from_provider
> root@...2h-evk:~#
>
>
> Case 4# assigned-clock-rates = <37500000>;
> [ 71.710064] parent_rate=800000000, req-rate=37500000 div=64
> [ 71.718567] parent_rate=800000000, req-rate=37500000 div=32
> [ 71.725137] parent_rate=600000000, req-rate=37500000 div=16
> [ 71.731550] parent_rate=0, req-rate=37500000 div=8
> [ 71.740622] parent_rate=0, req-rate=37500000 div=6
> [ 71.746376] parent_rate=800000000, req-rate=37500000 div=64
> [ 71.752887] parent_rate=800000000, req-rate=37500000 div=32
> [ 71.767422] parent_rate=600000000, req-rate=37500000 div=16
> [ 71.778671] parent_rate=0, req-rate=37500000 div=8
> [ 71.790895] parent_rate=0, req-rate=37500000 div=6
>
> root@...2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep -e
> "xspi0" -e "XSPI_CLK0" -e "divselxspi0"
> .divselxspi0 1 1 0
> 600000000 0 0 50000 Y deviceless
> no_connection_id
> XSPI_CLK0 1 1 0
> 37500000 0 0 50000 Y
> 801c0000.spi spi
> xspi0 0 1 0
> 250000000 0 0 50000 N deviceless
> of_clk_get_from_provider
> root@...2h-evk:~#
>
>
> Case 5# assigned-clock-rates = <25000000>;
> [ 12.411660] parent_rate=800000000, req-rate=25000000 div=64
> [ 12.429285] parent_rate=800000000, req-rate=25000000 div=32
> [ 12.436144] parent_rate=0, req-rate=25000000 div=16
> [ 12.448110] parent_rate=0, req-rate=25000000 div=8
> [ 12.458785] parent_rate=0, req-rate=25000000 div=6
> [ 12.465401] parent_rate=800000000, req-rate=25000000 div=64
> [ 12.482547] parent_rate=800000000, req-rate=25000000 div=32
> [ 12.497126] parent_rate=0, req-rate=25000000 div=16
> [ 12.509619] parent_rate=0, req-rate=25000000 div=8
> [ 12.518212] parent_rate=0, req-rate=25000000 div=6
>
> root@...2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep -e
> "xspi0" -e "XSPI_CLK0" -e "divselxspi0"
> .divselxspi0 1 1 0
> 800000000 0 0 50000 Y deviceless
> no_connection_id
> XSPI_CLK0 1 1 0
> 25000000 0 0 50000 Y
> 801c0000.spi spi
> xspi0 0 1 0
> 250000000 0 0 50000 N deviceless
> of_clk_get_from_provider
> root@...2h-evk:~#
>
> Case 6# assigned-clock-rates = <12500000>;
> [ 87.409877] parent_rate=800000000, req-rate=12500000 div=64
> [ 87.470663] parent_rate=0, req-rate=12500000 div=32
> [ 87.485940] parent_rate=0, req-rate=12500000 div=16
> [ 87.492760] parent_rate=0, req-rate=12500000 div=8
> [ 87.498313] parent_rate=0, req-rate=12500000 div=6
>
>
> root@...2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep -e
> "xspi0" -e "XSPI_CLK0" -e "divselxspi0"
> .divselxspi0 1 1 0
> 800000000 0 0 50000 Y deviceless
> no_connection_id
> XSPI_CLK0 1 1 0
> 12500000 0 0 50000 Y
> 801c0000.spi spi
> xspi0 0 1 0
> 250000000 0 0 50000 N deviceless
> of_clk_get_from_provider
> root@...2h-evk:~#
>
> Looking at the logs I think I could optimize the code to continue when
> parent_rate == 0
>
> Based on the above logs, would you prefer me to represent it as a
> composite clock?
>
Gentle ping.
Cheers,
Prabhakar
> > > + (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?
> >
> Agreed.
>
> > > + } else {
> > > + div = 16;
> > > + }
> > > + }
> > > +
> > > + req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
> >
> > No need to cast to u64.
> >
> Ok.
>
> Cheers,
> Prabhakar
Powered by blists - more mailing lists