Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 12 December 2024 15:33
> Subject: Re: [PATCH v2 07/13] clk: renesas: rzv2h-cpg: Add MSTOP support
>
> Hi Biju,
>
> On Tue, Dec 3, 2024 at 11:50 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > Add bus MSTOP support for RZ/{V2H, G3E}. For some module clocks, there
> > are no MSTOP bits and the sequence ordering for mstop and clock on is
> > different compared to the RZ/G2L family.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> > * This patch has dependency on [1]
> > * Added MSTOP data for RZ/V2H CRU IP.
> > * Fixed typo clock->clk in error path of rzv2h_cpg_register_mod_clk()
>
> Thanks for the update!
>
> > drivers/clk/renesas/r9a09g047-cpg.c | 6 +-
> > drivers/clk/renesas/r9a09g057-cpg.c | 153 ++++++++++++++++++----------
> > drivers/clk/renesas/rzv2h-cpg.c | 92 ++++++++++++++++-
> > drivers/clk/renesas/rzv2h-cpg.h | 26 +++--
> > 4 files changed, 214 insertions(+), 63 deletions(-)
>
> Please split this patch in two parts, to ease backporting:
> A. New MSTOP support for RZ/V2H,
> B. RZ/G3E MSTOP support.
> Then, move part A forward in the series, and fold part B into "[PATCH
> v2 06/13] clk: renesas: Add support for RZ/G3E SoC".
Agreed.
>
> > --- a/drivers/clk/renesas/r9a09g057-cpg.c
> > +++ b/drivers/clk/renesas/r9a09g057-cpg.c
> > @@ -115,57 +115,108 @@ static const struct cpg_core_clk
> > r9a09g057_core_clks[] __initconst = { };
> >
> > static const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst =
> > {
>
> [...]
>
> > + BUS_MSTOP(1, BIT(8))),
> > + DEF_MOD("sdhi_0_imclk", CLK_PLLCLN_DIV8, 10, 3, 5, 3,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_0_imclk2", CLK_PLLCLN_DIV8, 10, 4, 5, 4,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_0_clk_hs", CLK_PLLCLN_DIV2, 10, 5, 5, 5,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_0_aclk", CLK_PLLDTY_ACPU_DIV4, 10, 6, 5, 6,
> > + BUS_MSTOP_NO_DATA),
>
> BUS_MSTOP(8), BIT(2)) for all four above?
Oops, missed to pick this from HW manual.
>
> > + DEF_MOD("sdhi_1_imclk", CLK_PLLCLN_DIV8, 10, 7, 5, 7,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_1_imclk2", CLK_PLLCLN_DIV8, 10, 8, 5, 8,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_1_clk_hs", CLK_PLLCLN_DIV2, 10, 9, 5, 9,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_1_aclk", CLK_PLLDTY_ACPU_DIV4, 10, 10, 5, 10,
> > + BUS_MSTOP_NO_DATA),
>
> BUS_MSTOP(8), BIT(3)) for all four above?
>
> > + DEF_MOD("sdhi_2_imclk", CLK_PLLCLN_DIV8, 10, 11, 5, 11,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_2_imclk2", CLK_PLLCLN_DIV8, 10, 12, 5, 12,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_2_clk_hs", CLK_PLLCLN_DIV2, 10, 13, 5, 13,
> > + BUS_MSTOP_NO_DATA),
> > + DEF_MOD("sdhi_2_aclk", CLK_PLLDTY_ACPU_DIV4, 10, 14, 5, 14,
> > + BUS_MSTOP_NO_DATA),
>
> BUS_MSTOP(8), BIT(4)) for all four above?
>
> > + DEF_MOD("cru_0_aclk", CLK_PLLDTY_ACPU_DIV2, 13, 2, 6, 18,
> > + BUS_MSTOP(9, BIT(4))),
> > + DEF_MOD_NO_PM("cru_0_vclk", CLK_PLLVDO_CRU0, 13, 3, 6, 19,
> > + BUS_MSTOP(9, BIT(4))),
> > + DEF_MOD("cru_0_pclk", CLK_PLLDTY_DIV16, 13, 4, 6, 20,
> > + BUS_MSTOP(9, BIT(4))),
> > + DEF_MOD("cru_1_aclk", CLK_PLLDTY_ACPU_DIV2, 13, 5, 6, 21,
> > + BUS_MSTOP(9, BIT(5))),
> > + DEF_MOD_NO_PM("cru_1_vclk", CLK_PLLVDO_CRU1, 13, 6, 6, 22,
> > + BUS_MSTOP(9, BIT(5))),
> > + DEF_MOD("cru_1_pclk", CLK_PLLDTY_DIV16, 13, 7, 6, 23,
> > + BUS_MSTOP(9, BIT(5))),
> > + DEF_MOD("cru_2_aclk", CLK_PLLDTY_ACPU_DIV2, 13, 8, 6, 24,
> > + BUS_MSTOP(9, BIT(6))),
> > + DEF_MOD_NO_PM("cru_2_vclk", CLK_PLLVDO_CRU2, 13, 9, 6, 25,
> > + BUS_MSTOP(9, BIT(6))),
> > + DEF_MOD("cru_2_pclk", CLK_PLLDTY_DIV16, 13, 10, 6, 26,
> > + BUS_MSTOP(9, BIT(6))),
> > + DEF_MOD("cru_3_aclk", CLK_PLLDTY_ACPU_DIV2, 13, 11, 6, 27,
> > + BUS_MSTOP(9, BIT(7))),
> > + DEF_MOD_NO_PM("cru_3_vclk", CLK_PLLVDO_CRU3, 13, 12, 6, 28,
> > + BUS_MSTOP(9, BIT(7))),
> > + DEF_MOD("cru_3_pclk", CLK_PLLDTY_DIV16, 13, 13, 6, 29,
> > + BUS_MSTOP(9, BIT(7))),
> > };
> >
> > static const struct rzv2h_reset r9a09g057_resets[] __initconst = {
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -83,6 +84,11 @@ struct rzv2h_cpg_priv {
> >
> > #define rcdev_to_priv(x) container_of(x, struct rzv2h_cpg_priv, rcdev)
> >
> > +struct rzv2h_mstop {
> > + u32 data;
>
> u16 idx;
> u16 mask;
Agreed, this will make the code simpler.
>
> > + refcount_t ref_cnt;
> > +};
> > +
> > struct pll_clk {
> > struct rzv2h_cpg_priv *priv;
> > void __iomem *base;
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.h
> > +++ b/drivers/clk/renesas/rzv2h-cpg.h
> > @@ -35,6 +35,7 @@ struct ddiv {
> > #define CPG_CDDIV1 (0x404)
> > #define CPG_CDDIV3 (0x40C)
> > #define CPG_CDDIV4 (0x410)
> > +#define CPG_BUS_1_MSTOP (0xd00)
> >
> > #define CDDIV0_DIVCTL2 DDIV_PACK(CPG_CDDIV0, 8, 3, 2) #define
> > CDDIV1_DIVCTL0 DDIV_PACK(CPG_CDDIV1, 0, 2, 4) @@ -46,6 +47,14 @@
> > struct ddiv { #define CDDIV4_DIVCTL1 DDIV_PACK(CPG_CDDIV4, 4, 1, 17)
> > #define CDDIV4_DIVCTL2 DDIV_PACK(CPG_CDDIV4, 8, 1, 18)
> >
> > +#define CPG_BUS_MSTOP_START (CPG_BUS_1_MSTOP - 4)
>
> No need for CPG_BUS_MSTOP_START...
>
> > +#define CPG_BUS_MSTOP(x) (CPG_BUS_MSTOP_START + (x) * 4)
>
> #define CPG_BUS_MSTOP(m) (CPG_BUS_1_MSTOP + ((m) - 1) * 4)
>
> cfr. the formula for Address in Section 4.4.4.37/39 ("MSTOP Registers (CPG_BUS_m_MSTOP) (m = 1 to
> 12/13)").
Agreed.
>
> > +
> > +#define BUS_MSTOP(index, mask) ((CPG_BUS_MSTOP(index) & 0xffff) << 16 | (mask))
> > +#define BUS_MSTOP_OFF(val) (((val) >> 16) & 0xffff)
> > +#define BUS_MSTOP_VAL(val) ((val) & 0xffff)
> > +#define BUS_MSTOP_NO_DATA GENMASK(31, 0)
>
> BUS_MSTOP_NONE?
Agreed. Will drop BUS_MSTOP_OFF and BUS_MSTOP_VAL as well.
#define BUS_MSTOP(idx, mask) (((idx) & 0xffff) << 16 | (mask))
Will send next version with these changes.
Cheers,
Biju
