Re: [PATCH 1/7] dt-bindings: clk: r9a07g044-cpg: Update clock definitions

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Biju,

On Tue, Jun 22, 2021 at 11:26 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH 1/7] dt-bindings: clk: r9a07g044-cpg: Update clock
> > definitions
> > On Fri, Jun 18, 2021 at 11:58 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > Update clock definitions as per the RZG2L_clock_list(Rev.02) manual.
> > >
> > > 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/include/dt-bindings/clock/r9a07g044-cpg.h
> > > +++ b/include/dt-bindings/clock/r9a07g044-cpg.h

> > I do think we need a separate list of definitions for resets.  While
> > simple modules like SCIF and I2C have a one-to-one mapping from clock bits
> > to reset bits for, this is not the case for all modules.
> > E.g. SDHI has 4 clocks per instance, but only a single reset signal per
> > instance, while CANFD has a single clock, but two reset signals.
>
> OK, Agreed. We will list separate definitions for resets like,
>
> #define R9A07G044_RST_SDHI0             X1
> #define R9A07G044_RST_SDHI1             X2
> #define R9A07G044_RST_CAN               X3

Please use names that match the documentation, like
R9A07G044_SDHI0_IXRST and R9A07G044_SDHI0_CANFD_RSTP_N.

> Clk definitions
>
>         DEF_MOD("sdhi0_imclk",  R9A07G044_SDHI0_IMCLK,
>                                 CLK_SD0_DIV4,
>                                 0x554, BIT(0)),
>         DEF_MOD("sdhi0_imclk2", R9A07G044_SDHI0_IMCLK2,
>                                 CLK_SD0_DIV4,
>                                 0x554, BIT(1)),
>         DEF_MOD("sdhi0_clk_hs", R9A07G044_SDHI0_CLK_HS,
>                                 R9A07G044_CLK_SD0,
>                                 0x554, BIT(2),
>         DEF_MOD("sdhi0_aclk",   R9A07G044_SDHI0_ACLK,
>                                 R9A07G044_CLK_P1,
>                                 0x554, BIT(3)),

As each clock now corresponds to a single bit, you can store the bit
number (e.g. "0") instead of the bitmask ("BIT(0)").  This also works
for bits > 8, without needing to enlarge rzg2l_mod_clk.onoff  ;-)

> Reset definitions
> --------------------
>         DEF_RST("sdhi0_RST",    R9A07G044_RST_SDHI0,
>                                 0x854, BIT(0)),

Same here.
Note that you do not need names for resets, unlike clocks.

> And DTS instantiate both reset and clock entries.

What do you mean by "instantiate"?
The "clocks" and "resets" properties?

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