[RFC PATCH v2 0/4] Add CPG support for RZ/V2H(P) SoC

[RFC PATCH v2 0/4] 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.

Im sending this as an RFC as I have dropped the module clk macros to
avoid adding reserved macros and instead now using the combination of
register index and bits as unique number for clks and resets.

v1->RFC 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 (4):
  dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG
  dt-bindings: clock: Add R9A09G057 core clocks
  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     |  81 +++
 drivers/clk/renesas/Kconfig                   |   9 +
 drivers/clk/renesas/Makefile                  |   2 +
 drivers/clk/renesas/r9a09g057-cpg.c           |  77 ++
 drivers/clk/renesas/rzv2h-cpg.c               | 676 ++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h               | 164 +++++
 include/dt-bindings/clock/r9a09g057-cpg.h     |  21 +
 7 files changed, 1030 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/r9a09g057-cpg.h

-- 
2.34.1

[RFC PATCH v2 1/4] 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

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Hi Geert,

WRT XIN_{RTCCLK/AUDCLK/MAINCLK)clks going to CPG I have created an
internal request for clarification if the clocks are inputs to CPG
or to respective clocks. As the board schematic doesnt have any of
these. For now I have just kept qextal clk as input to CPG.

Cheers, Prabhakar

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     | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml

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..03085308154c
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
@@ -0,0 +1,81 @@
+# 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:
+    maxItems: 1
+
+  clock-names:
+    description:
+      Clock source to CPG on QEXTAL pin.
+    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/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':
+    description:
+      SoC devices that are part of the CPG/Module Standby Mode Clock Domain and
+      can be power-managed through Module Standby should refer to the CPG device
+      node in their "power-domains" property, as documented by the generic PM
+      Domain bindings in Documentation/devicetree/bindings/power/power-domain.yaml.
+    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:
+  - |
+    cpg: clock-controller@10420000 {
+            compatible = "renesas,r9a09g057-cpg";
+            reg = <0x10420000 0x10000>;
+            clocks = <&extal_clk>;
+            clock-names = "qextal";
+            #clock-cells = <2>;
+            #power-domain-cells = <0>;
+            #reset-cells = <1>;
+    };
-- 
2.34.1

[RFC PATCH v2 2/4] dt-bindings: clock: Add R9A09G057 core clocks

[Prabhakar]

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

Define RZ/V2H(P) (R9A09G057) Clock Pulse Generator core clocks.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- Dropped the module clocks and just added the core clocks

Note the core clocks are the once which are listed as part
of section 4.4.2 which cannot be controlled by CLKON register.
---
 include/dt-bindings/clock/r9a09g057-cpg.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 include/dt-bindings/clock/r9a09g057-cpg.h

diff --git a/include/dt-bindings/clock/r9a09g057-cpg.h b/include/dt-bindings/clock/r9a09g057-cpg.h
new file mode 100644
index 000000000000..04653d90d78d
--- /dev/null
+++ b/include/dt-bindings/clock/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

[RFC PATCH v2 3/4] 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>
---
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 | 672 ++++++++++++++++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h | 162 ++++++++
 4 files changed, 839 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 d252150402e8..330c8bc03777 100644
--- a/drivers/clk/renesas/Kconfig
+++ b/drivers/clk/renesas/Kconfig
@@ -228,6 +228,10 @@ config CLK_RZG2L
 	bool "Renesas 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..f3c9f562234b
