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

Jie Luo
2023-09-08T11:11:06Z

On 9/8/2023 6:45 AM, Stephen Boyd wrote:
> Quoting Jie Luo (2023-09-07 01:36:50)
>>
>> 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/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.
>
> Don't rely on implicit includes. It leads to compile errors if headers
> are ever split/moved around. Just include mdio.h as you use it.
>
okay, will include mdio.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.
>
> Ok. So you're saying that the uniphy rate changes outside of the clk
> framework? That is potentially troublesome because the clk framework
> aggressively caches things to the point that if the parent of the RCG
> changes rates the branch rate won't reflect the new rate. It looks like
> none of that really matters in practice because the divider is always 1
> here, but this will be confusing if a driver calls clk_get_rate() when
> the uniphy rate has changed.
>
> Why can't that be driven from the clk framework? Or why can't the uniphy
> implement a clk provider that supports changing rates? If that was done,
> then a driver could change the uniphy rate and the clk framework would
> propagate the frequency down to all the children, recalculating the
> rates along the way. It may even mean that there's nothing to do when
> changing these clks, besides perhaps changing the parent?
>

Hi Stephen,
Yes, the uniphy implements the clock provider that supports changing
rate, which will be upstream later, and nss_cc_mac5_rx_clk_src is the
special case, which is only used in the switch device qca8386.

For the phy device qca8084(uniphy has only 312.5M fix clock which is
registered by device tree), this clock nss_cc_mac5_rx_clk_src is not used.

The issue for the switch device(qca8386) here is the clock rate of
parent uniphy can't be changed because of the clock rate requirement of
branch clock, since the uniphy clock rate is decided by the current
working interface mode(PHY_INTERFACE_MODE_2500BASEX with 312.5M or
PHY_INTERFACE_MODE_SGMII with 125M).

For example, when the uniphy works on PHY_INTERFACE_MODE_2500BASEX, then
the parent uniphy clock rate is 312.5M, which is decided by hardware and
can't be changed. when a branch clock requires a 25M clock, the parent
uniphy clock maybe updated to 125M by clock framework if the flag
CLK_SET_RATE_PARENT is set here, but the actual hardware clock rate of
uniphy is still 315.5M since the uniphy still works in the interface
mode PHY_INTERFACE_MODE_2500BASEX.

>>
>>>
>>>> + },
>>>> +};
>>>> +
>>>> +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.
>
> Sure, but it also means you use the despised CLK_IS_CRITICAL flag when
> it could simply be some code in probe that sets some bits for "boot
> configuration". The benefit is that we don't register clks that do
> practically nothing besides use resources in the kernel for a one time
> operation at probe.
>

Okay, that makes sense, i will remove this flag CLK_IS_CRITICAL, and
enable these clocks in the probe function of the consumer driver.

Thanks Stephen for the review and suggestions!
>>
>>>
>>>> +};
>>>> +
>>>> +/* 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?
>
> Sure!
>
>>>
>>>> + *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.
>
> Ok.
>
>>>
>>>> +};
>>>> +
>>>> +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.
>
> Ok.