[PATCH v3 0/3] Add CPG support for RZ/V2H(P) SoC

[PATCH v3 0/3] Add CPG support for RZ/V2H(P) SoC

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series aims to add the CPG support for the Renesas
RZ/V2H(P) SoC.

v2 -> v3
- Dropped '|' for CPG description
- Dropped description for '#power-domain-cells' property
- Added 3 clock inputs for CPG
- Dropped label in example node
- Used 4 spaces for example node
- Renamed r9a09g057-cpg.h -> renesas,r9a09g057-cpg.h
- Merged adding renesas,r9a09g057-cpg.h in DT binding patch
- Updated commit message for patch 1/3
- Dropped num_hw_resets from struct rzv2h_cpg_priv
- Dropped range_check for module clocks
- Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
- Added support for critical module clocks with DEF_MOD_CRITICAL
- Added check for mon_index in rzv2h_mod_clock_endisable and
  rzv2h_mod_clock_is_enabled()
- Added CLK_PLLDTY
- Added core clocks sys_0_pclk and iotop_0_shclk
- Dropped r9a09g057_crit_mod_clks

v1->v2
- Updated commit message
- Updated description for binding as suggested by Geert
- Updated descriptions for clocks and resets property
- Renamed extal->qextal
- Updated '#power-domain-cells' value
- Dropped the module clocks and just added the core clocks
- Introduced family specific config option
- Now using register indexes for CLKON/CLKMON/RST/RSTMON
- Introduced PLL_CONF macro
- Dropped function pointer to get PLL_CLK1/2 offsets
- Added range check for core clks
- Dropped NULLified clocks check
- Dropped pll_clk1/clk2_offset
- Made r9a09g057_mod_clks/r9a09g057_resets as static const

v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/20240524082800.333991-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Lad Prabhakar (3):
  dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG
  clk: renesas: Add family-specific clock driver for RZ/V2H(P)
  clk: renesas: Add RZ/V2H(P) CPG driver

 .../bindings/clock/renesas,rzv2h-cpg.yaml     |  80 +++
 drivers/clk/renesas/Kconfig                   |   9 +
 drivers/clk/renesas/Makefile                  |   2 +
 drivers/clk/renesas/r9a09g057-cpg.c           |  79 +++
 drivers/clk/renesas/rzv2h-cpg.c               | 667 ++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h               | 161 +++++
 .../dt-bindings/clock/renesas,r9a09g057-cpg.h |  21 +
 7 files changed, 1019 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
 create mode 100644 drivers/clk/renesas/r9a09g057-cpg.c
 create mode 100644 drivers/clk/renesas/rzv2h-cpg.c
 create mode 100644 drivers/clk/renesas/rzv2h-cpg.h
 create mode 100644 include/dt-bindings/clock/renesas,r9a09g057-cpg.h

-- 
2.34.1

[PATCH v3 1/3] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Document the device tree bindings for the Renesas RZ/V2H(P) SoC
Clock Pulse Generator (CPG).

CPG block handles the below operations:
- Generation and control of clock signals for the IP modules
- Generation and control of resets
- Control over booting
- Low power consumption and power supply domains

Also define constants for the core clocks of the RZ/V2H(P) SoC. Note the
core clocks are a subset of the ones which are listed as part of section
4.4.2 of HW manual Rev.1.01 which cannot be controlled by CLKON register.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3
- Dropped '|' for CPG description
- Dropped description for '#power-domain-cells' property
- Added 3 clock inputs for CPG
- Dropped label in example node
- Used 4 spaces for example node
- Renamed r9a09g057-cpg.h -> renesas,r9a09g057-cpg.h
- Merged adding renesas,r9a09g057-cpg.h in DT binding patch
- Updated commit message

v1->v2
- Updated commit message
- Updated description for binding as suggested by Geert
- Updated descriptions for clocks and resets property
- Renamed extal->qextal
- Updated '#power-domain-cells' value
---
 .../bindings/clock/renesas,rzv2h-cpg.yaml     | 80 +++++++++++++++++++
 .../dt-bindings/clock/renesas,r9a09g057-cpg.h | 21 +++++
 2 files changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
 create mode 100644 include/dt-bindings/clock/renesas,r9a09g057-cpg.h

diff --git a/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
new file mode 100644
index 000000000000..dbc2705b3529
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/renesas,rzv2h-cpg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2H(P) Clock Pulse Generator (CPG)
+
+maintainers:
+  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+
+description:
+  On Renesas RZ/V2H(P) SoCs, the CPG (Clock Pulse Generator) handles generation
+  and control of clock signals for the IP modules, generation and control of resets,
+  and control over booting, low power consumption and power supply domains.
+
+properties:
+  compatible:
+    const: renesas,r9a09g057-cpg
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: AUDIO_EXTAL clock input
+      - description: RTXIN clock input
+      - description: QEXTAL clock input
+
+  clock-names:
+    items:
+      - const: audio_extal
+      - const: rtxin
+      - const: qextal
+
+  '#clock-cells':
+    description: |
+      - For CPG core clocks, the two clock specifier cells must be "CPG_CORE"
+        and a core clock reference, as defined in
+        <dt-bindings/clock/renesas,r9a09g057-cpg.h>,
+      - For module clocks, the two clock specifier cells must be "CPG_MOD" and
+        a module number.  The module number is calculated as the CLKON register
+        offset index multiplied by 16, plus the actual bit in the register
+        used to turn the CLK ON. For example, for CGC_GIC_0_GICCLK, the
+        calculation is (1 * 16 + 3) = 19.
+    const: 2
+
+  '#power-domain-cells':
+    const: 0
+
+  '#reset-cells':
+    description:
+      The single reset specifier cell must be the reset number. The reset number
+      is calculated as the reset register offset index multiplied by 16, plus the
+      actual bit in the register used to reset the specific IP block. For example,
+      for SYS_0_PRESETN, the calculation is (3 * 16 + 0) = 48.
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+  - '#power-domain-cells'
+  - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller@10420000 {
+        compatible = "renesas,r9a09g057-cpg";
+        reg = <0x10420000 0x10000>;
+        clocks = <&audio_extal_clk>, <&rtxin_clk>, <&qextal_clk>;
+        clock-names = "audio_extal", "rtxin", "qextal";
+        #clock-cells = <2>;
+        #power-domain-cells = <0>;
+        #reset-cells = <1>;
+    };
diff --git a/include/dt-bindings/clock/renesas,r9a09g057-cpg.h b/include/dt-bindings/clock/renesas,r9a09g057-cpg.h
new file mode 100644
index 000000000000..04653d90d78d
--- /dev/null
+++ b/include/dt-bindings/clock/renesas,r9a09g057-cpg.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+#ifndef __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__
+#define __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__
+
+#include <dt-bindings/clock/renesas-cpg-mssr.h>
+
+/* Core Clock list */
+#define R9A09G057_SYS_0_PCLK			0
+#define R9A09G057_CA55_0_CORE_CLK0		1
+#define R9A09G057_CA55_0_CORE_CLK1		2
+#define R9A09G057_CA55_0_CORE_CLK2		3
+#define R9A09G057_CA55_0_CORE_CLK3		4
+#define R9A09G057_CA55_0_PERIPHCLK		5
+#define R9A09G057_CM33_CLK0			6
+#define R9A09G057_CST_0_SWCLKTCK		7
+#define R9A09G057_IOTOP_0_SHCLK			8
+
+#endif /* __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__ */
-- 
2.34.1

Re: [PATCH v3 1/3] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG

[Krzysztof Kozlowski]

On 27/06/2024 18:13, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Document the device tree bindings for the Renesas RZ/V2H(P) SoC
> Clock Pulse Generator (CPG).
> 
> CPG block handles the below operations:
> - Generation and control of clock signals for the IP modules
> - Generation and control of resets
> - Control over booting
> - Low power consumption and power supply domains
> 
> Also define constants for the core clocks of the RZ/V2H(P) SoC. Note the
> core clocks are a subset of the ones which are listed as part of section
> 4.4.2 of HW manual Rev.1.01 which cannot be controlled by CLKON register.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Document the device tree bindings for the Renesas RZ/V2H(P) SoC
> Clock Pulse Generator (CPG).
>
> CPG block handles the below operations:
> - Generation and control of clock signals for the IP modules
> - Generation and control of resets
> - Control over booting
> - Low power consumption and power supply domains
>
> Also define constants for the core clocks of the RZ/V2H(P) SoC. Note the
> core clocks are a subset of the ones which are listed as part of section
> 4.4.2 of HW manual Rev.1.01 which cannot be controlled by CLKON register.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - Dropped '|' for CPG description
> - Dropped description for '#power-domain-cells' property
> - Added 3 clock inputs for CPG
> - Dropped label in example node
> - Used 4 spaces for example node
> - Renamed r9a09g057-cpg.h -> renesas,r9a09g057-cpg.h
> - Merged adding renesas,r9a09g057-cpg.h in DT binding patch
> - Updated commit message

Thanks for the update!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
> +  '#clock-cells':
> +    description: |
> +      - For CPG core clocks, the two clock specifier cells must be "CPG_CORE"
> +        and a core clock reference, as defined in
> +        <dt-bindings/clock/renesas,r9a09g057-cpg.h>,
> +      - For module clocks, the two clock specifier cells must be "CPG_MOD" and
> +        a module number.  The module number is calculated as the CLKON register
> +        offset index multiplied by 16, plus the actual bit in the register
> +        used to turn the CLK ON. For example, for CGC_GIC_0_GICCLK, the
> +        calculation is (1 * 16 + 3) = 19.

Using hexadecimal for the final number may be more readable,
also in the DTS?

(1 * 16 + 3) = 0x13?

> +    const: 2
> +
> +  '#power-domain-cells':
> +    const: 0
> +
> +  '#reset-cells':
> +    description:
> +      The single reset specifier cell must be the reset number. The reset number
> +      is calculated as the reset register offset index multiplied by 16, plus the
> +      actual bit in the register used to reset the specific IP block. For example,
> +      for SYS_0_PRESETN, the calculation is (3 * 16 + 0) = 48.

(3 * 16 + 0) = 0x30?

> --- /dev/null
> +++ b/include/dt-bindings/clock/renesas,r9a09g057-cpg.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +#ifndef __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__
> +#define __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__

