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

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

[Luo Jie]

The common PLL clock controller in Qualcomm IPQ chipsets provides
the clocks to the networking hardware blocks that are internal or
external to the SoC. This driver configures the common PLL clock
controller to enable the output clocks to such networking hardware
blocks. These networking blocks include the internal PPE (Packet
Process Engine), external connected Ethernet PHY, or external switch.
 
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 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>
---
Luo Jie (4):
      dt-bindings: clock: qcom: Add common PLL clock controller for IPQ SoC
      clk: qcom: Add common PLL clock controller driver for IPQ SoC
      arm64: defconfig: Enable Qualcomm IPQ common PLL clock controller
      arm64: dts: qcom: Add common PLL node for IPQ9574 SoC

 .../bindings/clock/qcom,ipq-cmn-pll.yaml           |  87 ++++++++
 arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi   |   6 +-
 arch/arm64/boot/dts/qcom/ipq9574.dtsi              |  22 +-
 arch/arm64/configs/defconfig                       |   1 +
 drivers/clk/qcom/Kconfig                           |  10 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/clk-ipq-cmn-pll.c                 | 233 +++++++++++++++++++++
 7 files changed, 358 insertions(+), 2 deletions(-)
---
base-commit: 222a3380f92b8791d4eeedf7cd750513ff428adf
change-id: 20240808-qcom_ipq_cmnpll-7c1119b25037

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

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

[Luo Jie]

The common PLL controller provides clocks to networking hardware
blocks on Qualcomm IPQ 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
output clocks are supplied to the Ethernet hardware such as PPE
(packet process engine) and the externally connected switch or PHY
device.

The common PLL driver is initially being supported for IPQ9574 SoC.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 .../bindings/clock/qcom,ipq-cmn-pll.yaml           | 87 ++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml
new file mode 100644
index 000000000000..c45b3a201751
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,ipq-cmn-pll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Common PLL Clock Controller on IPQ SoC
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+  - Luo Jie <quic_luoj@quicinc.com>
+
+description:
+  The 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 Ethernet
+  devices including PPE (packet process engine) and the connected
+  switch or PHY device.
+
+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 common PLL
+      clock registers.
+
+  clock-names:
+    items:
+      - const: ref
+      - const: ahb
+      - const: sys
+
+  clock-output-names:
+    items:
+      - const: ppe-353mhz
+      - const: eth0-50mhz
+      - const: eth1-50mhz
+      - const: eth2-50mhz
+      - const: eth-25mhz
+    description:
+      The output clocks are given to Ethernet blocks that includes PPE and
+      the connected switch or PHY device.
+
+  "#clock-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-output-names
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
+
+    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-output-names = "ppe-353mhz",
+                             "eth0-50mhz",
+                             "eth1-50mhz",
+                             "eth2-50mhz",
+                             "eth-25mhz";
+        #clock-cells = <1>;
+    };
+...

-- 
2.34.1

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

