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

[Bjorn Andersson]

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.

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

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

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

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