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

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

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 1/7] dt-bindings: clk: r9a07g044-cpg: Update clock
> definitions
> 
> 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.

Just rethinking by looking at R-Car approach, We may not need defining resets in dt-binding file.

We can create a 16 bit unique index with register offset in the last 12bits and control bits in last 4 bits.
Device tree passes this index and driver extracts this info for reset handling.

This will avoid dt-binding dependency. Are you ok this approach for resets?? What about clock, existing method or similar 16bit index method??

Please share your thoughts.

> > 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  ;-)

I agree, please see my above comment for unique index(offset + this val).

> > 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.

OK.

> > And DTS instantiate both reset and clock entries.
> 
> What do you mean by "instantiate"?
> The "clocks" and "resets" properties?

I mean clocks and reset properties.

Regards,
Biju