| Message ID | 20241018-qcs8300-mm-patches-v1-2-859095e0776c@quicinc.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
| Series | Add support for GPUCC, CAMCC and VIDEOCC on Qualcomm QCS8300 platform | expand |
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@quicinc.com> > --- > drivers/clk/qcom/gpucc-sa8775p.c | 47 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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@quicinc.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. Best regards, Krzysztof
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@quicinc.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.
On 21/10/2024 12:56, Dmitry Baryshkov wrote: >>> { } >>> }; >>> @@ -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 I don't understand how it could not scale. That's the entire point of match data - scaling. > 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 You have one driver, where are these tunings which are supposed to be different? You need here only enum or define, in the simplest choice. > point of view, this approach provides a nice balance between having a > completely duplicate driver and having a driver which self-patches the > tree. How duplicate driver got into this? I don't think we talk about the same. I meant ID table match data. > Best regards, Krzysztof
On 10/21/2024 8:41 PM, Krzysztof Kozlowski wrote: > On 21/10/2024 12:56, Dmitry Baryshkov wrote: >>>> { } >>>> }; >>>> @@ -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 > > I don't understand how it could not scale. That's the entire point of > match data - scaling. > >> 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 > > > You have one driver, where are these tunings which are supposed to be > different? You need here only enum or define, in the simplest choice. > >> point of view, this approach provides a nice balance between having a >> completely duplicate driver and having a driver which self-patches the >> tree. > > How duplicate driver got into this? I don't think we talk about the > same. I meant ID table match data. >> I agree with Dmitry. If I understand correctly, to add match data support, we need to define the gpu_cc_qcs8300_clocks struct by duplicating the entries from gpu_cc_sa8775p_clocks and then adding the additional qcs8300 clocks. The compatible approach is simpler and used across most existing platforms. Thanks, Imran > > Best regards, > Krzysztof >
On Mon, 21 Oct 2024 at 18:11, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 21/10/2024 12:56, Dmitry Baryshkov wrote: > >>> { } > >>> }; > >>> @@ -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 > > I don't understand how it could not scale. That's the entire point of > match data - scaling. > > > 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 > > > You have one driver, where are these tunings which are supposed to be > different? You need here only enum or define, in the simplest choice. I think adding enum / define just for the sake of the match data is an overkill. The driver checks for the compatible, tunes clock tree, registers it and then it's done. There is no scalability issue - IOW there are not so many compatible strings being handled by the driver, the is_compatible check is limited to a single point, etc. > > > point of view, this approach provides a nice balance between having a > > completely duplicate driver and having a driver which self-patches the > > tree. > > How duplicate driver got into this? I don't think we talk about the > same. I meant ID table match data. > > > > Best regards, > Krzysztof >
On 22/10/2024 08:34, Imran Shaik wrote: > > > On 10/21/2024 8:41 PM, Krzysztof Kozlowski wrote: >> On 21/10/2024 12:56, Dmitry Baryshkov wrote: >>>>> { } >>>>> }; >>>>> @@ -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 >> >> I don't understand how it could not scale. That's the entire point of >> match data - scaling. >> >>> 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 >> >> >> You have one driver, where are these tunings which are supposed to be >> different? You need here only enum or define, in the simplest choice. >> >>> point of view, this approach provides a nice balance between having a >>> completely duplicate driver and having a driver which self-patches the >>> tree. >> >> How duplicate driver got into this? I don't think we talk about the >> same. I meant ID table match data. >>> > > I agree with Dmitry. If I understand correctly, to add match data > support, we need to define the gpu_cc_qcs8300_clocks struct by > duplicating the entries from gpu_cc_sa8775p_clocks and then adding the > additional qcs8300 clocks. The compatible approach is simpler and used > across most existing platforms. > You don't have to define any structs. You pass enum and retrieve it... Best regards, Krzysztof
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" }, { } }; @@ -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")) { + gpu_cc_pll0_config.l = 0x31; + gpu_cc_pll0_config.alpha = 0xe555; + + gpu_cc_sa8775p_clocks[GPU_CC_CX_ACCU_SHIFT_CLK] = &gpu_cc_cx_accu_shift_clk.clkr; + gpu_cc_sa8775p_clocks[GPU_CC_GX_ACCU_SHIFT_CLK] = &gpu_cc_gx_accu_shift_clk.clkr; + } + clk_lucid_evo_pll_configure(&gpu_cc_pll0, regmap, &gpu_cc_pll0_config); clk_lucid_evo_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll1_config);
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@quicinc.com> --- drivers/clk/qcom/gpucc-sa8775p.c | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)