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: <492e3c19-c06d-4faa-8064-e6b73c46b13e@quicinc.com>
Date: Tue, 3 Sep 2024 22:00:31 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
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 9/3/2024 2:39 AM, Dmitry Baryshkov wrote:
> 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?
> 

The source (parent clock) for CMN PLL is always from on-board Wi-Fi
block for any given IPQ SoC.

 From the discussion so far, it seems there are two approaches possible
which I would like to summarize below to be clear. Please let us know
if this understanding or approach needs correction. Thanks.

1. clk_get_rate() requires the parent clock instance to be acquired by
devm_clk_get(). Per our understanding from Stephen's previous comment,
it is preferred that a clock provider driver (this) does not use the
_get_ APIs on the parent clock to get the rate. Instead the parent rate
should be passed to the clk_ops using parent data. So the parent clock
should be specified in the DT using assigned-clock-parents property, and
can be accessed from the clk_ops::set_parent(). This seems like a more
reasonable method.

2. Alternatively, if it is architecturally acceptable to use
devm_clk_get() and clk_get_rate() in this clock provider driver, we can
save this parent clock rate into a local driver data structure and then
access it from clk_ops::init() for configuring the PLL.