[Luo Jie]

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

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

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

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index cf6ad908327f..8e36cde64134 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 Common PLL Clock Controller"
+	depends on IPQ_GCC_9574
+	help
+	  Support for common PLL clock controller on IPQ platform. The
+	  common PLL feeds the reference clocks to the Ethernet devices
+	  based on IPQ SoC.
+	  Say Y or M if you want to support common 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 8a6f0dabd02f..35f656146de7 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) += clk-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/clk-ipq-cmn-pll.c b/drivers/clk/qcom/clk-ipq-cmn-pll.c
new file mode 100644
index 000000000000..c10c6a7e82e6
--- /dev/null
+++ b/drivers/clk/qcom/clk-ipq-cmn-pll.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+/*
+ * Common PLL block expects the reference clock from on-board Wi-Fi block,
+ * and supplies the fixed rate clocks as output to the Ethernet hardware
+ * blocks. The Ethernet related blocks include PPE (packet process engine)
+ * and the external connected PHY (or switch) chip receiving clocks from
+ * the common PLL.
+ *
+ * On the IPQ9574 SoC, There are three clocks with 50 MHZ, one clock with
+ * 25 MHZ which are output from the common PLL to Ethernet PHY (or switch),
+ * and one clock with 353 MHZ to PPE.
+ *
+ *               +---------+
+ *               |   GCC   |
+ *               +--+---+--+
+ *           AHB CLK|   |SYS CLK
+ *                  V   V
+ *          +-------+---+------+
+ *          |                  +-------------> eth0-50mhz
+ * REF CLK  |     IPQ9574      |
+ * -------->+                  +-------------> eth1-50mhz
+ *          |  CMN PLL block   |
+ *          |                  +-------------> eth2-50mhz
+ *          |                  |
+ *          +---------+--------+-------------> eth-25mhz
+ *                    |
+ *                    V
+ *                    ppe-353mhz
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define CMN_PLL_REFCLK_SRC_SELECTION		0x28
+#define CMN_PLL_REFCLK_SRC_DIV			GENMASK(9, 8)
+
+#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_POWER_ON_AND_RESET		0x780
+#define CMN_ANA_EN_SW_RSTN			BIT(6)
+
+/**
+ * struct cmn_pll_fixed_clk - Common PLL output clocks information
+ * @nrates:	Number of elements in rates
+ * @rates:	Array of clock rates supplied by common PLL
+ */
+struct cmn_pll_fixed_clk {
+	int nrates;
+	const unsigned long *rates;
+};
+
+/*
+ * The clock rates are for the output clock ppe-353mhz, eth0-50mhz
+ * eth1-50mhz, eth2-50mhz and eth-25mhz.
+ */
+static const unsigned long ipq9574_rates[] = {
+	353000000UL, 50000000UL, 50000000UL, 50000000UL, 25000000UL,
+};
+
+static const struct cmn_pll_fixed_clk ipq9574_fixed_clk = {
+	.nrates = ARRAY_SIZE(ipq9574_rates),
+	.rates = ipq9574_rates,
+};
+
+static int ipq_cmn_pll_config(struct device *dev, unsigned long parent_rate)
+{
+	void __iomem *base;
+	u32 val;
+
+	base = devm_of_iomap(dev, dev->of_node, 0, NULL);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	val = readl(base + CMN_PLL_REFCLK_CONFIG);
+	val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX);
+
+	/*
+	 * Configure the reference input clock selection as per the given rate.
+	 * The output clock rates are always of fixed value.
+	 */
+	switch (parent_rate) {
+	case 25000000:
+		val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3);
+		break;
+	case 31250000:
+		val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4);
+		break;
+	case 40000000:
+		val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6);
+		break;
+	case 48000000:
+		val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+		break;
+	case 50000000:
+		val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8);
+		break;
+	case 96000000:
+		val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+		val &= ~CMN_PLL_REFCLK_DIV;
+		val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	writel(val, base + CMN_PLL_REFCLK_CONFIG);
+
+	/* Update the source clock rate selection. Only 96 MHZ uses 0. */
+	val = readl(base + CMN_PLL_REFCLK_SRC_SELECTION);
+	val &= ~CMN_PLL_REFCLK_SRC_DIV;
+	if (parent_rate != 96000000)
+		val |= FIELD_PREP(CMN_PLL_REFCLK_SRC_DIV, 1);
+
+	writel(val, base + CMN_PLL_REFCLK_SRC_SELECTION);
+
+	/*
+	 * Reset the common PLL block by asserting/de-asserting for 100 ms
+	 * each, to ensure the updated configurations take effect.
+	 */
+	val = readl(base + CMN_PLL_POWER_ON_AND_RESET);
+	val &= ~CMN_ANA_EN_SW_RSTN;
+	writel(val, base);
+	msleep(100);
+
+	val |= CMN_ANA_EN_SW_RSTN;
+	writel(val, base + CMN_PLL_POWER_ON_AND_RESET);
+	msleep(100);
+
+	return 0;
+}
+
+static int ipq_cmn_pll_clk_register(struct device *dev, const char *parent)
+{
+	const struct cmn_pll_fixed_clk *fixed_clk;
+	struct clk_hw_onecell_data *data;
+	const char *clk_name;
+	struct clk_hw *hw;
+	int index;
+
+	fixed_clk = of_device_get_match_data(dev);
+	if (!fixed_clk)
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, struct_size(data, hws, fixed_clk->nrates),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/*
+	 * Register the fixed rate output clocks with the correct clock names,
+	 * the number of clocks and clock names are guaranteed by DTS.
+	 */
+	for (index = 0; index < fixed_clk->nrates; index++) {
+		if (of_property_read_string_index(dev->of_node,
+						  "clock-output-names",
+						  index, &clk_name))
+			return -ENODEV;
+
+		hw = devm_clk_hw_register_fixed_rate(dev, clk_name, parent, 0,
+						     fixed_clk->rates[index]);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		data->hws[index] = hw;
+	}
+	data->num = fixed_clk->nrates;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, data);
+}
+
+static int ipq_cmn_pll_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * To access the common PLL registers, the GCC AHB & SYSY clocks
+	 * for common PLL block need to be enabled.
+	 */
+	clk = devm_clk_get_enabled(dev, "ahb");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Enable AHB clock failed\n");
+
+	clk = devm_clk_get_enabled(dev, "sys");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Enable SYS clock failed\n");
+
+	clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Get reference clock failed\n");
+
+	/* Configure common PLL to apply the reference clock. */
+	ret = ipq_cmn_pll_config(dev, clk_get_rate(clk));
+	if (ret)
+		return dev_err_probe(dev, ret, "Configure common PLL failed\n");
+
+	return ipq_cmn_pll_clk_register(dev, __clk_get_name(clk));
+}
+
+static const struct of_device_id ipq_cmn_pll_clk_ids[] = {
+	{ .compatible = "qcom,ipq9574-cmn-pll", .data = &ipq9574_fixed_clk },
+	{ }
+};
+
+static struct platform_driver ipq_cmn_pll_clk_driver = {
+	.probe = ipq_cmn_pll_clk_probe,
+	.driver = {
+		.name = "ipq_cmn_pll",
+		.of_match_table = ipq_cmn_pll_clk_ids,
+	},
+};
+
+module_platform_driver(ipq_cmn_pll_clk_driver);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. IPQ Common PLL Driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

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

