| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <nk7o2fb6to26zt5fnbvfqhx2xg4rzmoeko2wfj5azh2ns4hisr@s76c3m33s774>
Date: Thu, 17 Aug 2023 20:55:44 -0700
From: Bjorn Andersson <andersson@...nel.org>
To: Luo Jie <quic_luoj@...cinc.com>
Cc: agross@...nel.org, konrad.dybcio@...aro.org,
mturquette@...libre.com, sboyd@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
catalin.marinas@....com, will@...nel.org, p.zabel@...gutronix.de,
linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
quic_srichara@...cinc.com
Subject: 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.
> Signed-off-by: Luo Jie <quic_luoj@...cinc.com>
> ---
> 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
>
Powered by blists - more mailing lists