linux-kernel - Re: [PATCH v3 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6sk7sx4pz2gnne2tg3d5lsphmnp6vqjj2tjogqcop7fwn3yk3r@ftevsz77w6pt>
Date: Mon, 2 Sep 2024 21:39:01 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jie Luo <quic_luoj@...cinc.com>
Cc: Stephen Boyd <sboyd@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Catalin Marinas <catalin.marinas@....com>, 
	Conor Dooley <conor+dt@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Michael Turquette <mturquette@...libre.com>, 
	Rob Herring <robh@...nel.org>, Will Deacon <will@...nel.org>, linux-arm-msm@...r.kernel.org, 
	linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, quic_kkumarcs@...cinc.com, quic_suruchia@...cinc.com, 
	quic_pavir@...cinc.com, quic_linchen@...cinc.com, quic_leiwei@...cinc.com, 
	bartosz.golaszewski@...aro.org, srinivas.kandagatla@...aro.org
Subject: Re: [PATCH v3 2/4] clk: qcom: Add CMN PLL clock controller driver
 for IPQ SoC

On Mon, Sep 02, 2024 at 11:33:57PM GMT, Jie Luo wrote:
> 
> 
> On 8/31/2024 6:24 AM, Stephen Boyd wrote:
> > Quoting Jie Luo (2024-08-30 09:14:28)
> > > Hi Stephen,
> > > Please find below a minor update to my earlier message on clk_ops usage.
> > 
> > Ok. Next time you can trim the reply to save me time.
> 
> OK.
> 
> > 
> > > On 8/28/2024 1:44 PM, Jie Luo wrote:
> > > > On 8/28/2024 7:50 AM, Stephen Boyd wrote:
> > > > > Quoting Luo Jie (2024-08-27 05:46:00)
> > > > > > +       case 48000000:
> > > > > > +               val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
> > > > > > +               break;
> > > > > > +       case 50000000:
> > > > > > +               val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8);
> > > > > > +               break;
> > > > > > +       case 96000000:
> > > > > > +               val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
> > > > > > +               val &= ~CMN_PLL_REFCLK_DIV;
> > > > > > +               val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2);
> > > > > > +               break;
> > > > > > +       default:
> > > > > > +               return -EINVAL;
> > > > > > +       }
> > > > > 
> > > > > Why isn't this done with struct clk_ops::set_rate() or clk_ops::init()?
> > > > 
> > > > OK, I will move this code into the clk_ops::init().
> > > 
> > > This code is expected to be executed once for initializing the CMN PLL
> > > to enable output clocks, and requires the parent clock rate to be
> > > available. However the parent clock rate is not available in the
> > > clk_ops::init(). Hence clk_ops::set_rate() seems to be the right option
> > > for this. Please let us know if this approach is fine. Thanks.
> > 
> > Sure. It actually sounds like the PLL has a mux to select different
> > reference clks. Is that right? If so, it seems like there should be
> > multiple 'clocks' for the DT property and many parents possible. If
> > that's the case then it should be possible to have something like
> > 
> > 	clocks = <0>, <&refclk>, <0>;
> > 
> > in the DT node and then have clk_set_rate() from the consumer actually
> > set the parent index in hardware. If that's all static then it can be
> > done with assigned-clock-parents or assigned-clock-rates.
> 
> Thanks Stephen. The CMN PLL block always uses a single input reference
> clock pin on any given IPQ SoC, however its rate may be different on
> different IPQ SoC. For example, its rate is 48MHZ on IPQ9574 and 96MHZ
> on IPQ5018.
> 
> Your second suggestion seems more apt for this device. I can define the
> DT property 'assigned-clock-parents' to configure the clock parent of
> CMN PLL. The code for reference clock selection will be added in
> clk_ops::set_parent(). Please let us know if this approach is fine.

What is the source of this clock? Can you call clk_get_rate() on this
input?

-- 
With best wishes
Dmitry