[Luo Jie]

The common PLL clock controller provides fixed rate output clocks to
the hardware blocks that enable ethernet function on IPQ platform.

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 01dd286ba7ef..1bc7bd86e589 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1300,6 +1300,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 4/4] arm64: dts: qcom: Add common PLL node for IPQ9574 SoC

[Luo Jie]

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

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            | 22 +++++++++++++++++++++-
 2 files changed, 26 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..26bc2de7f99a 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/;
@@ -167,3 +167,7 @@ &usb3 {
 &xo_board_clk {
 	clock-frequency = <24000000>;
 };
+
+&cmn_pll_ref_clk {
+	clock-frequency = <48000000>;
+};
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 48dfafea46a7..ad7789dc686e 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -3,7 +3,7 @@
  * 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>
@@ -28,6 +28,11 @@ xo_board_clk: xo-board-clk {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
 		};
+
+		cmn_pll_ref_clk: cmn-pll-ref-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+	       };
 	};
 
 	cpus {
@@ -226,6 +231,21 @@ rpm_msg_ram: sram@60000 {
 			reg = <0x00060000 0x6000>;
 		};
 
+		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-output-names = "ppe-353mhz",
+					     "eth0-50mhz",
+					     "eth1-50mhz",
+					     "eth2-50mhz",
+					     "eth-25mhz";
+			#clock-cells = <1>;
+		};
+
 		rng: rng@e3000 {
 			compatible = "qcom,prng-ee";
 			reg = <0x000e3000 0x1000>;

-- 
2.34.1

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

[Krzysztof Kozlowski]

On 08/08/2024 16:03, Luo Jie wrote:
> The common PLL controller provides clocks to networking hardware
> blocks on Qualcomm IPQ 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
> output clocks are supplied to the Ethernet hardware such as PPE
> (packet process engine) and the externally connected switch or PHY
> device.
> 
> The common PLL driver is initially being supported for IPQ9574 SoC.

Drop references to driver and explain the hardware.

Above with the usage of "common" looks like this is all for some common
driver, not for particular hardware.

> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  .../bindings/clock/qcom,ipq-cmn-pll.yaml           | 87 ++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml
> new file mode 100644
> index 000000000000..c45b3a201751
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml

Use compatible as filename.

> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/qcom,ipq-cmn-pll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Common PLL Clock Controller on IPQ SoC
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Luo Jie <quic_luoj@quicinc.com>
> +
> +description:
> +  The 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 Ethernet
> +  devices including PPE (packet process engine) and the connected
> +  switch or PHY device.
> +
> +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 common PLL
> +      clock registers.
> +
> +  clock-names:
> +    items:
> +      - const: ref
> +      - const: ahb
> +      - const: sys
> +
> +  clock-output-names:
> +    items:
> +      - const: ppe-353mhz
> +      - const: eth0-50mhz
> +      - const: eth1-50mhz
> +      - const: eth2-50mhz
> +      - const: eth-25mhz

Drop entire property. If the names are fixed, what's the point of having
it in DTS? There is no.

Best regards,
Krzysztof

Re: [PATCH 3/4] arm64: defconfig: Enable Qualcomm IPQ common PLL clock controller

[Krzysztof Kozlowski]

On 08/08/2024 16:03, Luo Jie wrote:
> The common PLL clock controller provides fixed rate output clocks to
> the hardware blocks that enable ethernet function on IPQ platform.

That's defconfig for all platforms, so how anyone can guess which one
you target here? Be specific, which company, which Soc, which board
needs it.



Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 08/08/2024 16:03, Luo Jie wrote:

>  
>  /dts-v1/;
> @@ -167,3 +167,7 @@ &usb3 {
>  &xo_board_clk {
>  	clock-frequency = <24000000>;
>  };
> +
> +&cmn_pll_ref_clk {

Please follow DTS coding style.

> +	clock-frequency = <48000000>;
> +};



Best regards,
Krzysztof

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

[Jie Luo]

On 8/8/2024 10:45 PM, Krzysztof Kozlowski wrote:
> On 08/08/2024 16:03, Luo Jie wrote:
> 
>>   
>>   /dts-v1/;
>> @@ -167,3 +167,7 @@ &usb3 {
>>   &xo_board_clk {
>>   	clock-frequency = <24000000>;
>>   };
>> +
>> +&cmn_pll_ref_clk {
> 
> Please follow DTS coding style.

Ok. Thanks for pointing to this. I will move &cmn_pll_ref_clk before
&xo_board_clk to make the DTS ordering alpha-numerical by the node name.

> 
>> +	clock-frequency = <48000000>;
>> +};
> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 3/4] arm64: defconfig: Enable Qualcomm IPQ common PLL clock controller

[Jie Luo]

On 8/8/2024 10:41 PM, Krzysztof Kozlowski wrote:
> On 08/08/2024 16:03, Luo Jie wrote:
>> The common PLL clock controller provides fixed rate output clocks to
>> the hardware blocks that enable ethernet function on IPQ platform.
> 
> That's defconfig for all platforms, so how anyone can guess which one
> you target here? Be specific, which company, which Soc, which board
> needs it.
> 

Sure, I will update the commit message as below to provide the details
required.

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

> 
> 
> Best regards,
> Krzysztof
> 

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

[Jie Luo]

On 8/8/2024 10:38 PM, Krzysztof Kozlowski wrote:
> On 08/08/2024 16:03, Luo Jie wrote:
>> The common PLL controller provides clocks to networking hardware
>> blocks on Qualcomm IPQ 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
>> output clocks are supplied to the Ethernet hardware such as PPE
>> (packet process engine) and the externally connected switch or PHY
>> device.
>>
>> The common PLL driver is initially being supported for IPQ9574 SoC.
> 
> Drop references to driver and explain the hardware.
> 
> Above with the usage of "common" looks like this is all for some common
> driver, not for particular hardware.

Understand, will remove this driver reference.

> 
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   .../bindings/clock/qcom,ipq-cmn-pll.yaml           | 87 ++++++++++++++++++++++
>>   1 file changed, 87 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml
>> new file mode 100644
>> index 000000000000..c45b3a201751
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml
> 
> Use compatible as filename.

OK.

> 
>> @@ -0,0 +1,87 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/qcom,ipq-cmn-pll.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Common PLL Clock Controller on IPQ SoC
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +  - Luo Jie <quic_luoj@quicinc.com>
>> +
>> +description:
>> +  The 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 Ethernet
>> +  devices including PPE (packet process engine) and the connected
>> +  switch or PHY device.
>> +
>> +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 common PLL
>> +      clock registers.
>> +
>> +  clock-names:
>> +    items:
>> +      - const: ref
>> +      - const: ahb
>> +      - const: sys
>> +
>> +  clock-output-names:
>> +    items:
>> +      - const: ppe-353mhz
>> +      - const: eth0-50mhz
>> +      - const: eth1-50mhz
>> +      - const: eth2-50mhz
>> +      - const: eth-25mhz
> 
> Drop entire property. If the names are fixed, what's the point of having
> it in DTS? There is no.

We had added the output names here for the reasons below. Can you please
let us know your suggestion whether keeping these here is fine?

1.) These output clocks are used as input reference clocks to other
consumer blocks. For example, an on-board Ethernet PHY device may be
wired to receive a specific clock from the above output clocks as
reference clock input, and hence the PHY's DTS node would need to
reference a particular index in this output clock array.

Without these output clocks being made available in this DTS, the PHY
driver in above case would not know the clock specifier to access the
handle for the desired input clock.

2.) One of the suggestions from the internal code review with Linaro was
to name the output clocks specifically based on rate and destination
(Ex: 'ppe-353mhz' for fixed rate 353 MHZ output clock connected to
Packet Process Engine block), so that the dt-bindings describe the
input/output clocks clearly.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 3/4] arm64: defconfig: Enable Qualcomm IPQ common PLL clock controller