--- /dev/null
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -0,0 +1,672 @@
+// 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
+ *
+ * @rcdev: Reset controller entity
+ * @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[]
+ * @num_hw_resets: Number of resets supported by HW
+ * @last_dt_core_clk: ID of the last Core Clock exported to DT
+ * @info: Pointer to platform data
+ */
+struct rzv2h_cpg_priv {
+	struct reset_controller_dev rcdev;
+	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 num_hw_resets;
+	unsigned int last_dt_core_clk;
+
+	const struct rzv2h_cpg_info *info;
+};
+
+struct pll_clk {
+	struct clk_hw hw;
+	unsigned int conf;
+	unsigned int type;
+	void __iomem *base;
+	struct rzv2h_cpg_priv *priv;
+};
+
+#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 clk **clks,
+			   void __iomem *base,
+			   struct rzv2h_cpg_priv *priv,
+			   const struct clk_ops *ops)
+{
+	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 clk_register(NULL, &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;
+	int range_check;
+	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";
+		range_check = 15 - (clkidx % 16);
+		if (range_check < 0 || 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,
+			    const struct rzv2h_cpg_info *info,
+			    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->clks, priv->base, 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 %s clock %s: %ld\n", "core",
+		core->name, PTR_ERR(clk));
+}
+
+/**
+ * struct mod_clock - Module clock
+ *
+ * @hw: handle between common and hardware-specific interfaces
+ * @off: register offset
+ * @bit: ON/MON bit
+ * @monoff: monitor register offset
+ * @monbit: montor bit
+ * @priv: CPG private data
+ */
+struct mod_clock {
+	struct clk_hw hw;
+	u8 on_index;
+	u8 on_bit;
+	u16 mon_index;
+	u8 mon_bit;
+	struct rzv2h_cpg_priv *priv;
+};
+
+#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)
+		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 offset = GET_CLK_MON_OFFSET(clock->mon_index);
+	u32 bitmask = BIT(clock->mon_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,
+			   const struct rzv2h_cpg_info *info,
+			   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;
+	unsigned int i;
+
+	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;
+	for (i = 0; i < info->num_crit_mod_clks; i++)
+		if (id == info->crit_mod_clks[i]) {
+			dev_dbg(dev, "CPG %s setting CLK_IS_CRITICAL\n",
+				mod->name);
+			init.flags |= CLK_IS_CRITICAL;
+			break;
+		}
+
+	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 = clk_register(NULL, &clock->hw);
+	if (IS_ERR(clk))
+		goto fail;
+
+	priv->clks[id] = clk;
+
+	return;
+
+fail:
+	dev_err(dev, "Failed to register %s clock %s: %ld\n", "module",
+		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);
+	s8 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);
+	s8 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);
+	s8 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 || ((id % 16) > 15)) {
+		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)
+{
+	WARN_DEBUG(priv->num_resets >= priv->num_hw_resets);
+
+	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
+ * @genpd: generic PM domain
+ * @priv: pointer to CPG private data structure
+ */
+struct rzv2h_cpg_pd {
+	struct generic_pm_domain genpd;
+	struct rzv2h_cpg_priv *priv;
+};
+
+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;
+	priv->num_hw_resets = info->num_hw_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], info, priv);
+
+	for (i = 0; i < info->num_mod_clks; i++)
+		rzv2h_cpg_register_mod_clk(&info->mod_clks[i], info, 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..d2791a3a23a0
--- /dev/null
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -0,0 +1,162 @@
+/* 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
+ * @id: clock index in array containing all Core and Module Clocks
+ * @parent: id of parent clock
+ * @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;
+	u8 on_index;
+	u8 on_bit;
+	u8 mon_index;
+	u8 mon_bit;
+};
+
+#define DEF_MOD_BASE(_name, _parent, _id, _onindex, _onbit, _monindex, _monbit)	\
+	{ \
+		.name = (_name), \
+		.parent = (_parent), \
+		.id = (_id), \
+		.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)),	\
+		     _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[]
+ * @num_hw_resets: Number of resets supported by the hardware
+ *
+ * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
+ *                 should not be disabled without a knowledgeable driver
+ * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
+ */
+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;
+	unsigned int num_hw_resets;
+
+	/* Critical Module Clocks that should not be disabled */
+	const unsigned int *crit_mod_clks;
+	unsigned int num_crit_mod_clks;
+};
+
+#endif	/* __RENESAS_RZV2H_CPG_H__ */
-- 
2.34.1

[RFC PATCH v2 4/4] 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>
---
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 | 77 +++++++++++++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.c     |  4 ++
 drivers/clk/renesas/rzv2h-cpg.h     |  2 +
 5 files changed, 89 insertions(+)
 create mode 100644 drivers/clk/renesas/r9a09g057-cpg.c

diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
index 330c8bc03777..3f3f84eb357b 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..d47b4365b2de
--- /dev/null
+++ b/drivers/clk/renesas/r9a09g057-cpg.c
@@ -0,0 +1,77 @@
+// 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/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_QEXTAL,
+
+	/* Internal Core Clocks */
+	CLK_PLLCM33,
+	CLK_PLLCM33_DIV16,
+	CLK_PLLCA55,
+
+	/* Module Clocks */
+	MOD_CLK_BASE,
+};
+
+static const struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
+	/* External Clock Inputs */
+	DEF_INPUT("qextal", CLK_QEXTAL),
+
+	/* Internal Core Clocks */
+	DEF_FIXED(".pllcm33", CLK_PLLCM33, CLK_QEXTAL, 200, 3),
+	DEF_FIXED(".pllcm33_div16", CLK_PLLCM33_DIV16, CLK_PLLCM33, 1, 16),
+
+	DEF_PLL(".pllca55", CLK_PLLCA55, CLK_QEXTAL, PLL_CONF(0x64)),
+};
+
+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 */
+};
+
+static const unsigned int r9a09g057_crit_mod_clks[] __initconst = {
+	MOD_CLK_BASE + 5,		/* ICU_0_PCLK_I */
+	MOD_CLK_BASE + 19,		/* GIC_0_GICCLK */
+};
+
+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,
+
+	/* Critical Module Clocks */
+	.crit_mod_clks = r9a09g057_crit_mod_clks,
+	.num_crit_mod_clks = ARRAY_SIZE(r9a09g057_crit_mod_clks),
+
+	/* 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),
+	.num_hw_resets = 18 * 16,
+};
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index f3c9f562234b..7882cfb36998 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -652,6 +652,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 d2791a3a23a0..2a4411618b8a 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -159,4 +159,6 @@ struct rzv2h_cpg_info {
 	unsigned int num_crit_mod_clks;
 };
 
+extern const struct rzv2h_cpg_info r9a09g057_cpg_info;
+
 #endif	/* __RENESAS_RZV2H_CPG_H__ */
