RE: [PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI clock and reset entries

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI
> clock and reset entries
> 
> Hi Biju,
> 
> On Wed, Aug 4, 2021 at 8:08 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Add SDHI{0,1} mux, clock and reset entries to CPG driver.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> 
> > @@ -77,6 +85,11 @@ static const struct cpg_core_clk
> r9a07g044_core_clks[] __initconst = {
> >         DEF_FIXED(".pll6_2", CLK_PLL6_2, CLK_PLL6, 1, 1),
> >
> >         DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
> > +       DEF_FIXED(".clk800fix_c", CLK_800FIX_C, CLK_PLL2, 1, 2),
> > +       DEF_FIXED(".clk533fix_c", CLK_533FIX_C, CLK_PLL2, 2, 6),
> 
> "2, 6" can be simplified to "1, 3".

OK, Will change it to 1,3.

> 
> > +       DEF_FIXED(".div_pll2_div8", CLK_DIV_PLL2_DIV8, CLK_800FIX_C, 1,
> 2),
> > +       DEF_FIXED(".div_pll2_div12", CLK_DIV_PLL2_DIV12, CLK_533FIX_C,
> > + 1, 2),
> 
> I just love the confusing clock naming in the User's Manual!
> DIV_PLL2_DIV8 runs at PLL2 / 4, and DIV_PLL2_DIV12 runs at PLL2 / 6 :-(
> 

There is an update on latest HW manual(Rev1.00, sep,2021)

As per this, it is just 400 MHZ and 266 MHz. I will send new patch based on this.


> > +
> >         DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
> >         DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
> >
> > @@ -103,6 +116,12 @@ static const struct cpg_core_clk
> r9a07g044_core_clks[] __initconst = {
> >         DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
> >         DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
> >                 sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0,
> > CLK_MUX_HIWORD_MASK),
> > +       DEF_SD_MUX("SD0", R9A07G044_CLK_SD0, SEL_SDHI0,
> > +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
> > +       DEF_SD_MUX("SD1", R9A07G044_CLK_SD1, SEL_SDHI1,
> > +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
> 
> Looks like both .flag and .mux_flags are unneeded?
OK. Will remove it.

Regards,
Biju

> 
> > +       DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A07G044_CLK_SD0, 1, 4),
> > +       DEF_FIXED("SD1_DIV4", CLK_SD1_DIV4, R9A07G044_CLK_SD1, 1, 4),
> >  };
> >
> >  static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> 
> The rest looks good to me.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> 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