On 8/28/2024 7:50 AM, Stephen Boyd wrote:

Quoting Luo Jie (2024-08-27 05:46:00)

quoted

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

I don't see many other filenames with clk- prefix in this directory, so
probably drop it.

OK.

quoted

  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..a9775c39b2f3
--- /dev/null
+++ b/drivers/clk/qcom/clk-ipq-cmn-pll.c

@@ -0,0 +1,241 @@
+// 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 Ethernet hardware blocks.
+ * The Ethernet related blocks include PPE (packet process engine) and the
+ * external connected PHY (or switch) chip receiving clocks from the CMN PLL.
+ *
+ * On the IPQ9574 SoC, There are three clocks with 50 MHZ, 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.
+ *
+ *               +---------+
+ *               |   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 <dt-bindings/clock/qcom,ipq-cmn-pll.h>

Include dt-bindings after linux please.

OK, will update.

quoted

+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/iopoll.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_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)
+
+/**
+ * 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;
+       const unsigned long rate;
+};
+
+#define CLK_PLL_OUTPUT(_id, _name, _rate) {            \
+       .id = _id,                                      \
+       .name = _name,                                  \
+       .rate = _rate,                                  \
+}
+
+static const struct cmn_pll_fixed_output_clk ipq9574_output_clks[] = {
+       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 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);

Use platform_device APIs please. This is a platform driver.

OK. Will update to use API devm_platform_ioremap_resource().

quoted

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

Why isn't this done with struct clk_ops::set_rate() or clk_ops::init()?

OK, I will move this code into the clk_ops::init().

quoted

+
+       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);
+
+       /* Enable PLL locked detect. */
+       val = readl(base + CMN_PLL_CTRL);
+       val |= CMN_PLL_CTRL_LOCK_DETECT_EN;
+       writel(val, base + CMN_PLL_CTRL);
+
+       /*
+        * Reset the CMN PLL block 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 + CMN_PLL_POWER_ON_AND_RESET);
+       usleep_range(1000, 1200);
+
+       val |= CMN_ANA_EN_SW_RSTN;
+       writel(val, base + CMN_PLL_POWER_ON_AND_RESET);
+
+       /* Stability check of CMN PLL output clocks. */
+       return readl_poll_timeout(base + CMN_PLL_LOCKED, val,
+                                 (val & CMN_PLL_CLKS_LOCKED),
+                                 100, 100000);
+}
+
+static int ipq_cmn_pll_clk_register(struct device *dev, const char *parent)

Please don't use string names to describe topology.

OK, I will update to use an instance of 'struct clk_parent_data' to
describe the parent clock. This will be part of clk_hw instance that we
will define now, to represent the PLL clock and its ops (such as .init)
along with the parent.

quoted

+{
+       const struct cmn_pll_fixed_output_clk *fixed_clk;
+       struct clk_hw_onecell_data *data;
+       unsigned int num_clks;
+       struct clk_hw *hw;
+     
+
+       num_clks = ARRAY_SIZE(ipq9574_output_clks);
+       fixed_clk = ipq9574_output_clks;
+
+       data = devm_kzalloc(dev, struct_size(data, hws, num_clks), GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       for (i = 0; i < num_clks; i++) {
+               hw = devm_clk_hw_register_fixed_rate(dev, fixed_clk[i].name,
+                                                    parent, 0,
+                                                    fixed_clk[i].rate);
+               if (IS_ERR(hw))
+                       return PTR_ERR(hw);
+
+               data->hws[fixed_clk[i].id] = hw;
+       }
+       data->num = num_clks;
+
+       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 CMN PLL registers, the GCC AHB & SYSY clocks
+        * for CMN 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");

Usually qcom clk drivers do this with pm_clk_add() and runtime PM. Why
can't that be done here?

Yes, the pm_clk_add() can be used to manage clocks, I will udpate to use
the PM framework. Thanks for the suggestion.

quoted

+
+       clk = devm_clk_get(dev, "ref");
+       if (IS_ERR(clk))
+               return dev_err_probe(dev, PTR_ERR(clk),
+                                    "Get reference clock failed\n");

We don't want clk providers to be clk consumers. Presumably this is the
PLL's parent clk, and so the frequency should be passed to the clk_ops
via the parent rate.

Yes, this is the PLL's parent clock. OK, I will remove this code and 
update to use clk_parent_data to describe this parent clock.

quoted

+
+       /* Configure CMN 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 CMN 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", },
+       { }
+};

module device table?

I will add the MODULE_DEVICE_TABLE.