| 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: <16d09acf-7bdd-04ee-6faf-936c0366df03@quicinc.com>
Date: Thu, 7 Sep 2023 16:36:50 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Stephen Boyd <sboyd@...nel.org>, <agross@...nel.org>,
<andersson@...nel.org>, <catalin.marinas@....com>,
<conor+dt@...nel.org>, <konrad.dybcio@...aro.org>,
<krzysztof.kozlowski+dt@...aro.org>, <mturquette@...libre.com>,
<p.zabel@...gutronix.de>, <robh+dt@...nel.org>, <will@...nel.org>
CC: <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 v6 4/4] clk: qcom: add clock controller driver for
qca8386/qca8084
On 9/6/2023 5:36 AM, Stephen Boyd wrote:
> 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?
>
Yes, the driver depends on MDIO BUS, will add this depends in the next
patchset.
>> + 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?
>
will remove this.
>> +#include <linux/regmap.h>
>> +#include <linux/phy.h>
>
> Is the phy include used? Where is the mdio.h include?
there is no PHY include, just the mdio_device is included, however the
mii_bus->mdio_lock is needed by this clock controller.
so "struct mii_bus" is needed and included by the header file phy.h,
the mdio.h is included by phy.h.
>
>> +
>> +#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...
Yes, the parent need to change the rate for the different interface
mode, PHY_INTERFACE_MODE_2500BASEX use 312.5M, PHY_INTERFACE_MODE_SGMII
use 125M.
>
>> + { }
>> +};
>> +
>> +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?
since it has the different parent clock rate 312.5M and 125M for the
deffernet interface mode used. If the flag CLK_SET_RATE_PARENT is set,
when we require to configure 25M clock rate for example, it may lead to
the parent rate changed(312.5M/12.5 or 125M/5), which is not expected,
the parent rate(312.5M or 125M) can't be changed, since the parent rate
is decided by interface mode(PHY_INTERFACE_MODE_2500BASEX or
PHY_INTERFACE_MODE_SGMII).
the work flow:
the parent of nss_cc_mac5_rx_clk_src is selected as 312.5M or 125M
firstly, then configure the required clock rate of clk_branch.
uniphy(312.5M or 125M) ---> RCG(nss_cc_mac5_rx_clk_src) ---> clk_branch.
>
>> + },
>> +};
>> +
>> +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?
since these clocks with the flag CLK_IS_CRITICAL are the common clocks
needed to be enabled for all devices no matter what work mode(qca8084 or
qca8386) used, which is base clock to enable to use the clock driver, to
enable these clocks by using flag CLK_IS_CRITICAL is simplier way and
can simply the device probe driver and device tree definations.
>
>> + .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.
will correct it.
>
>> +};
>> +
>> +/* 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.
okay, maybe convert_reg_to_mii_addr?
>
>> +{
>> + *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);
>
okay, thanks Stephen for the suggestion, will take this in the next
patch set.
>> +}
>> +
>> +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()?
>
yes, we can use __mdiobus_read and will verify it, thanks.
>> + 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?
okay, will add the "#define DATA_UPPER_16_BITS BIT(1)"? which is for
writing or reading upper 16 bit data when BIT(1) is set.
when the ret is negative, the ret should be not treated as data, will
take this case into account in the next updated patch.
>
>> + *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?
Hi Stephen,
No, we can't depend on regmap to pick the page, since the MDIO bus is
shared by qca8k device and PHY device, if there is a PHY device access,
even if the page is not changed, we still need to configure the page
again, so the page is alwasy configured for each register access, the
sequence can't be interrupted.
>
>> +{
>> + 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.
okay, will update this.
>
>> +{
>> + 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.
okay, will correct this.
>
>> + .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?
yes, REGCACHE_NONE should be the default value, we can remove this line
config.
>
>> +};
>> +
>> +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()?
Hi Stephen,
No, we can't use devm_regmap_init_mdio, which is for the standard PHY
device access(clause22 and clause 45), but the clock device needs the
special MDIO sequences for the register access.
Thanks Stephen for your previous time and the detail review comments.
>
>> + 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);
>> +}
>> +
Powered by blists - more mailing lists