Hi Stephen,
Please find below a minor update to my earlier message on clk_ops usage.
On 8/28/2024 1:44 PM, Jie Luo wrote:
On 8/28/2024 7:50 AM, Stephen Boyd wrote:
Quoting Luo Jie (2024-08-27 05:46:00)
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.
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.
+#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().
+ 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().
This code is expected to be executed once for initializing the CMN PLL
to enable output clocks, and requires the parent clock rate to be
available. However the parent clock rate is not available in the
clk_ops::init(). Hence clk_ops::set_rate() seems to be the right option
for this. Please let us know if this approach is fine. Thanks.
+
+ 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.
+{
+ 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.
+
+ 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.
+
+ /* 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.
