| 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-a8sC44HeShCFdk2xwTHMdcOo+8btNh9i0hthTEUMdnhqAQ@mail.gmail.com>
Date: Mon, 10 Nov 2025 21:38:28 +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,
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?
> > + (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