__DT_BINDINGS_CLOCK_RENESAS_R9A09G057_CPG_H__

[...]

> +#endif /* __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__ */

__DT_BINDINGS_CLOCK_RENESAS_R9A09G057_CPG_H__

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 v3 1/3] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Fri, Jul 12, 2024 at 12:58 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
<snip>
> > +++ b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
> > +  '#clock-cells':
> > +    description: |
> > +      - For CPG core clocks, the two clock specifier cells must be "CPG_CORE"
> > +        and a core clock reference, as defined in
> > +        <dt-bindings/clock/renesas,r9a09g057-cpg.h>,
> > +      - For module clocks, the two clock specifier cells must be "CPG_MOD" and
> > +        a module number.  The module number is calculated as the CLKON register
> > +        offset index multiplied by 16, plus the actual bit in the register
> > +        used to turn the CLK ON. For example, for CGC_GIC_0_GICCLK, the
> > +        calculation is (1 * 16 + 3) = 19.
>
> Using hexadecimal for the final number may be more readable,
> also in the DTS?
>
> (1 * 16 + 3) = 0x13?
>
Agreed I will update the value to hex.

> > +    const: 2
> > +
> > +  '#power-domain-cells':
> > +    const: 0
> > +
> > +  '#reset-cells':
> > +    description:
> > +      The single reset specifier cell must be the reset number. The reset number
> > +      is calculated as the reset register offset index multiplied by 16, plus the
> > +      actual bit in the register used to reset the specific IP block. For example,
> > +      for SYS_0_PRESETN, the calculation is (3 * 16 + 0) = 48.
>
> (3 * 16 + 0) = 0x30?
>
OK.

> > --- /dev/null
> > +++ b/include/dt-bindings/clock/renesas,r9a09g057-cpg.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > + *
> > + * Copyright (C) 2024 Renesas Electronics Corp.
> > + */
> > +#ifndef __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__
> > +#define __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__
>
> __DT_BINDINGS_CLOCK_RENESAS_R9A09G057_CPG_H__
>
> [...]
>
> > +#endif /* __DT_BINDINGS_CLOCK_R9A09G057_CPG_H__ */
>
> __DT_BINDINGS_CLOCK_RENESAS_R9A09G057_CPG_H__
>
Oops, I missed updating it after the file name change.

Cheers,
Prabhakar

[PATCH v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add family-specific clock driver for RZ/V2H(P) SoCs.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3
- Dropped num_hw_resets from struct rzv2h_cpg_priv
- Dropped range_check for module clocks
- Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
- Added support for critical module clocks with DEF_MOD_CRITICAL
- Added check for mon_index in rzv2h_mod_clock_endisable and
  rzv2h_mod_clock_is_enabled()

v1->v2
- Introduced family specific config option
- Now using register indexes for CLKON/CLKMON/RST/RSTMON
- Introduced PLL_CONF macro
- Dropped function pointer to get PLL_CLK1/2 offsets
- Added range check for core clks
- Dropped NULLified clocks check
- Updated commit description
---
 drivers/clk/renesas/Kconfig     |   4 +
 drivers/clk/renesas/Makefile    |   1 +
 drivers/clk/renesas/rzv2h-cpg.c | 663 ++++++++++++++++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h | 159 ++++++++
 4 files changed, 827 insertions(+)
 create mode 100644 drivers/clk/renesas/rzv2h-cpg.c
 create mode 100644 drivers/clk/renesas/rzv2h-cpg.h

diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
index 4410d16de4e2..f078ccb635bb 100644
--- a/drivers/clk/renesas/Kconfig
+++ b/drivers/clk/renesas/Kconfig
@@ -228,6 +228,10 @@ config CLK_RZG2L
 	bool "RZ/{G2L,G2UL,G3S,V2L} family clock support" if COMPILE_TEST
 	select RESET_CONTROLLER
 
+config CLK_RZV2H
+	bool "RZ/V2H(P) family clock support" if COMPILE_TEST
+	select RESET_CONTROLLER
+
 # Generic
 config CLK_RENESAS_CPG_MSSR
 	bool "CPG/MSSR clock support" if COMPILE_TEST
diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile
index f7e18679c3b8..d81a62e78345 100644
--- a/drivers/clk/renesas/Makefile
+++ b/drivers/clk/renesas/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_CLK_RCAR_GEN3_CPG)		+= rcar-gen3-cpg.o
 obj-$(CONFIG_CLK_RCAR_GEN4_CPG)		+= rcar-gen4-cpg.o
 obj-$(CONFIG_CLK_RCAR_USB2_CLOCK_SEL)	+= rcar-usb2-clock-sel.o
 obj-$(CONFIG_CLK_RZG2L)			+= rzg2l-cpg.o