[Andrew Lunn]

On Fri, Aug 09, 2024 at 07:36:35PM +0800, Jie Luo wrote:
> 
> 
> On 8/8/2024 10:41 PM, Krzysztof Kozlowski wrote:
> > On 08/08/2024 16:03, Luo Jie wrote:
> > > The common PLL clock controller provides fixed rate output clocks to
> > > the hardware blocks that enable ethernet function on IPQ platform.
> > 
> > That's defconfig for all platforms, so how anyone can guess which one
> > you target here? Be specific, which company, which Soc, which board
> > needs it.
> > 
> 
> Sure, I will update the commit message as below to provide the details
> required.
> 
> The common 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.

Does it provide more than Ethernet clocks? I'm just wondering why the
name `common`, when it seems pretty uncommon, specialised for Ethernet
clocks on a couple of SoCs.

   Andrew

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

[Krzysztof Kozlowski]

On 09/08/2024 15:01, Jie Luo wrote:
>>> +  clock-names:
>>> +    items:
>>> +      - const: ref
>>> +      - const: ahb
>>> +      - const: sys
>>> +
>>> +  clock-output-names:
>>> +    items:
>>> +      - const: ppe-353mhz
>>> +      - const: eth0-50mhz
>>> +      - const: eth1-50mhz
>>> +      - const: eth2-50mhz
>>> +      - const: eth-25mhz
>>
>> Drop entire property. If the names are fixed, what's the point of having
>> it in DTS? There is no.
> 
> We had added the output names here for the reasons below. Can you please
> let us know your suggestion whether keeping these here is fine?
> 
> 1.) These output clocks are used as input reference clocks to other
> consumer blocks. For example, an on-board Ethernet PHY device may be
> wired to receive a specific clock from the above output clocks as
> reference clock input, and hence the PHY's DTS node would need to
> reference a particular index in this output clock array.
> 
> Without these output clocks being made available in this DTS, the PHY
> driver in above case would not know the clock specifier to access the
> handle for the desired input clock.