-- 
2.34.1

Re: [RFC PATCH v2 2/4] dt-bindings: clock: Add R9A09G057 core clocks

[Krzysztof Kozlowski]

On 11/06/2024 01:32, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Define RZ/V2H(P) (R9A09G057) Clock Pulse Generator core clocks.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2
> - Dropped the module clocks and just added the core clocks
> 
> Note the core clocks are the once which are listed as part
> of section 4.4.2 which cannot be controlled by CLKON register.
> ---
>  include/dt-bindings/clock/r9a09g057-cpg.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 include/dt-bindings/clock/r9a09g057-cpg.h

Missing vendor prefix.

This belongs to the binding patch, so squash it.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 11/06/2024 01:32, 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
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Since this is not a finished work (RFC), only limited review follows.


> +$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: |

Do not need '|' unless you need to preserve formatting.

> +  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:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      Clock source to CPG on QEXTAL pin.
> +    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/r9a09g057-cpg.h>,

So second cell is not used?

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

You should not have different values. Make it const: 1 and just use IDs.

> +    const: 2
> +
> +  '#power-domain-cells':
> +    description:
> +      SoC devices that are part of the CPG/Module Standby Mode Clock Domain and
> +      can be power-managed through Module Standby should refer to the CPG device
> +      node in their "power-domains" property, as documented by the generic PM
> +      Domain bindings in Documentation/devicetree/bindings/power/power-domain.yaml.

Drop description, it's redundant.

> +    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:
> +  - |
> +    cpg: clock-controller@10420000 {

Drop unused label.

> +            compatible = "renesas,r9a09g057-cpg";

Use 4 spaces for example indentation.
> +    };

Best regards,
Krzysztof

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

[Lad, Prabhakar]

Hi Krzysztof,

Thank you for the review.

On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 11/06/2024 01:32, 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
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Since this is not a finished work (RFC), only limited review follows.
>
>
> > +$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: |
>
> Do not need '|' unless you need to preserve formatting.
>
OK.

> > +  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:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    description:
> > +      Clock source to CPG on QEXTAL pin.
> > +    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/r9a09g057-cpg.h>,
>
> So second cell is not used?
>
It will be used for blocks using core clocks.

> > +      - 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.
>
> You should not have different values. Make it const: 1 and just use IDs.
>
Are you suggesting not to differentiate between core/mod clocks. They
are differentiated because the MOD clocks can turned ON/OFF but where
as with the core clocks we cannot turn them ON/OF so the driver needs
to know this, hence two specifiers are used.

> > +    const: 2
> > +
> > +  '#power-domain-cells':
> > +    description:
> > +      SoC devices that are part of the CPG/Module Standby Mode Clock Domain and
> > +      can be power-managed through Module Standby should refer to the CPG device
> > +      node in their "power-domains" property, as documented by the generic PM
> > +      Domain bindings in Documentation/devicetree/bindings/power/power-domain.yaml.
>
> Drop description, it's redundant.
>
OK.