+obj-$(CONFIG_CLK_RZV2H)			+= rzv2h-cpg.o
 
 # Generic
 obj-$(CONFIG_CLK_RENESAS_CPG_MSSR)	+= renesas-cpg-mssr.o
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
new file mode 100644
index 000000000000..058d79673ae6
--- /dev/null
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -0,0 +1,663 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2H(P) Clock Pulse Generator
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ *
+ * Based on rzg2l-cpg.c
+ *
+ * Copyright (C) 2015 Glider bvba
+ * Copyright (C) 2013 Ideas On Board SPRL
+ * Copyright (C) 2015 Renesas Electronics Corp.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_domain.h>
+#include <linux/reset-controller.h>
+
+#include <dt-bindings/clock/renesas-cpg-mssr.h>
+
+#include "rzv2h-cpg.h"
+
+#ifdef DEBUG
+#define WARN_DEBUG(x)		WARN_ON(x)
+#else
+#define WARN_DEBUG(x)		do { } while (0)
+#endif
+
+#define GET_CLK_ON_OFFSET(x)	(0x600 + ((x) * 4))
+#define GET_CLK_MON_OFFSET(x)	(0x800 + ((x) * 4))
+#define GET_RST_OFFSET(x)	(0x900 + ((x) * 4))
+#define GET_RST_MON_OFFSET(x)	(0xA00 + ((x) * 4))
+
+#define KDIV(val)		((s16)FIELD_GET(GENMASK(31, 16), (val)))
+#define MDIV(val)		FIELD_GET(GENMASK(15, 6), (val))
+#define PDIV(val)		FIELD_GET(GENMASK(5, 0), (val))
+#define SDIV(val)		FIELD_GET(GENMASK(2, 0), (val))
+
+/**
+ * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
+ *
+ * @info: Pointer to platform data
+ * @dev: CPG device
+ * @base: CPG register block base address
+ * @clks: Array containing all Core and Module Clocks
+ * @num_core_clks: Number of Core Clocks in clks[]
+ * @num_mod_clks: Number of Module Clocks in clks[]
+ * @num_resets: Number of Module Resets in info->resets[]
+ * @last_dt_core_clk: ID of the last Core Clock exported to DT
+ * @rcdev: Reset controller entity
+ */
+struct rzv2h_cpg_priv {
+	const struct rzv2h_cpg_info *info;
+	struct device *dev;
+	void __iomem *base;
+
+	struct clk **clks;
+	unsigned int num_core_clks;
+	unsigned int num_mod_clks;
+	unsigned int num_resets;
+	unsigned int last_dt_core_clk;
+
+	struct reset_controller_dev rcdev;
+};
+
+struct pll_clk {
+	struct rzv2h_cpg_priv *priv;
+	void __iomem *base;
+	struct clk_hw hw;
+	unsigned int conf;
+	unsigned int type;
+};
+
+#define to_pll(_hw)	container_of(_hw, struct pll_clk, hw)
+
+static unsigned long rzv2h_cpg_pll_clk_recalc_rate(struct clk_hw *hw,
+						   unsigned long parent_rate)
+{
+	struct pll_clk *pll_clk = to_pll(hw);
+	struct rzv2h_cpg_priv *priv = pll_clk->priv;
+	unsigned int clk1, clk2;
+	u64 rate;
+
+	if (!PLL_CLK_ACCESS(pll_clk->conf))
+		return 0;
+
+	clk1 = readl(priv->base + PLL_CLK1_OFFSET(pll_clk->conf));
+	clk2 = readl(priv->base + PLL_CLK2_OFFSET(pll_clk->conf));
+
+	rate = mul_u64_u32_shr(parent_rate, (MDIV(clk1) << 16) + KDIV(clk1),
+			       16 + SDIV(clk2));
+
+	return DIV_ROUND_CLOSEST_ULL(rate, PDIV(clk1));
+}
+
+static const struct clk_ops rzv2h_cpg_pll_ops = {
+	.recalc_rate = rzv2h_cpg_pll_clk_recalc_rate,
+};
+
+static struct clk * __init
+rzv2h_cpg_pll_clk_register(const struct cpg_core_clk *core,
+			   struct rzv2h_cpg_priv *priv,
+			   const struct clk_ops *ops)
+{
+	void __iomem *base = priv->base;
+	struct clk **clks = priv->clks;
+	struct device *dev = priv->dev;
+	struct clk_init_data init;
+	const struct clk *parent;
+	const char *parent_name;
+	struct pll_clk *pll_clk;
+
+	parent = clks[core->parent & 0xffff];
+	if (IS_ERR(parent))
+		return ERR_CAST(parent);
+
+	pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
+	if (!pll_clk)
+		return ERR_PTR(-ENOMEM);
+
+	parent_name = __clk_get_name(parent);
+	init.name = core->name;
+	init.ops = ops;
+	init.flags = 0;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	pll_clk->hw.init = &init;
+	pll_clk->conf = core->conf;
+	pll_clk->base = base;
+	pll_clk->priv = priv;
+	pll_clk->type = core->type;
+
+	return devm_clk_register(dev, &pll_clk->hw);
+}
+
+static struct clk
+*rzv2h_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
+			       void *data)
+{
+	unsigned int clkidx = clkspec->args[1];
+	struct rzv2h_cpg_priv *priv = data;
+	struct device *dev = priv->dev;
+	const char *type;
+	struct clk *clk;
+
+	switch (clkspec->args[0]) {
+	case CPG_CORE:
+		type = "core";
+		if (clkidx > priv->last_dt_core_clk) {
+			dev_err(dev, "Invalid %s clock index %u\n", type, clkidx);
+			return ERR_PTR(-EINVAL);
+		}
+		clk = priv->clks[clkidx];
+		break;
+
+	case CPG_MOD:
+		type = "module";
+		if (clkidx >= priv->num_mod_clks) {
+			dev_err(dev, "Invalid %s clock index %u\n", type, clkidx);
+			return ERR_PTR(-EINVAL);
+		}
+		clk = priv->clks[priv->num_core_clks + clkidx];
+		break;
+
+	default:
+		dev_err(dev, "Invalid CPG clock type %u\n", clkspec->args[0]);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (IS_ERR(clk))
+		dev_err(dev, "Cannot get %s clock %u: %ld", type, clkidx,
+			PTR_ERR(clk));
+	else
+		dev_dbg(dev, "clock (%u, %u) is %pC at %lu Hz\n",
+			clkspec->args[0], clkspec->args[1], clk,
+			clk_get_rate(clk));
+	return clk;
+}
+
+static void __init
+rzv2h_cpg_register_core_clk(const struct cpg_core_clk *core,
+			    struct rzv2h_cpg_priv *priv)
+{
+	struct clk *clk = ERR_PTR(-EOPNOTSUPP), *parent;
+	struct device *dev = priv->dev;
+	unsigned int id = core->id, div = core->div;
+	const char *parent_name;
+
+	WARN_DEBUG(id >= priv->num_core_clks);
+	WARN_DEBUG(PTR_ERR(priv->clks[id]) != -ENOENT);
+
+	switch (core->type) {
+	case CLK_TYPE_IN:
+		clk = of_clk_get_by_name(priv->dev->of_node, core->name);
+		break;
+	case CLK_TYPE_FF:
+		WARN_DEBUG(core->parent >= priv->num_core_clks);
+		parent = priv->clks[core->parent];
+		if (IS_ERR(parent)) {
+			clk = parent;
+			goto fail;
+		}
+
+		parent_name = __clk_get_name(parent);
+		clk = clk_register_fixed_factor(NULL, core->name,
+						parent_name, CLK_SET_RATE_PARENT,
+						core->mult, div);
+		break;
+	case CLK_TYPE_PLL:
+		clk = rzv2h_cpg_pll_clk_register(core, priv, &rzv2h_cpg_pll_ops);
+		break;
+	default:
+		goto fail;
+	}
+
+	if (IS_ERR_OR_NULL(clk))
+		goto fail;
+
+	dev_dbg(dev, "Core clock %pC at %lu Hz\n", clk, clk_get_rate(clk));
+	priv->clks[id] = clk;
+	return;
+
+fail:
+	dev_err(dev, "Failed to register core clock %s: %ld\n",
+		core->name, PTR_ERR(clk));
+}
+
+/**
+ * struct mod_clock - Module clock
+ *
+ * @priv: CPG private data
+ * @hw: handle between common and hardware-specific interfaces
+ * @on_index: register offset
+ * @on_bit: ON/MON bit
+ * @mon_index: monitor register offset
+ * @mon_bit: montor bit
+ */
+struct mod_clock {
+	struct rzv2h_cpg_priv *priv;
+	struct clk_hw hw;
+	u8 on_index;
+	u8 on_bit;
+	s8 mon_index;
+	u8 mon_bit;
+};
+
+#define to_mod_clock(_hw) container_of(_hw, struct mod_clock, hw)
+
+static int rzv2h_mod_clock_endisable(struct clk_hw *hw, bool enable)
+{
+	struct mod_clock *clock = to_mod_clock(hw);
+	unsigned int reg = GET_CLK_ON_OFFSET(clock->on_index);
+	struct rzv2h_cpg_priv *priv = clock->priv;
+	u32 bitmask = BIT(clock->on_bit);
+	struct device *dev = priv->dev;
+	u32 value;
+	int error;
+
+	dev_dbg(dev, "CLK_ON 0x%x/%pC %s\n", reg, hw->clk,
+		enable ? "ON" : "OFF");
+
+	value = bitmask << 16;
+	if (enable)
+		value |= bitmask;
+
+	writel(value, priv->base + reg);
+
+	if (!enable || clock->mon_index < 0)
+		return 0;
+
+	reg = GET_CLK_MON_OFFSET(clock->mon_index);
+	bitmask = BIT(clock->mon_bit);
+	error = readl_poll_timeout_atomic(priv->base + reg, value,
+					  value & bitmask, 0, 10);
+	if (error)
+		dev_err(dev, "Failed to enable CLK_ON %p\n",
+			priv->base + reg);
+
+	return error;
+}
+
+static int rzv2h_mod_clock_enable(struct clk_hw *hw)
+{
+	return rzv2h_mod_clock_endisable(hw, true);
+}
+
+static void rzv2h_mod_clock_disable(struct clk_hw *hw)
+{
+	rzv2h_mod_clock_endisable(hw, false);
+}
+
+static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw)
+{
+	struct mod_clock *clock = to_mod_clock(hw);
+	struct rzv2h_cpg_priv *priv = clock->priv;
+	u32 bitmask;
+	u32 offset;
+
+	if (clock->mon_index >= 0) {
+		offset = GET_CLK_MON_OFFSET(clock->mon_index);
+		bitmask = BIT(clock->mon_bit);
+	} else {
+		offset = GET_CLK_ON_OFFSET(clock->on_index);
+		bitmask = BIT(clock->on_bit);
+	}
+
+	return readl(priv->base + offset) & bitmask;
+}
+
+static const struct clk_ops rzv2h_mod_clock_ops = {
+	.enable = rzv2h_mod_clock_enable,
+	.disable = rzv2h_mod_clock_disable,
+	.is_enabled = rzv2h_mod_clock_is_enabled,
+};
+
+static void __init
+rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
+			   struct rzv2h_cpg_priv *priv)
+{
+	struct mod_clock *clock = NULL;
+	struct device *dev = priv->dev;
+	struct clk_init_data init;
+	unsigned int id = mod->id;
+	struct clk *parent, *clk;
+	const char *parent_name;
+
+	WARN_DEBUG(id < priv->num_core_clks);
+	WARN_DEBUG(id >= priv->num_core_clks + priv->num_mod_clks);
+	WARN_DEBUG(mod->parent >= priv->num_core_clks + priv->num_mod_clks);
+	WARN_DEBUG(PTR_ERR(priv->clks[id]) != -ENOENT);
+
+	parent = priv->clks[mod->parent];
+	if (IS_ERR(parent)) {
+		clk = parent;
+		goto fail;
+	}
+
+	clock = devm_kzalloc(dev, sizeof(*clock), GFP_KERNEL);
+	if (!clock) {
+		clk = ERR_PTR(-ENOMEM);
+		goto fail;
+	}
+
+	init.name = mod->name;
+	init.ops = &rzv2h_mod_clock_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	if (mod->critical)
+		init.flags |= CLK_IS_CRITICAL;
+
+	parent_name = __clk_get_name(parent);
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	clock->on_index = mod->on_index;
+	clock->on_bit = mod->on_bit;
+	clock->mon_index = mod->mon_index;
+	clock->mon_bit = mod->mon_bit;
+	clock->priv = priv;
+	clock->hw.init = &init;
+
+	clk = devm_clk_register(dev, &clock->hw);
+	if (IS_ERR(clk))
+		goto fail;
+
+	priv->clks[id] = clk;
+
+	return;
+
+fail:
+	dev_err(dev, "Failed to register module clock %s: %ld\n",
+		mod->name, PTR_ERR(clk));
+}
+
+#define rcdev_to_priv(x)	container_of(x, struct rzv2h_cpg_priv, rcdev)
+
+static int rzv2h_cpg_assert(struct reset_controller_dev *rcdev,
+			    unsigned long id)
+{
+	struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev);
+	const struct rzv2h_cpg_info *info = priv->info;
+	unsigned int reg = GET_RST_OFFSET(info->resets[id].reset_index);
+	u32 mask = BIT(info->resets[id].reset_bit);
+	u8 monbit = info->resets[id].mon_bit;
+	u32 value = mask << 16;
+
+	dev_dbg(rcdev->dev, "assert id:%ld offset:0x%x\n", id, reg);
+
+	writel(value, priv->base + reg);
+
+	reg = GET_RST_MON_OFFSET(info->resets[id].mon_index);
+	mask = BIT(monbit);
+
+	return readl_poll_timeout_atomic(priv->base + reg, value,
+					 value & mask, 10, 200);
+}
+
+static int rzv2h_cpg_deassert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev);
+	const struct rzv2h_cpg_info *info = priv->info;
+	unsigned int reg = GET_RST_OFFSET(info->resets[id].reset_index);
+	u32 mask = BIT(info->resets[id].reset_bit);
+	u8 monbit = info->resets[id].mon_bit;
+	u32 value = (mask << 16) | mask;
+
+	dev_dbg(rcdev->dev, "deassert id:%ld offset:0x%x\n", id, reg);
+
+	writel(value, priv->base + reg);
+
+	reg = GET_RST_MON_OFFSET(info->resets[id].mon_index);
+	mask = BIT(monbit);
+
+	return readl_poll_timeout_atomic(priv->base + reg, value,
+					 !(value & mask), 10, 200);
+}
+
+static int rzv2h_cpg_reset(struct reset_controller_dev *rcdev,
+			   unsigned long id)
+{
+	int ret;
+
+	ret = rzv2h_cpg_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	return rzv2h_cpg_deassert(rcdev, id);
+}
+
+static int rzv2h_cpg_status(struct reset_controller_dev *rcdev,
+			    unsigned long id)
+{
+	struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev);
+	const struct rzv2h_cpg_info *info = priv->info;
+	unsigned int reg = GET_RST_MON_OFFSET(info->resets[id].mon_index);
+	u8 monbit = info->resets[id].mon_bit;
+
+	return !!(readl(priv->base + reg) & BIT(monbit));
+}
+
+static const struct reset_control_ops rzv2h_cpg_reset_ops = {
+	.reset = rzv2h_cpg_reset,
+	.assert = rzv2h_cpg_assert,
+	.deassert = rzv2h_cpg_deassert,
+	.status = rzv2h_cpg_status,
+};
+
+static int rzv2h_cpg_reset_xlate(struct reset_controller_dev *rcdev,
+				 const struct of_phandle_args *reset_spec)
+{
+	unsigned int id = reset_spec->args[0];
+
+	if (id >= rcdev->nr_resets) {
+		dev_err(rcdev->dev, "Invalid reset index %u\n", id);
+		return -EINVAL;
+	}
+
+	return id;
+}
+
+static int rzv2h_cpg_reset_controller_register(struct rzv2h_cpg_priv *priv)
+{
+	priv->rcdev.ops = &rzv2h_cpg_reset_ops;
+	priv->rcdev.of_node = priv->dev->of_node;
+	priv->rcdev.dev = priv->dev;
+	priv->rcdev.of_reset_n_cells = 1;
+	priv->rcdev.of_xlate = rzv2h_cpg_reset_xlate;
+	priv->rcdev.nr_resets = priv->num_resets;
+
+	return devm_reset_controller_register(priv->dev, &priv->rcdev);
+}
+
+/**
+ * struct rzv2h_cpg_pd - RZ/V2H power domain data structure
+ * @priv: pointer to CPG private data structure
+ * @genpd: generic PM domain
+ */
+struct rzv2h_cpg_pd {
+	struct rzv2h_cpg_priv *priv;
+	struct generic_pm_domain genpd;
+};
+
+static int rzv2h_cpg_attach_dev(struct generic_pm_domain *domain, struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args clkspec;
+	bool once = true;
+	struct clk *clk;
+	int error;
+	int i = 0;
+
+	while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
+					   &clkspec)) {
+		if (once) {
+			once = false;
+			error = pm_clk_create(dev);
+			if (error) {
+				of_node_put(clkspec.np);
+				goto err;
+			}
+		}
+		clk = of_clk_get_from_provider(&clkspec);
+		of_node_put(clkspec.np);
+		if (IS_ERR(clk)) {
+			error = PTR_ERR(clk);
+			goto fail_destroy;
+		}
+
+		error = pm_clk_add_clk(dev, clk);
+		if (error) {
+			dev_err(dev, "pm_clk_add_clk failed %d\n",
+				error);
+			goto fail_put;
+		}
+		i++;
+	}
+
+	return 0;
+
+fail_put:
+	clk_put(clk);
+
+fail_destroy:
+	pm_clk_destroy(dev);
+err:
+	return error;
+}
+
+static void rzv2h_cpg_detach_dev(struct generic_pm_domain *unused, struct device *dev)
+{
+	if (!pm_clk_no_clocks(dev))
+		pm_clk_destroy(dev);
+}
+
+static void rzv2h_cpg_genpd_remove_simple(void *data)
+{
+	pm_genpd_remove(data);
+}
+
+static int __init rzv2h_cpg_add_pm_domains(struct rzv2h_cpg_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct rzv2h_cpg_pd *pd;
+	int ret;
+
+	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	pd->genpd.name = np->name;
+	pd->priv = priv;
+	pd->genpd.flags |= GENPD_FLAG_ALWAYS_ON | GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
+	pd->genpd.attach_dev = rzv2h_cpg_attach_dev;
+	pd->genpd.detach_dev = rzv2h_cpg_detach_dev;
+	ret = pm_genpd_init(&pd->genpd, &pm_domain_always_on_gov, false);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, rzv2h_cpg_genpd_remove_simple, &pd->genpd);
+	if (ret)
+		return ret;
+
+	return of_genpd_add_provider_simple(np, &pd->genpd);
+}
+
+static void rzv2h_cpg_del_clk_provider(void *data)
+{
+	of_clk_del_provider(data);
+}
+
+static int __init rzv2h_cpg_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct rzv2h_cpg_info *info;
+	struct rzv2h_cpg_priv *priv;
+	unsigned int nclks, i;
+	struct clk **clks;
+	int error;
+
+	info = of_device_get_match_data(dev);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->info = info;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	nclks = info->num_total_core_clks + info->num_hw_mod_clks;
+	clks = devm_kmalloc_array(dev, nclks, sizeof(*clks), GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, priv);
+	priv->clks = clks;
+	priv->num_core_clks = info->num_total_core_clks;
+	priv->num_mod_clks = info->num_hw_mod_clks;
+	priv->last_dt_core_clk = info->last_dt_core_clk;
+	priv->num_resets = info->num_resets;
+
+	for (i = 0; i < nclks; i++)
+		clks[i] = ERR_PTR(-ENOENT);
+
+	for (i = 0; i < info->num_core_clks; i++)
+		rzv2h_cpg_register_core_clk(&info->core_clks[i], priv);
+
+	for (i = 0; i < info->num_mod_clks; i++)
+		rzv2h_cpg_register_mod_clk(&info->mod_clks[i], priv);
+
+	error = of_clk_add_provider(np, rzv2h_cpg_clk_src_twocell_get, priv);
+	if (error)
+		return error;
+
+	error = devm_add_action_or_reset(dev, rzv2h_cpg_del_clk_provider, np);
+	if (error)
+		return error;
+
+	error = rzv2h_cpg_add_pm_domains(priv);
+	if (error)
+		return error;
+
+	error = rzv2h_cpg_reset_controller_register(priv);
+	if (error)
+		return error;
+
+	return 0;
+}
+
+static const struct of_device_id rzv2h_cpg_match[] = {
+	{ /* sentinel */ }
+};
+
+static struct platform_driver rzv2h_cpg_driver = {
+	.driver		= {
+		.name	= "rzv2h-cpg",
+		.of_match_table = rzv2h_cpg_match,
+	},
+};
+
+static int __init rzv2h_cpg_init(void)
+{
+	return platform_driver_probe(&rzv2h_cpg_driver, rzv2h_cpg_probe);
+}
+
+subsys_initcall(rzv2h_cpg_init);
+
+MODULE_DESCRIPTION("Renesas RZ/V2H CPG Driver");
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
new file mode 100644
index 000000000000..2358782388c4
--- /dev/null
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Renesas RZ/V2H(P) Clock Pulse Generator
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#ifndef __RENESAS_RZV2H_CPG_H__
+#define __RENESAS_RZV2H_CPG_H__
+
+/**
+ * Definitions of CPG Core Clocks
+ *
+ * These include:
+ *   - Clock outputs exported to DT
+ *   - External input clocks
+ *   - Internal CPG clocks
+ */
+struct cpg_core_clk {
+	const char *name;
+	unsigned int id;
+	unsigned int parent;
+	unsigned int div;
+	unsigned int mult;
+	unsigned int type;
+	unsigned int conf;
+};
+
+enum clk_types {
+	/* Generic */
+	CLK_TYPE_IN,		/* External Clock Input */
+	CLK_TYPE_FF,		/* Fixed Factor Clock */
+	CLK_TYPE_PLL,
+};
+
+/* BIT(31) indicates if CLK1/2 are accessible or not */
+#define PLL_CONF(n)		(BIT(31) | ((n) & ~GENMASK(31, 16)))
+#define PLL_CLK_ACCESS(n)	((n) & BIT(31) ? 1 : 0)
+#define PLL_CLK1_OFFSET(n)	((n) & ~GENMASK(31, 16))
+#define PLL_CLK2_OFFSET(n)	(((n) & ~GENMASK(31, 16)) + (0x4))
+
+#define DEF_TYPE(_name, _id, _type...) \
+	{ .name = _name, .id = _id, .type = _type }
+#define DEF_BASE(_name, _id, _type, _parent...) \
+	DEF_TYPE(_name, _id, _type, .parent = _parent)
+#define DEF_PLL(_name, _id, _parent, _conf) \
+	DEF_TYPE(_name, _id, CLK_TYPE_PLL, .parent = _parent, .conf = _conf)
+#define DEF_INPUT(_name, _id) \
+	DEF_TYPE(_name, _id, CLK_TYPE_IN)
+#define DEF_FIXED(_name, _id, _parent, _mult, _div) \
+	DEF_BASE(_name, _id, CLK_TYPE_FF, _parent, .div = _div, .mult = _mult)
+
+/**
+ * struct rzv2h_mod_clk - Module Clocks definitions
+ *
+ * @name: handle between common and hardware-specific interfaces
+ * @parent: id of parent clock
+ * @id: clock index in array containing all Core and Module Clocks
+ * @critical: flag to indicate the clock is critical
+ * @on_index: control register index
+ * @on_bit: ON bit
+ * @mon_index: monitor register index
+ * @mon_bit: monitor bit
+ */
+struct rzv2h_mod_clk {
+	const char *name;
+	unsigned int parent;
+	unsigned int id;
+	bool critical;
+	u8 on_index;
+	u8 on_bit;
+	s8 mon_index;
+	u8 mon_bit;
+};
+
+#define DEF_MOD_BASE(_name, _parent, _id, _critical, _onindex, _onbit, _monindex, _monbit) \
+	{ \
+		.name = (_name), \
+		.parent = (_parent), \
+		.id = (_id), \
+		.critical = (_critical), \
+		.on_index = (_onindex), \
+		.on_bit = (_onbit), \
+		.mon_index = (_monindex), \
+		.mon_bit = (_monbit), \
+	}
+
+#define MOD_CLK_ID(x)	(MOD_CLK_BASE + (x))
+#define MOD_ID(x, y)	((((x) * 16)) + (y))
+
+#define DEF_MOD(_name, _parent, _onindex, _onbit, _monindex, _monbit)		\
+	DEF_MOD_BASE(_name, _parent, MOD_CLK_ID(MOD_ID(_onindex, _onbit)),	\
+		     false, _onindex, _onbit, _monindex, _monbit)
+
+#define DEF_MOD_CRITICAL(_name, _parent, _onindex, _onbit, _monindex, _monbit)	\
+	DEF_MOD_BASE(_name, _parent, MOD_CLK_ID(MOD_ID(_onindex, _onbit)),	\
+		     true, _onindex, _onbit, _monindex, _monbit)
+
+/**
+ * struct rzv2h_reset - Reset definitions
+ *
+ * @reset_index: reset register index
+ * @reset_bit: reset bit
+ * @mon_index: monitor register index
+ * @mon_bit: monitor bit
+ */
+struct rzv2h_reset {
+	u8 reset_index;
+	u8 reset_bit;
+	u8 mon_index;
+	u8 mon_bit;
+};
+
+#define RST_ID(x, y)	((((x) * 16)) + (y))
+
+#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)	\
+	[_id] = { \
+		.reset_index = (_resindex), \
+		.reset_bit = (_resbit), \
+		.mon_index = (_monindex), \
+		.mon_bit = (_monbit), \
+	}
+
+#define DEF_RST(_resindex, _resbit, _monindex, _monbit)	\
+	DEF_RST_BASE(RST_ID((_resindex), (_resbit)), _resindex, _resbit, _monindex, _monbit)
+
+/**
+ * struct rzv2h_cpg_info - SoC-specific CPG Description
+ *
+ * @core_clks: Array of Core Clock definitions
+ * @num_core_clks: Number of entries in core_clks[]
+ * @last_dt_core_clk: ID of the last Core Clock exported to DT
+ * @num_total_core_clks: Total number of Core Clocks (exported + internal)
+ *
+ * @mod_clks: Array of Module Clock definitions
+ * @num_mod_clks: Number of entries in mod_clks[]
+ * @num_hw_mod_clks: Number of Module Clocks supported by the hardware
+ *
+ * @resets: Array of Module Reset definitions
+ * @num_resets: Number of entries in resets[]
+ */
+struct rzv2h_cpg_info {
+	/* Core Clocks */
+	const struct cpg_core_clk *core_clks;
+	unsigned int num_core_clks;
+	unsigned int last_dt_core_clk;
+	unsigned int num_total_core_clks;
+
+	/* Module Clocks */
+	const struct rzv2h_mod_clk *mod_clks;
+	unsigned int num_mod_clks;
+	unsigned int num_hw_mod_clks;
+
+	/* Resets */
+	const struct rzv2h_reset *resets;
+	unsigned int num_resets;
+};
+
+#endif	/* __RENESAS_RZV2H_CPG_H__ */
-- 
2.34.1

