| Message ID | 20231214-x1e80100-clock-controllers-v2-0-2b0739bebd27@linaro.org |
|---|---|
| Headers | show |
| Series | clk: qcom: Add TCSR, GPU, CAM and DISP clock controllers for X1E80100 | expand |
On 14/12/2023 16:49, Abel Vesa wrote: > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > Add the camcc clock driver for x1e80100 > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- .name = "cam_cc-x1e80100", This is some very akward naming here. "camcc-x1e80100" Same comment with the defines not "CAM_CC_THING" just "CAMCC_THING" With those two nits fixed. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > + .of_match_table = cam_cc_x1e80100_match_table, > + }, > +}; > + > +module_platform_driver(cam_cc_x1e80100_driver); > + > +MODULE_DESCRIPTION("QTI CAM_CC x1e80100 Driver"); Again with the "CAM_CC" this should be -> "QTI Camera Clock Controller" --- bod
On 14/12/2023 16:49, Abel Vesa wrote: > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > Add Graphics Clock Controller (GPUCC) support for X1E80100 platform. > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > +static struct platform_driver gpu_cc_x1e80100_driver = { > + .probe = gpu_cc_x1e80100_probe, > + .driver = { > + .name = "gpu_cc-x1e80100", I think these underscores are very unnecessary and subtractive of meaning. .name = "gpucc-x1e80100" > + .of_match_table = gpu_cc_x1e80100_match_table, > + }, > +}; > +module_platform_driver(gpu_cc_x1e80100_driver); > + > +MODULE_DESCRIPTION("QTI GPU_CC x1e80100 Driver"); "QTI GPU Clock Controller Driver" --- bod
On 14/12/2023 16:49, Abel Vesa wrote: > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > Add the dispcc clock driver for x1e80100. > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > +static struct platform_driver disp_cc_x1e80100_driver = { > + .probe = disp_cc_x1e80100_probe, > + .driver = { > + .name = "disp_cc-x1e80100", > + .of_match_table = disp_cc_x1e80100_match_table, > + }, > +}; > + > +static int __init disp_cc_x1e80100_init(void) > +{ > + return platform_driver_register(&disp_cc_x1e80100_driver); > +} > +subsys_initcall(disp_cc_x1e80100_init); > + > +static void __exit disp_cc_x1e80100_exit(void) > +{ > + platform_driver_unregister(&disp_cc_x1e80100_driver); > +} > +module_exit(disp_cc_x1e80100_exit); > + > +MODULE_DESCRIPTION("QTI DISPCC X1E80100 Driver"); > +MODULE_LICENSE("GPL"); > And we don't even do the odd underscore insertion consistently. For whatever reason "DISPCC" instead of "DISP_CC" Just to reiterate the underscores should be dropped from these clock controller names and defines entirely, they just eat up bytes in databases. .name = "dispcc-x1e80100" ("QTI DISPCC X1E80100 Driver"); better but IMO we could just a complete word here "Display Clock Controller" there's no need to abbreviate. --- bod
On 14.12.2023 17:49, Abel Vesa wrote: > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > Add the dispcc clock driver for x1e80100. > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- [...] > + > +static const struct parent_map disp_cc_parent_map_7[] = { > + { P_SLEEP_CLK, 0 }, > +}; > + > +static const struct clk_parent_data disp_cc_parent_data_7_ao[] = { > + { .index = DT_SLEEP_CLK }, What's _ao about this? Konrad
On 14.12.2023 17:49, Abel Vesa wrote: > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > Add Graphics Clock Controller (GPUCC) support for X1E80100 platform. > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- [...] I see no usage of clk_branch2_aon_ops, is that intended? [...] > +static struct gdsc gpu_cc_cx_gdsc = { > + .gdscr = 0x9108, > + .gds_hw_ctrl = 0x953c, > + .en_rest_wait_val = 0x2, > + .en_few_wait_val = 0x2, > + .clk_dis_wait_val = 0xf, > + .pd = { > + .name = "gpu_cc_cx_gdsc", > + }, > + .pwrsts = PWRSTS_OFF_ON, > + .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE, That's.. unusual.. Can you doublecheck these flags? > + > + /* > + * Keep clocks always enabled: > + * gpu_cc_cb_clk > + */ > + regmap_update_bits(regmap, 0x93a4, BIT(0), BIT(0)); Please make the comment inline, so: regmap_update_bits(regmap, 0x93a4, BIT(0), BIT(0)); /* GPU_CC_CB_CLK */ I have submitted another series cleaning this up and adding a helper Konrad
On 14.12.2023 17:49, Abel Vesa wrote: > The TCSR clock controller found on X1E80100 provides refclks > for PCIE, USB and UFS. Add clock driver for it. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 14.12.2023 17:49, Abel Vesa wrote: > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > Add the camcc clock driver for x1e80100 > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- [...] > +enum { > + DT_BI_TCXO, > + DT_BI_TCXO_AO, > + DT_SLEEP_CLK, > +}; > + > +enum { > + P_BI_TCXO, Please don't overload this define with DT_BI_TCXO_AO, add a new one for the active-only clock. Please also do this in other drivers in this series. [...] > + clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config); > + clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config); > + clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config); > + clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config); > + clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config); > + clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config); > + clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config); Do we know whether these configure calls are actually necessary? > + > + /* > + * Keep clocks always enabled: > + * cam_cc_gdsc_clk > + * cam_cc_sleep_clk > + */ > + regmap_update_bits(regmap, 0x13a9c, BIT(0), BIT(0)); > + regmap_update_bits(regmap, 0x13ab8, BIT(0), BIT(0)); Please make the comments inline with each line Konrad
On 23-12-15 12:45:11, Bryan O'Donoghue wrote: > On 14/12/2023 16:49, Abel Vesa wrote: > > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > > > Add the dispcc clock driver for x1e80100. > > > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > > > +static struct platform_driver disp_cc_x1e80100_driver = { > > + .probe = disp_cc_x1e80100_probe, > > + .driver = { > > + .name = "disp_cc-x1e80100", > > + .of_match_table = disp_cc_x1e80100_match_table, > > + }, > > +}; > > + > > +static int __init disp_cc_x1e80100_init(void) > > +{ > > + return platform_driver_register(&disp_cc_x1e80100_driver); > > +} > > +subsys_initcall(disp_cc_x1e80100_init); > > + > > +static void __exit disp_cc_x1e80100_exit(void) > > +{ > > + platform_driver_unregister(&disp_cc_x1e80100_driver); > > +} > > +module_exit(disp_cc_x1e80100_exit); > > + > > +MODULE_DESCRIPTION("QTI DISPCC X1E80100 Driver"); > > +MODULE_LICENSE("GPL"); > > > > And we don't even do the odd underscore insertion consistently. For whatever > reason "DISPCC" instead of "DISP_CC" > > Just to reiterate the underscores should be dropped from these clock > controller names and defines entirely, they just eat up bytes in databases. > > .name = "dispcc-x1e80100" > > ("QTI DISPCC X1E80100 Driver"); better but IMO we could just a complete word > here > > "Display Clock Controller" there's no need to abbreviate. Maybe, but I think it's better to stay aligned with other platforms. So please check SM8550, SM8650, and so on. > > --- > bod
On 23-12-15 12:40:45, Bryan O'Donoghue wrote: > On 14/12/2023 16:49, Abel Vesa wrote: > > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > > > Add Graphics Clock Controller (GPUCC) support for X1E80100 platform. > > > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > +static struct platform_driver gpu_cc_x1e80100_driver = { > > + .probe = gpu_cc_x1e80100_probe, > > + .driver = { > > + .name = "gpu_cc-x1e80100", > > I think these underscores are very unnecessary and subtractive of meaning. > > .name = "gpucc-x1e80100" > > > + .of_match_table = gpu_cc_x1e80100_match_table, > > + }, > > +}; > > +module_platform_driver(gpu_cc_x1e80100_driver); > > + > > +MODULE_DESCRIPTION("QTI GPU_CC x1e80100 Driver"); > > "QTI GPU Clock Controller Driver" Again, please look at other platforms (SM8650, SM8550, etc). > > --- > bod
On 23-12-16 14:39:48, Konrad Dybcio wrote: > On 14.12.2023 17:49, Abel Vesa wrote: > > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > > > Add the camcc clock driver for x1e80100 > > > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > [...] > > > +enum { > > + DT_BI_TCXO, > > + DT_BI_TCXO_AO, > > + DT_SLEEP_CLK, > > +}; > > + > > +enum { > > + P_BI_TCXO, > Please don't overload this define with DT_BI_TCXO_AO, add a new one > for the active-only clock. Please also do this in other drivers in > this series. Nope, that needs to stay if we want to align the dt bindings between SM8550, SM8650 and this. At least for dispcc. But I would like to have the same dt schema for the rest of the clock controller drivers between platforms that share basically the same ip block. > > [...] > > > + clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config); > > + clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config); > Do we know whether these configure calls are actually necessary? > > + > > + /* > > + * Keep clocks always enabled: > > + * cam_cc_gdsc_clk > > + * cam_cc_sleep_clk > > + */ > > + regmap_update_bits(regmap, 0x13a9c, BIT(0), BIT(0)); > > + regmap_update_bits(regmap, 0x13ab8, BIT(0), BIT(0)); > Please make the comments inline with each line > > Konrad
On 1/23/24 12:49, Abel Vesa wrote: > On 23-12-16 14:39:48, Konrad Dybcio wrote: >> On 14.12.2023 17:49, Abel Vesa wrote: >>> From: Rajendra Nayak <quic_rjendra@quicinc.com> >>> >>> Add the camcc clock driver for x1e80100 >>> >>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>> --- >> [...] >> >>> +enum { >>> + DT_BI_TCXO, >>> + DT_BI_TCXO_AO, >>> + DT_SLEEP_CLK, >>> +}; >>> + >>> +enum { >>> + P_BI_TCXO, >> Please don't overload this define with DT_BI_TCXO_AO, add a new one >> for the active-only clock. Please also do this in other drivers in >> this series. > > Nope, that needs to stay if we want to align the dt bindings between > SM8550, SM8650 and this. At least for dispcc. But I would like to have > the same dt schema for the rest of the clock controller drivers between > platforms that share basically the same ip block. No, you're confusing the dt ordering enum (the first one) with the parent list enum (the one below that I'm commenting on). Konrad
On 24-01-23 18:56:03, Konrad Dybcio wrote: > > > On 1/23/24 12:49, Abel Vesa wrote: > > On 23-12-16 14:39:48, Konrad Dybcio wrote: > > > On 14.12.2023 17:49, Abel Vesa wrote: > > > > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > > > > > > > Add the camcc clock driver for x1e80100 > > > > > > > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > --- > > > [...] > > > > > > > +enum { > > > > + DT_BI_TCXO, > > > > + DT_BI_TCXO_AO, > > > > + DT_SLEEP_CLK, > > > > +}; > > > > + > > > > +enum { > > > > + P_BI_TCXO, > > > Please don't overload this define with DT_BI_TCXO_AO, add a new one > > > for the active-only clock. Please also do this in other drivers in > > > this series. > > > > Nope, that needs to stay if we want to align the dt bindings between > > SM8550, SM8650 and this. At least for dispcc. But I would like to have > > the same dt schema for the rest of the clock controller drivers between > > platforms that share basically the same ip block. > > No, you're confusing the dt ordering enum (the first one) with the > parent list enum (the one below that I'm commenting on). Got it. P_BI_TCXO_AO it is then. > > Konrad
On 23-12-16 14:39:48, Konrad Dybcio wrote: > On 14.12.2023 17:49, Abel Vesa wrote: > > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > > > Add the camcc clock driver for x1e80100 > > > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > [...] > > > +enum { > > + DT_BI_TCXO, > > + DT_BI_TCXO_AO, > > + DT_SLEEP_CLK, > > +}; > > + > > +enum { > > + P_BI_TCXO, > Please don't overload this define with DT_BI_TCXO_AO, add a new one > for the active-only clock. Please also do this in other drivers in > this series. > > [...] > > > + clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config); > > + clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config); > > + clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config); > Do we know whether these configure calls are actually necessary? So camera support hasn't been fully brought up yet, but based on the SM8550 driver (which is quite similar), they seem to be needed. Once camera is up, we can confirm for sure. > > + > > + /* > > + * Keep clocks always enabled: > > + * cam_cc_gdsc_clk > > + * cam_cc_sleep_clk > > + */ > > + regmap_update_bits(regmap, 0x13a9c, BIT(0), BIT(0)); > > + regmap_update_bits(regmap, 0x13ab8, BIT(0), BIT(0)); > Please make the comments inline with each line Will do. > > Konrad
This patchset adds all the missing clock controllers for Qualcomm X1E80100 platform. Another important change is the dropping of the dedicated schema of the SM8650 DISP CC as a preparatory work for documenting the DISP CC compatible for X1E801800. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- Changes in v2: - Added Krzysztof's R-b tag to patches no. 1, 3, 4 and 5 - Added Dmitry's R-b tag to patch 7 - Reordered Signed-off-by tags in patch 6 - Lower-cased hex values in patch 6, 8 and 10 - Link to v1: https://lore.kernel.org/r/20231212-x1e80100-clock-controllers-v1-0-0de1af44dcb3@linaro.org --- Abel Vesa (3): dt-bindings: clock: Drop the SM8650 DISPCC dedicated schema dt-bindings: clock: qcom: Document the X1E80100 TCSR Clock Controller clk: qcom: Add TCSR clock driver for x1e80100 Rajendra Nayak (7): dt-bindings: clock: qcom: Document the X1E80100 Display Clock Controller dt-bindings: clock: qcom: Document the X1E80100 GPU Clock Controller dt-bindings: clock: qcom: Document the X1E80100 Camera Clock Controller clk: qcom: clk-alpha-pll: Add support for zonda ole pll configure clk: qcom: Add dispcc clock driver for x1e80100 clk: qcom: Add GPU clock driver for x1e80100 clk: qcom: Add camcc clock driver for x1e80100 .../bindings/clock/qcom,sm8450-camcc.yaml | 2 + .../bindings/clock/qcom,sm8450-gpucc.yaml | 2 + .../bindings/clock/qcom,sm8550-dispcc.yaml | 7 +- .../bindings/clock/qcom,sm8550-tcsr.yaml | 1 + .../bindings/clock/qcom,sm8650-dispcc.yaml | 106 - drivers/clk/qcom/Kconfig | 35 + drivers/clk/qcom/Makefile | 4 + drivers/clk/qcom/camcc-x1e80100.c | 2489 ++++++++++++++++++++ drivers/clk/qcom/clk-alpha-pll.c | 26 + drivers/clk/qcom/clk-alpha-pll.h | 4 + drivers/clk/qcom/dispcc-x1e80100.c | 1699 +++++++++++++ drivers/clk/qcom/gpucc-x1e80100.c | 659 ++++++ drivers/clk/qcom/tcsrcc-x1e80100.c | 285 +++ include/dt-bindings/clock/qcom,x1e80100-camcc.h | 135 ++ include/dt-bindings/clock/qcom,x1e80100-dispcc.h | 98 + include/dt-bindings/clock/qcom,x1e80100-gpucc.h | 41 + include/dt-bindings/clock/qcom,x1e80100-tcsr.h | 23 + include/dt-bindings/reset/qcom,x1e80100-gpucc.h | 19 + 18 files changed, 5528 insertions(+), 107 deletions(-) --- base-commit: 7b0e611dc474ffa67d3a6ea235085bf423ee5f2a change-id: 20231201-x1e80100-clock-controllers-ba42b0575f8a Best regards,