| Message ID | 20230413-topic-lahaina_vidcc-v2-2-f721d507e555@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SM8350 VIDEOCC | expand |
On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 14.04.2023 18:31, Dmitry Baryshkov wrote: > > On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >> > >> Add support for the Video Clock Controller found on the SM8350 SoC. > >> > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >> --- > > [...] > > >> +static struct clk_rcg2 video_cc_ahb_clk_src = { > >> + .cmd_rcgr = 0xbd4, > >> + .mnd_width = 0, > >> + .hid_width = 5, > >> + .parent_map = video_cc_parent_map_0, > >> + .freq_tbl = ftbl_video_cc_ahb_clk_src, > >> + .clkr.hw.init = &(const struct clk_init_data) { > >> + .name = "video_cc_ahb_clk_src", > >> + .parent_data = video_cc_parent_data_0, > >> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), > >> + .flags = CLK_SET_RATE_PARENT, > >> + .ops = &clk_rcg2_shared_ops, > >> + }, > >> +}; > > > > Do we need this clock at all? We don't have the child > > video_cc_ahb_clk, so potentially CCF can try disabling or modifying > > this clock. > Hm.. I see a few things: > > 1. downstream kona has it, upstream does not > 2. it's shared so we never actually hard-shut it off.. > 2a. ..but it'd be good to ensure it's on when it's ready.. > 2b. ..but we never do anyway.. > 2c. ..but should we even? doesn't Venus govern it internally? > > > > > >> + > >> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = { > >> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >> + { } > >> +}; > >> + > > [...] > > >> +static struct clk_branch video_cc_mvs1_clk = { > >> + .halt_reg = 0xdb4, > >> + .halt_check = BRANCH_HALT_VOTED, > > > > As a note, sm8250 has BRANCH_HALT here. > No, it does on the div2 clk, and so do we: Excuse me, I got confused by all the syllables. I was looking at the video_cc_mvs1c_clk. On sm8250 it is _VOTED, in this patch it is not. I can not say that either one of those is incorrect, but such a difference looks a bit suspicious for me. Maybe Tanya or somebody else can comment here. > [...] > > >> +}; > >> + > >> +static struct clk_branch video_cc_mvs1_div2_clk = { > >> + .halt_reg = 0xdf4, > >> + .halt_check = BRANCH_HALT_VOTED, > >> + .hwcg_reg = 0xdf4, > > [...] > > >> + > >> +static const struct qcom_reset_map video_cc_sm8350_resets[] = { > >> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 }, > >> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 }, > > > > Would it be better to use common VIDEO_CC prefix here (IOW: > > VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc. > My best guess would be that the ones prefixed with CVP_ > are actual INTF/INSTANCEn(CORE) reset lines whereas > the ones containing _CLK_ reset their clock sub-branches. Note, again, on sm8250 all resets start with VIDEO_CC, even CVP ones. I think we can follow that. > > > > >> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 }, > >> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 }, > >> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 }, > >> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 }, > >> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 }, > >> +}; > > [...] > > >> + ret = pm_runtime_resume_and_get(&pdev->dev); > >> + if (ret) > >> + return ret; > >> + > >> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc); > >> + if (IS_ERR(regmap)) { > >> + pm_runtime_put(&pdev->dev); > >> + return PTR_ERR(regmap); > >> + }; > > > > Extra semicolon > Ooeh! > > > > >> + > >> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config); > >> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config); > >> + > >> + /* > >> + * Keep clocks always enabled: > >> + * video_cc_ahb_clk > >> + * video_cc_xo_clk > >> + */ > >> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0)); > >> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0)); > >> + > >> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap); > >> + pm_runtime_put(&pdev->dev); > >> + > >> + return ret; > >> +} > >> + > >> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = { > >> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL) > > > > The driver doesn't use pm_clk at all. Are these PM_OPS correct? > I'm unsure. I see the pm state changing in debugfs when the clocks are > (not) consumed. But let's continue our discussion about using pm_clks > for AHB. Well, those are two separate questions. One is that w/o additional pm_clk calls this string is useless (and should be removed). Another on is a possible restructure of our cc drivers to use pm_clk for AHB clocks (which would require adding more than that). > > > > >> +}; > >> + > >> +static const struct of_device_id video_cc_sm8350_match_table[] = { > >> + { .compatible = "qcom,sm8350-videocc" }, > >> + { } > >> +}; > >> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table); > >> + > >> +static struct platform_driver video_cc_sm8350_driver = { > >> + .probe = video_cc_sm8350_probe, > >> + .driver = { > >> + .name = "sm8350-videocc", > >> + .of_match_table = video_cc_sm8350_match_table, > >> + .pm = &video_cc_sm8350_pm_ops, > >> + }, > >> +}; > >> +module_platform_driver(video_cc_sm8350_driver); > >> + > >> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver"); > >> +MODULE_LICENSE("GPL"); > >> > >> -- > >> 2.40.0 > >> > > > > Generic note: the register layout follows closely sm8250. However the > > existing differences probably do not warrant merging them. > No, I don't think merging any designs that are farther away > than 8150 and 8155 or 8992 and 8994 etc. is a good idea.. > > I don't want to ever look at something like dispcc-sm8[123]50.c > again! Me too!
On 14.04.2023 22:54, Dmitry Baryshkov wrote: > On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> >> >> On 14.04.2023 18:31, Dmitry Baryshkov wrote: >>> On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >>>> >>>> Add support for the Video Clock Controller found on the SM8350 SoC. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >> >> [...] >> >>>> +static struct clk_rcg2 video_cc_ahb_clk_src = { >>>> + .cmd_rcgr = 0xbd4, >>>> + .mnd_width = 0, >>>> + .hid_width = 5, >>>> + .parent_map = video_cc_parent_map_0, >>>> + .freq_tbl = ftbl_video_cc_ahb_clk_src, >>>> + .clkr.hw.init = &(const struct clk_init_data) { >>>> + .name = "video_cc_ahb_clk_src", >>>> + .parent_data = video_cc_parent_data_0, >>>> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), >>>> + .flags = CLK_SET_RATE_PARENT, >>>> + .ops = &clk_rcg2_shared_ops, >>>> + }, >>>> +}; >>> >>> Do we need this clock at all? We don't have the child >>> video_cc_ahb_clk, so potentially CCF can try disabling or modifying >>> this clock. >> Hm.. I see a few things: >> >> 1. downstream kona has it, upstream does not >> 2. it's shared so we never actually hard-shut it off.. >> 2a. ..but it'd be good to ensure it's on when it's ready.. >> 2b. ..but we never do anyway.. >> 2c. ..but should we even? doesn't Venus govern it internally? >> >> >>> >>>> + >>>> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = { >>>> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >>>> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >>>> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >>>> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >>>> + { } >>>> +}; >>>> + >> >> [...] >> >>>> +static struct clk_branch video_cc_mvs1_clk = { >>>> + .halt_reg = 0xdb4, >>>> + .halt_check = BRANCH_HALT_VOTED, >>> >>> As a note, sm8250 has BRANCH_HALT here. >> No, it does on the div2 clk, and so do we: > > Excuse me, I got confused by all the syllables. I was looking at the > video_cc_mvs1c_clk. On sm8250 it is _VOTED, in this patch it is not. I > can not say that either one of those is incorrect, but such a > difference looks a bit suspicious for me. Maybe Tanya or somebody else > can comment here. I'd say this could be a design decision, with div2 clocks being treated differently, but it's how downstream does it on shipping devices and while generally it's not a great thing to say, it seems to be the "right enough" thing.. > >> [...] >> >>>> +}; >>>> + >>>> +static struct clk_branch video_cc_mvs1_div2_clk = { >>>> + .halt_reg = 0xdf4, >>>> + .halt_check = BRANCH_HALT_VOTED, >>>> + .hwcg_reg = 0xdf4, >> >> [...] >> >>>> + >>>> +static const struct qcom_reset_map video_cc_sm8350_resets[] = { >>>> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 }, >>>> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 }, >>> >>> Would it be better to use common VIDEO_CC prefix here (IOW: >>> VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc. >> My best guess would be that the ones prefixed with CVP_ >> are actual INTF/INSTANCEn(CORE) reset lines whereas >> the ones containing _CLK_ reset their clock sub-branches. > > Note, again, on sm8250 all resets start with VIDEO_CC, even CVP ones. > I think we can follow that. Or get rid of that, as it's always called with a phandle to videocc.. Thoughts? > >> >>> >>>> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 }, >>>> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 }, >>>> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 }, >>>> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 }, >>>> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 }, >>>> +}; >> >> [...] >> >>>> + ret = pm_runtime_resume_and_get(&pdev->dev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc); >>>> + if (IS_ERR(regmap)) { >>>> + pm_runtime_put(&pdev->dev); >>>> + return PTR_ERR(regmap); >>>> + }; >>> >>> Extra semicolon >> Ooeh! >> >>> >>>> + >>>> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config); >>>> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config); >>>> + >>>> + /* >>>> + * Keep clocks always enabled: >>>> + * video_cc_ahb_clk >>>> + * video_cc_xo_clk >>>> + */ >>>> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0)); >>>> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0)); >>>> + >>>> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap); >>>> + pm_runtime_put(&pdev->dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = { >>>> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL) >>> >>> The driver doesn't use pm_clk at all. Are these PM_OPS correct? >> I'm unsure. I see the pm state changing in debugfs when the clocks are >> (not) consumed. But let's continue our discussion about using pm_clks >> for AHB. > > Well, those are two separate questions. One is that w/o additional > pm_clk calls this string is useless (and should be removed). Another > on is a possible restructure of our cc drivers to use pm_clk for AHB > clocks (which would require adding more than that). Right, I had an impression that you needed any sort of pm ops at all to be registered with pm_genpd correctly, but that seems not to be the case.. With that commented out, I still see "suspended" / "active" and not "unsupported".. Konrad > > >> >>> >>>> +}; >>>> + >>>> +static const struct of_device_id video_cc_sm8350_match_table[] = { >>>> + { .compatible = "qcom,sm8350-videocc" }, >>>> + { } >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table); >>>> + >>>> +static struct platform_driver video_cc_sm8350_driver = { >>>> + .probe = video_cc_sm8350_probe, >>>> + .driver = { >>>> + .name = "sm8350-videocc", >>>> + .of_match_table = video_cc_sm8350_match_table, >>>> + .pm = &video_cc_sm8350_pm_ops, >>>> + }, >>>> +}; >>>> +module_platform_driver(video_cc_sm8350_driver); >>>> + >>>> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver"); >>>> +MODULE_LICENSE("GPL"); >>>> >>>> -- >>>> 2.40.0 >>>> >>> >>> Generic note: the register layout follows closely sm8250. However the >>> existing differences probably do not warrant merging them. >> No, I don't think merging any designs that are farther away >> than 8150 and 8155 or 8992 and 8994 etc. is a good idea.. >> >> I don't want to ever look at something like dispcc-sm8[123]50.c >> again! > > Me too! >
On Mon, 17 Apr 2023 at 21:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 14.04.2023 22:54, Dmitry Baryshkov wrote: > > On Fri, 14 Apr 2023 at 20:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >> > >> > >> > >> On 14.04.2023 18:31, Dmitry Baryshkov wrote: > >>> On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >>>> > >>>> Add support for the Video Clock Controller found on the SM8350 SoC. > >>>> > >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>>> --- > >> > >> [...] > >> > >>>> +static struct clk_rcg2 video_cc_ahb_clk_src = { > >>>> + .cmd_rcgr = 0xbd4, > >>>> + .mnd_width = 0, > >>>> + .hid_width = 5, > >>>> + .parent_map = video_cc_parent_map_0, > >>>> + .freq_tbl = ftbl_video_cc_ahb_clk_src, > >>>> + .clkr.hw.init = &(const struct clk_init_data) { > >>>> + .name = "video_cc_ahb_clk_src", > >>>> + .parent_data = video_cc_parent_data_0, > >>>> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), > >>>> + .flags = CLK_SET_RATE_PARENT, > >>>> + .ops = &clk_rcg2_shared_ops, > >>>> + }, > >>>> +}; > >>> > >>> Do we need this clock at all? We don't have the child > >>> video_cc_ahb_clk, so potentially CCF can try disabling or modifying > >>> this clock. > >> Hm.. I see a few things: > >> > >> 1. downstream kona has it, upstream does not > >> 2. it's shared so we never actually hard-shut it off.. > >> 2a. ..but it'd be good to ensure it's on when it's ready.. > >> 2b. ..but we never do anyway.. > >> 2c. ..but should we even? doesn't Venus govern it internally? > >> > >> > >>> > >>>> + > >>>> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = { > >>>> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >>>> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >>>> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >>>> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), > >>>> + { } > >>>> +}; > >>>> + > >> > >> [...] > >> > >>>> +static struct clk_branch video_cc_mvs1_clk = { > >>>> + .halt_reg = 0xdb4, > >>>> + .halt_check = BRANCH_HALT_VOTED, > >>> > >>> As a note, sm8250 has BRANCH_HALT here. > >> No, it does on the div2 clk, and so do we: > > > > Excuse me, I got confused by all the syllables. I was looking at the > > video_cc_mvs1c_clk. On sm8250 it is _VOTED, in this patch it is not. I > > can not say that either one of those is incorrect, but such a > > difference looks a bit suspicious for me. Maybe Tanya or somebody else > > can comment here. > I'd say this could be a design decision, with div2 clocks being > treated differently, but it's how downstream does it on shipping > devices and while generally it's not a great thing to say, it seems > to be the "right enough" thing.. Ack. Fair enough. > > > > >> [...] > >> > >>>> +}; > >>>> + > >>>> +static struct clk_branch video_cc_mvs1_div2_clk = { > >>>> + .halt_reg = 0xdf4, > >>>> + .halt_check = BRANCH_HALT_VOTED, > >>>> + .hwcg_reg = 0xdf4, > >> > >> [...] > >> > >>>> + > >>>> +static const struct qcom_reset_map video_cc_sm8350_resets[] = { > >>>> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 }, > >>>> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 }, > >>> > >>> Would it be better to use common VIDEO_CC prefix here (IOW: > >>> VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc. > >> My best guess would be that the ones prefixed with CVP_ > >> are actual INTF/INSTANCEn(CORE) reset lines whereas > >> the ones containing _CLK_ reset their clock sub-branches. > > > > Note, again, on sm8250 all resets start with VIDEO_CC, even CVP ones. > > I think we can follow that. > Or get rid of that, as it's always called with a phandle to videocc.. > > Thoughts? I'd say, switch to VIDEO_CC prefix, please. We can not drop the prefix, as we risc getting conflicts otherwise. > > > > >> > >>> > >>>> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 }, > >>>> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 }, > >>>> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 }, > >>>> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 }, > >>>> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 }, > >>>> +}; > >> > >> [...] > >> > >>>> + ret = pm_runtime_resume_and_get(&pdev->dev); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc); > >>>> + if (IS_ERR(regmap)) { > >>>> + pm_runtime_put(&pdev->dev); > >>>> + return PTR_ERR(regmap); > >>>> + }; > >>> > >>> Extra semicolon > >> Ooeh! > >> > >>> > >>>> + > >>>> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config); > >>>> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config); > >>>> + > >>>> + /* > >>>> + * Keep clocks always enabled: > >>>> + * video_cc_ahb_clk > >>>> + * video_cc_xo_clk > >>>> + */ > >>>> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0)); > >>>> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0)); > >>>> + > >>>> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap); > >>>> + pm_runtime_put(&pdev->dev); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = { > >>>> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL) > >>> > >>> The driver doesn't use pm_clk at all. Are these PM_OPS correct? > >> I'm unsure. I see the pm state changing in debugfs when the clocks are > >> (not) consumed. But let's continue our discussion about using pm_clks > >> for AHB. > > > > Well, those are two separate questions. One is that w/o additional > > pm_clk calls this string is useless (and should be removed). Another > > on is a possible restructure of our cc drivers to use pm_clk for AHB > > clocks (which would require adding more than that). > Right, I had an impression that you needed any sort of pm ops at > all to be registered with pm_genpd correctly, but that seems not to > be the case.. With that commented out, I still see "suspended" / "active" > and not "unsupported".. Let's just drop them for now. > > Konrad > > > > > >> > >>> > >>>> +}; > >>>> + > >>>> +static const struct of_device_id video_cc_sm8350_match_table[] = { > >>>> + { .compatible = "qcom,sm8350-videocc" }, > >>>> + { } > >>>> +}; > >>>> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table); > >>>> + > >>>> +static struct platform_driver video_cc_sm8350_driver = { > >>>> + .probe = video_cc_sm8350_probe, > >>>> + .driver = { > >>>> + .name = "sm8350-videocc", > >>>> + .of_match_table = video_cc_sm8350_match_table, > >>>> + .pm = &video_cc_sm8350_pm_ops, > >>>> + }, > >>>> +}; > >>>> +module_platform_driver(video_cc_sm8350_driver); > >>>> + > >>>> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver"); > >>>> +MODULE_LICENSE("GPL"); > >>>> > >>>> -- > >>>> 2.40.0 > >>>> > >>> > >>> Generic note: the register layout follows closely sm8250. However the > >>> existing differences probably do not warrant merging them. > >> No, I don't think merging any designs that are farther away > >> than 8150 and 8155 or 8992 and 8994 etc. is a good idea.. > >> > >> I don't want to ever look at something like dispcc-sm8[123]50.c > >> again! > > > > Me too! > >
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index d71c9d6036bb..dbb1dfcddb31 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -916,6 +916,15 @@ config SM_VIDEOCC_8250 Say Y if you want to support video devices and functionality such as video encode and decode. +config SM_VIDEOCC_8350 + tristate "SM8350 Video Clock Controller" + select SM_GCC_8350 + select QCOM_GDSC + help + Support for the video clock controller on SM8350 devices. + Say Y if you want to support video devices and functionality such as + video encode and decode. + config SPMI_PMIC_CLKDIV tristate "SPMI PMIC clkdiv Support" depends on SPMI || COMPILE_TEST diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile index b54085e579a0..53290040523b 100644 --- a/drivers/clk/qcom/Makefile +++ b/drivers/clk/qcom/Makefile @@ -126,6 +126,7 @@ obj-$(CONFIG_SM_GPUCC_8350) += gpucc-sm8350.o obj-$(CONFIG_SM_TCSRCC_8550) += tcsrcc-sm8550.o obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o obj-$(CONFIG_SM_VIDEOCC_8250) += videocc-sm8250.o +obj-$(CONFIG_SM_VIDEOCC_8350) += videocc-sm8350.o obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o obj-$(CONFIG_QCOM_HFPLL) += hfpll.o diff --git a/drivers/clk/qcom/videocc-sm8350.c b/drivers/clk/qcom/videocc-sm8350.c new file mode 100644 index 000000000000..e0cf474a632d --- /dev/null +++ b/drivers/clk/qcom/videocc-sm8350.c @@ -0,0 +1,557 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved. + * Copyright (c) 2023, Linaro Limited + */ + +#include <linux/clk-provider.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pm_clock.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> + +#include <dt-bindings/clock/qcom,sm8350-videocc.h> +#include <dt-bindings/reset/qcom,sm8350-videocc.h> + +#include "clk-alpha-pll.h" +#include "clk-branch.h" +#include "clk-rcg.h" +#include "clk-regmap.h" +#include "clk-regmap-divider.h" +#include "common.h" +#include "reset.h" +#include "gdsc.h" + +enum { + DT_BI_TCXO, + DT_BI_TCXO_AO, + DT_SLEEP_CLK, +}; + +enum { + P_BI_TCXO, + P_BI_TCXO_AO, + P_SLEEP_CLK, + P_VIDEO_PLL0_OUT_MAIN, + P_VIDEO_PLL1_OUT_MAIN, +}; + +static const struct pll_vco lucid_5lpe_vco[] = { + { 249600000, 1750000000, 0 }, +}; + +static const struct alpha_pll_config video_pll0_config = { + .l = 0x25, + .alpha = 0x8000, + .config_ctl_val = 0x20485699, + .config_ctl_hi_val = 0x00002261, + .config_ctl_hi1_val = 0x2a9a699c, + .test_ctl_val = 0x00000000, + .test_ctl_hi_val = 0x00000000, + .test_ctl_hi1_val = 0x01800000, + .user_ctl_val = 0x00000000, + .user_ctl_hi_val = 0x00000805, + .user_ctl_hi1_val = 0x00000000, +}; + +static struct clk_alpha_pll video_pll0 = { + .offset = 0x42c, + .vco_table = lucid_5lpe_vco, + .num_vco = ARRAY_SIZE(lucid_5lpe_vco), + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID], + .clkr = { + .hw.init = &(const struct clk_init_data) { + .name = "video_pll0", + .parent_data = &(const struct clk_parent_data){ + .index = DT_BI_TCXO, + }, + .num_parents = 1, + .ops = &clk_alpha_pll_lucid_5lpe_ops, + }, + }, +}; + +static const struct alpha_pll_config video_pll1_config = { + .l = 0x2b, + .alpha = 0xc000, + .config_ctl_val = 0x20485699, + .config_ctl_hi_val = 0x00002261, + .config_ctl_hi1_val = 0x2a9a699c, + .test_ctl_val = 0x00000000, + .test_ctl_hi_val = 0x00000000, + .test_ctl_hi1_val = 0x01800000, + .user_ctl_val = 0x00000000, + .user_ctl_hi_val = 0x00000805, + .user_ctl_hi1_val = 0x00000000, +}; + +static struct clk_alpha_pll video_pll1 = { + .offset = 0x7d0, + .vco_table = lucid_5lpe_vco, + .num_vco = ARRAY_SIZE(lucid_5lpe_vco), + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID], + .clkr = { + .hw.init = &(const struct clk_init_data) { + .name = "video_pll1", + .parent_data = &(const struct clk_parent_data){ + .index = DT_BI_TCXO, + }, + .num_parents = 1, + .ops = &clk_alpha_pll_lucid_5lpe_ops, + }, + }, +}; + +static const struct parent_map video_cc_parent_map_0[] = { + { P_BI_TCXO_AO, 0 }, +}; + +static const struct clk_parent_data video_cc_parent_data_0[] = { + { .index = DT_BI_TCXO_AO }, +}; + +static const struct parent_map video_cc_parent_map_1[] = { + { P_BI_TCXO, 0 }, + { P_VIDEO_PLL0_OUT_MAIN, 1 }, +}; + +static const struct clk_parent_data video_cc_parent_data_1[] = { + { .index = DT_BI_TCXO }, + { .hw = &video_pll0.clkr.hw }, +}; + +static const struct parent_map video_cc_parent_map_2[] = { + { P_BI_TCXO, 0 }, + { P_VIDEO_PLL1_OUT_MAIN, 1 }, +}; + +static const struct clk_parent_data video_cc_parent_data_2[] = { + { .index = DT_BI_TCXO }, + { .hw = &video_pll1.clkr.hw }, +}; + +static const struct freq_tbl ftbl_video_cc_ahb_clk_src[] = { + F(19200000, P_BI_TCXO, 1, 0, 0), + { } +}; + +static struct clk_rcg2 video_cc_ahb_clk_src = { + .cmd_rcgr = 0xbd4, + .mnd_width = 0, + .hid_width = 5, + .parent_map = video_cc_parent_map_0, + .freq_tbl = ftbl_video_cc_ahb_clk_src, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_ahb_clk_src", + .parent_data = video_cc_parent_data_0, + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_rcg2_shared_ops, + }, +}; + +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = { + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), + { } +}; + +static struct clk_rcg2 video_cc_mvs0_clk_src = { + .cmd_rcgr = 0xb94, + .mnd_width = 0, + .hid_width = 5, + .parent_map = video_cc_parent_map_1, + .freq_tbl = ftbl_video_cc_mvs0_clk_src, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs0_clk_src", + .parent_data = video_cc_parent_data_1, + .num_parents = ARRAY_SIZE(video_cc_parent_data_1), + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_rcg2_shared_ops, + }, +}; + +static const struct freq_tbl ftbl_video_cc_mvs1_clk_src[] = { + F(840000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0), + F(1098000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0), + F(1332000000, P_VIDEO_PLL1_OUT_MAIN, 1, 0, 0), + { } +}; + +static struct clk_rcg2 video_cc_mvs1_clk_src = { + .cmd_rcgr = 0xbb4, + .mnd_width = 0, + .hid_width = 5, + .parent_map = video_cc_parent_map_2, + .freq_tbl = ftbl_video_cc_mvs1_clk_src, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs1_clk_src", + .parent_data = video_cc_parent_data_2, + .num_parents = ARRAY_SIZE(video_cc_parent_data_2), + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_rcg2_shared_ops, + }, +}; + +static const struct freq_tbl ftbl_video_cc_sleep_clk_src[] = { + F(32000, P_SLEEP_CLK, 1, 0, 0), + { } +}; + +static struct clk_rcg2 video_cc_sleep_clk_src = { + .cmd_rcgr = 0xef0, + .mnd_width = 0, + .hid_width = 5, + .freq_tbl = ftbl_video_cc_sleep_clk_src, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_sleep_clk_src", + .parent_data = &(const struct clk_parent_data){ + .index = DT_SLEEP_CLK, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_rcg2_ops, + }, +}; + +static struct clk_rcg2 video_cc_xo_clk_src = { + .cmd_rcgr = 0xecc, + .mnd_width = 0, + .hid_width = 5, + .parent_map = video_cc_parent_map_0, + .freq_tbl = ftbl_video_cc_ahb_clk_src, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_xo_clk_src", + .parent_data = video_cc_parent_data_0, + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_rcg2_ops, + }, +}; + +static struct clk_regmap_div video_cc_mvs0_div_clk_src = { + .reg = 0xd54, + .shift = 0, + .width = 4, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs0_div_clk_src", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs0_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_regmap_div_ro_ops, + }, +}; + +static struct clk_regmap_div video_cc_mvs0c_div2_div_clk_src = { + .reg = 0xc54, + .shift = 0, + .width = 4, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs0c_div2_div_clk_src", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs0_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_regmap_div_ro_ops, + }, +}; + +static struct clk_regmap_div video_cc_mvs1_div_clk_src = { + .reg = 0xdd4, + .shift = 0, + .width = 4, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs1_div_clk_src", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs1_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_regmap_div_ro_ops, + }, +}; + +static struct clk_regmap_div video_cc_mvs1c_div2_div_clk_src = { + .reg = 0xcf4, + .shift = 0, + .width = 4, + .clkr.hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs1c_div2_div_clk_src", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs1_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_regmap_div_ro_ops, + }, +}; + +static struct clk_branch video_cc_mvs0_clk = { + .halt_reg = 0xd34, + .halt_check = BRANCH_HALT_VOTED, + .hwcg_reg = 0xd34, + .hwcg_bit = 1, + .clkr = { + .enable_reg = 0xd34, + .enable_mask = BIT(0), + .hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs0_clk", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs0_div_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_branch2_ops, + }, + }, +}; + +static struct clk_branch video_cc_mvs0c_clk = { + .halt_reg = 0xc34, + .halt_check = BRANCH_HALT, + .clkr = { + .enable_reg = 0xc34, + .enable_mask = BIT(0), + .hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs0c_clk", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs0c_div2_div_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_branch2_ops, + }, + }, +}; + +static struct clk_branch video_cc_mvs1_clk = { + .halt_reg = 0xdb4, + .halt_check = BRANCH_HALT_VOTED, + .hwcg_reg = 0xdb4, + .hwcg_bit = 1, + .clkr = { + .enable_reg = 0xdb4, + .enable_mask = BIT(0), + .hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs1_clk", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs1_div_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_branch2_ops, + }, + }, +}; + +static struct clk_branch video_cc_mvs1_div2_clk = { + .halt_reg = 0xdf4, + .halt_check = BRANCH_HALT_VOTED, + .hwcg_reg = 0xdf4, + .hwcg_bit = 1, + .clkr = { + .enable_reg = 0xdf4, + .enable_mask = BIT(0), + .hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs1_div2_clk", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs1c_div2_div_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_branch2_ops, + }, + }, +}; + +static struct clk_branch video_cc_mvs1c_clk = { + .halt_reg = 0xcd4, + .halt_check = BRANCH_HALT, + .clkr = { + .enable_reg = 0xcd4, + .enable_mask = BIT(0), + .hw.init = &(const struct clk_init_data) { + .name = "video_cc_mvs1c_clk", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_mvs1c_div2_div_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_branch2_ops, + }, + }, +}; + +static struct clk_branch video_cc_sleep_clk = { + .halt_reg = 0xf10, + .halt_check = BRANCH_HALT, + .clkr = { + .enable_reg = 0xf10, + .enable_mask = BIT(0), + .hw.init = &(const struct clk_init_data) { + .name = "video_cc_sleep_clk", + .parent_hws = (const struct clk_hw*[]){ + &video_cc_sleep_clk_src.clkr.hw, + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_branch2_ops, + }, + }, +}; + +static struct gdsc mvs0c_gdsc = { + .gdscr = 0xbf8, + .pd = { + .name = "mvs0c_gdsc", + }, + .flags = RETAIN_FF_ENABLE, + .pwrsts = PWRSTS_OFF_ON, +}; + +static struct gdsc mvs1c_gdsc = { + .gdscr = 0xc98, + .pd = { + .name = "mvs1c_gdsc", + }, + .flags = RETAIN_FF_ENABLE, + .pwrsts = PWRSTS_OFF_ON, +}; + +static struct gdsc mvs0_gdsc = { + .gdscr = 0xd18, + .pd = { + .name = "mvs0_gdsc", + }, + .flags = HW_CTRL | RETAIN_FF_ENABLE, + .pwrsts = PWRSTS_OFF_ON, +}; + +static struct gdsc mvs1_gdsc = { + .gdscr = 0xd98, + .pd = { + .name = "mvs1_gdsc", + }, + .flags = HW_CTRL | RETAIN_FF_ENABLE, + .pwrsts = PWRSTS_OFF_ON, +}; + +static struct clk_regmap *video_cc_sm8350_clocks[] = { + [VIDEO_CC_AHB_CLK_SRC] = &video_cc_ahb_clk_src.clkr, + [VIDEO_CC_MVS0_CLK] = &video_cc_mvs0_clk.clkr, + [VIDEO_CC_MVS0_CLK_SRC] = &video_cc_mvs0_clk_src.clkr, + [VIDEO_CC_MVS0_DIV_CLK_SRC] = &video_cc_mvs0_div_clk_src.clkr, + [VIDEO_CC_MVS0C_CLK] = &video_cc_mvs0c_clk.clkr, + [VIDEO_CC_MVS0C_DIV2_DIV_CLK_SRC] = &video_cc_mvs0c_div2_div_clk_src.clkr, + [VIDEO_CC_MVS1_CLK] = &video_cc_mvs1_clk.clkr, + [VIDEO_CC_MVS1_CLK_SRC] = &video_cc_mvs1_clk_src.clkr, + [VIDEO_CC_MVS1_DIV2_CLK] = &video_cc_mvs1_div2_clk.clkr, + [VIDEO_CC_MVS1_DIV_CLK_SRC] = &video_cc_mvs1_div_clk_src.clkr, + [VIDEO_CC_MVS1C_CLK] = &video_cc_mvs1c_clk.clkr, + [VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC] = &video_cc_mvs1c_div2_div_clk_src.clkr, + [VIDEO_CC_SLEEP_CLK] = &video_cc_sleep_clk.clkr, + [VIDEO_CC_SLEEP_CLK_SRC] = &video_cc_sleep_clk_src.clkr, + [VIDEO_CC_XO_CLK_SRC] = &video_cc_xo_clk_src.clkr, + [VIDEO_PLL0] = &video_pll0.clkr, + [VIDEO_PLL1] = &video_pll1.clkr, +}; + +static const struct qcom_reset_map video_cc_sm8350_resets[] = { + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 }, + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 }, + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 }, + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 }, + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 }, + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 }, + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 }, +}; + +static struct gdsc *video_cc_sm8350_gdscs[] = { + [MVS0C_GDSC] = &mvs0c_gdsc, + [MVS1C_GDSC] = &mvs1c_gdsc, + [MVS0_GDSC] = &mvs0_gdsc, + [MVS1_GDSC] = &mvs1_gdsc, +}; + +static const struct regmap_config video_cc_sm8350_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .max_register = 0x10000, + .fast_io = true, +}; + +static struct qcom_cc_desc video_cc_sm8350_desc = { + .config = &video_cc_sm8350_regmap_config, + .clks = video_cc_sm8350_clocks, + .num_clks = ARRAY_SIZE(video_cc_sm8350_clocks), + .resets = video_cc_sm8350_resets, + .num_resets = ARRAY_SIZE(video_cc_sm8350_resets), + .gdscs = video_cc_sm8350_gdscs, + .num_gdscs = ARRAY_SIZE(video_cc_sm8350_gdscs), +}; + +static int video_cc_sm8350_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_sm8350_desc); + if (IS_ERR(regmap)) { + pm_runtime_put(&pdev->dev); + return PTR_ERR(regmap); + }; + + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config); + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config); + + /* + * Keep clocks always enabled: + * video_cc_ahb_clk + * video_cc_xo_clk + */ + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0)); + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0)); + + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap); + pm_runtime_put(&pdev->dev); + + return ret; +} + +static const struct dev_pm_ops video_cc_sm8350_pm_ops = { + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL) +}; + +static const struct of_device_id video_cc_sm8350_match_table[] = { + { .compatible = "qcom,sm8350-videocc" }, + { } +}; +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table); + +static struct platform_driver video_cc_sm8350_driver = { + .probe = video_cc_sm8350_probe, + .driver = { + .name = "sm8350-videocc", + .of_match_table = video_cc_sm8350_match_table, + .pm = &video_cc_sm8350_pm_ops, + }, +}; +module_platform_driver(video_cc_sm8350_driver); + +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver"); +MODULE_LICENSE("GPL");
Add support for the Video Clock Controller found on the SM8350 SoC. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/clk/qcom/Kconfig | 9 + drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/videocc-sm8350.c | 557 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 567 insertions(+)