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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux