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

From: Geert Uytterhoeven <geert@linux-m68k.org>
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
Date: Wed, 23 Jun 2021 13:59:51 +0200	[thread overview]
Message-ID: <CAMuHMdWqknU6TAauueUHcCMh0cXOn7Xwy_HrnVM5-q7GX2UMaQ@mail.gmail.com> (raw)
In-Reply-To: <OS0PR01MB592205558A2738A271D727D486089@OS0PR01MB5922.jpnprd01.prod.outlook.com>

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

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