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

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Geert,

Thank you for the review.

On Fri, Oct 24, 2025 at 11:08 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, 14 Oct 2025 at 17:13, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add module and core clocks used by xSPI (Expanded SPI) IP on the
> > R9A09G077 SoC.
> >
> > The xSPI block uses PCLKH as its bus clock, while the operation clock
> > (XSPI_CLKn) is derived from PLL4. To support this, define new selectors
> > and dividers (FSELXSPI0/1 and DIVSEL_XSPI0/1) in SCKCR.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/clk/renesas/r9a09g077-cpg.c
> > +++ b/drivers/clk/renesas/r9a09g077-cpg.c
>
> > @@ -105,6 +113,15 @@ static const struct clk_div_table dtable_1_2[] = {
> >         {0, 0},
> >  };
> >
> > +static const struct clk_div_table dtable_6_8_16_32_64[] = {
> > +       {6, 64},
> > +       {5, 32},
> > +       {4, 16},
> > +       {3, 8},
> > +       {2, 6},
> > +       {0, 0},
> > +};
> > +
> >  static const struct clk_div_table dtable_24_25_30_32[] = {
> >         {0, 32},
> >         {1, 30},
> > @@ -119,6 +136,7 @@ static const char * const sel_clk_pll0[] = { ".loco", ".pll0" };
> >  static const char * const sel_clk_pll1[] = { ".loco", ".pll1" };
> >  static const char * const sel_clk_pll2[] = { ".loco", ".pll2" };
> >  static const char * const sel_clk_pll4[] = { ".loco", ".pll4" };
> > +static const char * const sel_clk_pll4d1_div3_div4[] = { ".pll4d1_div3", ".pll4d1_div4" };
> >
> >  static const struct cpg_core_clk r9a09g077_core_clks[] __initconst = {
> >         /* External Clock Inputs */
> > @@ -154,6 +172,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),
> > +       DEF_MUX(".divselxspi0", CLK_DIVSELXSPI0_SCKCR, DIVSEL_XSPI0,
> > +               sel_clk_pll4d1_div3_div4,
> > +               ARRAY_SIZE(sel_clk_pll4d1_div3_div4), CLK_MUX_HIWORD_MASK),
> > +       DEF_MUX(".divselxspi1", CLK_DIVSELXSPI1_SCKCR, DIVSEL_XSPI1,
> > +               sel_clk_pll4d1_div3_div4,
> > +               ARRAY_SIZE(sel_clk_pll4d1_div3_div4), CLK_MUX_HIWORD_MASK),
> > +
> >         /* Core output clk */
> >         DEF_DIV("CA55C0", R9A09G077_CLK_CA55C0, CLK_SEL_CLK_PLL0, DIVCA55C0,
> >                 dtable_1_2),
> > @@ -178,9 +205,15 @@ static const struct cpg_core_clk r9a09g077_core_clks[] __initconst = {
> >         DEF_FIXED("ETCLKC", R9A09G077_ETCLKC, CLK_SEL_CLK_PLL1, 10, 1),
> >         DEF_FIXED("ETCLKD", R9A09G077_ETCLKD, CLK_SEL_CLK_PLL1, 20, 1),
> >         DEF_FIXED("ETCLKE", R9A09G077_ETCLKE, CLK_SEL_CLK_PLL1, 40, 1),
> > +       DEF_DIV("XSPI_CLK0", R9A09G077_XSPI_CLK0, CLK_DIVSELXSPI0_SCKCR,
> > +               FSELXSPI0, dtable_6_8_16_32_64),
> > +       DEF_DIV("XSPI_CLK1", R9A09G077_XSPI_CLK1, CLK_DIVSELXSPI1_SCKCR,
> > +               FSELXSPI1, dtable_6_8_16_32_64),
> >  };
>
> Perhaps we need a custom clock for this?
> According to Section 7.3.1 "SCKCR : System Clock Control Register",
> some divider combinations are prohibited:
>   - 4 x 6,
>   - 4 x 32,
>   - 4 x 64.
> The last two are probably not an issue iff the xSPI driver never tries
> to set the corresponding clock rates.
> However, the first one may be an issue, as both 3 x 8 (valid) and 4 x 6
> (prohibited) yield the same resulting divider, and I believe we cannot
> be sure the clock core will never pick the prohibited combination.
>
Agreed, I think I will have to compose both MUX and the divider into a
single clock so that the dividers can be adjusted based on the MUX
value, or do you have any suggestion to just adjust the divider clocks
and leave the MUX clocks as is?

Cheers,
Prabhakar