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

Message-ID: <7bumydtmwbd7tecurxioqqzw4xj4rkm6mpm527fpwgetai5xzh@rdekj226xfbr>
Date: Fri, 18 Oct 2024 21:46:29 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Taniya Das <quic_tdas@...cinc.com>
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 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).

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

-- 
With best wishes
Dmitry