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

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

[Biju Das]

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>
---
 include/dt-bindings/clock/r9a07g044-cpg.h | 153 ++++++++++++++--------
 1 file changed, 100 insertions(+), 53 deletions(-)

diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h
index 1d8986563fc5..7e0127646973 100644
--- a/include/dt-bindings/clock/r9a07g044-cpg.h
+++ b/include/dt-bindings/clock/r9a07g044-cpg.h
@@ -32,58 +32,105 @@
 #define R9A07G044_OSCCLK		21
 
 /* R9A07G044 Module Clocks */
-#define R9A07G044_CLK_GIC600		0
-#define R9A07G044_CLK_IA55		1
-#define R9A07G044_CLK_SYC		2
-#define R9A07G044_CLK_DMAC		3
-#define R9A07G044_CLK_SYSC		4
-#define R9A07G044_CLK_MTU		5
-#define R9A07G044_CLK_GPT		6
-#define R9A07G044_CLK_ETH0		7
-#define R9A07G044_CLK_ETH1		8
-#define R9A07G044_CLK_I2C0		9
-#define R9A07G044_CLK_I2C1		10
-#define R9A07G044_CLK_I2C2		11
-#define R9A07G044_CLK_I2C3		12
-#define R9A07G044_CLK_SCIF0		13
-#define R9A07G044_CLK_SCIF1		14
-#define R9A07G044_CLK_SCIF2		15
-#define R9A07G044_CLK_SCIF3		16
-#define R9A07G044_CLK_SCIF4		17
-#define R9A07G044_CLK_SCI0		18
-#define R9A07G044_CLK_SCI1		19
-#define R9A07G044_CLK_GPIO		20
-#define R9A07G044_CLK_SDHI0		21
-#define R9A07G044_CLK_SDHI1		22
-#define R9A07G044_CLK_USB0		23
-#define R9A07G044_CLK_USB1		24
-#define R9A07G044_CLK_CANFD		25
-#define R9A07G044_CLK_SSI0		26
-#define R9A07G044_CLK_SSI1		27
-#define R9A07G044_CLK_SSI2		28
-#define R9A07G044_CLK_SSI3		29
-#define R9A07G044_CLK_MHU		30
-#define R9A07G044_CLK_OSTM0		31
-#define R9A07G044_CLK_OSTM1		32
-#define R9A07G044_CLK_OSTM2		33
-#define R9A07G044_CLK_WDT0		34
-#define R9A07G044_CLK_WDT1		35
-#define R9A07G044_CLK_WDT2		36
-#define R9A07G044_CLK_WDT_PON		37
-#define R9A07G044_CLK_GPU		38
-#define R9A07G044_CLK_ISU		39
-#define R9A07G044_CLK_H264		40
-#define R9A07G044_CLK_CRU		41
-#define R9A07G044_CLK_MIPI_DSI		42
-#define R9A07G044_CLK_LCDC		43
-#define R9A07G044_CLK_SRC		44
-#define R9A07G044_CLK_RSPI0		45
-#define R9A07G044_CLK_RSPI1		46
-#define R9A07G044_CLK_RSPI2		47
-#define R9A07G044_CLK_ADC		48
-#define R9A07G044_CLK_TSU_PCLK		49
-#define R9A07G044_CLK_SPI		50
-#define R9A07G044_CLK_MIPI_DSI_V	51
-#define R9A07G044_CLK_MIPI_DSI_PIN	52
+#define R9A07G044_CA55_SCLK		0
+#define R9A07G044_CA55_PCLK		1
+#define R9A07G044_CA55_ATCLK		2
+#define R9A07G044_CA55_GICCLK		3
+#define R9A07G044_CA55_PERICLK		4
+#define R9A07G044_CA55_ACLK		5
+#define R9A07G044_CA55_TSCLK		6
+#define R9A07G044_GIC600_GICCLK		7
+#define R9A07G044_IA55_CLK		8
+#define R9A07G044_IA55_PCLK		9
+#define R9A07G044_MHU_PCLK		10
+#define R9A07G044_SYC_CNT_CLK		11
+#define R9A07G044_DMAC_ACLK		12
+#define R9A07G044_DMAC_PCLK		13
+#define R9A07G044_OSTM0_PCLK		14
+#define R9A07G044_OSTM1_PCLK		15
+#define R9A07G044_OSTM2_PCLK		16
+#define R9A07G044_MTU_X_MCK_MTU3	17
+#define R9A07G044_POE3_CLKM_POE		18
+#define R9A07G044_GPT_PCLK		19
+#define R9A07G044_POEG_A_CLKP		20
+#define R9A07G044_POEG_B_CLKP		21
+#define R9A07G044_POEG_C_CLKP		22
+#define R9A07G044_POEG_D_CLKP		23
+#define R9A07G044_WDT0_PCLK		24
+#define R9A07G044_WDT0_CLK		25
+#define R9A07G044_WDT1_PCLK		26
+#define R9A07G044_WDT1_CLK		27
+#define R9A07G044_WDT2_PCLK		28
+#define R9A07G044_WDT2_CLK		29
+#define R9A07G044_SPI_CLK2		30
+#define R9A07G044_SPI_CLK		31
+#define R9A07G044_SDHI0_IMCLK		32
+#define R9A07G044_SDHI0_IMCLK2		33
+#define R9A07G044_SDHI0_CLK_HS		34
+#define R9A07G044_SDHI0_ACLK		35
+#define R9A07G044_SDHI1_IMCLK		36
+#define R9A07G044_SDHI1_IMCLK2		37
+#define R9A07G044_SDHI1_CLK_HS		38
+#define R9A07G044_SDHI1_ACLK		39
+#define R9A07G044_GPU_CLK		40
+#define R9A07G044_GPU_AXI_CLK		41
+#define R9A07G044_GPU_ACE_CLK		42
+#define R9A07G044_ISU_ACLK		43
+#define R9A07G044_ISU_PCLK		44
+#define R9A07G044_H264_CLK_A		45
+#define R9A07G044_H264_CLK_P		46
+#define R9A07G044_CRU_SYSCLK		47
+#define R9A07G044_CRU_VCLK		48
+#define R9A07G044_CRU_PCLK		49
+#define R9A07G044_CRU_ACLK		50
+#define R9A07G044_MIPI_DSI_PLLCLK	51
+#define R9A07G044_MIPI_DSI_SYSCLK	52
+#define R9A07G044_MIPI_DSI_ACLK		53
+#define R9A07G044_MIPI_DSI_PCLK		54
+#define R9A07G044_MIPI_DSI_VCLK		55
+#define R9A07G044_MIPI_DSI_LPCLK	56
+#define R9A07G044_LCDC_CLK_A		57
+#define R9A07G044_LCDC_CLK_P		58
+#define R9A07G044_LCDC_CLK_D		59
+#define R9A07G044_SSI0_PCLK2		60
+#define R9A07G044_SSI0_PCLK_SFR		61
+#define R9A07G044_SSI1_PCLK2		62
+#define R9A07G044_SSI1_PCLK_SFR		63
+#define R9A07G044_SSI2_PCLK2		64
+#define R9A07G044_SSI2_PCLK_SFR		65
+#define R9A07G044_SSI3_PCLK2		66
+#define R9A07G044_SSI3_PCLK_SFR		67
+#define R9A07G044_SRC_CLKP		68
+#define R9A07G044_USB_U2H0_HCLK		69
+#define R9A07G044_USB_U2H1_HCLK		70
+#define R9A07G044_USB_U2P_EXR_CPUCLK	71
+#define R9A07G044_USB_PCLK		72
+#define R9A07G044_USB_SCLK		73
+#define R9A07G044_ETH0_CLK_AXI		74
+#define R9A07G044_ETH0_CLK_CHI		75
+#define R9A07G044_ETH0_REFCLK		76
+#define R9A07G044_ETH1_CLK_AXI		77
+#define R9A07G044_ETH1_CLK_CHI		78
+#define R9A07G044_ETH1_REFCLK		79
+#define R9A07G044_I2C0_PCLK		80
+#define R9A07G044_I2C1_PCLK		81
+#define R9A07G044_I2C2_PCLK		82
+#define R9A07G044_I2C3_PCLK		83
+#define R9A07G044_SCIF0_CLK_PCK		84
+#define R9A07G044_SCIF1_CLK_PCK		85
+#define R9A07G044_SCIF2_CLK_PCK		86
+#define R9A07G044_SCIF3_CLK_PCK		87
+#define R9A07G044_SCIF4_CLK_PCK		88
+#define R9A07G044_SCI0_CLKP		89
+#define R9A07G044_SCI1_CLKP		90
+#define R9A07G044_IRDA_CLKP		91
+#define R9A07G044_RSPI0_CLKB		92
+#define R9A07G044_RSPI1_CLKB		93
+#define R9A07G044_RSPI2_CLKB		94
+#define R9A07G044_CANFD_PCLK		95
+#define R9A07G044_GPIO_HCLK		96
+#define R9A07G044_ADC_ADCLK		97
+#define R9A07G044_ADC_PCLK		98
+#define R9A07G044_TSU_PCLK		99
 
 #endif /* __DT_BINDINGS_CLOCK_R9A07G044_CPG_H__ */
