Quoting Luo Jie (2023-09-01 02:18:23)Yes, the driver depends on MDIO BUS, will add this depends in the next patchset.
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 263e55d75e3f..785cb6eb514f 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -195,6 +195,14 @@ config IPQ_GCC_9574
i2c, USB, SD/eMMC, etc. Select this for the root clock
of ipq9574.
+config IPQ_NSSCC_QCA8K
+ tristate "QCA8K(QCA8386 or QCA8084) NSS Clock Controller"
This needs to be limited by a depends.
depends on MDIO_BUS || COMPILE_TEST
perhaps?
will remove this.+ help
+ Support for NSS(Network SubSystem) clock controller on
+ qca8386/qca8084 chip.
+ Say Y or M if you want to use network features of switch or
+ PHY device. Select this for the root clock of qca8k.
+
config MSM_GCC_8660
tristate "MSM8660 Global Clock Controller"
depends on ARM || COMPILE_TEST
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?
+#include <linux/regmap.h>
+#include <linux/phy.h>
Is the phy include used? Where is the mdio.h include?
+[...]
+#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...
+ { }
+};
+
+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?
+ },[...]
+};
+
+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?
+ .ops = &clk_branch2_prepare_ops,[...]
+ },
+ },
+};
+
+static const struct clk_parent_data nss_cc_xo_data[] = {
+ { .index = DT_XO },
+};
+
+static const struct parent_map nss_cc_xo_map[] = {
+ { P_XO, 0 },
+};
+
+static const struct freq_tbl ftbl_nss_cc_sys_clk_src[] = {
+ F(25000000, P_XO, 2, 0, 0),
+ { }
+};
+[...]
+static const struct qcom_reset_map nss_cc_qca8k_resets[] = {
+ [NSS_CC_GEPHY1_ARES] = { 0x304, 1 },
+ [NSS_CC_GEPHY2_ARES] = { 0x304, 2 },
+ [NSS_CC_GEPHY3_ARES] = { 0x304, 3 },
+ [NSS_CC_DSP_ARES] = { 0x304, 4 },
+ [NSS_CC_GLOBAL_ARES] = { 0x308, 0 },
+ [NSS_CC_XPCS_ARES] = { 0x30C, 0 },
Lowercase hex please.
+};
+
+/* 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, thanks Stephen for the suggestion, will take this in the next patch set.
+{
+ *r1 = regaddr & 0x1c;
+
+ regaddr >>= 5;
+ *r2 = regaddr & 0x7;
+
+ regaddr >>= 3;
+ *page = regaddr & 0xffff;
Instead of this can you use FIELD_GET and have some macros for the part
of the address? Something like
#define QCA8K_CLK_REG_MASK GENMASK(4, 2)
#define QCA8K_CLK_PHY_ADDR_MASK GENMASK(7, 5)
#define QCA8K_CLK_PAGE_MASK GENMASK(24, 8)
and then rename 'r1' and 'r2' to something else?
*reg = FIELD_GET(QCA8K_CLK_REG_MASK, regaddr);
*phy_addr = FIELD_GET(QCA8K_CLK_PHY_ADDR_MASK, regaddr) | QCA8K_LOW_ADDR_PREFIX;
*page = FIELD_GET(QCA8K_CLK_PAGE_MASK);
+}
+
+int qca8k_mii_read(struct mii_bus *bus, u16 switch_phy_id, u32 reg, u32 *val)
+{
+ int ret;
+
+ ret = bus->read(bus, switch_phy_id, reg);
Why can't we use __mdiobus_read()?
+ if (ret >= 0) {
+ *val = ret;
+ ret = bus->read(bus, switch_phy_id, (reg | BIT(1)));
What is BIT(1)? Can it have a #define? What if ret is negative? We
shouldn't treat that as data, right?
+ *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?
+{
+ int ret;
+
+ ret = bus->write(bus, switch_phy_id, reg, page);
+ if (ret < 0)
+ dev_err_ratelimited(&bus->dev, "fail to set page\n");
+
+ return ret;
+}
+
+int qca8k_regmap_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct mii_bus *bus = context;
+ u16 r1, r2, page;
+ int ret;
+
+ reg += QCA8K_CLK_REG_BASE;
+ split_addr(reg, &r1, &r2, &page);
+
+ mutex_lock(&bus->mdio_lock);
+ ret = qca8k_mii_page_set(bus, QCA8K_HIGH_ADDR_PREFIX, QCA8K_CFG_PAGE_REG, page);
+ if (ret < 0)
+ goto qca8k_read_exit;
+
+ ret = qca8k_mii_read(bus, QCA8K_LOW_ADDR_PREFIX | r2, r1, val);
+
+qca8k_read_exit:
+ mutex_unlock(&bus->mdio_lock);
+ return ret;
+};
+
+int qca8k_regmap_write(void *context, unsigned int reg, unsigned int val)
These wrappers should be static. Please run sparse.
+{
+ struct mii_bus *bus = context;
+ u16 r1, r2, page;
+ int ret;
+
+ reg += QCA8K_CLK_REG_BASE;
+ split_addr(reg, &r1, &r2, &page);
+
+ mutex_lock(&bus->mdio_lock);
+ ret = qca8k_mii_page_set(bus, QCA8K_HIGH_ADDR_PREFIX, QCA8K_CFG_PAGE_REG, page);
+ if (ret < 0)
+ goto qca8k_write_exit;
+
+ qca8k_mii_write(bus, QCA8K_LOW_ADDR_PREFIX | r2, r1, val);
+
+qca8k_write_exit:
+ mutex_unlock(&bus->mdio_lock);
+ return ret;
+};
+
+int qca8k_regmap_update_bits(void *context, unsigned int reg, unsigned int mask, unsigned int value)
+{
+ struct mii_bus *bus = context;
+ u16 r1, r2, page;
+ int ret;
+ u32 val;
+
+ reg += QCA8K_CLK_REG_BASE;
+ split_addr(reg, &r1, &r2, &page);
+
+ mutex_lock(&bus->mdio_lock);
+ ret = qca8k_mii_page_set(bus, QCA8K_HIGH_ADDR_PREFIX, QCA8K_CFG_PAGE_REG, page);
+ if (ret < 0)
+ goto qca8k_update_exit;
+
+ ret = qca8k_mii_read(bus, QCA8K_LOW_ADDR_PREFIX | r2, r1, &val);
+ if (ret < 0)
+ goto qca8k_update_exit;
+
+ val &= ~mask;
+ val |= value;
+ qca8k_mii_write(bus, QCA8K_LOW_ADDR_PREFIX | r2, r1, val);
+
+qca8k_update_exit:
+ mutex_unlock(&bus->mdio_lock);
+ return ret;
+}
+
+static const struct regmap_config nss_cc_qca8k_regmap_config = {
+ .reg_bits = 12,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .max_register = 0x30C,
Lowercase hex please.
+ .reg_read = qca8k_regmap_read,
+ .reg_write = qca8k_regmap_write,
+ .reg_update_bits = qca8k_regmap_update_bits,
+ .disable_locking = true,
+ .cache_type = REGCACHE_NONE,
Isn't this the default?
+};
+
+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()?
+ if (IS_ERR(regmap))
+ return dev_err_probe(&mdiodev->dev, PTR_ERR(regmap), "Failed to init regmap\n");
+
+ return qcom_cc_really_probe(&mdiodev->dev, &nss_cc_qca8k_desc, regmap);
+}
+