[PATCH v4 0/4] Add CMN PLL clock controller driver for IPQ9574

[PATCH v4 0/4] Add CMN PLL clock controller driver for IPQ9574

[Luo Jie]

The CMN PLL clock controller in Qualcomm IPQ chipsets provides
the clocks to the networking hardware blocks that are internal
or external to the SoC, and to the GCC. This driver configures
the CMN PLL clock controller to enable the output clocks. The
networking blocks include the internal blocks such as PPE
(Packet Process Engine) and PCS blocks, and external hardware
such as Ethernet PHY or switch. The CMN PLL block also outputs
fixed rate clocks to GCC, such as 24 MHZ as XO clock and 32 KHZ
as sleep clock supplied to GCC.

The controller expects the input reference clock from the internal
Wi-Fi block acting as the clock source. The output clocks supplied
by the controller are fixed rate clocks.

The CMN PLL hardware block does not include any other function
other than enabling the clocks to the networking hardware blocks
and GCC.

The driver is being enabled to support IPQ9574 SoC initially, and
will be extended for other SoCs.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
Changes in v4:
- Rename driver file to ipq-cmn-pll.c
- Register CMN PLL as a 12 GHZ clock.
- Configure CMN PLL input ref clock using clk_ops::determine_rate().
  Add the additional output clocks to GCC and PCS.
- Update the same information in dtbindings.
- Use PM clock APIs for input clock enablement.
- Link to v3: https://lore.kernel.org/r/20240827-qcom_ipq_cmnpll-v3-0-8e009cece8b2@quicinc.com

Changes in v3:
- Update description of dt-binding to explain scope of 'CMN' in CMN PLL.
- Collect Reviewed-by tags for dtbindings and defconfig patches.
- Enable PLL_LOCKED check for the stability of output clocks.
- Link to v2: https://lore.kernel.org/r/20240820-qcom_ipq_cmnpll-v2-0-b000dd335280@quicinc.com

Changes in v2:
- Rename the dt-binding file with the compatible.
- Remove property 'clock-output-names' from dt-bindings and define
  names in the driver. Add qcom,ipq-cmn-pll.h to export the output
  clock specifier.
- Alphanumeric ordering of 'cmn_pll_ref_clk' node in DTS.
- Fix allmodconfig error reported by test robot.
- Replace usage of "common" to "CMN" to match the name with the
  hardware specification.
- Clarify in commit message on scope of CMN PLL function.
- Link to v1: https://lore.kernel.org/r/20240808-qcom_ipq_cmnpll-v1-0-b0631dcbf785@quicinc.com

---
Luo Jie (4):
      dt-bindings: clock: qcom: Add CMN PLL clock controller for IPQ SoC
      clk: qcom: Add CMN PLL clock controller driver for IPQ SoC
      arm64: defconfig: Enable Qualcomm IPQ CMN PLL clock controller
      arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

 .../bindings/clock/qcom,ipq9574-cmn-pll.yaml       |  85 +++++
 arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi   |   6 +-
 arch/arm64/boot/dts/qcom/ipq9574.dtsi              |  20 +-
 arch/arm64/configs/defconfig                       |   1 +
 drivers/clk/qcom/Kconfig                           |  10 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/ipq-cmn-pll.c                     | 411 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,ipq-cmn-pll.h       |  22 ++
 8 files changed, 554 insertions(+), 2 deletions(-)
---
base-commit: d61a00525464bfc5fe92c6ad713350988e492b88
change-id: 20241014-qcom_ipq_cmnpll-bde0638f4116

Best regards,
-- 
Luo Jie <quic_luoj@quicinc.com>

[PATCH v4 1/4] dt-bindings: clock: qcom: Add CMN PLL clock controller for IPQ SoC

[Luo Jie]

The CMN PLL controller provides clocks to networking hardware blocks
and to GCC on Qualcomm IPQ9574 SoC. It receives input clock from the
on-chip Wi-Fi, and produces output clocks at fixed rates. These output
rates are predetermined, and are unrelated to the input clock rate.
The primary purpose of CMN PLL is to supply clocks to the networking
hardware such as PPE (packet process engine), PCS and the externally
connected switch or PHY device. The CMN PLL block also outputs fixed
rate clocks to GCC, such as 24 MHZ as XO clock and 32 KHZ as sleep
clock supplied to GCC.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 .../bindings/clock/qcom,ipq9574-cmn-pll.yaml       | 85 ++++++++++++++++++++++
 include/dt-bindings/clock/qcom,ipq-cmn-pll.h       | 22 ++++++
 2 files changed, 107 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-cmn-pll.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-cmn-pll.yaml
new file mode 100644
index 000000000000..db8a3ee56067
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-cmn-pll.yaml
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,ipq9574-cmn-pll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm CMN PLL Clock Controller on IPQ SoC
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+  - Luo Jie <quic_luoj@quicinc.com>
+
+description:
+  The CMN (or common) PLL clock controller expects a reference
+  input clock. This reference clock is from the on-board Wi-Fi.
+  The CMN PLL supplies a number of fixed rate output clocks to
+  the devices providing networking functions and to GCC. These
+  networking hardware include PPE (packet process engine), PCS
+  and the externally connected switch or PHY devices. The CMN
+  PLL block also outputs fixed rate clocks to GCC. The PLL's
+  primary function is to enable fixed rate output clocks for
+  networking hardware functions used with the IPQ SoC.
+
+properties:
+  compatible:
+    enum:
+      - qcom,ipq9574-cmn-pll
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: The reference clock. The supported clock rates include
+          25000000, 31250000, 40000000, 48000000, 50000000 and 96000000 HZ.
+      - description: The AHB clock
+      - description: The SYS clock
+    description:
+      The reference clock is the source clock of CMN PLL, which is from the
+      Wi-Fi. The AHB and SYS clocks must be enabled to access CMN PLL
+      clock registers.
+
+  clock-names:
+    items:
+      - const: ref
+      - const: ahb
+      - const: sys
+
+  "#clock-cells":
+    const: 1
+
+  assigned-clocks:
+    maxItems: 1
+
+  assigned-clock-rates-u64:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - "#clock-cells"
+  - assigned-clocks
+  - assigned-clock-rates-u64
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
+    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
+
+    cmn_pll: clock-controller@9b000 {
+        compatible = "qcom,ipq9574-cmn-pll";
+        reg = <0x0009b000 0x800>;
+        clocks = <&cmn_pll_ref_clk>,
+                 <&gcc GCC_CMN_12GPLL_AHB_CLK>,
+                 <&gcc GCC_CMN_12GPLL_SYS_CLK>;
+        clock-names = "ref", "ahb", "sys";
+        #clock-cells = <1>;
+        assigned-clocks = <&cmn_pll CMN_PLL_CLK>;
+        assigned-clock-rates-u64 = /bits/ 64 <12000000000>;
+    };
+...
diff --git a/include/dt-bindings/clock/qcom,ipq-cmn-pll.h b/include/dt-bindings/clock/qcom,ipq-cmn-pll.h
new file mode 100644
index 000000000000..936e92b3b62c
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,ipq-cmn-pll.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_QCOM_IPQ_CMN_PLL_H
+#define _DT_BINDINGS_CLK_QCOM_IPQ_CMN_PLL_H
+
+/* CMN PLL core clock. */
+#define CMN_PLL_CLK			0
+
+/* The output clocks from CMN PLL of IPQ9574. */
+#define XO_24MHZ_CLK			1
+#define SLEEP_32KHZ_CLK			2
+#define PCS_31P25MHZ_CLK		3
+#define NSS_1200MHZ_CLK			4
+#define PPE_353MHZ_CLK			5
+#define ETH0_50MHZ_CLK			6
+#define ETH1_50MHZ_CLK			7
+#define ETH2_50MHZ_CLK			8
+#define ETH_25MHZ_CLK			9
+#endif

-- 
2.34.1

