| Message ID | 20230519155602.6642-1-quic_jkona@quicinc.com |
|---|---|
| Headers | show |
| Series | Add camera clock controller support for SM8550 | expand |
Hi Konrad, Thanks for your review! On 5/19/2023 10:19 PM, Konrad Dybcio wrote: > > > On 19.05.2023 17:55, Jagadeesh Kona wrote: >> Add support for rivian ole pll ops and ole pll registers to >> configure and control the rivian ole pll. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> --- >> drivers/clk/qcom/clk-alpha-pll.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h >> index 4d9b6d5b7062..e6df398bee6d 100644 >> --- a/drivers/clk/qcom/clk-alpha-pll.h >> +++ b/drivers/clk/qcom/clk-alpha-pll.h >> @@ -24,6 +24,7 @@ enum { >> CLK_ALPHA_PLL_TYPE_LUCID_EVO, >> CLK_ALPHA_PLL_TYPE_LUCID_OLE, >> CLK_ALPHA_PLL_TYPE_RIVIAN_EVO, >> + CLK_ALPHA_PLL_TYPE_RIVIAN_OLE = CLK_ALPHA_PLL_TYPE_RIVIAN_EVO, > This makes sense > > The rest is just sugar syntax, I don't think it makes sense to keep > adding meaningless defines.. > Yes, will remove these and reuse existing ops in next series. > Konrad >> CLK_ALPHA_PLL_TYPE_DEFAULT_EVO, >> CLK_ALPHA_PLL_TYPE_BRAMMO_EVO, >> CLK_ALPHA_PLL_TYPE_STROMER, >> @@ -181,6 +182,7 @@ extern const struct clk_ops clk_alpha_pll_postdiv_lucid_evo_ops; >> #define clk_alpha_pll_postdiv_lucid_ole_ops clk_alpha_pll_postdiv_lucid_evo_ops >> >> extern const struct clk_ops clk_alpha_pll_rivian_evo_ops; >> +#define clk_alpha_pll_rivian_ole_ops clk_alpha_pll_rivian_evo_ops >> #define clk_alpha_pll_postdiv_rivian_evo_ops clk_alpha_pll_postdiv_fabia_ops >> >> void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap, >> @@ -202,6 +204,8 @@ void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regma >> clk_lucid_evo_pll_configure(pll, regmap, config) >> void clk_rivian_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap, >> const struct alpha_pll_config *config); >> +#define clk_rivian_ole_pll_configure(pll, regmap, config) \ >> + clk_rivian_evo_pll_configure(pll, regmap, config) >> void clk_stromer_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap, >> const struct alpha_pll_config *config); >> Thanks & Regards, Jagadeesh
Hi Konrad, Thanks for your review! On 5/19/2023 10:22 PM, Konrad Dybcio wrote: > > > On 19.05.2023 17:56, Jagadeesh Kona wrote: >> Add support for the camera clock controller for camera clients to be >> able to request for camcc clocks on SM8550 platform. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> --- >> drivers/clk/qcom/Kconfig | 7 + >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/camcc-sm8550.c | 3572 +++++++++++++++++++++++++++++++ >> 3 files changed, 3580 insertions(+) >> create mode 100644 drivers/clk/qcom/camcc-sm8550.c >> >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig >> index 6bb9b4aff047..c8eccd428736 100644 >> --- a/drivers/clk/qcom/Kconfig >> +++ b/drivers/clk/qcom/Kconfig >> @@ -700,6 +700,13 @@ config SM_CAMCC_8450 >> Support for the camera clock controller on SM8450 devices. >> Say Y if you want to support camera devices and camera functionality. >> >> +config SM_CAMCC_8550 >> + tristate "SM8550 Camera Clock Controller" >> + select SM_GCC_8550 >> + help >> + Support for the camera clock controller on SM8550 devices. >> + Say Y if you want to support camera devices and camera functionality. >> + >> config SM_DISPCC_6115 >> tristate "SM6115 Display Clock Controller" >> depends on SM_GCC_6115 >> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile >> index f0b95fc217aa..2b1fcd5c920a 100644 >> --- a/drivers/clk/qcom/Makefile >> +++ b/drivers/clk/qcom/Makefile >> @@ -100,6 +100,7 @@ obj-$(CONFIG_SDX_GCC_65) += gcc-sdx65.o >> obj-$(CONFIG_SM_CAMCC_6350) += camcc-sm6350.o >> obj-$(CONFIG_SM_CAMCC_8250) += camcc-sm8250.o >> obj-$(CONFIG_SM_CAMCC_8450) += camcc-sm8450.o >> +obj-$(CONFIG_SM_CAMCC_8550) += camcc-sm8550.o >> obj-$(CONFIG_SM_DISPCC_6115) += dispcc-sm6115.o >> obj-$(CONFIG_SM_DISPCC_6125) += dispcc-sm6125.o >> obj-$(CONFIG_SM_DISPCC_6350) += dispcc-sm6350.o >> diff --git a/drivers/clk/qcom/camcc-sm8550.c b/drivers/clk/qcom/camcc-sm8550.c >> new file mode 100644 >> index 000000000000..2c3d2436da0f >> --- /dev/null >> +++ b/drivers/clk/qcom/camcc-sm8550.c >> @@ -0,0 +1,3572 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/regmap.h> >> + >> +#include <dt-bindings/clock/qcom,sm8550-camcc.h> >> + >> +#include "clk-alpha-pll.h" >> +#include "clk-branch.h" >> +#include "clk-rcg.h" >> +#include "clk-regmap.h" >> +#include "common.h" >> +#include "gdsc.h" >> +#include "reset.h" >> + >> +enum { >> + DT_BI_TCXO, >> + DT_BI_TCXO_AO, >> + DT_SLEEP_CLK, >> +}; >> + >> +enum { >> + P_BI_TCXO, >> + P_CAM_CC_PLL0_OUT_EVEN, >> + P_CAM_CC_PLL0_OUT_MAIN, >> + P_CAM_CC_PLL0_OUT_ODD, >> + P_CAM_CC_PLL10_OUT_EVEN, >> + P_CAM_CC_PLL11_OUT_EVEN, >> + P_CAM_CC_PLL12_OUT_EVEN, > While it makes sense for a computer, I think it would make more > sense to keep the >=10 entries after 9 > Yes, will take care of this in next series >> + P_CAM_CC_PLL1_OUT_EVEN, >> + P_CAM_CC_PLL2_OUT_EVEN, >> + P_CAM_CC_PLL2_OUT_MAIN, >> + P_CAM_CC_PLL3_OUT_EVEN, >> + P_CAM_CC_PLL4_OUT_EVEN, >> + P_CAM_CC_PLL5_OUT_EVEN, >> + P_CAM_CC_PLL6_OUT_EVEN, >> + P_CAM_CC_PLL7_OUT_EVEN, >> + P_CAM_CC_PLL8_OUT_EVEN, >> + P_CAM_CC_PLL9_OUT_EVEN, >> + P_CAM_CC_PLL9_OUT_ODD, >> + P_SLEEP_CLK, >> +}; >> + >> +static const struct pll_vco lucid_ole_vco[] = { >> + { 249600000, 2300000000, 0 }, >> +}; >> + >> +static const struct pll_vco rivian_ole_vco[] = { >> + { 777000000, 1285000000, 0 }, >> +}; >> + >> +static const struct alpha_pll_config cam_cc_pll0_config = { >> + .l = 0x4400003E, > Lowercase hex, everywhere, please. > Will take care of this in next series >> + .alpha = 0x8000, >> + .config_ctl_val = 0x20485699, >> + .config_ctl_hi_val = 0x00182261, >> + .config_ctl_hi1_val = 0x82AA299C, >> + .test_ctl_val = 0x00000000, >> + .test_ctl_hi_val = 0x00000003, >> + .test_ctl_hi1_val = 0x00009000, >> + .test_ctl_hi2_val = 0x00000034, >> + .user_ctl_val = 0x00008400, >> + .user_ctl_hi_val = 0x00000005, >> +}; >> + > [...] > >> + >> + 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_lucid_ole_pll_configure(&cam_cc_pll10, regmap, &cam_cc_pll10_config); > Same as the first comment > Will take care in next series > Konrad >> + clk_lucid_ole_pll_configure(&cam_cc_pll11, regmap, &cam_cc_pll11_config); >> + clk_lucid_ole_pll_configure(&cam_cc_pll12, regmap, &cam_cc_pll12_config); >> + clk_rivian_ole_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_pll5, regmap, &cam_cc_pll5_config); >> + clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config); >> + clk_lucid_ole_pll_configure(&cam_cc_pll7, regmap, &cam_cc_pll7_config); >> + clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config); >> + clk_lucid_ole_pll_configure(&cam_cc_pll9, regmap, &cam_cc_pll9_config); >> + [skipped] >> +MODULE_DESCRIPTION("QTI CAM_CC SM8550 Driver"); >> +MODULE_LICENSE("GPL"); Thanks & Regards, Jagadeesh
Hi Bryan, Thanks for your review! On 5/20/2023 1:56 AM, Bryan O'Donoghue wrote: > On 19/05/2023 16:56, Jagadeesh Kona wrote: >> +MODULE_DESCRIPTION("QTI CAM_CC SM8550 Driver"); > > This pattern looks odd and inconsistent in our clock drivers. We mostly > have CAMCC or VIDEOCC but occasionally have CAM_CC and VIDEO_CC. > > I'd drop that underscore to, in addition to Konrad's comment on Ucase hex. > > Other than that, LGTM. > Will take care of this in next series. > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Thanks & Regards, Jagadeesh
Hi Konrad, Thanks for your review! On 5/19/2023 10:22 PM, Konrad Dybcio wrote: > > > On 19.05.2023 17:56, Jagadeesh Kona wrote: >> Add device node for camera clock controller on Qualcomm >> SM8550 platform. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sm8550.dtsi | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> index e67e7c69dae6..ac82d3774ed8 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> @@ -4,6 +4,7 @@ >> */ >> >> #include <dt-bindings/clock/qcom,rpmh.h> >> +#include <dt-bindings/clock/qcom,sm8550-camcc.h> >> #include <dt-bindings/clock/qcom,sm8550-gcc.h> >> #include <dt-bindings/clock/qcom,sm8550-tcsr.h> >> #include <dt-bindings/clock/qcom,sm8550-dispcc.h> >> @@ -2397,6 +2398,20 @@ opp-202000000 { >> }; >> }; >> >> + camcc: clock-controller@ade0000 { >> + compatible = "qcom,sm8550-camcc"; >> + reg = <0 0xade0000 0 0x20000>; > Please pad the non-zero address part to 8 hex digits > Will take care of this in next series. > Konrad >> + clocks = <&bi_tcxo_div2>, >> + <&bi_tcxo_ao_div2>, >> + <&sleep_clk>, >> + <&gcc GCC_CAMERA_AHB_CLK>; >> + power-domains = <&rpmhpd SM8550_MMCX>; >> + required-opps = <&rpmhpd_opp_low_svs>; >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + #power-domain-cells = <1>; >> + }; >> + >> mdss: display-subsystem@ae00000 { >> compatible = "qcom,sm8550-mdss"; >> reg = <0 0x0ae00000 0 0x1000>; Thanks & Regards, Jagadeesh
Hi Dmitry, Thanks for your review! On 5/20/2023 12:59 AM, Dmitry Baryshkov wrote: > On 19/05/2023 18:55, Jagadeesh Kona wrote: >> Add bindings, driver and devicetree node for camera clock controller >> on SM8550. >> >> Depends on [1] for lucid ole pll ops definition >> [1] >> https://patchwork.kernel.org/project/linux-clk/list/?series=746186&state=%2A&archive=both >> >> Jagadeesh Kona (4): >> clk: qcom: clk-alpha-pll: Add support for rivian ole pll ops >> dt-bindings: clock: qcom: Add SM8550 camera clock controller >> clk: qcom: camcc-sm8550: Add camera clock controller driver for SM8550 > > This patch didn't make it to the list. Please consider splitting it > somehow. > Yes, will split it in next series. >> arm64: dts: qcom: sm8550: Add camera clock controller >> >> .../bindings/clock/qcom,sm8550-camcc.yaml | 82 + >> arch/arm64/boot/dts/qcom/sm8550.dtsi | 15 + >> drivers/clk/qcom/Kconfig | 7 + >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/camcc-sm8550.c | 3572 +++++++++++++++++ >> drivers/clk/qcom/clk-alpha-pll.h | 4 + >> include/dt-bindings/clock/qcom,sm8550-camcc.h | 187 + >> 7 files changed, 3868 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/clock/qcom,sm8550-camcc.yaml >> create mode 100644 drivers/clk/qcom/camcc-sm8550.c >> create mode 100644 include/dt-bindings/clock/qcom,sm8550-camcc.h >> > Thanks & Regards, Jagadeesh