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@xxxxxxxxxxx>
> > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > 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
