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.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 23 June 2021 13:00
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; open list:OPEN FIRMWARE AND
> FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; Chris
> Paterson <Chris.Paterson2@renesas.com>; Linux-Renesas <linux-renesas-
> soc@vger.kernel.org>
> Subject: Re: [PATCH 1/7] dt-bindings: clk: r9a07g044-cpg: Update clock
> definitions
> 
> Hi Biju,
> 
> On Wed, Jun 23, 2021 at 1:11 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH 1/7] dt-bindings: clk: r9a07g044-cpg: Update
> > > clock definitions 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.
> 
> I did consider that option, too.  However, you would still need a bit of
> thought/processing to convert from register offsets and bit indices to
> clock/reset numbers and vice versa.

For resets, I have made some prototype(I2C/USB) with both the options and it works OK.

I2C0-->0x8800 (Offset:-0x880, bit index:0)
I2C1-->0x8801 (Offset:-0x880, bit index:1)
I2C2-->0x8802 (Offset:-0x880, bit index:2)
I2C3-->0x8803 (Offset:-0x880, bit index:3)

For USBHost0 reset:- 0x8783 and 0x8780
For USBHost1 reset:- 0x8783 and 0x8781
For USBdevice reset:- 0x8783 and 0x8782

On the code, reg = (index & 0xffff) >> 4; 
             Bitmask = BIT(index & 0xf);


> Compare this to MSTP clock numbers on R-Car (and GIC SPI IDs, and DMA
> slave MID/RIDs), where you can just read the number from a table in the
> Hardware User's Manual.
> So I think it's easier to have a list of clock definitions in a dt-
> bindings file.

OK. I will send V2 with this options.

Regards,
Biju

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