[PATCH v4 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

[Luo Jie]

The CMN PLL clock controller supplies clocks to the hardware
blocks that together make up the Ethernet function on Qualcomm
IPQ SoCs and to GCC. The driver is initially supported for
IPQ9574 SoC.

The CMN PLL clock controller expects a reference input clock
from the on-board Wi-Fi block acting as clock source. The input
reference clock needs to be configured to one of the supported
clock rates.

The controller supplies a number of fixed-rate output clocks.
For the IPQ9574, there is one output clock of 353 MHZ to PPE
(Packet Process Engine) hardware block, three 50 MHZ output
clocks and an additional 25 MHZ output clock supplied to the
connected Ethernet devices. The PLL also supplies a 24 MHZ
clock as XO and a 32 KHZ sleep clock to GCC, and one 31.25
MHZ clock to PCS.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 drivers/clk/qcom/Kconfig       |  10 +
 drivers/clk/qcom/Makefile      |   1 +
 drivers/clk/qcom/ipq-cmn-pll.c | 411 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 422 insertions(+)

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 30eb8236c9d8..3def659fc5cb 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -190,6 +190,16 @@ config IPQ_APSS_6018
 	  Say Y if you want to support CPU frequency scaling on
 	  ipq based devices.
 
+config IPQ_CMN_PLL
+	tristate "IPQ CMN PLL Clock Controller"
+	depends on IPQ_GCC_9574
+	help
+	  Support for CMN PLL clock controller on IPQ platform. The
+	  CMN PLL feeds the reference clocks to the Ethernet devices
+	  based on IPQ SoC.
+	  Say Y or M if you want to support CMN PLL clock on the IPQ
+	  based devices.
+
 config IPQ_GCC_4019
 	tristate "IPQ4019 Global Clock Controller"
 	help
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 2b378667a63f..83d11434714c 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_CLK_X1E80100_TCSRCC) += tcsrcc-x1e80100.o
 obj-$(CONFIG_CLK_QCM2290_GPUCC) += gpucc-qcm2290.o
 obj-$(CONFIG_IPQ_APSS_PLL) += apss-ipq-pll.o
 obj-$(CONFIG_IPQ_APSS_6018) += apss-ipq6018.o
+obj-$(CONFIG_IPQ_CMN_PLL) += ipq-cmn-pll.o
 obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o
 obj-$(CONFIG_IPQ_GCC_5018) += gcc-ipq5018.o
 obj-$(CONFIG_IPQ_GCC_5332) += gcc-ipq5332.o
diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
new file mode 100644
index 000000000000..f5ebc7d93ed8
--- /dev/null
+++ b/drivers/clk/qcom/ipq-cmn-pll.c
@@ -0,0 +1,411 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+/*
+ * CMN PLL block expects the reference clock from on-board Wi-Fi block,
+ * and supplies fixed rate clocks as output to the networking hardware
+ * blocks and to GCC. The networking related blocks include PPE (packet
+ * process engine), the externally connected PHY or switch devices, and
+ * the PCS.
+ *
+ * On the IPQ9574 SoC, there are three clocks with 50 MHZ and one clock
+ * with 25 MHZ which are output from the CMN PLL to Ethernet PHY (or switch),
+ * and one clock with 353 MHZ to PPE. The other fixed rate output clocks
+ * are supplied to GCC (24 MHZ as XO and 32 KHZ as sleep clock), and to PCS
+ * with 31.25 MHZ.
+ *
+ *               +---------+
+ *               |   GCC   |
+ *               +--+---+--+
+ *           AHB CLK|   |SYS CLK
+ *                  V   V
+ *          +-------+---+------+
+ *          |                  +-------------> eth0-50mhz
+ * REF CLK  |     IPQ9574      |
+ * -------->+                  +-------------> eth1-50mhz
+ *          |  CMN PLL block   |
+ *          |                  +-------------> eth2-50mhz
+ *          |                  |
+ *          +----+----+----+---+-------------> eth-25mhz
+ *               |    |    |
+ *               V    V    V
+ *              GCC  PCS  NSS/PPE
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
+
+#define CMN_PLL_REFCLK_SRC_SELECTION		0x28
+#define CMN_PLL_REFCLK_SRC_DIV			GENMASK(9, 8)
+
+#define CMN_PLL_LOCKED				0x64
+#define CMN_PLL_CLKS_LOCKED			BIT(8)
+
+#define CMN_PLL_POWER_ON_AND_RESET		0x780
+#define CMN_ANA_EN_SW_RSTN			BIT(6)
+
+#define CMN_PLL_REFCLK_CONFIG			0x784
+#define CMN_PLL_REFCLK_EXTERNAL			BIT(9)
+#define CMN_PLL_REFCLK_DIV			GENMASK(8, 4)
+#define CMN_PLL_REFCLK_INDEX			GENMASK(3, 0)
+
+#define CMN_PLL_CTRL				0x78c
+#define CMN_PLL_CTRL_LOCK_DETECT_EN		BIT(15)
+
+#define CMN_PLL_DIVIDER_CTRL			0x794
+#define CMN_PLL_DIVIDER_CTRL_FACTOR		GENMASK(9, 0)
+
+/**
+ * struct cmn_pll_fixed_output_clk - CMN PLL output clocks information
+ * @id:	Clock specifier to be supplied
+ * @name: Clock name to be registered
+ * @rate: Clock rate
+ */
+struct cmn_pll_fixed_output_clk {
+	unsigned int id;
+	const char *name;
+	unsigned long rate;
+};
+
+/**
+ * struct clk_cmn_pll - CMN PLL hardware specific data
+ * @regmap: hardware regmap.
+ * @hw: handle between common and hardware-specific interfaces
+ */
+struct clk_cmn_pll {
+	struct regmap *regmap;
+	struct clk_hw hw;
+};
+
+#define CLK_PLL_OUTPUT(_id, _name, _rate) {		\
+	.id =		_id,				\
+	.name =		_name,				\
+	.rate =		_rate,				\
+}
+
+#define to_clk_cmn_pll(_hw) container_of(_hw, struct clk_cmn_pll, hw)
+
+static const struct regmap_config ipq_cmn_pll_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = 0x7fc,
+	.fast_io = true,
+};
+
+static const struct cmn_pll_fixed_output_clk ipq9574_output_clks[] = {
+	CLK_PLL_OUTPUT(XO_24MHZ_CLK, "xo-24mhz", 24000000UL),
+	CLK_PLL_OUTPUT(SLEEP_32KHZ_CLK, "sleep-32khz", 32000UL),
+	CLK_PLL_OUTPUT(PCS_31P25MHZ_CLK, "pcs-31p25mhz", 31250000UL),
+	CLK_PLL_OUTPUT(NSS_1200MHZ_CLK, "nss-1200mhz", 1200000000UL),
+	CLK_PLL_OUTPUT(PPE_353MHZ_CLK, "ppe-353mhz", 353000000UL),
+	CLK_PLL_OUTPUT(ETH0_50MHZ_CLK, "eth0-50mhz", 50000000UL),
+	CLK_PLL_OUTPUT(ETH1_50MHZ_CLK, "eth1-50mhz", 50000000UL),
+	CLK_PLL_OUTPUT(ETH2_50MHZ_CLK, "eth2-50mhz", 50000000UL),
+	CLK_PLL_OUTPUT(ETH_25MHZ_CLK, "eth-25mhz", 25000000UL),
+};
+
+static unsigned long clk_cmn_pll_recalc_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
+{
+	struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
+	u32 val, factor;
+
+	/*
+	 * The value of CMN_PLL_DIVIDER_CTRL_FACTOR is automatically adjusted
+	 * by HW according to the parent clock rate.
+	 */
+	regmap_read(cmn_pll->regmap, CMN_PLL_DIVIDER_CTRL, &val);
+	factor = FIELD_GET(CMN_PLL_DIVIDER_CTRL_FACTOR, val);
+
+	return parent_rate * 2 * factor;
+}
+
+/*
+ * This function is used to initialize the CMN PLL to enable the fixed
+ * rate output clocks. It is expected to be configured once.
+ */
+static int clk_cmn_pll_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
+	u32 val;
+	int ret;
+
+	/*
+	 * Configure the reference input clock selection as per the given
+	 * parent clock. The output clock rates are always of fixed value.
+	 */
+	switch (req->best_parent_rate) {
+	case 25000000:
+		val = 3;
+		break;
+	case 31250000:
+		val = 4;
+		break;
+	case 40000000:
+		val = 6;
+		break;
+	case 50000000:
+		val = 8;
+		break;
+	case 48000000:
+	case 96000000:
+		/*
+		 * Parent clock rate 48 MHZ and 96 MHZ take the same value
+		 * of reference clock index. 96 MHZ needs the source clock
+		 * divider to be programmed as 2.
+		 */
+		val = 7;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_REFCLK_CONFIG,
+				 CMN_PLL_REFCLK_INDEX,
+				 FIELD_PREP(CMN_PLL_REFCLK_INDEX, val));
+	if (ret)
+		return ret;
+
+	/*
+	 * Update the source clock rate selection and source clock
+	 * divider as 2 when the parent clock rate is 96 MHZ.
+	 */
+	if (req->best_parent_rate == 96000000) {
+		ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_REFCLK_CONFIG,
+					 CMN_PLL_REFCLK_DIV,
+					 FIELD_PREP(CMN_PLL_REFCLK_DIV, 2));
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_REFCLK_SRC_SELECTION,
+					 CMN_PLL_REFCLK_SRC_DIV,
+					 FIELD_PREP(CMN_PLL_REFCLK_SRC_DIV, 0));
+		if (ret)
+			return ret;
+	}
+
+	/* Enable PLL locked detect. */
+	ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_CTRL,
+				 CMN_PLL_CTRL_LOCK_DETECT_EN,
+				 CMN_PLL_CTRL_LOCK_DETECT_EN);
+	if (ret)
+		return ret;
+
+	/*
+	 * Reset the CMN PLL block to ensure the updated configurations
+	 * take effect.
+	 */
+	ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_POWER_ON_AND_RESET,
+				 CMN_ANA_EN_SW_RSTN, 0);
+	if (ret)
+		return ret;
+
+	usleep_range(1000, 1200);
+	ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_POWER_ON_AND_RESET,
+				 CMN_ANA_EN_SW_RSTN, CMN_ANA_EN_SW_RSTN);
+	if (ret)
+		return ret;
+
+	/* Stability check of CMN PLL output clocks. */
+	return regmap_read_poll_timeout(cmn_pll->regmap, CMN_PLL_LOCKED, val,
+					(val & CMN_PLL_CLKS_LOCKED),
+					100, 100 * USEC_PER_MSEC);
+}
+
+static const struct clk_ops clk_cmn_pll_ops = {
+	.determine_rate = clk_cmn_pll_determine_rate,
+	.recalc_rate = clk_cmn_pll_recalc_rate,
+};
+
+static struct clk_hw *ipq_cmn_pll_clk_hw_register(struct platform_device *pdev)
+{
+	struct clk_parent_data pdata = { .index = 0 };
+	struct device *dev = &pdev->dev;
+	struct clk_init_data init = {};
+	struct clk_cmn_pll *cmn_pll;
+	struct regmap *regmap;
+	void __iomem *base;
+	int ret;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return ERR_CAST(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &ipq_cmn_pll_regmap_config);
+	if (IS_ERR(regmap))
+		return ERR_CAST(regmap);
+
+	cmn_pll = devm_kzalloc(dev, sizeof(*cmn_pll), GFP_KERNEL);
+	if (!cmn_pll)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = "cmn_pll";
+	init.parent_data = &pdata;
+	init.num_parents = 1;
+	init.ops = &clk_cmn_pll_ops;
+
+	cmn_pll->hw.init = &init;
+	cmn_pll->regmap = regmap;
+
+	ret = devm_clk_hw_register(dev, &cmn_pll->hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &cmn_pll->hw;
+}
+
+static int ipq_cmn_pll_register_clks(struct platform_device *pdev)
+{
+	const struct cmn_pll_fixed_output_clk *fixed_clk;
+	struct clk_hw_onecell_data *hw_data;
+	struct device *dev = &pdev->dev;
+	struct clk_hw *cmn_pll_hw;
+	unsigned int num_clks;
+	struct clk_hw *hw;
+	int ret, i;
+
+	fixed_clk = ipq9574_output_clks;
+	num_clks = ARRAY_SIZE(ipq9574_output_clks);
+
+	hw_data = devm_kzalloc(dev, struct_size(hw_data, hws, num_clks + 1),
+			       GFP_KERNEL);
+	if (!hw_data)
+		return -ENOMEM;
+
+	/*
+	 * Register the CMN PLL clock, which is the parent clock of
+	 * the fixed rate output clocks.
+	 */
+	cmn_pll_hw = ipq_cmn_pll_clk_hw_register(pdev);
+	if (IS_ERR(cmn_pll_hw))
+		return PTR_ERR(cmn_pll_hw);
+
+	/* Register the fixed rate output clocks. */
+	for (i = 0; i < num_clks; i++) {
+		hw = clk_hw_register_fixed_rate_parent_hw(dev, fixed_clk[i].name,
+							  cmn_pll_hw, 0,
+							  fixed_clk[i].rate);
+		if (IS_ERR(hw)) {
+			ret = PTR_ERR(hw);
+			goto unregister_fixed_clk;
+		}
+
+		hw_data->hws[fixed_clk[i].id] = hw;
+	}
+
+	/*
+	 * Provide the CMN PLL clock. The clock rate of CMN PLL
+	 * is configured to 12 GHZ by DT property assigned-clock-rates-u64.
+	 */
+	hw_data->hws[CMN_PLL_CLK] = cmn_pll_hw;
+	hw_data->num = num_clks + 1;
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, hw_data);
+	if (ret)
+		goto unregister_fixed_clk;
+
+	platform_set_drvdata(pdev, hw_data);
+
+	return 0;
+
+unregister_fixed_clk:
+	while (i > 0)
+		clk_hw_unregister(hw_data->hws[fixed_clk[--i].id]);
+
+	return ret;
+}
+
+static int ipq_cmn_pll_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_clk_create(dev);
+	if (ret)
+		return ret;
+
+	/*
+	 * To access the CMN PLL registers, the GCC AHB & SYSY clocks
+	 * of CMN PLL block need to be enabled.
+	 */
+	ret = pm_clk_add(dev, "ahb");
+	if (ret)
+		return dev_err_probe(dev, ret, "Fail to add AHB clock\n");
+
+	ret = pm_clk_add(dev, "sys");
+	if (ret)
+		return dev_err_probe(dev, ret, "Fail to add SYS clock\n");
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	/* Register CMN PLL clock and fixed rate output clocks. */
+	ret = ipq_cmn_pll_register_clks(pdev);
+	pm_runtime_put(dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Fail to register CMN PLL clocks\n");
+
+	return 0;
+}
+
+static void ipq_cmn_pll_clk_remove(struct platform_device *pdev)
+{
+	struct clk_hw_onecell_data *hw_data = platform_get_drvdata(pdev);
+	int i;
+
+	/*
+	 * The clock with index CMN_PLL_CLK is unregistered by
+	 * device management.
+	 */
+	for (i = 0; i < hw_data->num; i++) {
+		if (i != CMN_PLL_CLK)
+			clk_hw_unregister(hw_data->hws[i]);
+	}
+}
+
+static const struct dev_pm_ops ipq_cmn_pll_pm_ops = {
+	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
+};
+
+static const struct of_device_id ipq_cmn_pll_clk_ids[] = {
+	{ .compatible = "qcom,ipq9574-cmn-pll", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ipq_cmn_pll_clk_ids);
+
+static struct platform_driver ipq_cmn_pll_clk_driver = {
+	.probe = ipq_cmn_pll_clk_probe,
+	.remove = ipq_cmn_pll_clk_remove,
+	.driver = {
+		.name = "ipq_cmn_pll",
+		.of_match_table = ipq_cmn_pll_clk_ids,
+		.pm = &ipq_cmn_pll_pm_ops,
+	},
+};
+module_platform_driver(ipq_cmn_pll_clk_driver);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. IPQ CMN PLL Driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v4 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

[Stephen Boyd]

Quoting Luo Jie (2024-10-15 07:16:52)
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 30eb8236c9d8..3def659fc5cb 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -190,6 +190,16 @@ config IPQ_APSS_6018
>           Say Y if you want to support CPU frequency scaling on
>           ipq based devices.
>  
> +config IPQ_CMN_PLL
> +       tristate "IPQ CMN PLL Clock Controller"
> +       depends on IPQ_GCC_9574

What is the build dependency?

> +       help
> +         Support for CMN PLL clock controller on IPQ platform. The
> +         CMN PLL feeds the reference clocks to the Ethernet devices
> +         based on IPQ SoC.
> +         Say Y or M if you want to support CMN PLL clock on the IPQ
> +         based devices.
> +
>  config IPQ_GCC_4019
>         tristate "IPQ4019 Global Clock Controller"
>         help
> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
> new file mode 100644
> index 000000000000..f5ebc7d93ed8
> --- /dev/null
> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/*
> + * CMN PLL block expects the reference clock from on-board Wi-Fi block,
> + * and supplies fixed rate clocks as output to the networking hardware
> + * blocks and to GCC. The networking related blocks include PPE (packet
> + * process engine), the externally connected PHY or switch devices, and
> + * the PCS.
> + *
> + * On the IPQ9574 SoC, there are three clocks with 50 MHZ and one clock
> + * with 25 MHZ which are output from the CMN PLL to Ethernet PHY (or switch),
> + * and one clock with 353 MHZ to PPE. The other fixed rate output clocks
> + * are supplied to GCC (24 MHZ as XO and 32 KHZ as sleep clock), and to PCS
> + * with 31.25 MHZ.
> + *
> + *               +---------+
> + *               |   GCC   |
> + *               +--+---+--+
> + *           AHB CLK|   |SYS CLK
> + *                  V   V
> + *          +-------+---+------+
> + *          |                  +-------------> eth0-50mhz
> + * REF CLK  |     IPQ9574      |
> + * -------->+                  +-------------> eth1-50mhz
> + *          |  CMN PLL block   |
> + *          |                  +-------------> eth2-50mhz
> + *          |                  |
> + *          +----+----+----+---+-------------> eth-25mhz
> + *               |    |    |
> + *               V    V    V
> + *              GCC  PCS  NSS/PPE
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>

What is of_address.h for? Did you mean mod_devicetable.h?

> +#include <linux/platform_device.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
> +
> +#define CMN_PLL_REFCLK_SRC_SELECTION           0x28
> +#define CMN_PLL_REFCLK_SRC_DIV                 GENMASK(9, 8)
> +
> +#define CMN_PLL_LOCKED                         0x64
> +#define CMN_PLL_CLKS_LOCKED                    BIT(8)
> +
> +#define CMN_PLL_POWER_ON_AND_RESET             0x780
> +#define CMN_ANA_EN_SW_RSTN                     BIT(6)
> +
> +#define CMN_PLL_REFCLK_CONFIG                  0x784
> +#define CMN_PLL_REFCLK_EXTERNAL                        BIT(9)
> +#define CMN_PLL_REFCLK_DIV                     GENMASK(8, 4)
[...]
> +
> +/*
> + * This function is used to initialize the CMN PLL to enable the fixed
> + * rate output clocks. It is expected to be configured once.
> + */
> +static int clk_cmn_pll_determine_rate(struct clk_hw *hw,
> +                                     struct clk_rate_request *req)
> +{
> +       struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
> +       u32 val;
> +       int ret;
> +
> +       /*
> +        * Configure the reference input clock selection as per the given
> +        * parent clock. The output clock rates are always of fixed value.
> +        */
> +       switch (req->best_parent_rate) {
> +       case 25000000:
> +               val = 3;
> +               break;
> +       case 31250000:
> +               val = 4;
> +               break;
> +       case 40000000:
> +               val = 6;
> +               break;
> +       case 50000000:
> +               val = 8;
> +               break;
> +       case 48000000:
> +       case 96000000:
> +               /*
> +                * Parent clock rate 48 MHZ and 96 MHZ take the same value
> +                * of reference clock index. 96 MHZ needs the source clock
> +                * divider to be programmed as 2.
> +                */
> +               val = 7;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_REFCLK_CONFIG,
> +                                CMN_PLL_REFCLK_INDEX,
> +                                FIELD_PREP(CMN_PLL_REFCLK_INDEX, val));

The determine_rate() function shouldn't modify the hardware. This should
be done in the set_rate() callback. Likely you'll need to use
assigned-clock-rates to do that.

Re: [PATCH v4 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

[Jie Luo]

On 10/17/2024 5:37 AM, Stephen Boyd wrote:
> Quoting Luo Jie (2024-10-15 07:16:52)
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index 30eb8236c9d8..3def659fc5cb 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -190,6 +190,16 @@ config IPQ_APSS_6018
>>            Say Y if you want to support CPU frequency scaling on
>>            ipq based devices.
>>   
>> +config IPQ_CMN_PLL
>> +       tristate "IPQ CMN PLL Clock Controller"
>> +       depends on IPQ_GCC_9574
> 
> What is the build dependency?

Will remove this dependency. There is no build dependency on 
IPQ_GCC_9574, but only a functional dependency on the SoC's GCC block
since the CMNPLL consumes AHB/SYS clocks from the GCC.

> 
>> +       help
>> +         Support for CMN PLL clock controller on IPQ platform. The
>> +         CMN PLL feeds the reference clocks to the Ethernet devices
>> +         based on IPQ SoC.
>> +         Say Y or M if you want to support CMN PLL clock on the IPQ
>> +         based devices.
>> +
>>   config IPQ_GCC_4019
>>          tristate "IPQ4019 Global Clock Controller"
>>          help
>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>> new file mode 100644
>> index 000000000000..f5ebc7d93ed8
>> --- /dev/null
>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
>> @@ -0,0 +1,411 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +/*
>> + * CMN PLL block expects the reference clock from on-board Wi-Fi block,
>> + * and supplies fixed rate clocks as output to the networking hardware
>> + * blocks and to GCC. The networking related blocks include PPE (packet
>> + * process engine), the externally connected PHY or switch devices, and
>> + * the PCS.
>> + *
>> + * On the IPQ9574 SoC, there are three clocks with 50 MHZ and one clock
>> + * with 25 MHZ which are output from the CMN PLL to Ethernet PHY (or switch),
>> + * and one clock with 353 MHZ to PPE. The other fixed rate output clocks
>> + * are supplied to GCC (24 MHZ as XO and 32 KHZ as sleep clock), and to PCS
>> + * with 31.25 MHZ.
>> + *
>> + *               +---------+
>> + *               |   GCC   |
>> + *               +--+---+--+
>> + *           AHB CLK|   |SYS CLK
>> + *                  V   V
>> + *          +-------+---+------+
>> + *          |                  +-------------> eth0-50mhz
>> + * REF CLK  |     IPQ9574      |
>> + * -------->+                  +-------------> eth1-50mhz
>> + *          |  CMN PLL block   |
>> + *          |                  +-------------> eth2-50mhz
>> + *          |                  |
>> + *          +----+----+----+---+-------------> eth-25mhz
>> + *               |    |    |
>> + *               V    V    V
>> + *              GCC  PCS  NSS/PPE
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
> 
> What is of_address.h for? Did you mean mod_devicetable.h?

I will replace of_address.h with mod_devicetable.h for 'of_device_id'
structure usage.

> 
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
>> +
>> +#define CMN_PLL_REFCLK_SRC_SELECTION           0x28
>> +#define CMN_PLL_REFCLK_SRC_DIV                 GENMASK(9, 8)
>> +
>> +#define CMN_PLL_LOCKED                         0x64
>> +#define CMN_PLL_CLKS_LOCKED                    BIT(8)
>> +
>> +#define CMN_PLL_POWER_ON_AND_RESET             0x780
>> +#define CMN_ANA_EN_SW_RSTN                     BIT(6)
>> +
>> +#define CMN_PLL_REFCLK_CONFIG                  0x784
>> +#define CMN_PLL_REFCLK_EXTERNAL                        BIT(9)
>> +#define CMN_PLL_REFCLK_DIV                     GENMASK(8, 4)
> [...]
>> +
>> +/*
>> + * This function is used to initialize the CMN PLL to enable the fixed
>> + * rate output clocks. It is expected to be configured once.
>> + */
>> +static int clk_cmn_pll_determine_rate(struct clk_hw *hw,
>> +                                     struct clk_rate_request *req)
>> +{
>> +       struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
>> +       u32 val;
>> +       int ret;
>> +
>> +       /*
>> +        * Configure the reference input clock selection as per the given
>> +        * parent clock. The output clock rates are always of fixed value.
>> +        */
>> +       switch (req->best_parent_rate) {
>> +       case 25000000:
>> +               val = 3;
>> +               break;
>> +       case 31250000:
>> +               val = 4;
>> +               break;
>> +       case 40000000:
>> +               val = 6;
>> +               break;
>> +       case 50000000:
>> +               val = 8;
>> +               break;
>> +       case 48000000:
>> +       case 96000000:
>> +               /*
>> +                * Parent clock rate 48 MHZ and 96 MHZ take the same value
>> +                * of reference clock index. 96 MHZ needs the source clock
>> +                * divider to be programmed as 2.
>> +                */
>> +               val = 7;
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_REFCLK_CONFIG,
>> +                                CMN_PLL_REFCLK_INDEX,
>> +                                FIELD_PREP(CMN_PLL_REFCLK_INDEX, val));
> 
> The determine_rate() function shouldn't modify the hardware. This should
> be done in the set_rate() callback. Likely you'll need to use
> assigned-clock-rates to do that.

OK. I will move the hardware configuration code into clk_ops::set_rate().
We are using the DT property assigned-clock-rates-u64 to configure the
clock rate of CMN PLL to 12 GHZ since 64 bits are required.

Re: [PATCH v4 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

[Stephen Boyd]

Quoting Jie Luo (2024-10-17 08:35:43)
> On 10/17/2024 5:37 AM, Stephen Boyd wrote:
> > Quoting Luo Jie (2024-10-15 07:16:52)
> >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> >> index 30eb8236c9d8..3def659fc5cb 100644
> >> --- a/drivers/clk/qcom/Kconfig
> >> +++ b/drivers/clk/qcom/Kconfig
> >> @@ -190,6 +190,16 @@ config IPQ_APSS_6018
> >>            Say Y if you want to support CPU frequency scaling on
> >>            ipq based devices.
> >>   
> >> +config IPQ_CMN_PLL
> >> +       tristate "IPQ CMN PLL Clock Controller"
> >> +       depends on IPQ_GCC_9574
> > 
> > What is the build dependency?
> 
> Will remove this dependency. There is no build dependency on 
> IPQ_GCC_9574, but only a functional dependency on the SoC's GCC block
> since the CMNPLL consumes AHB/SYS clocks from the GCC.

Ok. It can probably be a select or imply statement then.

> 
> > 
> >> +       help
> >> +         Support for CMN PLL clock controller on IPQ platform. The
> >> +         CMN PLL feeds the reference clocks to the Ethernet devices
> >> +         based on IPQ SoC.
> >> +         Say Y or M if you want to support CMN PLL clock on the IPQ
> >> +         based devices.
> >> +
> >>   config IPQ_GCC_4019
> >>          tristate "IPQ4019 Global Clock Controller"
> >>          help
> >> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
> >> new file mode 100644
> >> index 000000000000..f5ebc7d93ed8
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
[...]
> >> +       }
> >> +
> >> +       ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_REFCLK_CONFIG,
> >> +                                CMN_PLL_REFCLK_INDEX,
> >> +                                FIELD_PREP(CMN_PLL_REFCLK_INDEX, val));
> > 
> > The determine_rate() function shouldn't modify the hardware. This should
> > be done in the set_rate() callback. Likely you'll need to use
> > assigned-clock-rates to do that.
> 
> OK. I will move the hardware configuration code into clk_ops::set_rate().
> We are using the DT property assigned-clock-rates-u64 to configure the
> clock rate of CMN PLL to 12 GHZ since 64 bits are required.
> 

Sounds good. Thanks.

Re: [PATCH v4 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

[Jie Luo]

On 10/18/2024 1:40 AM, Stephen Boyd wrote:
> Quoting Jie Luo (2024-10-17 08:35:43)
>> On 10/17/2024 5:37 AM, Stephen Boyd wrote:
>>> Quoting Luo Jie (2024-10-15 07:16:52)
>>>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>>>> index 30eb8236c9d8..3def659fc5cb 100644
>>>> --- a/drivers/clk/qcom/Kconfig
>>>> +++ b/drivers/clk/qcom/Kconfig
>>>> @@ -190,6 +190,16 @@ config IPQ_APSS_6018
>>>>             Say Y if you want to support CPU frequency scaling on
>>>>             ipq based devices.
>>>>    
>>>> +config IPQ_CMN_PLL
>>>> +       tristate "IPQ CMN PLL Clock Controller"
>>>> +       depends on IPQ_GCC_9574
>>> What is the build dependency?
>> Will remove this dependency. There is no build dependency on
>> IPQ_GCC_9574, but only a functional dependency on the SoC's GCC block
>> since the CMNPLL consumes AHB/SYS clocks from the GCC.
> Ok. It can probably be a select or imply statement then.

Thanks for suggestions. I will enhance the help text to imply the GCC
dependency.

[PATCH v4 3/4] arm64: defconfig: Enable Qualcomm IPQ CMN PLL clock controller

[Luo Jie]

The CMN PLL hardware block is available in the Qualcomm IPQ SoC such
as IPQ9574 and IPQ5332. It provides fixed rate output clocks to Ethernet
related hardware blocks such as external Ethernet PHY or switch. This
driver is initially being enabled for IPQ9574. All boards based on
IPQ9574 SoC will require to include this driver in the build.

This CMN PLL hardware block does not provide any other specific function
on the IPQ SoC other than enabling output clocks to Ethernet related
devices.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 5fdbfea7a5b2..11aefa9ef7b8 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1308,6 +1308,7 @@ CONFIG_QCOM_CLK_SMD_RPM=y
 CONFIG_QCOM_CLK_RPMH=y
 CONFIG_IPQ_APSS_6018=y
 CONFIG_IPQ_APSS_5018=y
+CONFIG_IPQ_CMN_PLL=m
 CONFIG_IPQ_GCC_5018=y
 CONFIG_IPQ_GCC_5332=y
 CONFIG_IPQ_GCC_6018=y

-- 
2.34.1

[PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Luo Jie]

The CMN PLL clock controller allows selection of an input
clock rate from a defined set of input clock rates. It in-turn
supplies fixed rate output clocks to the hardware blocks that
provide ethernet functions such as PPE (Packet Process Engine)
and connected switch or PHY, and to GCC.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
 arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
index 91e104b0f865..77e1e42083f3 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
@@ -3,7 +3,7 @@
  * IPQ9574 RDP board common device tree source
  *
  * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 /dts-v1/;
@@ -164,6 +164,10 @@ &usb3 {
 	status = "okay";
 };
 
+&cmn_pll_ref_clk {
+	clock-frequency = <48000000>;
+};
+
 &xo_board_clk {
 	clock-frequency = <24000000>;
 };
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 14c7b3a78442..93f66bb83c5a 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -3,10 +3,11 @@
  * IPQ9574 SoC device tree source
  *
  * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include <dt-bindings/clock/qcom,apss-ipq.h>
+#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
 #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
 #include <dt-bindings/interconnect/qcom,ipq9574.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -19,6 +20,11 @@ / {
 	#size-cells = <2>;
 
 	clocks {
+		cmn_pll_ref_clk: cmn-pll-ref-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+		};
+
 		sleep_clk: sleep-clk {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
@@ -243,6 +249,18 @@ mdio: mdio@90000 {
 			status = "disabled";
 		};
 
+		cmn_pll: clock-controller@9b000 {
+			compatible = "qcom,ipq9574-cmn-pll";
+			reg = <0x0009b000 0x800>;
+			clocks = <&cmn_pll_ref_clk>,
+				 <&gcc GCC_CMN_12GPLL_AHB_CLK>,
+				 <&gcc GCC_CMN_12GPLL_SYS_CLK>;
+			clock-names = "ref", "ahb", "sys";
+			#clock-cells = <1>;
+			assigned-clocks = <&cmn_pll CMN_PLL_CLK>;
+			assigned-clock-rates-u64 = /bits/ 64 <12000000000>;
+		};
+
 		qfprom: efuse@a4000 {
 			compatible = "qcom,ipq9574-qfprom", "qcom,qfprom";
 			reg = <0x000a4000 0x5a1>;

-- 
2.34.1

Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Dmitry Baryshkov]

On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
> The CMN PLL clock controller allows selection of an input
> clock rate from a defined set of input clock rates. It in-turn
> supplies fixed rate output clocks to the hardware blocks that
> provide ethernet functions such as PPE (Packet Process Engine)
> and connected switch or PHY, and to GCC.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> index 91e104b0f865..77e1e42083f3 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> @@ -3,7 +3,7 @@
>   * IPQ9574 RDP board common device tree source
>   *
>   * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  /dts-v1/;
> @@ -164,6 +164,10 @@ &usb3 {
>  	status = "okay";
>  };
>  
> +&cmn_pll_ref_clk {
> +	clock-frequency = <48000000>;
> +};
> +
>  &xo_board_clk {
>  	clock-frequency = <24000000>;
>  };
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 14c7b3a78442..93f66bb83c5a 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -3,10 +3,11 @@
>   * IPQ9574 SoC device tree source
>   *
>   * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #include <dt-bindings/clock/qcom,apss-ipq.h>
> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
>  #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
>  #include <dt-bindings/interconnect/qcom,ipq9574.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> @@ -19,6 +20,11 @@ / {
>  	#size-cells = <2>;
>  
>  	clocks {
> +		cmn_pll_ref_clk: cmn-pll-ref-clk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +		};

Which block provides this clock? If it is provided by the external XO
then it should not be a part of the SoC dtsi.

> +
>  		sleep_clk: sleep-clk {
>  			compatible = "fixed-clock";
>  			#clock-cells = <0>;
> @@ -243,6 +249,18 @@ mdio: mdio@90000 {
>  			status = "disabled";
>  		};
>  
> +		cmn_pll: clock-controller@9b000 {
> +			compatible = "qcom,ipq9574-cmn-pll";
> +			reg = <0x0009b000 0x800>;
> +			clocks = <&cmn_pll_ref_clk>,
> +				 <&gcc GCC_CMN_12GPLL_AHB_CLK>,
> +				 <&gcc GCC_CMN_12GPLL_SYS_CLK>;
> +			clock-names = "ref", "ahb", "sys";
> +			#clock-cells = <1>;
> +			assigned-clocks = <&cmn_pll CMN_PLL_CLK>;
> +			assigned-clock-rates-u64 = /bits/ 64 <12000000000>;
> +		};
> +
>  		qfprom: efuse@a4000 {
>  			compatible = "qcom,ipq9574-qfprom", "qcom,qfprom";
>  			reg = <0x000a4000 0x5a1>;
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Jie Luo]

On 10/18/2024 6:32 AM, Dmitry Baryshkov wrote:
> On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
>> The CMN PLL clock controller allows selection of an input
>> clock rate from a defined set of input clock rates. It in-turn
>> supplies fixed rate output clocks to the hardware blocks that
>> provide ethernet functions such as PPE (Packet Process Engine)
>> and connected switch or PHY, and to GCC.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
>>   2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>> index 91e104b0f865..77e1e42083f3 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>> @@ -3,7 +3,7 @@
>>    * IPQ9574 RDP board common device tree source
>>    *
>>    * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   /dts-v1/;
>> @@ -164,6 +164,10 @@ &usb3 {
>>   	status = "okay";
>>   };
>>   
>> +&cmn_pll_ref_clk {
>> +	clock-frequency = <48000000>;
>> +};
>> +
>>   &xo_board_clk {
>>   	clock-frequency = <24000000>;
>>   };
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> index 14c7b3a78442..93f66bb83c5a 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -3,10 +3,11 @@
>>    * IPQ9574 SoC device tree source
>>    *
>>    * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #include <dt-bindings/clock/qcom,apss-ipq.h>
>> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
>>   #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
>>   #include <dt-bindings/interconnect/qcom,ipq9574.h>
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>> @@ -19,6 +20,11 @@ / {
>>   	#size-cells = <2>;
>>   
>>   	clocks {
>> +		cmn_pll_ref_clk: cmn-pll-ref-clk {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +		};
> 
> Which block provides this clock? If it is provided by the external XO
> then it should not be a part of the SoC dtsi.

The on-chip WiFi block supplies this reference clock. So keeping it in
the SoC DTSI is perhaps appropriate.

> 
>> +
>>   		sleep_clk: sleep-clk {
>>   			compatible = "fixed-clock";
>>   			#clock-cells = <0>;
>> @@ -243,6 +249,18 @@ mdio: mdio@90000 {
>>   			status = "disabled";
>>   		};
>>   
>> +		cmn_pll: clock-controller@9b000 {
>> +			compatible = "qcom,ipq9574-cmn-pll";
>> +			reg = <0x0009b000 0x800>;
>> +			clocks = <&cmn_pll_ref_clk>,
>> +				 <&gcc GCC_CMN_12GPLL_AHB_CLK>,
>> +				 <&gcc GCC_CMN_12GPLL_SYS_CLK>;
>> +			clock-names = "ref", "ahb", "sys";
>> +			#clock-cells = <1>;
>> +			assigned-clocks = <&cmn_pll CMN_PLL_CLK>;
>> +			assigned-clock-rates-u64 = /bits/ 64 <12000000000>;
>> +		};
>> +
>>   		qfprom: efuse@a4000 {
>>   			compatible = "qcom,ipq9574-qfprom", "qcom,qfprom";
>>   			reg = <0x000a4000 0x5a1>;
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Dmitry Baryshkov]

On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@quicinc.com> wrote:
>
>
>
> On 10/18/2024 6:32 AM, Dmitry Baryshkov wrote:
> > On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
> >> The CMN PLL clock controller allows selection of an input
> >> clock rate from a defined set of input clock rates. It in-turn
> >> supplies fixed rate output clocks to the hardware blocks that
> >> provide ethernet functions such as PPE (Packet Process Engine)
> >> and connected switch or PHY, and to GCC.
> >>
> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> >> ---
> >>   arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
> >>   arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
> >>   2 files changed, 24 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> >> index 91e104b0f865..77e1e42083f3 100644
> >> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> >> @@ -3,7 +3,7 @@
> >>    * IPQ9574 RDP board common device tree source
> >>    *
> >>    * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> >> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> >> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> >>    */
> >>
> >>   /dts-v1/;
> >> @@ -164,6 +164,10 @@ &usb3 {
> >>      status = "okay";
> >>   };
> >>
> >> +&cmn_pll_ref_clk {
> >> +    clock-frequency = <48000000>;
> >> +};
> >> +
> >>   &xo_board_clk {
> >>      clock-frequency = <24000000>;
> >>   };
> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >> index 14c7b3a78442..93f66bb83c5a 100644
> >> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >> @@ -3,10 +3,11 @@
> >>    * IPQ9574 SoC device tree source
> >>    *
> >>    * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> >> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> >> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> >>    */
> >>
> >>   #include <dt-bindings/clock/qcom,apss-ipq.h>
> >> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
> >>   #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
> >>   #include <dt-bindings/interconnect/qcom,ipq9574.h>
> >>   #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> @@ -19,6 +20,11 @@ / {
> >>      #size-cells = <2>;
> >>
> >>      clocks {
> >> +            cmn_pll_ref_clk: cmn-pll-ref-clk {
> >> +                    compatible = "fixed-clock";
> >> +                    #clock-cells = <0>;
> >> +            };
> >
> > Which block provides this clock? If it is provided by the external XO
> > then it should not be a part of the SoC dtsi.
>
> The on-chip WiFi block supplies this reference clock. So keeping it in
> the SoC DTSI is perhaps appropriate.

Then maybe it should be provided by the WiFi device node? At least you
should document your design decisions in the commit message.

Also, I don't think this node passes DT schema validation. Did you check it?

>
> >
> >> +
> >>              sleep_clk: sleep-clk {
> >>                      compatible = "fixed-clock";
> >>                      #clock-cells = <0>;
> >> @@ -243,6 +249,18 @@ mdio: mdio@90000 {
> >>                      status = "disabled";
> >>              };
> >>
> >> +            cmn_pll: clock-controller@9b000 {
> >> +                    compatible = "qcom,ipq9574-cmn-pll";
> >> +                    reg = <0x0009b000 0x800>;
> >> +                    clocks = <&cmn_pll_ref_clk>,
> >> +                             <&gcc GCC_CMN_12GPLL_AHB_CLK>,
> >> +                             <&gcc GCC_CMN_12GPLL_SYS_CLK>;
> >> +                    clock-names = "ref", "ahb", "sys";
> >> +                    #clock-cells = <1>;
> >> +                    assigned-clocks = <&cmn_pll CMN_PLL_CLK>;
> >> +                    assigned-clock-rates-u64 = /bits/ 64 <12000000000>;
> >> +            };
> >> +
> >>              qfprom: efuse@a4000 {
> >>                      compatible = "qcom,ipq9574-qfprom", "qcom,qfprom";
> >>                      reg = <0x000a4000 0x5a1>;
> >>
> >> --
> >> 2.34.1
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Jie Luo]

On 10/18/2024 4:11 PM, Dmitry Baryshkov wrote:
> On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@quicinc.com> wrote:
>>
>>
>>
>> On 10/18/2024 6:32 AM, Dmitry Baryshkov wrote:
>>> On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
>>>> The CMN PLL clock controller allows selection of an input
>>>> clock rate from a defined set of input clock rates. It in-turn
>>>> supplies fixed rate output clocks to the hardware blocks that
>>>> provide ethernet functions such as PPE (Packet Process Engine)
>>>> and connected switch or PHY, and to GCC.
>>>>
>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
>>>>    arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
>>>>    2 files changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>> index 91e104b0f865..77e1e42083f3 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>> @@ -3,7 +3,7 @@
>>>>     * IPQ9574 RDP board common device tree source
>>>>     *
>>>>     * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>     */
>>>>
>>>>    /dts-v1/;
>>>> @@ -164,6 +164,10 @@ &usb3 {
>>>>       status = "okay";
>>>>    };
>>>>
>>>> +&cmn_pll_ref_clk {
>>>> +    clock-frequency = <48000000>;
>>>> +};
>>>> +
>>>>    &xo_board_clk {
>>>>       clock-frequency = <24000000>;
>>>>    };
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>> index 14c7b3a78442..93f66bb83c5a 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>> @@ -3,10 +3,11 @@
>>>>     * IPQ9574 SoC device tree source
>>>>     *
>>>>     * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>     */
>>>>
>>>>    #include <dt-bindings/clock/qcom,apss-ipq.h>
>>>> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
>>>>    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
>>>>    #include <dt-bindings/interconnect/qcom,ipq9574.h>
>>>>    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> @@ -19,6 +20,11 @@ / {
>>>>       #size-cells = <2>;
>>>>
>>>>       clocks {
>>>> +            cmn_pll_ref_clk: cmn-pll-ref-clk {
>>>> +                    compatible = "fixed-clock";
>>>> +                    #clock-cells = <0>;
>>>> +            };
>>>
>>> Which block provides this clock? If it is provided by the external XO
>>> then it should not be a part of the SoC dtsi.
>>
>> The on-chip WiFi block supplies this reference clock. So keeping it in
>> the SoC DTSI is perhaps appropriate.
> 
> Then maybe it should be provided by the WiFi device node? At least you
> should document your design decisions in the commit message.

This CMN PLL reference clock is fixed rate and is automatically
generated by the SoC's internal Wi-Fi hardware block with no software
configuration required from the Wi-Fi side.

Sure, I will enhance the commit message to add the information on the
fixed reference clock from Wi-Fi block. Hope this is ok.

> 
> Also, I don't think this node passes DT schema validation. Did you check it?

Yes, the DT is validated against the schema, I have shared the logs
below. Could you please let me know If anything needs rectification?

dt-doc-validate --version
2024.9

make ARCH=arm64 DT_SCHEMA_FILES=qcom,ipq9574-cmn-pll.yaml CHECK_DTBS=y 
qcom/ipq9574-rdp433.dtb
   DTC [C] arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb

make ARCH=arm64 dt_binding_check 
DT_SCHEMA_FILES=qcom,ipq9574-cmn-pll.yaml        SCHEMA 
Documentation/devicetree/bindings/processed-schema.json
   CHKDT   Documentation/devicetree/bindings
   LINT    Documentation/devicetree/bindings
   DTEX 
Documentation/devicetree/bindings/clock/qcom,ipq9574-cmn-pll.example.dts
   DTC [C] 
Documentation/devicetree/bindings/clock/qcom,ipq9574-cmn-pll.example.dtb

make ARCH=arm64 CHECK_DTBS=y qcom/ipq9574-rdp433.dtb 
          DTC [C] arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb
/local/mnt2/workspace/luoj/projects/opensrc/linux-next-cmnpll-validation/linux-next/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb: 
usb@8af8800: interrupt-names: ['pwr_event'] is too short
         from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
/local/mnt2/workspace/luoj/projects/opensrc/linux-next-cmnpll-validation/linux-next/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb: 
usb@8af8800: interrupts-extended: [[1, 0, 134, 4]] is too short
         from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#

> 
>>
>>>
>>>> +
>>>>               sleep_clk: sleep-clk {
>>>>                       compatible = "fixed-clock";
>>>>                       #clock-cells = <0>;
>>>> @@ -243,6 +249,18 @@ mdio: mdio@90000 {
>>>>                       status = "disabled";
>>>>               };
>>>>
>>>> +            cmn_pll: clock-controller@9b000 {
>>>> +                    compatible = "qcom,ipq9574-cmn-pll";
>>>> +                    reg = <0x0009b000 0x800>;
>>>> +                    clocks = <&cmn_pll_ref_clk>,
>>>> +                             <&gcc GCC_CMN_12GPLL_AHB_CLK>,
>>>> +                             <&gcc GCC_CMN_12GPLL_SYS_CLK>;
>>>> +                    clock-names = "ref", "ahb", "sys";
>>>> +                    #clock-cells = <1>;
>>>> +                    assigned-clocks = <&cmn_pll CMN_PLL_CLK>;
>>>> +                    assigned-clock-rates-u64 = /bits/ 64 <12000000000>;
>>>> +            };
>>>> +
>>>>               qfprom: efuse@a4000 {
>>>>                       compatible = "qcom,ipq9574-qfprom", "qcom,qfprom";
>>>>                       reg = <0x000a4000 0x5a1>;
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>>
>>
> 
> 

Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Dmitry Baryshkov]

On Fri, Oct 18, 2024 at 10:03:08PM +0800, Jie Luo wrote:
> 
> 
> On 10/18/2024 4:11 PM, Dmitry Baryshkov wrote:
> > On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@quicinc.com> wrote:
> > > 
> > > 
> > > 
> > > On 10/18/2024 6:32 AM, Dmitry Baryshkov wrote:
> > > > On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
> > > > > The CMN PLL clock controller allows selection of an input
> > > > > clock rate from a defined set of input clock rates. It in-turn
> > > > > supplies fixed rate output clocks to the hardware blocks that
> > > > > provide ethernet functions such as PPE (Packet Process Engine)
> > > > > and connected switch or PHY, and to GCC.
> > > > > 
> > > > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > > > ---
> > > > >    arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
> > > > >    arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
> > > > >    2 files changed, 24 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> > > > > index 91e104b0f865..77e1e42083f3 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> > > > > @@ -3,7 +3,7 @@
> > > > >     * IPQ9574 RDP board common device tree source
> > > > >     *
> > > > >     * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> > > > > - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > > + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > >     */
> > > > > 
> > > > >    /dts-v1/;
> > > > > @@ -164,6 +164,10 @@ &usb3 {
> > > > >       status = "okay";
> > > > >    };
> > > > > 
> > > > > +&cmn_pll_ref_clk {
> > > > > +    clock-frequency = <48000000>;
> > > > > +};
> > > > > +
> > > > >    &xo_board_clk {
> > > > >       clock-frequency = <24000000>;
> > > > >    };
> > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > > > index 14c7b3a78442..93f66bb83c5a 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > > > @@ -3,10 +3,11 @@
> > > > >     * IPQ9574 SoC device tree source
> > > > >     *
> > > > >     * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> > > > > - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > > + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > >     */
> > > > > 
> > > > >    #include <dt-bindings/clock/qcom,apss-ipq.h>
> > > > > +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
> > > > >    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
> > > > >    #include <dt-bindings/interconnect/qcom,ipq9574.h>
> > > > >    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > > @@ -19,6 +20,11 @@ / {
> > > > >       #size-cells = <2>;
> > > > > 
> > > > >       clocks {
> > > > > +            cmn_pll_ref_clk: cmn-pll-ref-clk {
> > > > > +                    compatible = "fixed-clock";
> > > > > +                    #clock-cells = <0>;
> > > > > +            };
> > > > 
> > > > Which block provides this clock? If it is provided by the external XO
> > > > then it should not be a part of the SoC dtsi.
> > > 
> > > The on-chip WiFi block supplies this reference clock. So keeping it in
> > > the SoC DTSI is perhaps appropriate.
> > 
> > Then maybe it should be provided by the WiFi device node? At least you
> > should document your design decisions in the commit message.
> 
> This CMN PLL reference clock is fixed rate and is automatically
> generated by the SoC's internal Wi-Fi hardware block with no software
> configuration required from the Wi-Fi side.
> 
> Sure, I will enhance the commit message to add the information on the
> fixed reference clock from Wi-Fi block. Hope this is ok.

We have other fixed clocks which are provided by hardware blocks.
Without additional details it is impossible to answer whether it is fine
or not.

> 
> > 
> > Also, I don't think this node passes DT schema validation. Did you check it?
> 
> Yes, the DT is validated against the schema, I have shared the logs
> below. Could you please let me know If anything needs rectification?

I see, you are setting the cmn_pll_ref_clk frequency in the
ipq9574-rdp-common.dtsi file. If the PLL is internal to the SoC, why is
the frequency set outside of it? Is it generated by multiplying the XO
clk? Should you be using fixed-factor clock instead?

-- 
With best wishes
Dmitry

Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Jie Luo]

On 10/18/2024 11:38 PM, Dmitry Baryshkov wrote:
> On Fri, Oct 18, 2024 at 10:03:08PM +0800, Jie Luo wrote:
>>
>>
>> On 10/18/2024 4:11 PM, Dmitry Baryshkov wrote:
>>> On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/18/2024 6:32 AM, Dmitry Baryshkov wrote:
>>>>> On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
>>>>>> The CMN PLL clock controller allows selection of an input
>>>>>> clock rate from a defined set of input clock rates. It in-turn
>>>>>> supplies fixed rate output clocks to the hardware blocks that
>>>>>> provide ethernet functions such as PPE (Packet Process Engine)
>>>>>> and connected switch or PHY, and to GCC.
>>>>>>
>>>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>>>> ---
>>>>>>     arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
>>>>>>     arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
>>>>>>     2 files changed, 24 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>>>> index 91e104b0f865..77e1e42083f3 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>>>> @@ -3,7 +3,7 @@
>>>>>>      * IPQ9574 RDP board common device tree source
>>>>>>      *
>>>>>>      * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>>>>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>>      */
>>>>>>
>>>>>>     /dts-v1/;
>>>>>> @@ -164,6 +164,10 @@ &usb3 {
>>>>>>        status = "okay";
>>>>>>     };
>>>>>>
>>>>>> +&cmn_pll_ref_clk {
>>>>>> +    clock-frequency = <48000000>;
>>>>>> +};
>>>>>> +
>>>>>>     &xo_board_clk {
>>>>>>        clock-frequency = <24000000>;
>>>>>>     };
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>>> index 14c7b3a78442..93f66bb83c5a 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>>> @@ -3,10 +3,11 @@
>>>>>>      * IPQ9574 SoC device tree source
>>>>>>      *
>>>>>>      * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>>>>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>>      */
>>>>>>
>>>>>>     #include <dt-bindings/clock/qcom,apss-ipq.h>
>>>>>> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
>>>>>>     #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
>>>>>>     #include <dt-bindings/interconnect/qcom,ipq9574.h>
>>>>>>     #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>>> @@ -19,6 +20,11 @@ / {
>>>>>>        #size-cells = <2>;
>>>>>>
>>>>>>        clocks {
>>>>>> +            cmn_pll_ref_clk: cmn-pll-ref-clk {
>>>>>> +                    compatible = "fixed-clock";
>>>>>> +                    #clock-cells = <0>;
>>>>>> +            };
>>>>>
>>>>> Which block provides this clock? If it is provided by the external XO
>>>>> then it should not be a part of the SoC dtsi.
>>>>
>>>> The on-chip WiFi block supplies this reference clock. So keeping it in
>>>> the SoC DTSI is perhaps appropriate.
>>>
>>> Then maybe it should be provided by the WiFi device node? At least you
>>> should document your design decisions in the commit message.
>>
>> This CMN PLL reference clock is fixed rate and is automatically
>> generated by the SoC's internal Wi-Fi hardware block with no software
>> configuration required from the Wi-Fi side.
>>
>> Sure, I will enhance the commit message to add the information on the
>> fixed reference clock from Wi-Fi block. Hope this is ok.
> 
> We have other fixed clocks which are provided by hardware blocks.
> Without additional details it is impossible to answer whether it is fine
> or not.

There is an XO on the board which supplies reference clock (48Mhz or
96Mhz) to the Wi-Fi block on the SoC. There is a multiplier/divider in
the Wi-Fi block, which ensures the output reference clock of 48Mhz is
supplied to CMN PLL block.

In summary, below is the path to receive the reference clock at CMN PLL:
The clock path is .XO (48 MHZ/96 MHZ) --> WiFi (multiplier/divider) 
-->(48 MHZ) --> CMN PLL.

There is no software configuration required for the entire path, as it
is fully controlled by bootstrap pins on the board. There are bootstrap
pins for selecting the selecting the XO frequency (48Mhz or 96Mhz) and
based on this, the divider is automatically selected by HW (1 for 48Mhz,
2 for 96Mhz), to ensure output clock to CMN PLL is 48Mhz.

> 
>>
>>>
>>> Also, I don't think this node passes DT schema validation. Did you check it?
>>
>> Yes, the DT is validated against the schema, I have shared the logs
>> below. Could you please let me know If anything needs rectification?
> 
> I see, you are setting the cmn_pll_ref_clk frequency in the
> ipq9574-rdp-common.dtsi file. If the PLL is internal to the SoC, why is
> the frequency set outside of it? Is it generated by multiplying the XO
> clk? Should you be using fixed-factor clock instead?
> 

Since the reference clock is controlled by bootstrap pins on the board,
it may be appropriate to define the frequency for this reference clock
in the board DTS. Given the clock tree described above, I will update
the cmn_pll_ref_clk to define it as a fixed-factor clock as per your
suggestion, with its frequency and factors configured in board DTSI.
These values defined in rdp-common.dtsi will be default values that can
be overridden if necessary by different boards. Hope this approach is
fine.

In ipq9574.dtsi:
cmn_pll_ref_clk: cmn-pll-ref-clk { 
  
  

         compatible = "fixed-factor-clock"; 
  
  

         clocks = <&xo_clk>; 
  
  
  
  
  

	#clock-cells = <0>;
};

xo_clk: xo {
	compatible = "fixed-clock";
	#clock-cells = <0>;
};

In ipq9574-rdp-common.dtsi.
&cmn_pll_ref_clk {
	clock-div = <1>;
	clock-mult = <1>;
};

&xo_clk {
	clock-frequency = <48000000>;
}

Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC

[Dmitry Baryshkov]

On Wed, Oct 23, 2024 at 09:05:09PM +0800, Jie Luo wrote:
> 
> 
> On 10/18/2024 11:38 PM, Dmitry Baryshkov wrote:
> > On Fri, Oct 18, 2024 at 10:03:08PM +0800, Jie Luo wrote:
> > > 
> > > 
> > > On 10/18/2024 4:11 PM, Dmitry Baryshkov wrote:
> > > > On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@quicinc.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 10/18/2024 6:32 AM, Dmitry Baryshkov wrote:
> > > > > > On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
> > > > > > > The CMN PLL clock controller allows selection of an input
> > > > > > > clock rate from a defined set of input clock rates. It in-turn
> > > > > > > supplies fixed rate output clocks to the hardware blocks that
> > > > > > > provide ethernet functions such as PPE (Packet Process Engine)
> > > > > > > and connected switch or PHY, and to GCC.
> > > > > > > 
> > > > > > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > > > > > ---
> > > > > > >     arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
> > > > > > >     arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
> > > > > > >     2 files changed, 24 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> > > > > > > index 91e104b0f865..77e1e42083f3 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
> > > > > > > @@ -3,7 +3,7 @@
> > > > > > >      * IPQ9574 RDP board common device tree source
> > > > > > >      *
> > > > > > >      * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> > > > > > > - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > > > > + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > > > >      */
> > > > > > > 
> > > > > > >     /dts-v1/;
> > > > > > > @@ -164,6 +164,10 @@ &usb3 {
> > > > > > >        status = "okay";
> > > > > > >     };
> > > > > > > 
> > > > > > > +&cmn_pll_ref_clk {
> > > > > > > +    clock-frequency = <48000000>;
> > > > > > > +};
> > > > > > > +
> > > > > > >     &xo_board_clk {
> > > > > > >        clock-frequency = <24000000>;
> > > > > > >     };
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > > > > > index 14c7b3a78442..93f66bb83c5a 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > > > > > @@ -3,10 +3,11 @@
> > > > > > >      * IPQ9574 SoC device tree source
> > > > > > >      *
> > > > > > >      * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> > > > > > > - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > > > > + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> > > > > > >      */
> > > > > > > 
> > > > > > >     #include <dt-bindings/clock/qcom,apss-ipq.h>
> > > > > > > +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
> > > > > > >     #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
> > > > > > >     #include <dt-bindings/interconnect/qcom,ipq9574.h>
> > > > > > >     #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > > > > @@ -19,6 +20,11 @@ / {
> > > > > > >        #size-cells = <2>;
> > > > > > > 
> > > > > > >        clocks {
> > > > > > > +            cmn_pll_ref_clk: cmn-pll-ref-clk {
> > > > > > > +                    compatible = "fixed-clock";
> > > > > > > +                    #clock-cells = <0>;
> > > > > > > +            };
> > > > > > 
> > > > > > Which block provides this clock? If it is provided by the external XO
> > > > > > then it should not be a part of the SoC dtsi.
> > > > > 
> > > > > The on-chip WiFi block supplies this reference clock. So keeping it in
> > > > > the SoC DTSI is perhaps appropriate.
> > > > 
> > > > Then maybe it should be provided by the WiFi device node? At least you
> > > > should document your design decisions in the commit message.
> > > 
> > > This CMN PLL reference clock is fixed rate and is automatically
> > > generated by the SoC's internal Wi-Fi hardware block with no software
> > > configuration required from the Wi-Fi side.
> > > 
> > > Sure, I will enhance the commit message to add the information on the
> > > fixed reference clock from Wi-Fi block. Hope this is ok.
> > 
> > We have other fixed clocks which are provided by hardware blocks.
> > Without additional details it is impossible to answer whether it is fine
> > or not.
> 
> There is an XO on the board which supplies reference clock (48Mhz or
> 96Mhz) to the Wi-Fi block on the SoC. There is a multiplier/divider in
> the Wi-Fi block, which ensures the output reference clock of 48Mhz is
> supplied to CMN PLL block.
> 
> In summary, below is the path to receive the reference clock at CMN PLL:
> The clock path is .XO (48 MHZ/96 MHZ) --> WiFi (multiplier/divider) -->(48
> MHZ) --> CMN PLL.
> 
> There is no software configuration required for the entire path, as it
> is fully controlled by bootstrap pins on the board. There are bootstrap
> pins for selecting the selecting the XO frequency (48Mhz or 96Mhz) and
> based on this, the divider is automatically selected by HW (1 for 48Mhz,
> 2 for 96Mhz), to ensure output clock to CMN PLL is 48Mhz.

If the clock is always fixed to this frequency, then it's ok, thank you.

> 
> > 
> > > 
> > > > 
> > > > Also, I don't think this node passes DT schema validation. Did you check it?
> > > 
> > > Yes, the DT is validated against the schema, I have shared the logs
> > > below. Could you please let me know If anything needs rectification?
> > 
> > I see, you are setting the cmn_pll_ref_clk frequency in the
> > ipq9574-rdp-common.dtsi file. If the PLL is internal to the SoC, why is
> > the frequency set outside of it? Is it generated by multiplying the XO
> > clk? Should you be using fixed-factor clock instead?
> > 
> 
> Since the reference clock is controlled by bootstrap pins on the board,
> it may be appropriate to define the frequency for this reference clock
> in the board DTS. Given the clock tree described above, I will update
> the cmn_pll_ref_clk to define it as a fixed-factor clock as per your
> suggestion, with its frequency and factors configured in board DTSI.
> These values defined in rdp-common.dtsi will be default values that can
> be overridden if necessary by different boards. Hope this approach is
> fine.
> 
> In ipq9574.dtsi:
> cmn_pll_ref_clk: cmn-pll-ref-clk {
> 
>         compatible = "fixed-factor-clock";
> 
>         clocks = <&xo_clk>;
> 
> 	#clock-cells = <0>;
> };
> 
> xo_clk: xo {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> };
> 
> In ipq9574-rdp-common.dtsi.
> &cmn_pll_ref_clk {
> 	clock-div = <1>;
> 	clock-mult = <1>;
> };
> 
> &xo_clk {
> 	clock-frequency = <48000000>;
> }

Sounds perfect, thank you!


-- 
With best wishes
Dmitry