> > +    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:
> > +  - |
> > +    cpg: clock-controller@10420000 {
>
> Drop unused label.
>
OK.

> > +            compatible = "renesas,r9a09g057-cpg";
>
> Use 4 spaces for example indentation.
>
Sure, I will update it.

Cheers,
Prabhakar

Re: [RFC PATCH v2 2/4] dt-bindings: clock: Add R9A09G057 core clocks

[Lad, Prabhakar]

Hi Krzysztof,

Thank you for the review.

On Tue, Jun 11, 2024 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 11/06/2024 01:32, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Define RZ/V2H(P) (R9A09G057) Clock Pulse Generator core clocks.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v1->v2
> > - Dropped the module clocks and just added the core clocks
> >
> > Note the core clocks are the once which are listed as part
> > of section 4.4.2 which cannot be controlled by CLKON register.
> > ---
> >  include/dt-bindings/clock/r9a09g057-cpg.h | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >  create mode 100644 include/dt-bindings/clock/r9a09g057-cpg.h
>
> Missing vendor prefix.
>
OK, Is this just for new includes being added, or do you want me to
rename the existing Renesas specific includes in here which dont have
vendor prefix?

> This belongs to the binding patch, so squash it.
>
OK, I will squash it.

Cheers,
Prabhakar

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

[Lad, Prabhakar]

Hi Geert,

On Tue, Jun 11, 2024 at 12:32 AM 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
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Hi Geert,
>
> WRT XIN_{RTCCLK/AUDCLK/MAINCLK)clks going to CPG I have created an
> internal request for clarification if the clocks are inputs to CPG
> or to respective clocks. As the board schematic doesnt have any of
> these. For now I have just kept qextal clk as input to CPG.
>
I have got the feedback from the manual team.

The XIN_* clocks will be renamed as below (and the block diagram will
be updated),
XIN_MAINCLK -> QXCLK
XIN_RTCCLK -> RTX_XCLK
XIN_AUDCLK -> AUDIO_XCLK

From section 4.2.1.7 Functional Block diagram (page 359) we have the below,

RTXIN ----------------> PFC ------> RTX_XCLK --------> CPG
QEXTAL--------------> PFC ------> QXCLK -------------> CPG
AUDIO_EXTAL-----> PFC ------> AUDIO_XCLK ----> CPG

How should we model this now, please provide your feedback?

Cheers,
Prabhakar

Re: [RFC PATCH v2 2/4] dt-bindings: clock: Add R9A09G057 core clocks

[Krzysztof Kozlowski]

On 13/06/2024 11:57, Lad, Prabhakar wrote:

>>> of section 4.4.2 which cannot be controlled by CLKON register.
>>> ---
>>>  include/dt-bindings/clock/r9a09g057-cpg.h | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>  create mode 100644 include/dt-bindings/clock/r9a09g057-cpg.h
>>
>> Missing vendor prefix.
>>
> OK, Is this just for new includes being added, or do you want me to
> rename the existing Renesas specific includes in here which dont have
> vendor prefix?

Didn't we discuss it?

I commented only about this binding.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 13/06/2024 11:53, Lad, Prabhakar wrote:
> Hi Krzysztof,
> 
> Thank you for the review.
> 
> On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 11/06/2024 01:32, 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
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>
>> Since this is not a finished work (RFC), only limited review follows.
>>
>>
>>> +$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: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
> OK.
> 
>>> +  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:
>>> +    maxItems: 1
>>> +
>>> +  clock-names:
>>> +    description:
>>> +      Clock source to CPG on QEXTAL pin.
>>> +    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/r9a09g057-cpg.h>,
>>
>> So second cell is not used?
>>
> It will be used for blocks using core clocks.
> 
>>> +      - 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.
>>
>> You should not have different values. Make it const: 1 and just use IDs.
>>
> Are you suggesting not to differentiate between core/mod clocks. They
> are differentiated because the MOD clocks can turned ON/OFF but where
> as with the core clocks we cannot turn them ON/OF so the driver needs
> to know this, hence two specifiers are used.

Every driver knows it... I am really, what is the problem here? Are you
saying the drivers create some unknown clocks?

Anyway, that's not an argument for bindings. Fix your driver design.


Best regards,
Krzysztof

Re: [RFC PATCH v2 2/4] dt-bindings: clock: Add R9A09G057 core clocks

[Geert Uytterhoeven]

Hi Krzysztof,

On Thu, Jun 13, 2024 at 2:56 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 13/06/2024 11:57, Lad, Prabhakar wrote:
> >>> of section 4.4.2 which cannot be controlled by CLKON register.
> >>> ---
> >>>  include/dt-bindings/clock/r9a09g057-cpg.h | 21 +++++++++++++++++++++
> >>>  1 file changed, 21 insertions(+)
> >>>  create mode 100644 include/dt-bindings/clock/r9a09g057-cpg.h
> >>
> >> Missing vendor prefix.
> >>
> > OK, Is this just for new includes being added, or do you want me to
> > rename the existing Renesas specific includes in here which dont have
> > vendor prefix?
>
> Didn't we discuss it?
>
> I commented only about this binding.

Yes we did, in the context of the R-Car V4M DT binding definitions,
which became include/dt-bindings/clock/renesas,r8a779h0-cpg-mssr.h
But Prabhakar was not involved there.

Note that I also asked to include the vendor prefix, see
https://lore.kernel.org/linux-renesas-soc/CAMuHMdU7+O-+v=2V83AjQmTWyGy_a-AHgU_nPMDHnVUtYt89iQ@mail.gmail.com/

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: [RFC PATCH v2 2/4] dt-bindings: clock: Add R9A09G057 core clocks

[Lad, Prabhakar]

Hi Geert,

On Thu, Jun 13, 2024 at 4:15 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Krzysztof,
>
> On Thu, Jun 13, 2024 at 2:56 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On 13/06/2024 11:57, Lad, Prabhakar wrote:
> > >>> of section 4.4.2 which cannot be controlled by CLKON register.
> > >>> ---
> > >>>  include/dt-bindings/clock/r9a09g057-cpg.h | 21 +++++++++++++++++++++
> > >>>  1 file changed, 21 insertions(+)
> > >>>  create mode 100644 include/dt-bindings/clock/r9a09g057-cpg.h
> > >>
> > >> Missing vendor prefix.
> > >>
> > > OK, Is this just for new includes being added, or do you want me to
> > > rename the existing Renesas specific includes in here which dont have
> > > vendor prefix?
> >
> > Didn't we discuss it?
> >
> > I commented only about this binding.
>
> Yes we did, in the context of the R-Car V4M DT binding definitions,
> which became include/dt-bindings/clock/renesas,r8a779h0-cpg-mssr.h
> But Prabhakar was not involved there.
>
> Note that I also asked to include the vendor prefix, see
> https://lore.kernel.org/linux-renesas-soc/CAMuHMdU7+O-+v=2V83AjQmTWyGy_a-AHgU_nPMDHnVUtYt89iQ@mail.gmail.com/
>
Oops I missed that review comment.

Cheers,
Prabhakar

Re: [RFC PATCH v2 2/4] dt-bindings: clock: Add R9A09G057 core clocks

[Geert Uytterhoeven]

On Tue, Jun 11, 2024 at 1:32 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Define RZ/V2H(P) (R9A09G057) Clock Pulse Generator core clocks.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2
> - Dropped the module clocks and just added the core clocks
>
> Note the core clocks are the once which are listed as part

s/the once/a subset of the ones/

> of section 4.4.2 which cannot be controlled by CLKON register.

and we can add more when needed ("append only").

> +++ b/include/dt-bindings/clock/r9a09g057-cpg.h

With the "renesas," prefix added:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [RFC PATCH v2 1/4] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG

[Geert Uytterhoeven]

Hi Krzysztof,

On Thu, Jun 13, 2024 at 2:57 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 13/06/2024 11:53, Lad, Prabhakar wrote:
> > On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On 11/06/2024 01:32, 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
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> >>> +  '#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/r9a09g057-cpg.h>,
> >>
> >> So second cell is not used?
> >>
> > It will be used for blocks using core clocks.
> >
> >>> +      - 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.
> >>
> >> You should not have different values. Make it const: 1 and just use IDs.
> >>
> > Are you suggesting not to differentiate between core/mod clocks. They
> > are differentiated because the MOD clocks can turned ON/OFF but where
> > as with the core clocks we cannot turn them ON/OF so the driver needs
> > to know this, hence two specifiers are used.
>
> Every driver knows it... I am really, what is the problem here? Are you
> saying the drivers create some unknown clocks?

The driver knows for sure which clocks are module clocks, and thus can
be used for power management.  To simplify the driver, two separate
numbers spaces are used:
  1. Core clock numbers come from IDs in the DT binding headers,
  2. Module clock numbers come straight[1] from the hardware docs.
As the latter are fixed, merging them into a single number space in
a future-proof way is hard[2], the bindings use 2 clock cells.

Alternatively, a unified number space using IDs in the DT binding
headers could be used, as you suggest.

[1] "straight" may be a misnomer here, as the DT writer still has to
    calculate the number from register index and bit index:

        n = register index * 16 + bit index

    i.e. register index 1 and register bit 3 become 19.

    In the R-Car series, this is handled slightly more elegant
    (IMHO ;-), and easier to the human eye, by using a sparse
    number space:

        n = register index * 100 + bit index

    i.e. register index 1 and register bit 3 become 103.
    Which also matches how the bits were named in older SH-Mobile
    hardware docs.

[2] One could use an offset to indicate core or module clocks, but
    future SoCs in the family may have more clocks.

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: [RFC PATCH v2 1/4] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG

[Krzysztof Kozlowski]

On 26/06/2024 11:35, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Thu, Jun 13, 2024 at 2:57 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 13/06/2024 11:53, Lad, Prabhakar wrote:
>>> On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> On 11/06/2024 01:32, 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
>>>>>
>>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
>>>>> +  '#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/r9a09g057-cpg.h>,
>>>>
>>>> So second cell is not used?
>>>>
>>> It will be used for blocks using core clocks.
>>>
>>>>> +      - 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.
>>>>
>>>> You should not have different values. Make it const: 1 and just use IDs.
>>>>
>>> Are you suggesting not to differentiate between core/mod clocks. They
>>> are differentiated because the MOD clocks can turned ON/OFF but where
>>> as with the core clocks we cannot turn them ON/OF so the driver needs
>>> to know this, hence two specifiers are used.
>>
>> Every driver knows it... I am really, what is the problem here? Are you
>> saying the drivers create some unknown clocks?
> 
> The driver knows for sure which clocks are module clocks, and thus can
> be used for power management.  To simplify the driver, two separate
> numbers spaces are used:
>   1. Core clock numbers come from IDs in the DT binding headers,
>   2. Module clock numbers come straight[1] from the hardware docs.
> As the latter are fixed, merging them into a single number space in
> a future-proof way is hard[2], the bindings use 2 clock cells.

IIUC, your module clock numbers are not DT ABI and should not be put
into the binding headers. I think that's the case currently, right?

If above is correct, considering your explanation I am fine. Thanks for
the time to make it clear.

> 
> Alternatively, a unified number space using IDs in the DT binding
> headers could be used, as you suggest.
> 
> [1] "straight" may be a misnomer here, as the DT writer still has to
>     calculate the number from register index and bit index:
> 
>         n = register index * 16 + bit index
> 
>     i.e. register index 1 and register bit 3 become 19.
> 
>     In the R-Car series, this is handled slightly more elegant
>     (IMHO ;-), and easier to the human eye, by using a sparse
>     number space:
> 
>         n = register index * 100 + bit index
> 
>     i.e. register index 1 and register bit 3 become 103.
>     Which also matches how the bits were named in older SH-Mobile
>     hardware docs.
> 
> [2] One could use an offset to indicate core or module clocks, but
>     future SoCs in the family may have more clocks.


> 

Best regards,
Krzysztof

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

[Geert Uytterhoeven]

Hi Prabhakar,

On Tue, Jun 11, 2024 at 1:32 AM 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>
> ---
> 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

Thanks for the update!

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

> +/**
> + * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
> + *
> + * @rcdev: Reset controller entity
> + * @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[]
> + * @num_hw_resets: Number of resets supported by HW
> + * @last_dt_core_clk: ID of the last Core Clock exported to DT
> + * @info: Pointer to platform data
> + */
> +struct rzv2h_cpg_priv {
> +       struct reset_controller_dev rcdev;
> +       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 num_hw_resets;

This is not really used, so please drop it.

> +       unsigned int last_dt_core_clk;
> +
> +       const struct rzv2h_cpg_info *info;
> +};

> +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;
> +       int range_check;
> +       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";
> +               range_check = 15 - (clkidx % 16);
> +               if (range_check < 0 || clkidx >= priv->num_mod_clks) {

range_check is never negative
(leftover from sparse number space?)

> +                       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;
> +}

> +/**
> + * struct mod_clock - Module clock
> + *
> + * @hw: handle between common and hardware-specific interfaces
> + * @off: register offset
> + * @bit: ON/MON bit
> + * @monoff: monitor register offset
> + * @monbit: montor bit
> + * @priv: CPG private data
> + */
> +struct mod_clock {
> +       struct clk_hw hw;
> +       u8 on_index;
> +       u8 on_bit;
> +       u16 mon_index;
> +       u8 mon_bit;

I noticed clock on and clock mon bits are related.
Clock on bits use only the lower 16 bits in a register, while clock
monitor bits use all 32 bits, hence:

    mon_index = on_index / 2
    mon_bit = (on_index % 2) * 16 + on_bit

Except for clocks without monitor bits, and for CGC_SPI_clk_spi and
CGC_SPI_clk_spix2, which share an on-bit, but have separate mon-bits.
So you cannot use these formulas.

Reset bits do not have such a relationship, as resets marked reserved
are skipped in the reset monitoring bit range.


> +       struct rzv2h_cpg_priv *priv;
> +};
> +
> +#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)
> +               return 0;
> +
> +       reg = GET_CLK_MON_OFFSET(clock->mon_index);

What if a clock does not have a clock monitor bit?
Clock bits in registers CPG_CLKON_22 and later do not have corresponding
clock monitor bits.

> +       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;
> +}

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

