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

[Stephen Boyd <sboyd@kernel.org>]

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?

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

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