Re: [PATCH v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add family-specific clock driver for RZ/V2H(P) SoCs.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - Dropped num_hw_resets from struct rzv2h_cpg_priv
> - Dropped range_check for module clocks
> - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
> - Added support for critical module clocks with DEF_MOD_CRITICAL
> - Added check for mon_index in rzv2h_mod_clock_endisable and
>   rzv2h_mod_clock_is_enabled()

Thanks for the update!

> --- /dev/null
> +++ b/drivers/clk/renesas/rzv2h-cpg.c

> +static struct clk * __init
> +rzv2h_cpg_pll_clk_register(const struct cpg_core_clk *core,
> +                          struct rzv2h_cpg_priv *priv,
> +                          const struct clk_ops *ops)
> +{
> +       void __iomem *base = priv->base;
> +       struct clk **clks = priv->clks;
> +       struct device *dev = priv->dev;
> +       struct clk_init_data init;
> +       const struct clk *parent;
> +       const char *parent_name;
> +       struct pll_clk *pll_clk;
> +
> +       parent = clks[core->parent & 0xffff];

No need to mask with 0xffff, as nothing is ever stored in the high bits.

> +static void __init
> +rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
> +                          struct rzv2h_cpg_priv *priv)
> +{
> +       struct mod_clock *clock = NULL;
> +       struct device *dev = priv->dev;
> +       struct clk_init_data init;
> +       unsigned int id = mod->id;

This is the sole user of mod->id, which can be calculated easily from
mod->on_index and mod->on_bit.

> --- /dev/null
> +++ b/drivers/clk/renesas/rzv2h-cpg.h

> +/**
> + * struct rzv2h_mod_clk - Module Clocks definitions
> + *
> + * @name: handle between common and hardware-specific interfaces
> + * @parent: id of parent clock
> + * @id: clock index in array containing all Core and Module Clocks
> + * @critical: flag to indicate the clock is critical
> + * @on_index: control register index
> + * @on_bit: ON bit
> + * @mon_index: monitor register index
> + * @mon_bit: monitor bit
> + */
> +struct rzv2h_mod_clk {
> +       const char *name;
> +       unsigned int parent;
> +       unsigned int id;

No need to store the id, as it can be calculated when needed.

> +       bool critical;
> +       u8 on_index;
> +       u8 on_bit;
> +       s8 mon_index;
> +       u8 mon_bit;

That leaves us with 1 64-bit pointer, 1 32-bit integer, and 5 bytes.
Using bitfields for the latter is complicated due to the mix of signed
and unsigned values.
However, parent can be reduced to u16, shaving off one 64-bit word
from each entry.

> +};

> +/**
> + * struct rzv2h_reset - Reset definitions
> + *
> + * @reset_index: reset register index
> + * @reset_bit: reset bit
> + * @mon_index: monitor register index
> + * @mon_bit: monitor bit
> + */
> +struct rzv2h_reset {
> +       u8 reset_index;
> +       u8 reset_bit;
> +       u8 mon_index;
> +       u8 mon_bit;
> +};
> +
> +#define RST_ID(x, y)   ((((x) * 16)) + (y))
> +
> +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)      \
> +       [_id] = { \

Indexing by _id means the reset array will be very sparse.  E.g. the
innocent-looking r9a09g057_resets[] with only a single entry takes
600 bytes.

If you do need the full array for indexing, please allocate and
populate it at runtime.
As a bonus, you would no longer need rzv2h_cpg_info.info, and
r9a09g057_resets[] and r9a09g057_cpg_info[] can become __initconst.

> +               .reset_index = (_resindex), \
> +               .reset_bit = (_resbit), \
> +               .mon_index = (_monindex), \
> +               .mon_bit = (_monbit), \
> +       }

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 v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Fri, Jul 12, 2024 at 12:59 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add family-specific clock driver for RZ/V2H(P) SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3
> > - Dropped num_hw_resets from struct rzv2h_cpg_priv
> > - Dropped range_check for module clocks
> > - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
> > - Added support for critical module clocks with DEF_MOD_CRITICAL
> > - Added check for mon_index in rzv2h_mod_clock_endisable and
> >   rzv2h_mod_clock_is_enabled()
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > +static struct clk * __init
> > +rzv2h_cpg_pll_clk_register(const struct cpg_core_clk *core,
> > +                          struct rzv2h_cpg_priv *priv,
> > +                          const struct clk_ops *ops)
> > +{
> > +       void __iomem *base = priv->base;
> > +       struct clk **clks = priv->clks;
> > +       struct device *dev = priv->dev;
> > +       struct clk_init_data init;
> > +       const struct clk *parent;
> > +       const char *parent_name;
> > +       struct pll_clk *pll_clk;
> > +
> > +       parent = clks[core->parent & 0xffff];
>
> No need to mask with 0xffff, as nothing is ever stored in the high bits.
>
OK, I will drop it.

> > +static void __init
> > +rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
> > +                          struct rzv2h_cpg_priv *priv)
> > +{
> > +       struct mod_clock *clock = NULL;
> > +       struct device *dev = priv->dev;
> > +       struct clk_init_data init;
> > +       unsigned int id = mod->id;
>
> This is the sole user of mod->id, which can be calculated easily from
> mod->on_index and mod->on_bit.
>
Agreed, I will drop id.

> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.h
>
> > +/**
> > + * struct rzv2h_mod_clk - Module Clocks definitions
> > + *
> > + * @name: handle between common and hardware-specific interfaces
> > + * @parent: id of parent clock
> > + * @id: clock index in array containing all Core and Module Clocks
> > + * @critical: flag to indicate the clock is critical
> > + * @on_index: control register index
> > + * @on_bit: ON bit
> > + * @mon_index: monitor register index
> > + * @mon_bit: monitor bit
> > + */
> > +struct rzv2h_mod_clk {
> > +       const char *name;
> > +       unsigned int parent;
> > +       unsigned int id;
>
> No need to store the id, as it can be calculated when needed.
>
OK.

> > +       bool critical;
> > +       u8 on_index;
> > +       u8 on_bit;
> > +       s8 mon_index;
> > +       u8 mon_bit;
>
> That leaves us with 1 64-bit pointer, 1 32-bit integer, and 5 bytes.
> Using bitfields for the latter is complicated due to the mix of signed
> and unsigned values.
> However, parent can be reduced to u16, shaving off one 64-bit word
> from each entry.
>
Agreed, I will update parent to u16.

> > +};
>
> > +/**
> > + * struct rzv2h_reset - Reset definitions
> > + *
> > + * @reset_index: reset register index
> > + * @reset_bit: reset bit
> > + * @mon_index: monitor register index
> > + * @mon_bit: monitor bit
> > + */
> > +struct rzv2h_reset {
> > +       u8 reset_index;
> > +       u8 reset_bit;
> > +       u8 mon_index;
> > +       u8 mon_bit;
> > +};
> > +
> > +#define RST_ID(x, y)   ((((x) * 16)) + (y))
> > +
> > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)      \
> > +       [_id] = { \
>
> Indexing by _id means the reset array will be very sparse.  E.g. the
> innocent-looking r9a09g057_resets[] with only a single entry takes
> 600 bytes.
>
> If you do need the full array for indexing, please allocate and
> populate it at runtime.
>
OK, I will use the radix tree for resets (is that OK)?

> As a bonus, you would no longer need rzv2h_cpg_info.info, and
> r9a09g057_resets[] and r9a09g057_cpg_info[] can become __initconst.
>
Agreed (and also r9a09g057_mod_clks).

Cheers,
Prabhakar

Re: [PATCH v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Geert Uytterhoeven]

Hi Prabhakar,

On Fri, Jul 12, 2024 at 5:14 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, Jul 12, 2024 at 12:59 PM Geert Uytterhoeven
> > On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Add family-specific clock driver for RZ/V2H(P) SoCs.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v2->v3
> > > - Dropped num_hw_resets from struct rzv2h_cpg_priv
> > > - Dropped range_check for module clocks
> > > - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
> > > - Added support for critical module clocks with DEF_MOD_CRITICAL
> > > - Added check for mon_index in rzv2h_mod_clock_endisable and
> > >   rzv2h_mod_clock_is_enabled()

> > > --- /dev/null
> > > +++ b/drivers/clk/renesas/rzv2h-cpg.h

> > > +/**
> > > + * struct rzv2h_reset - Reset definitions
> > > + *
> > > + * @reset_index: reset register index
> > > + * @reset_bit: reset bit
> > > + * @mon_index: monitor register index
> > > + * @mon_bit: monitor bit
> > > + */
> > > +struct rzv2h_reset {
> > > +       u8 reset_index;
> > > +       u8 reset_bit;
> > > +       u8 mon_index;
> > > +       u8 mon_bit;
> > > +};
> > > +
> > > +#define RST_ID(x, y)   ((((x) * 16)) + (y))
> > > +
> > > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)      \
> > > +       [_id] = { \
> >
> > Indexing by _id means the reset array will be very sparse.  E.g. the
> > innocent-looking r9a09g057_resets[] with only a single entry takes
> > 600 bytes.
> >
> > If you do need the full array for indexing, please allocate and
> > populate it at runtime.
> >
> OK, I will use the radix tree for resets (is that OK)?

You mean XArray? include/linux/radix-tree.h has:

    /* Keep unconverted code working */
    #define radix_tree_root         xarray
    #define radix_tree_node         xa_node

Given a single xa_node is already 576 bytes, just allocating the full
linear reset array at runtime is probably better.

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 v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Lad, Prabhakar]

Hi Geert,

On Fri, Jul 12, 2024 at 4:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Jul 12, 2024 at 5:14 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Fri, Jul 12, 2024 at 12:59 PM Geert Uytterhoeven
> > > On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Add family-specific clock driver for RZ/V2H(P) SoCs.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > v2->v3
> > > > - Dropped num_hw_resets from struct rzv2h_cpg_priv
> > > > - Dropped range_check for module clocks
> > > > - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
> > > > - Added support for critical module clocks with DEF_MOD_CRITICAL
> > > > - Added check for mon_index in rzv2h_mod_clock_endisable and
> > > >   rzv2h_mod_clock_is_enabled()
>
> > > > --- /dev/null
> > > > +++ b/drivers/clk/renesas/rzv2h-cpg.h
>
> > > > +/**
> > > > + * struct rzv2h_reset - Reset definitions
> > > > + *
> > > > + * @reset_index: reset register index
> > > > + * @reset_bit: reset bit
> > > > + * @mon_index: monitor register index
> > > > + * @mon_bit: monitor bit
> > > > + */
> > > > +struct rzv2h_reset {
> > > > +       u8 reset_index;
> > > > +       u8 reset_bit;
> > > > +       u8 mon_index;
> > > > +       u8 mon_bit;
> > > > +};
> > > > +
> > > > +#define RST_ID(x, y)   ((((x) * 16)) + (y))
> > > > +
> > > > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)      \
> > > > +       [_id] = { \
> > >
> > > Indexing by _id means the reset array will be very sparse.  E.g. the
> > > innocent-looking r9a09g057_resets[] with only a single entry takes
> > > 600 bytes.
> > >
> > > If you do need the full array for indexing, please allocate and
> > > populate it at runtime.
> > >
> > OK, I will use the radix tree for resets (is that OK)?
>
> You mean XArray? include/linux/radix-tree.h has:
>
>     /* Keep unconverted code working */
>     #define radix_tree_root         xarray
>     #define radix_tree_node         xa_node
>
Yes, I meant the above.

> Given a single xa_node is already 576 bytes, just allocating the full
> linear reset array at runtime is probably better.
>
Agreed, I will create a linear reset array and loop through the array
based on reset index and reset bit to match with id whenever required.

Cheers,
Prabhakar

Re: [PATCH v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Geert Uytterhoeven]

Hi Prabhakar,

On Fri, Jul 12, 2024 at 5:29 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, Jul 12, 2024 at 4:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Jul 12, 2024 at 5:14 PM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > > On Fri, Jul 12, 2024 at 12:59 PM Geert Uytterhoeven
> > > > On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Add family-specific clock driver for RZ/V2H(P) SoCs.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > > v2->v3
> > > > > - Dropped num_hw_resets from struct rzv2h_cpg_priv
> > > > > - Dropped range_check for module clocks
> > > > > - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
> > > > > - Added support for critical module clocks with DEF_MOD_CRITICAL
> > > > > - Added check for mon_index in rzv2h_mod_clock_endisable and
> > > > >   rzv2h_mod_clock_is_enabled()
> >
> > > > > --- /dev/null
> > > > > +++ b/drivers/clk/renesas/rzv2h-cpg.h
> >
> > > > > +/**
> > > > > + * struct rzv2h_reset - Reset definitions
> > > > > + *
> > > > > + * @reset_index: reset register index
> > > > > + * @reset_bit: reset bit
> > > > > + * @mon_index: monitor register index
> > > > > + * @mon_bit: monitor bit
> > > > > + */
> > > > > +struct rzv2h_reset {
> > > > > +       u8 reset_index;
> > > > > +       u8 reset_bit;
> > > > > +       u8 mon_index;
> > > > > +       u8 mon_bit;
> > > > > +};
> > > > > +
> > > > > +#define RST_ID(x, y)   ((((x) * 16)) + (y))
> > > > > +
> > > > > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)      \
> > > > > +       [_id] = { \
> > > >
> > > > Indexing by _id means the reset array will be very sparse.  E.g. the
> > > > innocent-looking r9a09g057_resets[] with only a single entry takes
> > > > 600 bytes.
> > > >
> > > > If you do need the full array for indexing, please allocate and
> > > > populate it at runtime.
> > > >
> > > OK, I will use the radix tree for resets (is that OK)?
> >
> > You mean XArray? include/linux/radix-tree.h has:
> >
> >     /* Keep unconverted code working */
> >     #define radix_tree_root         xarray
> >     #define radix_tree_node         xa_node
> >
> Yes, I meant the above.
>
> > Given a single xa_node is already 576 bytes, just allocating the full
> > linear reset array at runtime is probably better.
> >
> Agreed, I will create a linear reset array and loop through the array
> based on reset index and reset bit to match with id whenever required.

With a full allocated linear reset array you do not need to loop,
but you can just index it by the reset ID??

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 v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Lad, Prabhakar]

