Re: [PATCH v4 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

[Jie Luo <quic_luoj@quicinc.com>]

On 10/17/2024 5:37 AM, Stephen Boyd wrote:
> Quoting Luo Jie (2024-10-15 07:16:52)
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index 30eb8236c9d8..3def659fc5cb 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -190,6 +190,16 @@ config IPQ_APSS_6018
>>            Say Y if you want to support CPU frequency scaling on
>>            ipq based devices.
>>   
>> +config IPQ_CMN_PLL
>> +       tristate "IPQ CMN PLL Clock Controller"
>> +       depends on IPQ_GCC_9574
> 
> What is the build dependency?

Will remove this dependency. There is no build dependency on 
IPQ_GCC_9574, but only a functional dependency on the SoC's GCC block
since the CMNPLL consumes AHB/SYS clocks from the GCC.

> 
>> +       help
>> +         Support for CMN PLL clock controller on IPQ platform. The
>> +         CMN PLL feeds the reference clocks to the Ethernet devices
>> +         based on IPQ SoC.
>> +         Say Y or M if you want to support CMN PLL clock on the IPQ
>> +         based devices.
>> +
>>   config IPQ_GCC_4019
>>          tristate "IPQ4019 Global Clock Controller"
>>          help
>> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
>> new file mode 100644
>> index 000000000000..f5ebc7d93ed8
>> --- /dev/null
>> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
>> @@ -0,0 +1,411 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +/*
>> + * CMN PLL block expects the reference clock from on-board Wi-Fi block,
>> + * and supplies fixed rate clocks as output to the networking hardware
>> + * blocks and to GCC. The networking related blocks include PPE (packet
>> + * process engine), the externally connected PHY or switch devices, and
>> + * the PCS.
>> + *
>> + * On the IPQ9574 SoC, there are three clocks with 50 MHZ and one clock
>> + * with 25 MHZ which are output from the CMN PLL to Ethernet PHY (or switch),
>> + * and one clock with 353 MHZ to PPE. The other fixed rate output clocks
>> + * are supplied to GCC (24 MHZ as XO and 32 KHZ as sleep clock), and to PCS
>> + * with 31.25 MHZ.
>> + *
>> + *               +---------+
>> + *               |   GCC   |
>> + *               +--+---+--+
>> + *           AHB CLK|   |SYS CLK
>> + *                  V   V
>> + *          +-------+---+------+
>> + *          |                  +-------------> eth0-50mhz
>> + * REF CLK  |     IPQ9574      |
>> + * -------->+                  +-------------> eth1-50mhz
>> + *          |  CMN PLL block   |
>> + *          |                  +-------------> eth2-50mhz
>> + *          |                  |
>> + *          +----+----+----+---+-------------> eth-25mhz
>> + *               |    |    |
>> + *               V    V    V
>> + *              GCC  PCS  NSS/PPE
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
> 
> What is of_address.h for? Did you mean mod_devicetable.h?

I will replace of_address.h with mod_devicetable.h for 'of_device_id'
structure usage.

> 
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
>> +
>> +#define CMN_PLL_REFCLK_SRC_SELECTION           0x28
>> +#define CMN_PLL_REFCLK_SRC_DIV                 GENMASK(9, 8)
>> +
>> +#define CMN_PLL_LOCKED                         0x64
>> +#define CMN_PLL_CLKS_LOCKED                    BIT(8)
>> +
>> +#define CMN_PLL_POWER_ON_AND_RESET             0x780
>> +#define CMN_ANA_EN_SW_RSTN                     BIT(6)
>> +
>> +#define CMN_PLL_REFCLK_CONFIG                  0x784
>> +#define CMN_PLL_REFCLK_EXTERNAL                        BIT(9)
>> +#define CMN_PLL_REFCLK_DIV                     GENMASK(8, 4)
> [...]
>> +
>> +/*
>> + * This function is used to initialize the CMN PLL to enable the fixed
>> + * rate output clocks. It is expected to be configured once.
>> + */
>> +static int clk_cmn_pll_determine_rate(struct clk_hw *hw,
>> +                                     struct clk_rate_request *req)
>> +{
>> +       struct clk_cmn_pll *cmn_pll = to_clk_cmn_pll(hw);
>> +       u32 val;
>> +       int ret;
>> +
>> +       /*
>> +        * Configure the reference input clock selection as per the given
>> +        * parent clock. The output clock rates are always of fixed value.
>> +        */
>> +       switch (req->best_parent_rate) {
>> +       case 25000000:
>> +               val = 3;
>> +               break;
>> +       case 31250000:
>> +               val = 4;
>> +               break;
>> +       case 40000000:
>> +               val = 6;
>> +               break;
>> +       case 50000000:
>> +               val = 8;
>> +               break;
>> +       case 48000000:
>> +       case 96000000:
>> +               /*
>> +                * Parent clock rate 48 MHZ and 96 MHZ take the same value
>> +                * of reference clock index. 96 MHZ needs the source clock
>> +                * divider to be programmed as 2.
>> +                */
>> +               val = 7;
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       ret = regmap_update_bits(cmn_pll->regmap, CMN_PLL_REFCLK_CONFIG,
>> +                                CMN_PLL_REFCLK_INDEX,
>> +                                FIELD_PREP(CMN_PLL_REFCLK_INDEX, val));
> 
> The determine_rate() function shouldn't modify the hardware. This should
> be done in the set_rate() callback. Likely you'll need to use
> assigned-clock-rates to do that.

OK. I will move the hardware configuration code into clk_ops::set_rate().
We are using the DT property assigned-clock-rates-u64 to configure the
clock rate of CMN PLL to 12 GHZ since 64 bits are required.