> +/**
> + * 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[]
> + * @num_hw_resets: Number of resets supported by the hardware
> + *
> + * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
> + *                 should not be disabled without a knowledgeable driver
> + * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
> + */
> +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;
> +       unsigned int num_hw_resets;

This is not really used, so please drop it.

> +
> +       /* Critical Module Clocks that should not be disabled */
> +       const unsigned int *crit_mod_clks;
> +       unsigned int num_crit_mod_clks;
> +};
> +
> +#endif /* __RENESAS_RZV2H_CPG_H__ */

The rest LGTM.

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: [RFC PATCH v2 4/4] clk: renesas: Add RZ/V2H(P) CPG driver

[Geert Uytterhoeven]

On Tue, Jun 11, 2024 at 1:32 AM 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>
> ---
> v1->v2
> - Updated commit description
> - Dropped pll_clk1/clk2_offset
> - Made r9a09g057_mod_clks/r9a09g057_resets as static const
> - Now using register indexes

Thanks for the update!

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

> +static const struct rzv2h_mod_clk r9a09g057_mod_clks[] = {
> +       DEF_MOD("scif_0_clk_pck", CLK_PLLCM33_DIV16, 8, 15, 4, 15),

So this relates to module clock 8 * 16 + 15 = 143 in DTS...

> +};
> +
> +static const struct rzv2h_reset r9a09g057_resets[] = {
> +       DEF_RST(9, 5, 4, 6),            /* SCIF_0_RST_SYSTEM_N */
> +};
> +
> +static const unsigned int r9a09g057_crit_mod_clks[] __initconst = {
> +       MOD_CLK_BASE + 5,               /* ICU_0_PCLK_I */
> +       MOD_CLK_BASE + 19,              /* GIC_0_GICCLK */

So these relate to module clocks 5 and 19 in DTS.

Actually none of these clocks are created in the driver yet, so I think
these critical clocks belong to the patch that will introduce them.

I am wondering if critical clocks should just use a flag in DEF_MOD()
instead...

The rest LGTM.

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: [RFC PATCH v2 1/4] dt-bindings: clock: renesas: Document RZ/V2H(P) SoC CPG

[Geert Uytterhoeven]

Hi Krzysztof,

On Wed, Jun 26, 2024 at 11:41 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 26/06/2024 11:35, Geert Uytterhoeven wrote:
> > On Thu, Jun 13, 2024 at 2:57 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On 13/06/2024 11:53, Lad, Prabhakar wrote:
> >>> On Tue, Jun 11, 2024 at 8:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> On 11/06/2024 01:32, 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
> >>>>>
> >>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> >>>>> +  '#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/r9a09g057-cpg.h>,
> >>>>
> >>>> So second cell is not used?
> >>>>
> >>> It will be used for blocks using core clocks.
> >>>
> >>>>> +      - 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.
> >>>>
> >>>> You should not have different values. Make it const: 1 and just use IDs.
> >>>>
> >>> Are you suggesting not to differentiate between core/mod clocks. They
> >>> are differentiated because the MOD clocks can turned ON/OFF but where
> >>> as with the core clocks we cannot turn them ON/OF so the driver needs
> >>> to know this, hence two specifiers are used.
> >>
> >> Every driver knows it... I am really, what is the problem here? Are you
> >> saying the drivers create some unknown clocks?
> >
> > The driver knows for sure which clocks are module clocks, and thus can
> > be used for power management.  To simplify the driver, two separate
> > numbers spaces are used:
> >   1. Core clock numbers come from IDs in the DT binding headers,
> >   2. Module clock numbers come straight[1] from the hardware docs.
> > As the latter are fixed, merging them into a single number space in
> > a future-proof way is hard[2], the bindings use 2 clock cells.
>
> IIUC, your module clock numbers are not DT ABI and should not be put
> into the binding headers. I think that's the case currently, right?

Exactly: they are hardware ABI, just like e.g. GIC interrupt numbers.

> If above is correct, considering your explanation I am fine. Thanks for
> the time to make it clear.

Thanks!

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: [RFC PATCH v2 3/4] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Wed, Jun 26, 2024 at 11:07 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Jun 11, 2024 at 1:32 AM 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>
> > ---
> > 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
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > +/**
> > + * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
> > + *
> > + * @rcdev: Reset controller entity
> > + * @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[]
> > + * @num_hw_resets: Number of resets supported by HW
> > + * @last_dt_core_clk: ID of the last Core Clock exported to DT
> > + * @info: Pointer to platform data
> > + */
> > +struct rzv2h_cpg_priv {
> > +       struct reset_controller_dev rcdev;
> > +       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 num_hw_resets;
>
> This is not really used, so please drop it.
>
OK, I will drop it.

> > +       unsigned int last_dt_core_clk;
> > +
> > +       const struct rzv2h_cpg_info *info;
> > +};
>
> > +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;
> > +       int range_check;
> > +       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";
> > +               range_check = 15 - (clkidx % 16);
> > +               if (range_check < 0 || clkidx >= priv->num_mod_clks) {
>
> range_check is never negative
> (leftover from sparse number space?)
>
Agreed (we are doing % 16). I will drop this check.

> > +                       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;
> > +}
>
> > +/**
> > + * struct mod_clock - Module clock
> > + *
> > + * @hw: handle between common and hardware-specific interfaces
> > + * @off: register offset
> > + * @bit: ON/MON bit
> > + * @monoff: monitor register offset
> > + * @monbit: montor bit
> > + * @priv: CPG private data
> > + */
> > +struct mod_clock {
> > +       struct clk_hw hw;
> > +       u8 on_index;
> > +       u8 on_bit;
> > +       u16 mon_index;
> > +       u8 mon_bit;
>
> I noticed clock on and clock mon bits are related.
> Clock on bits use only the lower 16 bits in a register, while clock
> monitor bits use all 32 bits, hence:
>
>     mon_index = on_index / 2
>     mon_bit = (on_index % 2) * 16 + on_bit
>
> Except for clocks without monitor bits, and for CGC_SPI_clk_spi and
> CGC_SPI_clk_spix2, which share an on-bit, but have separate mon-bits.
> So you cannot use these formulas.
>
Ahh we could have  saved some memory!

> Reset bits do not have such a relationship, as resets marked reserved
> are skipped in the reset monitoring bit range.
>
Yep.

>
> > +       struct rzv2h_cpg_priv *priv;
> > +};
> > +
> > +#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)
> > +               return 0;
> > +
> > +       reg = GET_CLK_MON_OFFSET(clock->mon_index);
>
> What if a clock does not have a clock monitor bit?
> Clock bits in registers CPG_CLKON_22 and later do not have corresponding
> clock monitor bits.
>
Oops I had missed this case.

I'll introduce a macro (NO_MON_REG_INDEX) for clocks which do not have
monitor support and add a check above to skip clk monitor operation if
clock->mon_index == NO_MON_REG_INDEX.

/* monitor index for clocks which do not have CLKMON support */
#define NO_MON_REG_INDEX    0xff

Does this sound OK?

> > +       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;
> > +}
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.h
>
> > +/**
> > + * 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[]
> > + * @num_hw_resets: Number of resets supported by the hardware
> > + *
> > + * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
> > + *                 should not be disabled without a knowledgeable driver
> > + * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
> > + */
> > +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;
> > +       unsigned int num_hw_resets;
>
> This is not really used, so please drop it.
>
OK.