Hi Geert,

On Fri, Jul 12, 2024 at 6:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Jul 12, 2024 at 5:29 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Fri, Jul 12, 2024 at 4:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Jul 12, 2024 at 5:14 PM Lad, Prabhakar
> > > <prabhakar.csengg@gmail.com> wrote:
> > > > On Fri, Jul 12, 2024 at 12:59 PM Geert Uytterhoeven
> > > > > On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > Add family-specific clock driver for RZ/V2H(P) SoCs.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > > v2->v3
> > > > > > - Dropped num_hw_resets from struct rzv2h_cpg_priv
> > > > > > - Dropped range_check for module clocks
> > > > > > - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
> > > > > > - Added support for critical module clocks with DEF_MOD_CRITICAL
> > > > > > - Added check for mon_index in rzv2h_mod_clock_endisable and
> > > > > >   rzv2h_mod_clock_is_enabled()
> > >
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/clk/renesas/rzv2h-cpg.h
> > >
> > > > > > +/**
> > > > > > + * struct rzv2h_reset - Reset definitions
> > > > > > + *
> > > > > > + * @reset_index: reset register index
> > > > > > + * @reset_bit: reset bit
> > > > > > + * @mon_index: monitor register index
> > > > > > + * @mon_bit: monitor bit
> > > > > > + */
> > > > > > +struct rzv2h_reset {
> > > > > > +       u8 reset_index;
> > > > > > +       u8 reset_bit;
> > > > > > +       u8 mon_index;
> > > > > > +       u8 mon_bit;
> > > > > > +};
> > > > > > +
> > > > > > +#define RST_ID(x, y)   ((((x) * 16)) + (y))
> > > > > > +
> > > > > > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)      \
> > > > > > +       [_id] = { \
> > > > >
> > > > > Indexing by _id means the reset array will be very sparse.  E.g. the
> > > > > innocent-looking r9a09g057_resets[] with only a single entry takes
> > > > > 600 bytes.
> > > > >
> > > > > If you do need the full array for indexing, please allocate and
> > > > > populate it at runtime.
> > > > >
> > > > OK, I will use the radix tree for resets (is that OK)?
> > >
> > > You mean XArray? include/linux/radix-tree.h has:
> > >
> > >     /* Keep unconverted code working */
> > >     #define radix_tree_root         xarray
> > >     #define radix_tree_node         xa_node
> > >
> > Yes, I meant the above.
> >
> > > Given a single xa_node is already 576 bytes, just allocating the full
> > > linear reset array at runtime is probably better.
> > >
> > Agreed, I will create a linear reset array and loop through the array
> > based on reset index and reset bit to match with id whenever required.
>
> With a full allocated linear reset array you do not need to loop,
> but you can just index it by the reset ID??
>
Instead of having a sparse array, to save memory I was thinking
something like below:

/**
 * struct rzv2h_reset - Reset definitions
 *
 * @reset_index: reset register index
 * @reset_bit: reset bit
 * @mon_index: monitor register index
 * @mon_bit: monitor bit
 */
struct rzv2h_reset {
    u8 reset_index;
    u8 reset_bit;
    u8 mon_index;
    u8 mon_bit;
};

#define DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)    \
    { \
        .reset_index = (_resindex), \
        .reset_bit = (_resbit), \
        .mon_index = (_monindex), \
        .mon_bit = (_monbit), \
    }

#define DEF_RST(_resindex, _resbit, _monindex, _monbit)    \
    DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)


