Re: [PATCH v6 4/4] clk: qcom: add clock controller driver for qca8386/qca8084
From: Jie Luo
Date: Fri Sep 08 2023 - 07:11:07 EST
On 9/8/2023 6:45 AM, Stephen Boyd wrote:
Quoting Jie Luo (2023-09-07 01:36:50)
On 9/6/2023 5:36 AM, Stephen Boyd wrote:
Quoting Luo Jie (2023-09-01 02:18:23)
diff --git a/drivers/clk/qcom/nsscc-qca8k.c b/drivers/clk/qcom/nsscc-qca8k.c
new file mode 100644
index 000000000000..f9312735daf3
--- /dev/null
+++ b/drivers/clk/qcom/nsscc-qca8k.c
@@ -0,0 +1,2179 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
Is platform_device include used?
will remove this.
+#include <linux/regmap.h>
+#include <linux/phy.h>
Is the phy include used? Where is the mdio.h include?
there is no PHY include, just the mdio_device is included, however the
mii_bus->mdio_lock is needed by this clock controller.
so "struct mii_bus" is needed and included by the header file phy.h,
the mdio.h is included by phy.h.
Don't rely on implicit includes. It leads to compile errors if headers
are ever split/moved around. Just include mdio.h as you use it.
okay, will include mdio.h
+
+#include <dt-bindings/clock/qcom,qca8k-nsscc.h>
+#include <dt-bindings/reset/qcom,qca8k-nsscc.h>
+
+#include "clk-branch.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "clk-regmap-divider.h"
+#include "clk-regmap-mux.h"
[...]
+
+static const struct freq_tbl ftbl_nss_cc_mac5_rx_clk_src[] = {
+ F(50000000, P_XO, 1, 0, 0),
+ F(125000000, P_UNIPHY0_RX, 1, 0, 0),
+ F(125000000, P_UNIPHY0_TX, 1, 0, 0),
+ F(312500000, P_UNIPHY0_RX, 1, 0, 0),
+ F(312500000, P_UNIPHY0_TX, 1, 0, 0),
This frequency table looks like the parent should change rate...
Yes, the parent need to change the rate for the different interface
mode, PHY_INTERFACE_MODE_2500BASEX use 312.5M, PHY_INTERFACE_MODE_SGMII
use 125M.
+ { }
+};
+
+static struct clk_rcg2 nss_cc_mac5_rx_clk_src = {
+ .cmd_rcgr = 0x154,
+ .freq_tbl = ftbl_nss_cc_mac5_rx_clk_src,
+ .hid_width = 5,
+ .parent_map = nss_cc_uniphy0_rx_tx_map,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "nss_cc_mac5_rx_clk_src",
+ .parent_data = nss_cc_uniphy0_rx_tx_data,
+ .num_parents = ARRAY_SIZE(nss_cc_uniphy0_rx_tx_data),
+ .ops = &clk_rcg2_ops,
... but this doesn't have any CLK_SET_RATE_PARENT flag set. How does it
work?
since it has the different parent clock rate 312.5M and 125M for the
deffernet interface mode used. If the flag CLK_SET_RATE_PARENT is set,
when we require to configure 25M clock rate for example, it may lead to
the parent rate changed(312.5M/12.5 or 125M/5), which is not expected,
the parent rate(312.5M or 125M) can't be changed, since the parent rate
is decided by interface mode(PHY_INTERFACE_MODE_2500BASEX or
PHY_INTERFACE_MODE_SGMII).
the work flow:
the parent of nss_cc_mac5_rx_clk_src is selected as 312.5M or 125M
firstly, then configure the required clock rate of clk_branch.
uniphy(312.5M or 125M) ---> RCG(nss_cc_mac5_rx_clk_src) ---> clk_branch.
Ok. So you're saying that the uniphy rate changes outside of the clk
framework? That is potentially troublesome because the clk framework
aggressively caches things to the point that if the parent of the RCG
changes rates the branch rate won't reflect the new rate. It looks like
none of that really matters in practice because the divider is always 1
here, but this will be confusing if a driver calls clk_get_rate() when
the uniphy rate has changed.
Why can't that be driven from the clk framework? Or why can't the uniphy
implement a clk provider that supports changing rates? If that was done,
then a driver could change the uniphy rate and the clk framework would
propagate the frequency down to all the children, recalculating the
rates along the way. It may even mean that there's nothing to do when
changing these clks, besides perhaps changing the parent?
Hi Stephen,
Yes, the uniphy implements the clock provider that supports changing
rate, which will be upstream later, and nss_cc_mac5_rx_clk_src is the
special case, which is only used in the switch device qca8386.
For the phy device qca8084(uniphy has only 312.5M fix clock which is
registered by device tree), this clock nss_cc_mac5_rx_clk_src is not used.
The issue for the switch device(qca8386) here is the clock rate of
parent uniphy can't be changed because of the clock rate requirement of
branch clock, since the uniphy clock rate is decided by the current
working interface mode(PHY_INTERFACE_MODE_2500BASEX with 312.5M or
PHY_INTERFACE_MODE_SGMII with 125M).
For example, when the uniphy works on PHY_INTERFACE_MODE_2500BASEX, then
the parent uniphy clock rate is 312.5M, which is decided by hardware and
can't be changed. when a branch clock requires a 25M clock, the parent
uniphy clock maybe updated to 125M by clock framework if the flag
CLK_SET_RATE_PARENT is set here, but the actual hardware clock rate of
uniphy is still 315.5M since the uniphy still works in the interface
mode PHY_INTERFACE_MODE_2500BASEX.
+ },
+};
+
+static struct clk_regmap_div nss_cc_mac5_rx_div_clk_src = {
+ .reg = 0x15c,
+ .shift = 0,
+ .width = 4,
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "nss_cc_mac5_rx_div_clk_src",
[...]
+
+static struct clk_branch nss_cc_mdio_master_ahb_clk = {
+ .halt_reg = 0x19c,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0x19c,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "nss_cc_mdio_master_ahb_clk",
+ .parent_hws = (const struct clk_hw *[]) {
+ &nss_cc_ahb_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
Why can't we simply enable clks in probe that are critical? The regmap
operations are complicated?
since these clocks with the flag CLK_IS_CRITICAL are the common clocks
needed to be enabled for all devices no matter what work mode(qca8084 or
qca8386) used, which is base clock to enable to use the clock driver, to
enable these clocks by using flag CLK_IS_CRITICAL is simplier way and
can simply the device probe driver and device tree definations.
Sure, but it also means you use the despised CLK_IS_CRITICAL flag when
it could simply be some code in probe that sets some bits for "boot
configuration". The benefit is that we don't register clks that do
practically nothing besides use resources in the kernel for a one time
operation at probe.
Okay, that makes sense, i will remove this flag CLK_IS_CRITICAL, and
enable these clocks in the probe function of the consumer driver.
Thanks Stephen for the review and suggestions!
+};
+
+/* For each read/write operation of clock register, there are three MDIO frames
+ * sent to the device.
+ *
+ * 1. The high address part[31:8] of register is packaged into the first MDIO frame.
+ * 2. The low address part[7:0] of register is packaged into the second MDIO frame
+ * with the low 16bit data to read/write.
+ * 3. The low address part[7:0] of register is packaged into the last MDIO frame
+ * with the high 16bit data to read/write.
+ *
+ * The clause22 MDIO frame format used by device is as below.
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | ST| OP| ADDR | REG | TA| DATA |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+static inline void split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
split_addr() is too generic of a name. Please namespace this function to
something else.
okay, maybe convert_reg_to_mii_addr?
Sure!
+ *val |= ret << 16;
+ }
+
+ if (ret < 0)
+ dev_err_ratelimited(&bus->dev, "fail to read qca8k mii register\n");
+
+ return ret < 0 ? ret : 0;
+}
+
+void qca8k_mii_write(struct mii_bus *bus, u16 switch_phy_id, u32 reg, u32 val)
+{
+ int ret;
+
+ ret = bus->write(bus, switch_phy_id, reg, lower_16_bits(val));
+ if (ret >= 0)
+ ret = bus->write(bus, switch_phy_id, (reg | BIT(1)), upper_16_bits(val));
+
+ if (ret < 0)
+ dev_err_ratelimited(&bus->dev, "fail to write qca8k mii register\n");
+}
+
+int qca8k_mii_page_set(struct mii_bus *bus, u16 switch_phy_id, u32 reg, u16 page)
Regmap core has support for picking pages. Can that be used here?
Hi Stephen,
No, we can't depend on regmap to pick the page, since the MDIO bus is
shared by qca8k device and PHY device, if there is a PHY device access,
even if the page is not changed, we still need to configure the page
again, so the page is alwasy configured for each register access, the
sequence can't be interrupted.
Ok.
+};
+
+static const struct qcom_cc_desc nss_cc_qca8k_desc = {
+ .config = &nss_cc_qca8k_regmap_config,
+ .clks = nss_cc_qca8k_clocks,
+ .num_clks = ARRAY_SIZE(nss_cc_qca8k_clocks),
+ .resets = nss_cc_qca8k_resets,
+ .num_resets = ARRAY_SIZE(nss_cc_qca8k_resets),
+};
+
+static int nss_cc_qca8k_probe(struct mdio_device *mdiodev)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init(&mdiodev->dev, NULL, mdiodev->bus, nss_cc_qca8k_desc.config);
Why can't we use devm_regmap_init_mdio() here? Is it because the device
needs special data marshaling per split_addr()?
Hi Stephen,
No, we can't use devm_regmap_init_mdio, which is for the standard PHY
device access(clause22 and clause 45), but the clock device needs the
special MDIO sequences for the register access.
Ok.