That's not true. clock-output-names do not have anything to do with
clock specifier.

> 
> 2.) One of the suggestions from the internal code review with Linaro was
> to name the output clocks specifically based on rate and destination
> (Ex: 'ppe-353mhz' for fixed rate 353 MHZ output clock connected to
> Packet Process Engine block), so that the dt-bindings describe the
> input/output clocks clearly.

Again, that's unrelated. None of above points address my concern. It's
like you talk about some entirely different topic. Again:
clock-output-names have nothing to do with what you want to achieve here.

Best regards,
Krzysztof

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

[kernel test robot]

Hi Luo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 222a3380f92b8791d4eeedf7cd750513ff428adf]

url:    https://github.com/intel-lab-lkp/linux/commits/Luo-Jie/dt-bindings-clock-qcom-Add-common-PLL-clock-controller-for-IPQ-SoC/20240808-221059
base:   222a3380f92b8791d4eeedf7cd750513ff428adf
patch link:    https://lore.kernel.org/r/20240808-qcom_ipq_cmnpll-v1-2-b0631dcbf785%40quicinc.com
patch subject: [PATCH 2/4] clk: qcom: Add common PLL clock controller driver for IPQ SoC
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240811/202408110756.rSXn1ZRu-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240811/202408110756.rSXn1ZRu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408110756.rSXn1ZRu-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/clk/qcom/clk-ipq-cmn-pll.c: In function 'ipq_cmn_pll_config':
>> drivers/clk/qcom/clk-ipq-cmn-pll.c:96:24: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
      96 |                 val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3);
         |                        ^~~~~~~~~~


