| 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 17:49, Abel Vesa wrote: > From: Rajendra Nayak <quic_rjendra@quicinc.com> > > Add bindings documentation for the X1E80100 Display Clock Controller. > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> It seems I missed to review this one: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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 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 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 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,