Re: [PATCH v4 4/4] clk: qcom: add clock controller driver for qca8386/qca8084

From: Jie Luo
Date: Fri Aug 18 2023 - 10:08:40 EST

Next message: Chuck Lever III: "Re: Commit 'sunrpc: Use sendmsg(MSG_SPLICE_PAGES) rather then sendpage' broke O_DIRECT over NFS"
Previous message: Andy Shevchenko: "Re: [PATCH 3/6] soc/tegra: fuse: Add function to add lookups"
In reply to: Bjorn Andersson: "Re: [PATCH v4 4/4] clk: qcom: add clock controller driver for qca8386/qca8084"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 8/18/2023 11:55 AM, Bjorn Andersson wrote:

On Tue, Aug 15, 2023 at 04:52:05PM +0800, Luo Jie wrote:

Add clock & reset controller driver for qca8386/qca8084.

Not everyone is familiar with the QCA components, or the mdiobus dance.
Please do take the opportunity to educate us here.

Thanks Bjorn for the comments, i will add the detail message here in the next patch set.

This clock controller driver is for the device where register is accessed by MDIO bus, the framework of clock is same as the existed clock controller of QCA driver such gcc-ipq9574.

The MDIO bus is for accessing the PHY device by ieee802.3-c45 or ieee802.3-c22, the clock register of qca8386/qca8084 is also accessed by the MDIO bus, which has the special sequence to access the register with multiple MDIO read/write operations.

Change-Id: Ic4b768626b47f28073332885ae62972640821709

No Change-Id upstream, please.

okay, will remove this.

Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
---
drivers/clk/qcom/Kconfig | 8 +
drivers/clk/qcom/Makefile | 1 +
drivers/clk/qcom/nsscc-qca8k.c | 2118 ++++++++++++++++++++++++++++++++
3 files changed, 2127 insertions(+)
create mode 100644 drivers/clk/qcom/nsscc-qca8k.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 263e55d75e3f..d84705ff920d 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"
+ help
+ Support for NSS(Network SubSystem) clock controller on
+ qca8386/qca8084 chip.
+ Say Y if you want to use network features of switch or PHY
+ device. Select this for the root clock of qca8k.

Please make sure that this works as 'm' as well, and if it already does,
please loosen the language.

thanks for the comments, i will update this in the next patch set.

+
config MSM_GCC_8660
tristate "MSM8660 Global Clock Controller"
depends on ARM || COMPILE_TEST
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index e6e294274c35..17238177c39d 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_IPQ_GCC_6018) += gcc-ipq6018.o
obj-$(CONFIG_IPQ_GCC_806X) += gcc-ipq806x.o
obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
obj-$(CONFIG_IPQ_GCC_9574) += gcc-ipq9574.o
+obj-$(CONFIG_IPQ_NSSCC_QCA8K) += nsscc-qca8k.o

'N' > 'L'

will update this, thanks.

obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
obj-$(CONFIG_MDM_GCC_9607) += gcc-mdm9607.o
obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
diff --git a/drivers/clk/qcom/nsscc-qca8k.c b/drivers/clk/qcom/nsscc-qca8k.c

[..]

+static inline void split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
+{
+ *r1 = regaddr & 0x1c;
+
+ regaddr >>= 5;
+ *r2 = regaddr & 0x7;
+
+ regaddr >>= 3;
+ *page = regaddr & 0xffff;

Perhaps it's just me not knowing the details of the mdiobus, but it
would be really nice to have a comment with a detailed description of
the addressing scheme here.

Thanks for the review comments, i will explain the detail information for the sequence of accessing the switch register here in the next patch set.

+}
+
+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);
+ if (ret >= 0) {
+ *val = ret;
+ ret = bus->read(bus, switch_phy_id, (reg | BIT(1)));
+ *val |= ret << 16;
+ }
+
+ if (ret < 0) {
+ dev_err_ratelimited(&bus->dev, "fail to read qca8k mii register\n");
+
+ *val = 0;

It's good practice not to touch the return-by-reference parameter unless
you're returning success.

okay, will remove this in the next patch set.

+ return ret;
+ }
+
+ return 0;

Afaict ret is 0 here, so a single return ret; should be fine?

Yes, i will update to use the singel return ret here, thanks for the comments.

+}
+
+void qca8k_mii_write(struct mii_bus *bus, u16 switch_phy_id, u32 reg, u32 val)
+{
+ int ret;
+ u16 lo, hi;
+
+ lo = val & 0xffff;

lower_16_bits(val);

will update to use this in the next patch set.

+ hi = (u16)(val >> 16);

upper_16_bits(val);

will update to use this in the next patch set.

+
+ ret = bus->write(bus, switch_phy_id, reg, lo);
+ if (ret >= 0)
+ ret = bus->write(bus, switch_phy_id, (reg | BIT(1)), hi);
+
+ 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)
+{
+ 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_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+ 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)
+{
+ struct mii_bus *bus = context;
+ u16 r1, r2, page;
+ int ret;
+
+ reg += QCA8K_CLK_REG_BASE;
+ split_addr(reg, &r1, &r2, &page);
+
+ mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+ 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_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);

Why is this lock nested?

Thanks for the comments, i will update to use the normal lock mutex_lock() here.

+ 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,
+ .reg_read = qca8k_regmap_read,
+ .reg_write = qca8k_regmap_write,
+ .reg_update_bits = qca8k_regmap_update_bits,
+ .disable_locking = true,

Why do you disable_locking and then provide your own locking? Why not
specify fast_io = false, use mdiobus_read() and mdiobus_write() and let
the regmap framework implement update_bits for you?

Regards,
Bjorn

Hi Bjorn,
when we read/write the clock hardware register by MDIO bus, there are multiple MDIO operations where configuring page is needed before reading/writing the converted register, as the sequence mentioned in qca8k_regmap_read, and this sequence should be completed during the mutex_lock, that is why we can't use regmap mutex lock, which only protects single register access, but the clock register access needs multiple register access.

In addition, we also need to use the MDIO bus lock mii_bus->mdio_lock,
since this MDIO bus is also used by the PHY register accessed.

thanks,
Jie.

+ .cache_type = REGCACHE_NONE,
+};
+
+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);
+ 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);
+}
+
+static const struct of_device_id nss_cc_qca8k_match_table[] = {
+ { .compatible = "qcom,qca8084-nsscc" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, nss_cc_qca8k_match_table);
+
+static struct mdio_driver nss_cc_qca8k_driver = {
+ .mdiodrv.driver = {
+ .name = "qcom,qca8k-nsscc",
+ .of_match_table = nss_cc_qca8k_match_table,
+ },
+ .probe = nss_cc_qca8k_probe,
+};
+
+mdio_module_driver(nss_cc_qca8k_driver);
+
+MODULE_DESCRIPTION("QCOM NSS_CC QCA8K Driver");
+MODULE_LICENSE("GPL");
--
2.17.1

Next message: Chuck Lever III: "Re: Commit 'sunrpc: Use sendmsg(MSG_SPLICE_PAGES) rather then sendpage' broke O_DIRECT over NFS"
Previous message: Andy Shevchenko: "Re: [PATCH 3/6] soc/tegra: fuse: Add function to add lookups"
In reply to: Bjorn Andersson: "Re: [PATCH v4 4/4] clk: qcom: add clock controller driver for qca8386/qca8084"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]