in rzv2h_cpg_probe() (.num_resets = ARRAY_SIZE(r9a09g057_resets))

    resets = devm_kmalloc_array(dev, info->num_resets, sizeof(struct
rzv2h_reset), GFP_KERNEL);
    if (!resets)
        return -ENOMEM;

    for (i = 0; i < priv->num_resets; i++)
        memcpy(&resets[i], &info->resets[i], sizeof(struct rzv2h_reset));

And have the below xlate function that will convert id into index ie
index into rests array.

static int rzv2h_get_reset_index(struct rzv2h_cpg_priv *priv,
                 unsigned long id)
{
    u8 reset_index = id / 16;
    u8 reset_bit = id % 16;
    unsigned int i;

    for (i = 0; i < priv->num_resets; i++) {
        if (priv->resets[i].reset_index == reset_index &&
            priv->resets[i].reset_bit == reset_bit)
            return i;
    }

    return -EINVAL;
}

static int rzv2h_cpg_reset_xlate(struct reset_controller_dev *rcdev,
                 const struct of_phandle_args *reset_spec)
{
    struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev);
    unsigned int id = reset_spec->args[0];
    int index = rzv2h_get_reset_index(priv, id);

    if (index < 0) {
        dev_err(rcdev->dev, "Invalid reset index %u\n", id);
        return -EINVAL;
    }

    return index;
}


