linux-kernel - Re: [PATCH 2/6] clk: qcom: Add support for GPU Clock Controller on QCS8300

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: <o5v3fch5oxol4t7j4xlqswk6m6uo4tleck2cnfk6whpfqsrvjc@s2yrjumgvw6j>
Date: Mon, 21 Oct 2024 13:56:59 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Imran Shaik <quic_imrashai@...cinc.com>, 
	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>, Taniya Das <quic_tdas@...cinc.com>, 
	Jagadeesh Kona <quic_jkona@...cinc.com>, Satya Priya Kakitapalli <quic_skakitap@...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 2/6] clk: qcom: Add support for GPU Clock Controller on
 QCS8300

On Mon, Oct 21, 2024 at 09:56:08AM +0200, Krzysztof Kozlowski wrote:
> On Fri, Oct 18, 2024 at 04:42:30PM +0530, Imran Shaik wrote:
> > Add support to the QCS8300 GPU clock controller by extending
> > the SA8775P GPU clock controller, which is mostly identical
> > but QCS8300 has few additional clocks and minor differences.
> > 
> > Signed-off-by: Imran Shaik <quic_imrashai@...cinc.com>
> > ---
> >  drivers/clk/qcom/gpucc-sa8775p.c | 47 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/gpucc-sa8775p.c b/drivers/clk/qcom/gpucc-sa8775p.c
> > index f8a8ac343d70..99a8344b00db 100644
> > --- a/drivers/clk/qcom/gpucc-sa8775p.c
> > +++ b/drivers/clk/qcom/gpucc-sa8775p.c
> > @@ -317,6 +317,24 @@ static struct clk_branch gpu_cc_crc_ahb_clk = {
> >  	},
> >  };
> >  
> > +static struct clk_branch gpu_cc_cx_accu_shift_clk = {
> > +	.halt_reg = 0x95e8,
> > +	.halt_check = BRANCH_HALT,
> > +	.clkr = {
> > +		.enable_reg = 0x95e8,
> > +		.enable_mask = BIT(0),
> > +		.hw.init = &(const struct clk_init_data){
> > +			.name = "gpu_cc_cx_accu_shift_clk",
> > +			.parent_hws = (const struct clk_hw*[]){
> > +				&gpu_cc_xo_clk_src.clkr.hw,
> > +			},
> > +			.num_parents = 1,
> > +			.flags = CLK_SET_RATE_PARENT,
> > +			.ops = &clk_branch2_ops,
> > +		},
> > +	},
> > +};
> > +
> >  static struct clk_branch gpu_cc_cx_ff_clk = {
> >  	.halt_reg = 0x914c,
> >  	.halt_check = BRANCH_HALT,
> > @@ -420,6 +438,24 @@ static struct clk_branch gpu_cc_demet_clk = {
> >  	},
> >  };
> >  
> > +static struct clk_branch gpu_cc_gx_accu_shift_clk = {
> > +	.halt_reg = 0x95e4,
> > +	.halt_check = BRANCH_HALT,
> > +	.clkr = {
> > +		.enable_reg = 0x95e4,
> > +		.enable_mask = BIT(0),
> > +		.hw.init = &(const struct clk_init_data){
> > +			.name = "gpu_cc_gx_accu_shift_clk",
> > +			.parent_hws = (const struct clk_hw*[]){
> > +				&gpu_cc_xo_clk_src.clkr.hw,
> > +			},
> > +			.num_parents = 1,
> > +			.flags = CLK_SET_RATE_PARENT,
> > +			.ops = &clk_branch2_ops,
> > +		},
> > +	},
> > +};
> > +
> >  static struct clk_branch gpu_cc_hlos1_vote_gpu_smmu_clk = {
> >  	.halt_reg = 0x7000,
> >  	.halt_check = BRANCH_HALT_VOTED,
> > @@ -499,6 +535,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
> >  	[GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr,
> >  	[GPU_CC_CB_CLK] = &gpu_cc_cb_clk.clkr,
> >  	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,
> > +	[GPU_CC_CX_ACCU_SHIFT_CLK] = NULL,
> >  	[GPU_CC_CX_FF_CLK] = &gpu_cc_cx_ff_clk.clkr,
> >  	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
> >  	[GPU_CC_CX_SNOC_DVM_CLK] = &gpu_cc_cx_snoc_dvm_clk.clkr,
> > @@ -508,6 +545,7 @@ static struct clk_regmap *gpu_cc_sa8775p_clocks[] = {
> >  	[GPU_CC_DEMET_DIV_CLK_SRC] = &gpu_cc_demet_div_clk_src.clkr,
> >  	[GPU_CC_FF_CLK_SRC] = &gpu_cc_ff_clk_src.clkr,
> >  	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
> > +	[GPU_CC_GX_ACCU_SHIFT_CLK] = NULL,
> >  	[GPU_CC_HLOS1_VOTE_GPU_SMMU_CLK] = &gpu_cc_hlos1_vote_gpu_smmu_clk.clkr,
> >  	[GPU_CC_HUB_AHB_DIV_CLK_SRC] = &gpu_cc_hub_ahb_div_clk_src.clkr,
> >  	[GPU_CC_HUB_AON_CLK] = &gpu_cc_hub_aon_clk.clkr,
> > @@ -583,6 +621,7 @@ static const struct qcom_cc_desc gpu_cc_sa8775p_desc = {
> >  };
> >  
> >  static const struct of_device_id gpu_cc_sa8775p_match_table[] = {
> > +	{ .compatible = "qcom,qcs8300-gpucc" },
> >  	{ .compatible = "qcom,sa8775p-gpucc" },
> 
> I just wanted to comment on your binding that devices should be made
> compatible...
> 
> >  	{ }
> >  };
> > @@ -596,6 +635,14 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
> >  	if (IS_ERR(regmap))
> >  		return PTR_ERR(regmap);
> >  
> > +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,qcs8300-gpucc")) {
> 
> Why we cannot use match data? Seeing compatibles in the code is
> unexpected and does not scale.

Because using match data doesn't scale in such cases. We have been using
compatibles to patch clock trees for the platforms for quite a while.
You can see that each of the "tunings" is slightly different. From my
point of view, this approach provides a nice balance between having a
completely duplicate driver and having a driver which self-patches the
tree.

-- 
With best wishes
Dmitry