vim +/FIELD_PREP +96 drivers/clk/qcom/clk-ipq-cmn-pll.c

    77	
    78	static int ipq_cmn_pll_config(struct device *dev, unsigned long parent_rate)
    79	{
    80		void __iomem *base;
    81		u32 val;
    82	
    83		base = devm_of_iomap(dev, dev->of_node, 0, NULL);
    84		if (IS_ERR(base))
    85			return PTR_ERR(base);
    86	
    87		val = readl(base + CMN_PLL_REFCLK_CONFIG);
    88		val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX);
    89	
    90		/*
    91		 * Configure the reference input clock selection as per the given rate.
    92		 * The output clock rates are always of fixed value.
    93		 */
    94		switch (parent_rate) {
    95		case 25000000:
  > 96			val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3);
    97			break;
    98		case 31250000:
    99			val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4);
   100			break;
   101		case 40000000:
   102			val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6);
   103			break;
   104		case 48000000:
   105			val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
   106			break;
   107		case 50000000:
   108			val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8);
   109			break;
   110		case 96000000:
   111			val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
   112			val &= ~CMN_PLL_REFCLK_DIV;
   113			val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2);
   114			break;
   115		default:
   116			return -EINVAL;
   117		}
   118	
   119		writel(val, base + CMN_PLL_REFCLK_CONFIG);
   120	
   121		/* Update the source clock rate selection. Only 96 MHZ uses 0. */
   122		val = readl(base + CMN_PLL_REFCLK_SRC_SELECTION);
   123		val &= ~CMN_PLL_REFCLK_SRC_DIV;
   124		if (parent_rate != 96000000)
   125			val |= FIELD_PREP(CMN_PLL_REFCLK_SRC_DIV, 1);
   126	
   127		writel(val, base + CMN_PLL_REFCLK_SRC_SELECTION);
   128	
   129		/*
   130		 * Reset the common PLL block by asserting/de-asserting for 100 ms
   131		 * each, to ensure the updated configurations take effect.
   132		 */
   133		val = readl(base + CMN_PLL_POWER_ON_AND_RESET);
   134		val &= ~CMN_ANA_EN_SW_RSTN;
   135		writel(val, base);
   136		msleep(100);
   137	
   138		val |= CMN_ANA_EN_SW_RSTN;
   139		writel(val, base + CMN_PLL_POWER_ON_AND_RESET);
   140		msleep(100);
   141	
   142		return 0;
   143	}
   144	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/4] arm64: defconfig: Enable Qualcomm IPQ common PLL clock controller

