Re: [PATCH V4 2/3] clk: qcom: videocc-sm8450: Add video clock controller driver for SM8450

[Dmitry Baryshkov]

On Fri, 19 May 2023 at 13:53, Taniya Das <quic_tdas@xxxxxxxxxxx> wrote:
>
> Hello Dmitry,
>
> Thank you for your review.
>
> On 5/10/2023 2:03 AM, Dmitry Baryshkov wrote:
> > On Tue, 9 May 2023 at 20:22, Taniya Das <quic_tdas@xxxxxxxxxxx> wrote:
> >>
> >> Add support for the video clock controller driver for peripheral clock
> >> clients to be able to request for video cc clocks.
> >>
> >> Signed-off-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
> >> ---
> >> Changes since V3:
> >>   - Use lower case hex.
> >>   - Check the return value here and bail out early on failure in probe.
> >>
> >> Changes since V2:
> >>   - Update the header file name to match the latest upstream header
> >>     files.
> >>
> >> Changes since V1:
> >>   - Use DT indices instead of fw_name.
> >>   - Replace pm_runtime_enable with devm_pm_runtime_enable.
> >>   - Change license to GPL from GPL V2.
> >>
> >>   drivers/clk/qcom/Kconfig          |   9 +
> >>   drivers/clk/qcom/Makefile         |   1 +
> >>   drivers/clk/qcom/videocc-sm8450.c | 461 ++++++++++++++++++++++++++++++
> >>   3 files changed, 471 insertions(+)
> >>   create mode 100644 drivers/clk/qcom/videocc-sm8450.c
> >
> > [skipped]
> >
> >
> >> +static const struct qcom_reset_map video_cc_sm8450_resets[] = {
> >> +       [CVP_VIDEO_CC_INTERFACE_BCR] = { 0x80e0 },
> >> +       [CVP_VIDEO_CC_MVS0_BCR] = { 0x8098 },
> >> +       [CVP_VIDEO_CC_MVS0C_BCR] = { 0x8048 },
> >> +       [CVP_VIDEO_CC_MVS1_BCR] = { 0x80bc },
> >> +       [CVP_VIDEO_CC_MVS1C_BCR] = { 0x8070 },
> >
> > Can we have a common VIDEO_CC prefix here please?
>
> The BCR names are coming from hardware plan and software interface, thus
> we would like to keep them intact.

We have had a similar discussion on the sm8350-videocc driver. While
keeping the hardware names is nice, name uniformity also should not be
neglected. It is much easier to follow and verify the patches if all
videocc resets start with VIDEO_CC name.

>
>
> >
> >> +       [VIDEO_CC_MVS0C_CLK_ARES] = { 0x8064, 2 },
> >> +       [VIDEO_CC_MVS1C_CLK_ARES] = { 0x808c, 2 },
> >> +};
> >> +
>
> The ARES resets are coming from VideoCC clocks(CBCR), hence the name
> starts with VIDEO_CC.
>
> >> +static const struct regmap_config video_cc_sm8450_regmap_config = {
> >> +       .reg_bits = 32,
> >> +       .reg_stride = 4,
> >> +       .val_bits = 32,
> >> +       .max_register = 0x9f4c,
> >> +       .fast_io = true,
> >> +};
> >> +
> >> +static struct qcom_cc_desc video_cc_sm8450_desc = {
> >> +       .config = &video_cc_sm8450_regmap_config,
> >> +       .clks = video_cc_sm8450_clocks,
> >> +       .num_clks = ARRAY_SIZE(video_cc_sm8450_clocks),
> >> +       .resets = video_cc_sm8450_resets,
> >> +       .num_resets = ARRAY_SIZE(video_cc_sm8450_resets),
> >> +       .gdscs = video_cc_sm8450_gdscs,
> >> +       .num_gdscs = ARRAY_SIZE(video_cc_sm8450_gdscs),
> >> +};
> >> +
> >> +static const struct of_device_id video_cc_sm8450_match_table[] = {
> >> +       { .compatible = "qcom,sm8450-videocc" },
> >> +       { }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, video_cc_sm8450_match_table);
> >> +
> >> +static int video_cc_sm8450_probe(struct platform_device *pdev)
> >> +{
> >> +       struct regmap *regmap;
> >> +       int ret;
> >> +
> >> +       ret = devm_pm_runtime_enable(&pdev->dev);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       ret = pm_runtime_resume_and_get(&pdev->dev);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       regmap = qcom_cc_map(pdev, &video_cc_sm8450_desc);
> >> +       if (IS_ERR(regmap)) {
> >> +               pm_runtime_put(&pdev->dev);
> >> +               return PTR_ERR(regmap);
> >> +       }
> >> +
> >> +       clk_lucid_evo_pll_configure(&video_cc_pll0, regmap, &video_cc_pll0_config);
> >> +       clk_lucid_evo_pll_configure(&video_cc_pll1, regmap, &video_cc_pll1_config);
> >> +
> >> +       /*
> >> +        * Keep clocks always enabled:
> >> +        *      video_cc_ahb_clk
> >> +        *      video_cc_sleep_clk
> >> +        *      video_cc_xo_clk
> >> +        */
> >> +       regmap_update_bits(regmap, 0x80e4, BIT(0), BIT(0));
> >> +       regmap_update_bits(regmap, 0x8130, BIT(0), BIT(0));
> >> +       regmap_update_bits(regmap, 0x8114, BIT(0), BIT(0));
> >> +
> >> +       ret = qcom_cc_really_probe(pdev, &video_cc_sm8450_desc, regmap);
> >> +
> >> +       pm_runtime_put(&pdev->dev);
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static struct platform_driver video_cc_sm8450_driver = {
> >> +       .probe = video_cc_sm8450_probe,
> >> +       .driver = {
> >> +               .name = "video_cc-sm8450",
> >> +               .of_match_table = video_cc_sm8450_match_table,
> >> +       },
> >> +};
> >> +
> >> +static int __init video_cc_sm8450_init(void)
> >> +{
> >> +       return platform_driver_register(&video_cc_sm8450_driver);
> >> +}
> >> +subsys_initcall(video_cc_sm8450_init);
> >> +
> >> +static void __exit video_cc_sm8450_exit(void)
> >> +{
> >> +       platform_driver_unregister(&video_cc_sm8450_driver);
> >> +}
> >> +module_exit(video_cc_sm8450_exit);
> >
> > module_platform_driver() ?
> >
>
> We would like to keep the clock drivers all probed at subsys_initcall.
> We could revisit and update as cleanup if we want to move them to module
> init.

Any particular reason?

>
> >> +
> >> +MODULE_DESCRIPTION("QTI VIDEO_CC SM8450 Driver");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.17.1
> >>
> >
> >
>
> --
> Thanks & Regards,
> Taniya Das.



-- 
With best wishes
Dmitry