Re: [PATCH 7/7] clk: qcom: Add GPUCC driver support for SM4450

[Ajit Pandey]

On 4/3/2024 4:22 PM, Dmitry Baryshkov wrote:
> On Wed, 3 Apr 2024 at 13:49, Ajit Pandey <quic_ajipan@xxxxxxxxxxx> wrote:
> > On 4/3/2024 12:53 AM, Dmitry Baryshkov wrote:
> > > On Tue, 2 Apr 2024 at 21:26, Ajit Pandey <quic_ajipan@xxxxxxxxxxx> wrote:
> > > > On 3/31/2024 7:09 AM, Dmitry Baryshkov wrote:
> > > > > On Sat, 30 Mar 2024 at 20:30, Ajit Pandey <quic_ajipan@xxxxxxxxxxx> wrote:
> > > > > > Add Graphics Clock Controller (GPUCC) support for SM4450 platform.
> > > > > >
> > > > > > Signed-off-by: Ajit Pandey <quic_ajipan@xxxxxxxxxxx>
> > > > > > ---
> > > > > >     drivers/clk/qcom/Kconfig        |   9 +
> > > > > >     drivers/clk/qcom/Makefile       |   1 +
> > > > > >     drivers/clk/qcom/gpucc-sm4450.c | 806 ++++++++++++++++++++++++++++++++
> > > > > >     3 files changed, 816 insertions(+)
> > > > > >     create mode 100644 drivers/clk/qcom/gpucc-sm4450.c
> > > > >
> > > > > [skipped]
> > > > >
> > > > > > +static int gpu_cc_sm4450_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +       struct regmap *regmap;
> > > > > > +
> > > > > > +       regmap = qcom_cc_map(pdev, &gpu_cc_sm4450_desc);
> > > > > > +       if (IS_ERR(regmap))
> > > > > > +               return PTR_ERR(regmap);
> > > > > > +
> > > > > > +       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);
> > > > > > +
> > > > > > +       /* Keep some clocks always enabled */
> > > > > > +       qcom_branch_set_clk_en(regmap, 0x93a4); /* GPU_CC_CB_CLK */
> > > > > > +       qcom_branch_set_clk_en(regmap, 0x9004); /* GPU_CC_CXO_AON_CLK */
> > > > > > +       qcom_branch_set_clk_en(regmap, 0x900c); /* GPU_CC_DEMET_CLK */
> > > > >
> > > > > Why? At least other drivers model these three clocks properly.
> > > > These clocks are POR on in SM4450 and required to be kept always enabled
> > > > for GPU functionality hence keep them enabled from probe only.
> > >
> > > Please, check how this is handled on the other platforms, please.
> > > Hint: `git grep GPU_CC_DEMET_CLK`
> > yeah these clocks are modeled and handled via always enabled clk ops
> > (clk_branch2_aon_ops) in few other platforms like SM8450, SM8650 which
> > also do same functionality and keep them in always enabled POR state,
> > while we kept them enabled from GPUCC probe in SM8550.
> > Since we need such clock to be always enabled irrespective of consumer
> > votes I guess modeling with aon_ops isn't really required and we can
> > simply keep them enabled in probe similar to other always on clocks.
>
> Why are they required to be kept on even if there is no consumer?
We've GPU as consumer for these clocks but it's recommended to keep them
always enabled for basic GPU functionality as per HW recommendation.
Hence instead of modeling with _aon_ops we can simply keep them enabled 
all time from probe to avoid any issues.
> > > > > > +
> > > > > > +       return qcom_cc_really_probe(pdev, &gpu_cc_sm4450_desc, regmap);
> > > > > > +}
> > > > > > +
> > > > > > +static struct platform_driver gpu_cc_sm4450_driver = {
> > > > > > +       .probe = gpu_cc_sm4450_probe,
> > > > > > +       .driver = {
> > > > > > +               .name = "gpucc-sm4450",
> > > > > > +               .of_match_table = gpu_cc_sm4450_match_table,
> > > > > > +       },
> > > > > > +};
> > > > > > +
> > > > > > +module_platform_driver(gpu_cc_sm4450_driver);
> > > > > > +
> > > > > > +MODULE_DESCRIPTION("QTI GPUCC SM4450 Driver");
> > > > > > +MODULE_LICENSE("GPL");
> > > > > > --
> > > > > > 2.25.1
> > > >
> > > > --
> > > > Thanks, and Regards
> > > > Ajit
> >
> > --
> > Thanks, and Regards
> > Ajit

--
Thanks, and Regards
Ajit