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

From: Stephen Boyd
Date: Tue Sep 05 2023 - 17:36:29 EST

Quoting Luo Jie (2023-09-01 02:18:23)
> 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?

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

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