linux-kernel - Re: [PATCH v2 4/4] clk: qcom: gcc: Add support for QCS615 GCC clocks

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] [day] [month] [year] [list]

Message-ID: <d58dec6e-f77a-4663-9aca-706a462a5686@quicinc.com>
Date: Sat, 19 Oct 2024 00:31:59 +0530
From: Taniya Das <quic_tdas@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Bjorn Andersson <andersson@...nel.org>,
        Michael Turquette
	<mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Ajit Pandey <quic_ajipan@...cinc.com>,
        Imran Shaik
	<quic_imrashai@...cinc.com>,
        Jagadeesh Kona <quic_jkona@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/4] clk: qcom: gcc: Add support for QCS615 GCC clocks



On 10/19/2024 12:16 AM, Dmitry Baryshkov wrote:
> On Fri, Oct 18, 2024 at 11:14:00PM +0530, Taniya Das wrote:
>>
>>
>> On 10/16/2024 3:46 PM, Dmitry Baryshkov wrote:
>>> On Wed, Oct 16, 2024 at 09:40:07AM +0530, Taniya Das wrote:
>>>>
>>>>
>>>> On 9/20/2024 4:33 PM, Dmitry Baryshkov wrote:
>>>>> On Fri, Sep 20, 2024 at 04:08:18PM GMT, Taniya Das wrote:
>>>>>> Add the global clock controller support for QCS615 SoC.
>>>>>>
>>>>>> Signed-off-by: Taniya Das <quic_tdas@...cinc.com>
>>>>>> ---
>>>>>>     drivers/clk/qcom/Kconfig      |    9 +
>>>>>>     drivers/clk/qcom/Makefile     |    1 +
>>>>>>     drivers/clk/qcom/gcc-qcs615.c | 3035 +++++++++++++++++++++++++++++++++++++++++
>>>>>>     3 files changed, 3045 insertions(+)
>>
>>>>>> +};
>>>>>> +
>>>>>> +static struct clk_alpha_pll gpll0 = {
>>>>>> +	.offset = 0x0,
>>>>>> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>>>>> +	.clkr = {
>>>>>> +		.enable_reg = 0x52000,
>>>>>> +		.enable_mask = BIT(0),
>>>>>> +		.hw.init = &(const struct clk_init_data) {
>>>>>> +			.name = "gpll0",
>>>>>> +			.parent_data = &(const struct clk_parent_data) {
>>>>>> +				.index = DT_BI_TCXO,
>>>>>> +			},
>>>>>> +			.num_parents = 1,
>>>>>> +			.ops = &clk_alpha_pll_ops,
>>>>>> +		},
>>>>>> +	},
>>>>>> +};
>>>>>> +
>>>>>> +static struct clk_fixed_factor gpll0_out_aux2_div = {
>>>>>> +	.mult = 1,
>>>>>> +	.div = 2,
>>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>>> +		.name = "gpll0_out_aux2_div",
>>>>>> +		.parent_data = &(const struct clk_parent_data) {
>>>>>> +			.hw = &gpll0.clkr.hw,
>>>>>> +		},
>>>>>> +		.num_parents = 1,
>>>>>> +		.ops = &clk_fixed_factor_ops,
>>>>>> +	},
>>>>>> +};
>>>>>
>>>>> Should it be clk_alpha_pll_postdiv_foo_ops ?
>>>>>
>>>>
>>>> This is not the PLL output, but it is a fixed divider which is placed as
>>>> input to the RCG.
>>>> That is the reason to use the fixed factor.
>>>
>>> Usually OUT_AUX2 is the PLL output, isn't it? Even by its name. See
>>> gcc-qcm2290 / gcc-sm6115 and most of other clock controller drivers,
>>> except gcc-sm6125. Maybe I don't understand the difference between the
>>> two usecases. Is there a difference in the GCC / PLL design?
>>>
>>
>> Yes, your understanding is correct out_aux2/out_main are the PLL leaf
>> outputs. But on QCS615 the PLL dividers are not used and thus the aux2 and
>> the other leaf outputs are at the same frequency as the main output of the
>> VCO and instead there was a fixed divider placed after the PLL to divide the
>> VCO output. There was a GCC design change required to meet timing closures.
> 
> Ack, please add a comment that this PLL uses fixed divider instead of
> a normal postdiv (and to other out_aux2 clocks too).
> 

Thank you. Sure, will update the comment in the next patch set.

>>>>>
>>>>>> +
>>>>>> +static struct clk_branch gcc_pcie_0_pipe_clk = {
>>>>>> +	.halt_reg = 0x6b024,
>>>>>> +	.halt_check = BRANCH_HALT_SKIP,
>>>>>> +	.clkr = {
>>>>>> +		.enable_reg = 0x5200c,
>>>>>> +		.enable_mask = BIT(4),
>>>>>> +		.hw.init = &(const struct clk_init_data) {
>>>>>> +			.name = "gcc_pcie_0_pipe_clk",
>>>>>> +			.ops = &clk_branch2_ops,
>>>>>> +		},
>>>>>> +	},
>>>>>> +};
>>>>>
>>>>> No corresponding gcc_pcie_0_pipe_clk_src?
>>>>>
>>>>
>>>> On QCS615 the pipe clock source is not required to be modelled as the mux is
>>>> default Power on reset is set to external pipe clock.
>>>
>>> And do we need to toggle the source of the clk_src together with the
>>> GDSC toggling?
>>>
>>
>> AFAIR, QCS615 didn't require toggling for GDSC, as even on downstream kernel
>> we do not have the pipe_clk_src modelled in our driver.
> 
> OK, thanks for the explanation.
> 

-- 
Thanks & Regards,
Taniya Das.