[Jie Luo]

On 8/9/2024 9:34 PM, Andrew Lunn wrote:
> On Fri, Aug 09, 2024 at 07:36:35PM +0800, Jie Luo wrote:
>>
>>
>> On 8/8/2024 10:41 PM, Krzysztof Kozlowski wrote:
>>> On 08/08/2024 16:03, Luo Jie wrote:
>>>> The common PLL clock controller provides fixed rate output clocks to
>>>> the hardware blocks that enable ethernet function on IPQ platform.
>>>
>>> That's defconfig for all platforms, so how anyone can guess which one
>>> you target here? Be specific, which company, which Soc, which board
>>> needs it.
>>>
>>
>> Sure, I will update the commit message as below to provide the details
>> required.
>>
>> The common 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.
> 
> Does it provide more than Ethernet clocks? I'm just wondering why the
> name `common`, when it seems pretty uncommon, specialised for Ethernet
> clocks on a couple of SoCs.
> 
>     Andrew

No, this block does not provide any other functionality other than
allowing this PLL to be configured for supplying clocks to Ethernet
devices. The hardware programming guide names this block as the 'CMN'
block, so we included the 'cmn' phrase in the driver namespace. However,
I will update commit message to clarify that 'cmn' is the block name and
it does not provide any other function other than enabling clocks to
Ethernet related devices.

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

[Jie Luo]

On 8/10/2024 7:30 PM, Krzysztof Kozlowski wrote:
> On 09/08/2024 15:01, Jie Luo wrote:
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: ref
>>>> +      - const: ahb
>>>> +      - const: sys
>>>> +
>>>> +  clock-output-names:
>>>> +    items:
>>>> +      - const: ppe-353mhz
>>>> +      - const: eth0-50mhz
>>>> +      - const: eth1-50mhz
>>>> +      - const: eth2-50mhz
>>>> +      - const: eth-25mhz
>>>
>>> Drop entire property. If the names are fixed, what's the point of having
>>> it in DTS? There is no.
>>
>> We had added the output names here for the reasons below. Can you please
>> let us know your suggestion whether keeping these here is fine?
>>
>> 1.) These output clocks are used as input reference clocks to other
>> consumer blocks. For example, an on-board Ethernet PHY device may be
>> wired to receive a specific clock from the above output clocks as
>> reference clock input, and hence the PHY's DTS node would need to
>> reference a particular index in this output clock array.
>>
>> Without these output clocks being made available in this DTS, the PHY
>> driver in above case would not know the clock specifier to access the
>> handle for the desired input clock.
> 
> That's not true. clock-output-names do not have anything to do with
> clock specifier.
> 
>>
>> 2.) One of the suggestions from the internal code review with Linaro was
>> to name the output clocks specifically based on rate and destination
>> (Ex: 'ppe-353mhz' for fixed rate 353 MHZ output clock connected to
>> Packet Process Engine block), so that the dt-bindings describe the
>> input/output clocks clearly.
> 
> Again, that's unrelated. None of above points address my concern. It's
> like you talk about some entirely different topic. Again:
> clock-output-names have nothing to do with what you want to achieve here.

OK, understand. I will drop this property "clock-output-names" from the
bindings and DTS. These names will instead be defined in the driver. For
the consumer clock device DTS nodes that need to reference these output
clocks, I will export the clock specifiers for these output clocks from
a header file. Hope this approach is fine.

> 
> Best regards,
> Krzysztof
>