Message-ID: <17681a9f756cc70a190c674c51b90140.sboyd@kernel.org>
Date:   Thu, 07 Sep 2023 15:45:23 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Jie Luo <quic_luoj@...cinc.com>, 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

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.

> 
> > 
> >> +
> >> +#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?

> 
> > 
> >> +       },
> >> +};
> >> +
> >> +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.

> 
> > 
> >> +};
> >> +
> >> +/* 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.

Powered by blists - more mailing lists