-- 
2.17.1

[PATCH 4/7] arm64: dts: renesas: r9a07g044: Update SCIF0 clock

[Biju Das]

Update SCIF0 clock index in SoC DTSI.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 476ee9a69065..544f040a4e1d 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -82,10 +82,10 @@
 				     <GIC_SPI 384 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "eri", "rxi", "txi",
 					  "bri", "dri", "tei";
-			clocks = <&cpg CPG_MOD R9A07G044_CLK_SCIF0>;
+			clocks = <&cpg CPG_MOD R9A07G044_SCIF0_CLK_PCK>;
 			clock-names = "fck";
 			power-domains = <&cpg>;
-			resets = <&cpg R9A07G044_CLK_SCIF0>;
+			resets = <&cpg R9A07G044_SCIF0_CLK_PCK>;
 			status = "disabled";
 		};
 
-- 
2.17.1

[PATCH 7/7] arm64: dts: renesas: r9a07g044: Add I2C nodes

[Biju Das]

Add I2C{0,1,2,3} nodes to RZ/G2 (R9A07G044) SoC DTSI.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 80 ++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 544f040a4e1d..b573ce88f4da 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -89,6 +89,86 @@
 			status = "disabled";
 		};
 
+		i2c0: i2c@10058000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "renesas,riic-r9a07g044", "renesas,riic-rz";
+			reg = <0 0x10058000 0 0x400>;
+			interrupts = <GIC_SPI 350  IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 348 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 349 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 352 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 351 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_I2C0_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A07G044_I2C0_PCLK>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		i2c1: i2c@10058400 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "renesas,riic-r9a07g044", "renesas,riic-rz";
+			reg = <0 0x10058400 0 0x400>;
+			interrupts = <GIC_SPI 358  IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 356 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 357 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 361 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 362 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 363 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_I2C1_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A07G044_I2C1_PCLK>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		i2c2: i2c@10058800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "renesas,riic-r9a07g044", "renesas,riic-rz";
+			reg = <0 0x10058800 0 0x400>;
+			interrupts = <GIC_SPI 366  IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 368 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 369 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 367 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 370 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 371 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_I2C2_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A07G044_I2C2_PCLK>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		i2c3: i2c@10058c00 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "renesas,riic-r9a07g044", "renesas,riic-rz";
+			reg = <0 0x10058c00 0 0x400>;
+			interrupts = <GIC_SPI 374  IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 372 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 373 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 378 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 379 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_I2C3_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A07G044_I2C3_PCLK>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
 		cpg: clock-controller@11010000 {
 			compatible = "renesas,r9a07g044-cpg";
 			reg = <0 0x11010000 0 0x10000>;
-- 
2.17.1

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

[Geert Uytterhoeven]

Hi Biju,

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

> +#define R9A07G044_USB_SCLK             73

It looks like USB_SCLK cannot be gated, but is driven directly from
OSCCLK, so I think it should be left out, and the DTS should just
reference R9A07G044_OSCCLK

> +#define R9A07G044_ETH0_CLK_AXI         74
> +#define R9A07G044_ETH0_CLK_CHI         75
> +#define R9A07G044_ETH0_REFCLK          76
> +#define R9A07G044_ETH1_CLK_AXI         77
> +#define R9A07G044_ETH1_CLK_CHI         78
> +#define R9A07G044_ETH1_REFCLK          79

According to the Hardware User's Manual, ETH0_REFCLK and  ETH1_REFCLK
cannot be gated (see the note for CPG_CLKMON_ETH), so I think it
should be left out, and the DTS should just reference R9A07G044_CLK_HP.

The rest of the clocks look good to me.  Some are still missing, but they
can be added later.

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.

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

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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 1/7] dt-bindings: clk: r9a07g044-cpg: Update clock
> definitions
> 
> Hi Biju,
> 
> 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
> 
> > +#define R9A07G044_USB_SCLK             73
> 
> It looks like USB_SCLK cannot be gated, but is driven directly from
> OSCCLK, so I think it should be left out, and the DTS should just
> reference R9A07G044_OSCCLK