Cheers,
Prabhakar

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

[Geert Uytterhoeven]

Hi Prabhakar,

On Wed, Jun 26, 2024 at 7:36 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Wed, Jun 26, 2024 at 11:07 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Tue, Jun 11, 2024 at 1:32 AM 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>

> > > +/**
> > > + * struct mod_clock - Module clock
> > > + *
> > > + * @hw: handle between common and hardware-specific interfaces
> > > + * @off: register offset
> > > + * @bit: ON/MON bit
> > > + * @monoff: monitor register offset
> > > + * @monbit: montor bit
> > > + * @priv: CPG private data
> > > + */
> > > +struct mod_clock {
> > > +       struct clk_hw hw;
> > > +       u8 on_index;
> > > +       u8 on_bit;
> > > +       u16 mon_index;

BTW, why is this u16? The corresponding member in rzv2h_mod_clk is u8.

> > > +       u8 mon_bit;

> > > +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)
> > > +               return 0;
> > > +
> > > +       reg = GET_CLK_MON_OFFSET(clock->mon_index);
> >
> > What if a clock does not have a clock monitor bit?
> > Clock bits in registers CPG_CLKON_22 and later do not have corresponding
> > clock monitor bits.
> >
> Oops I had missed this case.
>
> I'll introduce a macro (NO_MON_REG_INDEX) for clocks which do not have
> monitor support and add a check above to skip clk monitor operation if
> clock->mon_index == NO_MON_REG_INDEX.
>
> /* monitor index for clocks which do not have CLKMON support */
> #define NO_MON_REG_INDEX    0xff
>
> Does this sound OK?

