RE: [PATCH v2 03/13] dt-bindings: clock: Add r9a09g011 CPG Clock Definitions

From: Phil Edworthy <phil.edworthy@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Biju Das <biju.das.jz@bp.renesas.com>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-clk <linux-clk@vger.kernel.org>
Subject: RE: [PATCH v2 03/13] dt-bindings: clock: Add r9a09g011 CPG Clock Definitions
Date: Fri, 22 Apr 2022 11:29:49 +0000	[thread overview]
Message-ID: <TYYPR01MB70867EDF52038CBA6FD42383F5F79@TYYPR01MB7086.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdXy7XPcAmBLuCeXwZcPxNrfBs2yBN+tLicjHLQxQO=B2Q@mail.gmail.com>

Hi Geert,

On 20 April 2022 22:13 Geert Uytterhoeven wrote:
> On Wed, Mar 30, 2022 at 5:41 PM Phil Edworthy wrote:
> > Define RZ/V2M (R9A09G011) Clock Pulse Generator core clocks, module
> clock
> 
> The definitions contain no core clocks, only module clocks and resets?
> Perhaps you will need a core clock for the Ethernet reference clock,
> like on RZ/G2L?
It looks like rz/v2m has a gate for every clock, so no need for any core
clocks.

> > outputs (CPG_CLK_ON* registers), and reset definitions (CPG_RST_*
> > registers) in Section 48.5 ("Register Description") of the RZ/V2M
> Hardware
> > User's Manual (Rev. 1.10, Sep. 2021).
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  include/dt-bindings/clock/r9a09g011-cpg.h | 337 ++++++++++++++++++++++
> >  1 file changed, 337 insertions(+)
> >  create mode 100644 include/dt-bindings/clock/r9a09g011-cpg.h
> >
> > diff --git a/include/dt-bindings/clock/r9a09g011-cpg.h b/include/dt-
> bindings/clock/r9a09g011-cpg.h
> > new file mode 100644
> > index 000000000000..b88dbb0d8c49
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/r9a09g011-cpg.h
> > @@ -0,0 +1,337 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corp.
> > + */
> > +#ifndef __DT_BINDINGS_CLOCK_R9A09G011_CPG_H__
> > +#define __DT_BINDINGS_CLOCK_R9A09G011_CPG_H__
> > +
> > +#include <dt-bindings/clock/renesas-cpg-mssr.h>
> > +
> > +/* Module Clocks */
> > +#define R9A09G011_SYS_CLK              0
> > +#define R9A09G011_PFC_PCLK             1
> > +#define R9A09G011_PMC_CORE_CLOCK       2
> > +#define R9A09G011_GIC_CLK              3
> > +#define R9A09G011_RAMA_ACLK            4
> 
> Missing ROM_ACLK?
Yes, will add.

> > +
> 
> No need for this blank line, as this is not a register boundary.
Agreed

...
> > +#define R9A09G011_CST_ATB_SB_CLK       14
> 
> Missing CST_TS_SB_CLK?
> Yes, it shares a bit with CST_ATB_SB_CLK, cfr.
> ETH0_CLK_AXI and ETH1_CLK_AXI.
Ah, right, I will add.

...
> > +#define R9A09G011_ETH_CLK_AXI          35
> > +#define R9A09G011_ETH_CLK_CHI          36
> > +#define R9A09G011_ETH_GPTP_EXT         37
> 
> s/ETH/ETH0/ for the three above?
Will do, though there is only one eth, ETH0 matches the doc.

...
> > +#define R9A09G011_GRPA_PCLK            70
> 
> CPERI_GRPA_PCLK
Ok, I'll replace all GRP*_PCLK with CPERI_GRP*_PCLK

> > +#define R9A09G011_TIM0_CLK             71
> > +#define R9A09G011_TIM1_CLK             72
> > +#define R9A09G011_TIM2_CLK             73
> > +#define R9A09G011_TIM3_CLK             74
> > +#define R9A09G011_TIM4_CLK             75
> > +#define R9A09G011_TIM5_CLK             76
> > +#define R9A09G011_TIM6_CLK             77
> > +#define R9A09G011_TIM7_CLK             78
> > +#define R9A09G011_IIC01_PCLK           79
> 
> IIC0_PCLK?
There are four IIC peripherals, this pclk if for iic0 and iic1.
Would IIC0_1_PCLK be a better name for this?

> > +#define R9A09G011_IIC23_PCLK           89
> IIC1_PCLK?
and IIC2_3_PCLK?

...
> > +#define R9A09G011_ICB_ACLK1            141
> 
> Missing (shared) ICB_GIC_CLK?
I didn’t consider all these shared clocks and reset as useful
to anyone.
As far as I can tell, nothing will need to get the clock rates
and anything that needs to enable one of the clocks will need
to enable the other to use the HW.

Still, as the binding describes the HW, I will add all of them.

> > +#define R9A09G011_RAMB_ACLK            175
> 
> Would it make sense to decouple this into
> RAMB0_ACLK, RAMB1_ACLK, RAMB2_ACLK, RAMB3_ACLK?
Will do.

Thanks
Phil