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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Jie Luo (2024-10-17 08:35:43)
> 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.

Ok. It can probably be a select or imply statement then.

> 
> > 
> >> +       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
[...]
> >> +       }
> >> +
> >> +       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.
> 

Sounds good. Thanks.