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

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.

Change-Id: Ic4b768626b47f28073332885ae62972640821709

No Change-Id upstream, please.

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

'N' > 'L'

[..]

+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.

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

+		return ret;
+	}
+
+	return 0;

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

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

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

upper_16_bits(val);

+
+	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?

+	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

+	.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