OK. Agreed, will remove this.

> 
> > +#define R9A07G044_ETH0_CLK_AXI         74
> > +#define R9A07G044_ETH0_CLK_CHI         75
> > +#define R9A07G044_ETH0_REFCLK          76
> > +#define R9A07G044_ETH1_CLK_AXI         77
> > +#define R9A07G044_ETH1_CLK_CHI         78
> > +#define R9A07G044_ETH1_REFCLK          79
> 
> According to the Hardware User's Manual, ETH0_REFCLK and  ETH1_REFCLK
> cannot be gated (see the note for CPG_CLKMON_ETH), so I think it should be
> left out, and the DTS should just reference R9A07G044_CLK_HP.

OK. Agreed. Will remove this.
> 
> The rest of the clocks look good to me.  Some are still missing, but they
> can be added later.
> 
> 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

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


Reset definitions
--------------------
	DEF_RST("sdhi0_RST",	R9A07G044_RST_SDHI0,
				0x854, BIT(0)),


And DTS instantiate both reset and clock entries.

Is it ok to you? What is your thoughts on this?

Regards,
Biju



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

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

[Geert Uytterhoeven]

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.

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

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

> And DTS instantiate both reset and clock entries.

What do you mean by "instantiate"?
The "clocks" and "resets" properties?

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

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

[Biju Das]

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

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

[Geert Uytterhoeven]

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

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

[Biju Das]

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