Either that, or make mon_index signed (which would reduce its
effective range by one bit).

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: [RFC PATCH v2 3/4] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Lad, Prabhakar]

Hi Geert,

On Thu, Jun 27, 2024 at 8:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, Jun 26, 2024 at 7:36 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Jun 26, 2024 at 11:07 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Tue, Jun 11, 2024 at 1:32 AM 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>
>
> > > > +/**
> > > > + * struct mod_clock - Module clock
> > > > + *
> > > > + * @hw: handle between common and hardware-specific interfaces
> > > > + * @off: register offset
> > > > + * @bit: ON/MON bit
> > > > + * @monoff: monitor register offset
> > > > + * @monbit: montor bit
> > > > + * @priv: CPG private data
> > > > + */
> > > > +struct mod_clock {
> > > > +       struct clk_hw hw;
> > > > +       u8 on_index;
> > > > +       u8 on_bit;
> > > > +       u16 mon_index;
>
> BTW, why is this u16? The corresponding member in rzv2h_mod_clk is u8.
>
Oops, I will fix this.

> > > > +       u8 mon_bit;
>
> > > > +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)
> > > > +               return 0;
> > > > +
> > > > +       reg = GET_CLK_MON_OFFSET(clock->mon_index);
> > >
> > > What if a clock does not have a clock monitor bit?
> > > Clock bits in registers CPG_CLKON_22 and later do not have corresponding
> > > clock monitor bits.
> > >
> > Oops I had missed this case.
> >
> > I'll introduce a macro (NO_MON_REG_INDEX) for clocks which do not have
> > monitor support and add a check above to skip clk monitor operation if
> > clock->mon_index == NO_MON_REG_INDEX.
> >
> > /* monitor index for clocks which do not have CLKMON support */
> > #define NO_MON_REG_INDEX    0xff
> >
> > Does this sound OK?
>
> Either that, or make mon_index signed (which would reduce its
> effective range by one bit).
>
Ok I'll make it to s8 instead and add a negative check monitor index.