rzv2h_cpg_assert() and rzv2h_cpg_deassert() which will use an id that
can directly index into resets[] array.

Please let me know if this is OK.

Cheers,
Prabhakar

Re: [PATCH v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Jul 15, 2024 at 10:44 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, Jul 12, 2024 at 6:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Jul 12, 2024 at 5:29 PM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > > On Fri, Jul 12, 2024 at 4:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Fri, Jul 12, 2024 at 5:14 PM Lad, Prabhakar
> > > > <prabhakar.csengg@gmail.com> wrote:
> > > > > On Fri, Jul 12, 2024 at 12:59 PM Geert Uytterhoeven
> > > > > > On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > >
> > > > > > > Add family-specific clock driver for RZ/V2H(P) SoCs.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > ---
> > > > > > > v2->v3
> > > > > > > - Dropped num_hw_resets from struct rzv2h_cpg_priv
> > > > > > > - Dropped range_check for module clocks
> > > > > > > - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk
> > > > > > > - Added support for critical module clocks with DEF_MOD_CRITICAL
> > > > > > > - Added check for mon_index in rzv2h_mod_clock_endisable and
> > > > > > >   rzv2h_mod_clock_is_enabled()
> > > >
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/clk/renesas/rzv2h-cpg.h
> > > >
> > > > > > > +/**
> > > > > > > + * struct rzv2h_reset - Reset definitions
> > > > > > > + *
> > > > > > > + * @reset_index: reset register index
> > > > > > > + * @reset_bit: reset bit
> > > > > > > + * @mon_index: monitor register index
> > > > > > > + * @mon_bit: monitor bit
> > > > > > > + */
> > > > > > > +struct rzv2h_reset {
> > > > > > > +       u8 reset_index;
> > > > > > > +       u8 reset_bit;
> > > > > > > +       u8 mon_index;
> > > > > > > +       u8 mon_bit;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define RST_ID(x, y)   ((((x) * 16)) + (y))
> > > > > > > +
> > > > > > > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit)      \
> > > > > > > +       [_id] = { \
> > > > > >
> > > > > > Indexing by _id means the reset array will be very sparse.  E.g. the
> > > > > > innocent-looking r9a09g057_resets[] with only a single entry takes
> > > > > > 600 bytes.
> > > > > >
> > > > > > If you do need the full array for indexing, please allocate and
> > > > > > populate it at runtime.
> > > > > >
> > > > > OK, I will use the radix tree for resets (is that OK)?
> > > >
> > > > You mean XArray? include/linux/radix-tree.h has:
> > > >
> > > >     /* Keep unconverted code working */
> > > >     #define radix_tree_root         xarray
> > > >     #define radix_tree_node         xa_node
> > > >
> > > Yes, I meant the above.
> > >
> > > > Given a single xa_node is already 576 bytes, just allocating the full
> > > > linear reset array at runtime is probably better.
> > > >
> > > Agreed, I will create a linear reset array and loop through the array
> > > based on reset index and reset bit to match with id whenever required.
> >
> > With a full allocated linear reset array you do not need to loop,
> > but you can just index it by the reset ID??
> >
> Instead of having a sparse array, to save memory I was thinking
> something like below:
>
> /**
>  * struct rzv2h_reset - Reset definitions
>  *
>  * @reset_index: reset register index
>  * @reset_bit: reset bit
>  * @mon_index: monitor register index
>  * @mon_bit: monitor bit
>  */
> struct rzv2h_reset {
>     u8 reset_index;
>     u8 reset_bit;
>     u8 mon_index;
>     u8 mon_bit;
> };
>
> #define DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)    \
>     { \
>         .reset_index = (_resindex), \
>         .reset_bit = (_resbit), \
>         .mon_index = (_monindex), \
>         .mon_bit = (_monbit), \
>     }
>
> #define DEF_RST(_resindex, _resbit, _monindex, _monbit)    \
>     DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)
>
>
> in rzv2h_cpg_probe() (.num_resets = ARRAY_SIZE(r9a09g057_resets))
>
>     resets = devm_kmalloc_array(dev, info->num_resets, sizeof(struct
> rzv2h_reset), GFP_KERNEL);
>     if (!resets)
>         return -ENOMEM;
>
>     for (i = 0; i < priv->num_resets; i++)
>         memcpy(&resets[i], &info->resets[i], sizeof(struct rzv2h_reset));

You can combine both using devm_kmemdup().

> And have the below xlate function that will convert id into index ie
> index into rests array.
>
> static int rzv2h_get_reset_index(struct rzv2h_cpg_priv *priv,
>                  unsigned long id)
> {
>     u8 reset_index = id / 16;
>     u8 reset_bit = id % 16;
>     unsigned int i;
>
>     for (i = 0; i < priv->num_resets; i++) {
>         if (priv->resets[i].reset_index == reset_index &&
>             priv->resets[i].reset_bit == reset_bit)
>             return i;
>     }
>
>     return -EINVAL;
> }
>
> static int rzv2h_cpg_reset_xlate(struct reset_controller_dev *rcdev,
>                  const struct of_phandle_args *reset_spec)
> {
>     struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev);
>     unsigned int id = reset_spec->args[0];
>     int index = rzv2h_get_reset_index(priv, id);
>
>     if (index < 0) {
>         dev_err(rcdev->dev, "Invalid reset index %u\n", id);
>         return -EINVAL;
>     }
>
>     return index;
> }
>
>
> rzv2h_cpg_assert() and rzv2h_cpg_deassert() which will use an id that
> can directly index into resets[] array.
>
> Please let me know if this is OK.

That would work, too, at the expense of needing a loop for look-up
(traditional trade-off between memory and time ;-)
But look-up is only done once (per device), so that should be fine.

It all depends on how many resets you will end up using...
Memory allocation also has a granularity, so once you have more
than a specific number of resets, you better use a sparse array,
and simple indexing.

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 v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Lad, Prabhakar]

Hi Geert,

On Mon, Jul 15, 2024 at 10:42 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 15, 2024 at 10:44 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Fri, Jul 12, 2024 at 6:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Jul 12, 2024 at 5:29 PM Lad, Prabhakar
> > > <prabhakar.csengg@gmail.com> wrote:
<snip>
> > /**
> >  * struct rzv2h_reset - Reset definitions
> >  *
> >  * @reset_index: reset register index
> >  * @reset_bit: reset bit
> >  * @mon_index: monitor register index
> >  * @mon_bit: monitor bit
> >  */
> > struct rzv2h_reset {
> >     u8 reset_index;
> >     u8 reset_bit;
> >     u8 mon_index;
> >     u8 mon_bit;
> > };
> >
> > #define DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)    \
> >     { \
> >         .reset_index = (_resindex), \
> >         .reset_bit = (_resbit), \
> >         .mon_index = (_monindex), \
> >         .mon_bit = (_monbit), \
> >     }
> >
> > #define DEF_RST(_resindex, _resbit, _monindex, _monbit)    \
> >     DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)
> >
> >
> > in rzv2h_cpg_probe() (.num_resets = ARRAY_SIZE(r9a09g057_resets))
> >
> >     resets = devm_kmalloc_array(dev, info->num_resets, sizeof(struct
> > rzv2h_reset), GFP_KERNEL);
> >     if (!resets)
> >         return -ENOMEM;
> >
> >     for (i = 0; i < priv->num_resets; i++)
> >         memcpy(&resets[i], &info->resets[i], sizeof(struct rzv2h_reset));
>
> You can combine both using devm_kmemdup().
>
Thanks for the pointer.

> > And have the below xlate function that will convert id into index ie
> > index into rests array.
> >
> > static int rzv2h_get_reset_index(struct rzv2h_cpg_priv *priv,
> >                  unsigned long id)
> > {
> >     u8 reset_index = id / 16;
> >     u8 reset_bit = id % 16;
> >     unsigned int i;
> >
> >     for (i = 0; i < priv->num_resets; i++) {
> >         if (priv->resets[i].reset_index == reset_index &&
> >             priv->resets[i].reset_bit == reset_bit)
> >             return i;
> >     }
> >
> >     return -EINVAL;
> > }
> >
> > static int rzv2h_cpg_reset_xlate(struct reset_controller_dev *rcdev,
> >                  const struct of_phandle_args *reset_spec)
> > {
> >     struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev);
> >     unsigned int id = reset_spec->args[0];
> >     int index = rzv2h_get_reset_index(priv, id);
> >
> >     if (index < 0) {
> >         dev_err(rcdev->dev, "Invalid reset index %u\n", id);
> >         return -EINVAL;
> >     }
> >
> >     return index;
> > }
> >
> >
> > rzv2h_cpg_assert() and rzv2h_cpg_deassert() which will use an id that
> > can directly index into resets[] array.
> >
> > Please let me know if this is OK.
>
> That would work, too, at the expense of needing a loop for look-up
> (traditional trade-off between memory and time ;-)
> But look-up is only done once (per device), so that should be fine.
>
> It all depends on how many resets you will end up using...
> Memory allocation also has a granularity, so once you have more
> than a specific number of resets, you better use a sparse array,
> and simple indexing.
>
Ok got you, I'll update it and send a new version.

Cheers,
Prabhakar

[PATCH v3 3/3] clk: renesas: Add RZ/V2H(P) CPG driver

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add RZ/V2H(P) CPG driver.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3
- Added CLK_PLLDTY
- Added core clocks sys_0_pclk and iotop_0_shclk
- Dropped r9a09g057_crit_mod_clks

v1->v2
- Updated commit description
- Dropped pll_clk1/clk2_offset
- Made r9a09g057_mod_clks/r9a09g057_resets as static const
- Now using register indexes
---
 drivers/clk/renesas/Kconfig         |  5 ++
 drivers/clk/renesas/Makefile        |  1 +
 drivers/clk/renesas/r9a09g057-cpg.c | 79 +++++++++++++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.c     |  4 ++
 drivers/clk/renesas/rzv2h-cpg.h     |  2 +
 5 files changed, 91 insertions(+)
 create mode 100644 drivers/clk/renesas/r9a09g057-cpg.c

diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
index f078ccb635bb..76791a1c50ac 100644
--- a/drivers/clk/renesas/Kconfig
+++ b/drivers/clk/renesas/Kconfig
@@ -40,6 +40,7 @@ config CLK_RENESAS
 	select CLK_R9A07G054 if ARCH_R9A07G054
 	select CLK_R9A08G045 if ARCH_R9A08G045
 	select CLK_R9A09G011 if ARCH_R9A09G011
+	select CLK_R9A09G057 if ARCH_R9A09G057
 	select CLK_SH73A0 if ARCH_SH73A0
 
 if CLK_RENESAS
@@ -193,6 +194,10 @@ config CLK_R9A09G011
 	bool "RZ/V2M clock support" if COMPILE_TEST
 	select CLK_RZG2L
 
+config CLK_R9A09G057
+       bool "RZ/V2H(P) clock support" if COMPILE_TEST
+       select CLK_RZV2H
+
 config CLK_SH73A0
 	bool "SH-Mobile AG5 clock support" if COMPILE_TEST
 	select CLK_RENESAS_CPG_MSTP
diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile
index d81a62e78345..23d2e26051c8 100644
--- a/drivers/clk/renesas/Makefile
+++ b/drivers/clk/renesas/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_CLK_R9A07G044)		+= r9a07g044-cpg.o
 obj-$(CONFIG_CLK_R9A07G054)		+= r9a07g044-cpg.o
 obj-$(CONFIG_CLK_R9A08G045)		+= r9a08g045-cpg.o
 obj-$(CONFIG_CLK_R9A09G011)		+= r9a09g011-cpg.o
+obj-$(CONFIG_CLK_R9A09G057)		+= r9a09g057-cpg.o
 obj-$(CONFIG_CLK_SH73A0)		+= clk-sh73a0.o
 
 # Family
diff --git a/drivers/clk/renesas/r9a09g057-cpg.c b/drivers/clk/renesas/r9a09g057-cpg.c
new file mode 100644
index 000000000000..8b2be17901b2
--- /dev/null
+++ b/drivers/clk/renesas/r9a09g057-cpg.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2H(P) CPG driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+
+#include <dt-bindings/clock/renesas,r9a09g057-cpg.h>
+
+#include "rzv2h-cpg.h"
+
+enum clk_ids {
+	/* Core Clock Outputs exported to DT */
+	LAST_DT_CORE_CLK = R9A09G057_IOTOP_0_SHCLK,
+
+	/* External Input Clocks */
+	CLK_AUDIO_EXTAL,
+	CLK_RTXIN,
+	CLK_QEXTAL,
+
+	/* PLL Clocks */
+	CLK_PLLCM33,
+	CLK_PLLDTY,
+	CLK_PLLCA55,
+
+	/* Internal Core Clocks */
+	CLK_PLLCM33_DIV16,
+
+	/* Module Clocks */
+	MOD_CLK_BASE,
+};
+
+static const struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
+	/* External Clock Inputs */
+	DEF_INPUT("audio_extal", CLK_AUDIO_EXTAL),
+	DEF_INPUT("rtxin", CLK_RTXIN),
+	DEF_INPUT("qextal", CLK_QEXTAL),
+
+	/* PLL Clocks */
+	DEF_FIXED(".pllcm33", CLK_PLLCM33, CLK_QEXTAL, 200, 3),
+	DEF_FIXED(".plldty", CLK_PLLDTY, CLK_QEXTAL, 200, 3),
+	DEF_PLL(".pllca55", CLK_PLLCA55, CLK_QEXTAL, PLL_CONF(0x64)),
+
+	/* Internal Core Clocks */
+	DEF_FIXED(".pllcm33_div16", CLK_PLLCM33_DIV16, CLK_PLLCM33, 1, 16),
+
+	DEF_FIXED("sys_0_pclk", R9A09G057_SYS_0_PCLK, CLK_QEXTAL, 1, 1),
+	DEF_FIXED("iotop_0_shclk", R9A09G057_IOTOP_0_SHCLK, CLK_PLLCM33_DIV16, 1, 1),
+};
+
+static const struct rzv2h_mod_clk r9a09g057_mod_clks[] = {
+	DEF_MOD("scif_0_clk_pck",		CLK_PLLCM33_DIV16, 8, 15, 4, 15),
+};
+
+static const struct rzv2h_reset r9a09g057_resets[] = {
+	DEF_RST(9, 5, 4, 6),		/* SCIF_0_RST_SYSTEM_N */
+};
+
+const struct rzv2h_cpg_info r9a09g057_cpg_info = {
+	/* Core Clocks */
+	.core_clks = r9a09g057_core_clks,
+	.num_core_clks = ARRAY_SIZE(r9a09g057_core_clks),
+	.last_dt_core_clk = LAST_DT_CORE_CLK,
+	.num_total_core_clks = MOD_CLK_BASE,
+
+	/* Module Clocks */
+	.mod_clks = r9a09g057_mod_clks,
+	.num_mod_clks = ARRAY_SIZE(r9a09g057_mod_clks),
+	.num_hw_mod_clks = 25 * 16,
+
+	/* Resets */
+	.resets = r9a09g057_resets,
+	.num_resets = ARRAY_SIZE(r9a09g057_resets),
+};
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index 058d79673ae6..6d5427d82412 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -643,6 +643,10 @@ static int __init rzv2h_cpg_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id rzv2h_cpg_match[] = {
+	{
+		.compatible = "renesas,r9a09g057-cpg",
+		.data = &r9a09g057_cpg_info,
+	},
 	{ /* sentinel */ }
 };
 
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index 2358782388c4..17a3c02528d6 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -156,4 +156,6 @@ struct rzv2h_cpg_info {
 	unsigned int num_resets;
 };
 
+extern const struct rzv2h_cpg_info r9a09g057_cpg_info;
+
 #endif	/* __RENESAS_RZV2H_CPG_H__ */
-- 
2.34.1

Re: [PATCH v3 3/3] clk: renesas: Add RZ/V2H(P) CPG driver

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add RZ/V2H(P) CPG driver.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - Added CLK_PLLDTY
> - Added core clocks sys_0_pclk and iotop_0_shclk
> - Dropped r9a09g057_crit_mod_clks

Thanks for the update!

> --- /dev/null
> +++ b/drivers/clk/renesas/r9a09g057-cpg.c

> +static const struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
> +       /* External Clock Inputs */
> +       DEF_INPUT("audio_extal", CLK_AUDIO_EXTAL),
> +       DEF_INPUT("rtxin", CLK_RTXIN),
> +       DEF_INPUT("qextal", CLK_QEXTAL),
> +
> +       /* PLL Clocks */
> +       DEF_FIXED(".pllcm33", CLK_PLLCM33, CLK_QEXTAL, 200, 3),
> +       DEF_FIXED(".plldty", CLK_PLLDTY, CLK_QEXTAL, 200, 3),
> +       DEF_PLL(".pllca55", CLK_PLLCA55, CLK_QEXTAL, PLL_CONF(0x64)),
> +
> +       /* Internal Core Clocks */
> +       DEF_FIXED(".pllcm33_div16", CLK_PLLCM33_DIV16, CLK_PLLCM33, 1, 16),
> +

Missing comment "/* Core Clocks */"?

> +       DEF_FIXED("sys_0_pclk", R9A09G057_SYS_0_PCLK, CLK_QEXTAL, 1, 1),
> +       DEF_FIXED("iotop_0_shclk", R9A09G057_IOTOP_0_SHCLK, CLK_PLLCM33_DIV16, 1, 1),
> +};
> +
> +static const struct rzv2h_mod_clk r9a09g057_mod_clks[] = {

__initconst

> +       DEF_MOD("scif_0_clk_pck",               CLK_PLLCM33_DIV16, 8, 15, 4, 15),
> +};
> +
> +static const struct rzv2h_reset r9a09g057_resets[] = {
> +       DEF_RST(9, 5, 4, 6),            /* SCIF_0_RST_SYSTEM_N */
> +};
>
> +const struct rzv2h_cpg_info r9a09g057_cpg_info = {

With my suggested changes to [2/3], these two can be __initconst, too.

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 v3 3/3] clk: renesas: Add RZ/V2H(P) CPG driver

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Fri, Jul 12, 2024 at 1:00 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add RZ/V2H(P) CPG driver.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3
> > - Added CLK_PLLDTY
> > - Added core clocks sys_0_pclk and iotop_0_shclk
> > - Dropped r9a09g057_crit_mod_clks
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/r9a09g057-cpg.c
>
> > +static const struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
> > +       /* External Clock Inputs */
> > +       DEF_INPUT("audio_extal", CLK_AUDIO_EXTAL),
> > +       DEF_INPUT("rtxin", CLK_RTXIN),
> > +       DEF_INPUT("qextal", CLK_QEXTAL),
> > +
> > +       /* PLL Clocks */
> > +       DEF_FIXED(".pllcm33", CLK_PLLCM33, CLK_QEXTAL, 200, 3),
> > +       DEF_FIXED(".plldty", CLK_PLLDTY, CLK_QEXTAL, 200, 3),
> > +       DEF_PLL(".pllca55", CLK_PLLCA55, CLK_QEXTAL, PLL_CONF(0x64)),
> > +
> > +       /* Internal Core Clocks */
> > +       DEF_FIXED(".pllcm33_div16", CLK_PLLCM33_DIV16, CLK_PLLCM33, 1, 16),
> > +
>
> Missing comment "/* Core Clocks */"?
>
Agreed.

> > +       DEF_FIXED("sys_0_pclk", R9A09G057_SYS_0_PCLK, CLK_QEXTAL, 1, 1),
> > +       DEF_FIXED("iotop_0_shclk", R9A09G057_IOTOP_0_SHCLK, CLK_PLLCM33_DIV16, 1, 1),
> > +};
> > +
> > +static const struct rzv2h_mod_clk r9a09g057_mod_clks[] = {
>
> __initconst
>
OK.

> > +       DEF_MOD("scif_0_clk_pck",               CLK_PLLCM33_DIV16, 8, 15, 4, 15),
> > +};
> > +
> > +static const struct rzv2h_reset r9a09g057_resets[] = {
> > +       DEF_RST(9, 5, 4, 6),            /* SCIF_0_RST_SYSTEM_N */
> > +};
> >
> > +const struct rzv2h_cpg_info r9a09g057_cpg_info = {
>
> With my suggested changes to [2/3], these two can be __initconst, too.
>
Agreed.

Cheers,
Prabhakar