Cheers,
Prabhakar

Re: [RFC PATCH v2 4/4] clk: renesas: Add RZ/V2H(P) CPG driver

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Wed, Jun 26, 2024 at 11:14 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> On Tue, Jun 11, 2024 at 1:32 AM 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>
> > ---
> > v1->v2
> > - Updated commit description
> > - Dropped pll_clk1/clk2_offset
> > - Made r9a09g057_mod_clks/r9a09g057_resets as static const
> > - Now using register indexes
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/r9a09g057-cpg.c
>
> > +static const struct rzv2h_mod_clk r9a09g057_mod_clks[] = {
> > +       DEF_MOD("scif_0_clk_pck", CLK_PLLCM33_DIV16, 8, 15, 4, 15),
>
> So this relates to module clock 8 * 16 + 15 = 143 in DTS...
>
Yep.

> > +};
> > +
> > +static const struct rzv2h_reset r9a09g057_resets[] = {
> > +       DEF_RST(9, 5, 4, 6),            /* SCIF_0_RST_SYSTEM_N */
> > +};
> > +
> > +static const unsigned int r9a09g057_crit_mod_clks[] __initconst = {
> > +       MOD_CLK_BASE + 5,               /* ICU_0_PCLK_I */
> > +       MOD_CLK_BASE + 19,              /* GIC_0_GICCLK */
>
> So these relate to module clocks 5 and 19 in DTS.
>
> Actually none of these clocks are created in the driver yet, so I think
> these critical clocks belong to the patch that will introduce them.
>
> I am wondering if critical clocks should just use a flag in DEF_MOD()
> instead...
>
Agreed, I will add a flag for it and have two macros like below,

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


Cheers,
Prabhakar