| Message ID | 20230609115058.9059-1-quic_jkona@quicinc.com |
|---|---|
| Headers | show |
| Series | Add camera clock controller support for SM8550 | expand |
On 9.06.2023 13:50, Jagadeesh Kona wrote: > Add device node for camera clock controller on Qualcomm > SM8550 platform. > > Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > --- > Changes since V3: > - No changes > Changes since V2: > - No changes > Changes since V1: > - Padded non-zero address part to 8 hex digits > > 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 75cd374943eb..4d2d610fc66a 100644 > --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi > @@ -5,6 +5,7 @@ > > #include <dt-bindings/clock/qcom,rpmh.h> > #include <dt-bindings/clock/qcom,sm8450-videocc.h> > +#include <dt-bindings/clock/qcom,sm8550-camcc.h> > #include <dt-bindings/clock/qcom,sm8550-gcc.h> > #include <dt-bindings/clock/qcom,sm8550-gpucc.h> > #include <dt-bindings/clock/qcom,sm8550-tcsr.h> > @@ -2419,6 +2420,20 @@ videocc: clock-controller@aaf0000 { > #power-domain-cells = <1>; > }; > > + camcc: clock-controller@ade0000 { > + compatible = "qcom,sm8550-camcc"; > + reg = <0 0x0ade0000 0 0x20000>; > + clocks = <&gcc GCC_CAMERA_AHB_CLK>, > + <&bi_tcxo_div2>, > + <&bi_tcxo_ao_div2>, > + <&sleep_clk>; > + power-domains = <&rpmhpd SM8550_MMCX>; I see that both MMCX ("mmcx.lvl") and MXC ("mxc.lvl") (and MX, FWIW) are consumed on msm-5.15, with the latter one powering camcc PLLs.. How are they related? Is that resolved internally or does it need manual intervention? Konrad > + 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>;
On 9.06.2023 13:50, Jagadeesh Kona wrote: > Add bindings, driver and devicetree node for camera clock controller on > SM8550. > > Jagadeesh Kona (4): > dt-bindings: clock: qcom: Add SM8550 camera clock controller > clk: qcom: camcc-sm8550: Add camera clock controller driver for SM8550 > clk: qcom: camcc-sm8550: Add support for qdss, sleep and xo clocks > arm64: dts: qcom: sm8550: Add camera clock controller What's the final verdict on RINGOSC_L etc.? Konrad > > .../bindings/clock/qcom,sm8450-camcc.yaml | 8 +- > arch/arm64/boot/dts/qcom/sm8550.dtsi | 15 + > drivers/clk/qcom/Kconfig | 7 + > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/camcc-sm8550.c | 3585 +++++++++++++++++ > include/dt-bindings/clock/qcom,sm8550-camcc.h | 187 + > 6 files changed, 3801 insertions(+), 2 deletions(-) > create mode 100644 drivers/clk/qcom/camcc-sm8550.c > create mode 100644 include/dt-bindings/clock/qcom,sm8550-camcc.h >
On Fri, 9 Jun 2023 at 14:52, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > Add support for the camera clock controller for camera clients to be > able to request for camcc clocks on SM8550 platform. > > Co-developed-by: Taniya Das <quic_tdas@quicinc.com> > Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > --- > Changes since V3: > - No changes > Changes since V2: > - No changes > Changes since V1: > - Sorted the PLL names in proper order > - Updated all PLL configurations to lower case hex > - Reused evo ops instead of adding new ops for ole pll > - Moved few clocks to separate patch to fix patch too long error > > drivers/clk/qcom/Kconfig | 7 + > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/camcc-sm8550.c | 3405 +++++++++++++++++++++++++++++++ > 3 files changed, 3413 insertions(+) > create mode 100644 drivers/clk/qcom/camcc-sm8550.c > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > index 9cd1f05d436b..85efed78dc9a 100644 > --- a/drivers/clk/qcom/Kconfig > +++ b/drivers/clk/qcom/Kconfig > @@ -756,6 +756,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 ARM64 || COMPILE_TEST > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile > index 75d035150118..97c8cefc2fd0 100644 > --- a/drivers/clk/qcom/Makefile > +++ b/drivers/clk/qcom/Makefile > @@ -101,6 +101,7 @@ obj-$(CONFIG_SDX_GCC_75) += gcc-sdx75.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..85f0c1e09b2b > --- /dev/null > +++ b/drivers/clk/qcom/camcc-sm8550.c > @@ -0,0 +1,3405 @@ > +// 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_IFACE, > + DT_BI_TCXO, > +}; > + > +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_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_CAM_CC_PLL10_OUT_EVEN, > + P_CAM_CC_PLL11_OUT_EVEN, > + P_CAM_CC_PLL12_OUT_EVEN, > +}; > + > +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 includes RINGOSC_CAL_L_VAL, CAL_L_VAL, L_VAL fields */ > + .l = 0x4444003e, I'd still insist on not touching the config.l field semantics. > + .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, > +}; > + [skipped the rest, LGTM] > + > +static struct platform_driver cam_cc_sm8550_driver = { > + .probe = cam_cc_sm8550_probe, > + .driver = { > + .name = "cam_cc-sm8550", > + .of_match_table = cam_cc_sm8550_match_table, > + }, > +}; > + > +static int __init cam_cc_sm8550_init(void) > +{ > + return platform_driver_register(&cam_cc_sm8550_driver); > +} > +subsys_initcall(cam_cc_sm8550_init); As it was pointed out, this driver is built as a module by default. Please perform the tesing and cleanup before sending the driver and use module_platform_driver. > + > +static void __exit cam_cc_sm8550_exit(void) > +{ > + platform_driver_unregister(&cam_cc_sm8550_driver); > +} > +module_exit(cam_cc_sm8550_exit); > + > +MODULE_DESCRIPTION("QTI CAMCC SM8550 Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.40.1 >
On 09/06/2023 12:50, Jagadeesh Kona wrote: > Add bindings, driver and devicetree node for camera clock controller on > SM8550. This is very confusing. Your cover letter doesn't detail any changes and your individual patches all say "no changes since v3", "no changes since v2" If this is a RESEND then mark it as a RESEND. Good practice is to for example add a note that says "I looked at updating the yaml for the camcc but opted to do this in another series" or "opted not to do this at this time" or "it doesn't make sense because of X" https://lore.kernel.org/linux-arm-msm/546876ba-970d-5cd5-648e-723698ca74fd@linaro.org/ Could you perhaps RESEND this V4 with a log that explains what has changed from one version to the next. If nothing has changed then don't bump the version prefix with RESEND.. Second thought even replying to your cover email with the changelog would do..... --- bod
On 12/06/2023 04:25, Bryan O'Donoghue wrote: > On 09/06/2023 12:50, Jagadeesh Kona wrote: >> Add bindings, driver and devicetree node for camera clock controller on >> SM8550. > > This is very confusing. > > Your cover letter doesn't detail any changes and your individual patches > all say "no changes since v3", "no changes since v2" > > If this is a RESEND then mark it as a RESEND. That's indeed odd. Three versions without changes... Best regards, Krzysztof
On 6/13/2023 2:07 PM, Krzysztof Kozlowski wrote: > On 12/06/2023 04:25, Bryan O'Donoghue wrote: >> On 09/06/2023 12:50, Jagadeesh Kona wrote: >>> Add bindings, driver and devicetree node for camera clock controller on >>> SM8550. >> >> This is very confusing. >> >> Your cover letter doesn't detail any changes and your individual patches >> all say "no changes since v3", "no changes since v2" >> >> If this is a RESEND then mark it as a RESEND. > > That's indeed odd. Three versions without changes... > > Best regards, > Krzysztof > This is not a RESEND, actually there were changes from each version to next version and change details were updated in respective patches. But the patches in which changes were present were dropped in v4 based on review comments. Will take care of updating cover letter as well with changes across versions if we push the next series. Please find the summary of changes across versions till v4. Changes in v4: - Dropped the extra patches added in v2, since the review comments on v3 recommended an alternate solution. Changes in v3: - Squashed 2 extra patches added in v2 into single patch as per review comments - Link to v3: https://patchwork.kernel.org/project/linux-clk/list/?series=753150 Changes in v2: - Took care of review comments from v1 - Added 2 extra patches updating L configuration value across chipsets to include CAL_L and RINGOSC_CAL_L fields and removed setting CAL_L field in clk_lucid_evo_pll_configure(). - Link to v2: https://patchwork.kernel.org/project/linux-clk/list/?series=751058 v1: - Initial CAMCC changes for SM8550 - Link to v1: https://patchwork.kernel.org/project/linux-clk/list/?series=749294 Thanks, Jagadeesh
On 6/9/2023 9:52 PM, Dmitry Baryshkov wrote: > On Fri, 9 Jun 2023 at 14:52, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> Add support for the camera clock controller for camera clients to be >> able to request for camcc clocks on SM8550 platform. >> >> Co-developed-by: Taniya Das <quic_tdas@quicinc.com> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> Changes since V3: >> - No changes >> Changes since V2: >> - No changes >> Changes since V1: >> - Sorted the PLL names in proper order >> - Updated all PLL configurations to lower case hex >> - Reused evo ops instead of adding new ops for ole pll >> - Moved few clocks to separate patch to fix patch too long error >> >> drivers/clk/qcom/Kconfig | 7 + >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/camcc-sm8550.c | 3405 +++++++++++++++++++++++++++++++ >> 3 files changed, 3413 insertions(+) >> create mode 100644 drivers/clk/qcom/camcc-sm8550.c >> >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig >> index 9cd1f05d436b..85efed78dc9a 100644 >> --- a/drivers/clk/qcom/Kconfig >> +++ b/drivers/clk/qcom/Kconfig >> @@ -756,6 +756,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 ARM64 || COMPILE_TEST >> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile >> index 75d035150118..97c8cefc2fd0 100644 >> --- a/drivers/clk/qcom/Makefile >> +++ b/drivers/clk/qcom/Makefile >> @@ -101,6 +101,7 @@ obj-$(CONFIG_SDX_GCC_75) += gcc-sdx75.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..85f0c1e09b2b >> --- /dev/null >> +++ b/drivers/clk/qcom/camcc-sm8550.c >> @@ -0,0 +1,3405 @@ >> +// 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_IFACE, >> + DT_BI_TCXO, >> +}; >> + >> +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_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_CAM_CC_PLL10_OUT_EVEN, >> + P_CAM_CC_PLL11_OUT_EVEN, >> + P_CAM_CC_PLL12_OUT_EVEN, >> +}; >> + >> +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 includes RINGOSC_CAL_L_VAL, CAL_L_VAL, L_VAL fields */ >> + .l = 0x4444003e, > > I'd still insist on not touching the config.l field semantics. > We feel it is better to update config->l field and reuse existing code than adding separate function for lucid ole pll configure. >> + .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, >> +}; >> + > > [skipped the rest, LGTM] > >> + >> +static struct platform_driver cam_cc_sm8550_driver = { >> + .probe = cam_cc_sm8550_probe, >> + .driver = { >> + .name = "cam_cc-sm8550", >> + .of_match_table = cam_cc_sm8550_match_table, >> + }, >> +}; >> + >> +static int __init cam_cc_sm8550_init(void) >> +{ >> + return platform_driver_register(&cam_cc_sm8550_driver); >> +} >> +subsys_initcall(cam_cc_sm8550_init); > > As it was pointed out, this driver is built as a module by default. > Please perform the tesing and cleanup before sending the driver and > use module_platform_driver. > We want clock drivers to be probed early in the bootup to avoid any probe deferrals of consumer drivers. If there is any scenario where clock drivers are built statically into kernel, then subsys_initcall() will ensure clock drivers are probed earlier. When built as module, subsys_initcall() will fallback to module_init() which is same as module_platform_driver(). Thanks, Jagadeesh >> + >> +static void __exit cam_cc_sm8550_exit(void) >> +{ >> + platform_driver_unregister(&cam_cc_sm8550_driver); >> +} >> +module_exit(cam_cc_sm8550_exit); >> + >> +MODULE_DESCRIPTION("QTI CAMCC SM8550 Driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.40.1 >> > >
On 6/9/2023 6:22 PM, Konrad Dybcio wrote: > > > On 9.06.2023 13:50, Jagadeesh Kona wrote: >> Add device node for camera clock controller on Qualcomm >> SM8550 platform. >> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> Changes since V3: >> - No changes >> Changes since V2: >> - No changes >> Changes since V1: >> - Padded non-zero address part to 8 hex digits >> >> 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 75cd374943eb..4d2d610fc66a 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> @@ -5,6 +5,7 @@ >> >> #include <dt-bindings/clock/qcom,rpmh.h> >> #include <dt-bindings/clock/qcom,sm8450-videocc.h> >> +#include <dt-bindings/clock/qcom,sm8550-camcc.h> >> #include <dt-bindings/clock/qcom,sm8550-gcc.h> >> #include <dt-bindings/clock/qcom,sm8550-gpucc.h> >> #include <dt-bindings/clock/qcom,sm8550-tcsr.h> >> @@ -2419,6 +2420,20 @@ videocc: clock-controller@aaf0000 { >> #power-domain-cells = <1>; >> }; >> >> + camcc: clock-controller@ade0000 { >> + compatible = "qcom,sm8550-camcc"; >> + reg = <0 0x0ade0000 0 0x20000>; >> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >> + <&bi_tcxo_div2>, >> + <&bi_tcxo_ao_div2>, >> + <&sleep_clk>; >> + power-domains = <&rpmhpd SM8550_MMCX>; > I see that both MMCX ("mmcx.lvl") and MXC ("mxc.lvl") (and MX, FWIW) > are consumed on msm-5.15, with the latter one powering camcc PLLs.. > > How are they related? Is that resolved internally or does it need > manual intervention? > > Konrad These are just different voltage rails, camcc clocks are powered by MMCX rail and camcc pll's are powered by MXC rail. Consumer drivers need to take care of voting on these rails properly based on the frequency of clocks requested. Thanks, Jagadeesh >> + 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>;
On 6/9/2023 6:24 PM, Konrad Dybcio wrote: > > > On 9.06.2023 13:50, Jagadeesh Kona wrote: >> Add bindings, driver and devicetree node for camera clock controller on >> SM8550. >> >> Jagadeesh Kona (4): >> dt-bindings: clock: qcom: Add SM8550 camera clock controller >> clk: qcom: camcc-sm8550: Add camera clock controller driver for SM8550 >> clk: qcom: camcc-sm8550: Add support for qdss, sleep and xo clocks >> arm64: dts: qcom: sm8550: Add camera clock controller > What's the final verdict on RINGOSC_L etc.? > > Konrad We would like to pass RINGOSC_CAL_L field directly in config->l value itself and reuse existing code rather than adding a separate function for lucid ole pll configure. Thanks, Jagadeesh >> >> .../bindings/clock/qcom,sm8450-camcc.yaml | 8 +- >> arch/arm64/boot/dts/qcom/sm8550.dtsi | 15 + >> drivers/clk/qcom/Kconfig | 7 + >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/camcc-sm8550.c | 3585 +++++++++++++++++ >> include/dt-bindings/clock/qcom,sm8550-camcc.h | 187 + >> 6 files changed, 3801 insertions(+), 2 deletions(-) >> create mode 100644 drivers/clk/qcom/camcc-sm8550.c >> create mode 100644 include/dt-bindings/clock/qcom,sm8550-camcc.h >>
On Wed, 14 Jun 2023 at 14:55, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 6/9/2023 9:52 PM, Dmitry Baryshkov wrote: > > On Fri, 9 Jun 2023 at 14:52, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >> > >> Add support for the camera clock controller for camera clients to be > >> able to request for camcc clocks on SM8550 platform. > >> > >> Co-developed-by: Taniya Das <quic_tdas@quicinc.com> > >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >> --- > >> Changes since V3: > >> - No changes > >> Changes since V2: > >> - No changes > >> Changes since V1: > >> - Sorted the PLL names in proper order > >> - Updated all PLL configurations to lower case hex > >> - Reused evo ops instead of adding new ops for ole pll > >> - Moved few clocks to separate patch to fix patch too long error > >> > >> drivers/clk/qcom/Kconfig | 7 + > >> drivers/clk/qcom/Makefile | 1 + > >> drivers/clk/qcom/camcc-sm8550.c | 3405 +++++++++++++++++++++++++++++++ > >> 3 files changed, 3413 insertions(+) > >> create mode 100644 drivers/clk/qcom/camcc-sm8550.c > >> > >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > >> index 9cd1f05d436b..85efed78dc9a 100644 > >> --- a/drivers/clk/qcom/Kconfig > >> +++ b/drivers/clk/qcom/Kconfig > >> @@ -756,6 +756,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 ARM64 || COMPILE_TEST > >> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile > >> index 75d035150118..97c8cefc2fd0 100644 > >> --- a/drivers/clk/qcom/Makefile > >> +++ b/drivers/clk/qcom/Makefile > >> @@ -101,6 +101,7 @@ obj-$(CONFIG_SDX_GCC_75) += gcc-sdx75.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..85f0c1e09b2b > >> --- /dev/null > >> +++ b/drivers/clk/qcom/camcc-sm8550.c > >> @@ -0,0 +1,3405 @@ > >> +// 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_IFACE, > >> + DT_BI_TCXO, > >> +}; > >> + > >> +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_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_CAM_CC_PLL10_OUT_EVEN, > >> + P_CAM_CC_PLL11_OUT_EVEN, > >> + P_CAM_CC_PLL12_OUT_EVEN, > >> +}; > >> + > >> +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 includes RINGOSC_CAL_L_VAL, CAL_L_VAL, L_VAL fields */ > >> + .l = 0x4444003e, > > > > I'd still insist on not touching the config.l field semantics. > > > > We feel it is better to update config->l field and reuse existing code > than adding separate function for lucid ole pll configure. As you probably got it, I'm not convinced that it is a better approach. You are feeding additional data in a single configuration field and passing constant data as variadic one. > > >> + .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, > >> +}; > >> + > > > > [skipped the rest, LGTM] > > > >> + > >> +static struct platform_driver cam_cc_sm8550_driver = { > >> + .probe = cam_cc_sm8550_probe, > >> + .driver = { > >> + .name = "cam_cc-sm8550", > >> + .of_match_table = cam_cc_sm8550_match_table, > >> + }, > >> +}; > >> + > >> +static int __init cam_cc_sm8550_init(void) > >> +{ > >> + return platform_driver_register(&cam_cc_sm8550_driver); > >> +} > >> +subsys_initcall(cam_cc_sm8550_init); > > > > As it was pointed out, this driver is built as a module by default. > > Please perform the tesing and cleanup before sending the driver and > > use module_platform_driver. > > > > We want clock drivers to be probed early in the bootup to avoid any > probe deferrals of consumer drivers. If there is any scenario where > clock drivers are built statically into kernel, then subsys_initcall() > will ensure clock drivers are probed earlier. When built as module, > subsys_initcall() will fallback to module_init() which is same as > module_platform_driver(). Consumer driver probe deferrals are nowadays significantly prevented by using devlink rather than depending on the initialisation level. And I think both GKI and defconfig build camcc as modules. > > Thanks, > Jagadeesh > > >> + > >> +static void __exit cam_cc_sm8550_exit(void) > >> +{ > >> + platform_driver_unregister(&cam_cc_sm8550_driver); > >> +} > >> +module_exit(cam_cc_sm8550_exit); > >> + > >> +MODULE_DESCRIPTION("QTI CAMCC SM8550 Driver"); > >> +MODULE_LICENSE("GPL"); > >> -- > >> 2.40.1 > >> > > > >
On Wed, 14 Jun 2023 at 14:56, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 6/9/2023 6:22 PM, Konrad Dybcio wrote: > > > > > > On 9.06.2023 13:50, Jagadeesh Kona wrote: > >> Add device node for camera clock controller on Qualcomm > >> SM8550 platform. > >> > >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >> --- > >> Changes since V3: > >> - No changes > >> Changes since V2: > >> - No changes > >> Changes since V1: > >> - Padded non-zero address part to 8 hex digits > >> > >> 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 75cd374943eb..4d2d610fc66a 100644 > >> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi > >> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi > >> @@ -5,6 +5,7 @@ > >> > >> #include <dt-bindings/clock/qcom,rpmh.h> > >> #include <dt-bindings/clock/qcom,sm8450-videocc.h> > >> +#include <dt-bindings/clock/qcom,sm8550-camcc.h> > >> #include <dt-bindings/clock/qcom,sm8550-gcc.h> > >> #include <dt-bindings/clock/qcom,sm8550-gpucc.h> > >> #include <dt-bindings/clock/qcom,sm8550-tcsr.h> > >> @@ -2419,6 +2420,20 @@ videocc: clock-controller@aaf0000 { > >> #power-domain-cells = <1>; > >> }; > >> > >> + camcc: clock-controller@ade0000 { > >> + compatible = "qcom,sm8550-camcc"; > >> + reg = <0 0x0ade0000 0 0x20000>; > >> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, > >> + <&bi_tcxo_div2>, > >> + <&bi_tcxo_ao_div2>, > >> + <&sleep_clk>; > >> + power-domains = <&rpmhpd SM8550_MMCX>; > > I see that both MMCX ("mmcx.lvl") and MXC ("mxc.lvl") (and MX, FWIW) > > are consumed on msm-5.15, with the latter one powering camcc PLLs.. > > > > How are they related? Is that resolved internally or does it need > > manual intervention? > > > > Konrad > > These are just different voltage rails, camcc clocks are powered by MMCX > rail and camcc pll's are powered by MXC rail. Consumer drivers need to > take care of voting on these rails properly based on the frequency of > clocks requested. Which rail powers registers of the camcc? Which rail is required to read PLL registers? > > Thanks, > Jagadeesh > > >> + 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>;
On Wed, 14 Jun 2023 at 14:58, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 6/9/2023 6:24 PM, Konrad Dybcio wrote: > > > > > > On 9.06.2023 13:50, Jagadeesh Kona wrote: > >> Add bindings, driver and devicetree node for camera clock controller on > >> SM8550. > >> > >> Jagadeesh Kona (4): > >> dt-bindings: clock: qcom: Add SM8550 camera clock controller > >> clk: qcom: camcc-sm8550: Add camera clock controller driver for SM8550 > >> clk: qcom: camcc-sm8550: Add support for qdss, sleep and xo clocks > >> arm64: dts: qcom: sm8550: Add camera clock controller > > What's the final verdict on RINGOSC_L etc.? > > > > Konrad > > We would like to pass RINGOSC_CAL_L field directly in config->l value > itself and reuse existing code rather than adding a separate function > for lucid ole pll configure. As I wrote in another email, it doesn't sound like a good approach. > > Thanks, > Jagadeesh > > >> > >> .../bindings/clock/qcom,sm8450-camcc.yaml | 8 +- > >> arch/arm64/boot/dts/qcom/sm8550.dtsi | 15 + > >> drivers/clk/qcom/Kconfig | 7 + > >> drivers/clk/qcom/Makefile | 1 + > >> drivers/clk/qcom/camcc-sm8550.c | 3585 +++++++++++++++++ > >> include/dt-bindings/clock/qcom,sm8550-camcc.h | 187 + > >> 6 files changed, 3801 insertions(+), 2 deletions(-) > >> create mode 100644 drivers/clk/qcom/camcc-sm8550.c > >> create mode 100644 include/dt-bindings/clock/qcom,sm8550-camcc.h > >>
On 6/14/2023 5:44 PM, Dmitry Baryshkov wrote: > On Wed, 14 Jun 2023 at 14:55, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 6/9/2023 9:52 PM, Dmitry Baryshkov wrote: >>> On Fri, 9 Jun 2023 at 14:52, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>> >>>> Add support for the camera clock controller for camera clients to be >>>> able to request for camcc clocks on SM8550 platform. >>>> >>>> Co-developed-by: Taniya Das <quic_tdas@quicinc.com> >>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>> --- >>>> Changes since V3: >>>> - No changes >>>> Changes since V2: >>>> - No changes >>>> Changes since V1: >>>> - Sorted the PLL names in proper order >>>> - Updated all PLL configurations to lower case hex >>>> - Reused evo ops instead of adding new ops for ole pll >>>> - Moved few clocks to separate patch to fix patch too long error >>>> >>>> drivers/clk/qcom/Kconfig | 7 + >>>> drivers/clk/qcom/Makefile | 1 + >>>> drivers/clk/qcom/camcc-sm8550.c | 3405 +++++++++++++++++++++++++++++++ >>>> 3 files changed, 3413 insertions(+) >>>> create mode 100644 drivers/clk/qcom/camcc-sm8550.c >>>> >>>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig >>>> index 9cd1f05d436b..85efed78dc9a 100644 >>>> --- a/drivers/clk/qcom/Kconfig >>>> +++ b/drivers/clk/qcom/Kconfig >>>> @@ -756,6 +756,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 ARM64 || COMPILE_TEST >>>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile >>>> index 75d035150118..97c8cefc2fd0 100644 >>>> --- a/drivers/clk/qcom/Makefile >>>> +++ b/drivers/clk/qcom/Makefile >>>> @@ -101,6 +101,7 @@ obj-$(CONFIG_SDX_GCC_75) += gcc-sdx75.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..85f0c1e09b2b >>>> --- /dev/null >>>> +++ b/drivers/clk/qcom/camcc-sm8550.c >>>> @@ -0,0 +1,3405 @@ >>>> +// 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_IFACE, >>>> + DT_BI_TCXO, >>>> +}; >>>> + >>>> +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_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_CAM_CC_PLL10_OUT_EVEN, >>>> + P_CAM_CC_PLL11_OUT_EVEN, >>>> + P_CAM_CC_PLL12_OUT_EVEN, >>>> +}; >>>> + >>>> +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 includes RINGOSC_CAL_L_VAL, CAL_L_VAL, L_VAL fields */ >>>> + .l = 0x4444003e, >>> >>> I'd still insist on not touching the config.l field semantics. >>> >> >> We feel it is better to update config->l field and reuse existing code >> than adding separate function for lucid ole pll configure. > > As you probably got it, I'm not convinced that it is a better > approach. You are feeding additional data in a single configuration > field and passing constant data as variadic one. > Will avoid this approach and will add separate lucid ole pll configure function 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, >>>> +}; >>>> + >>> >>> [skipped the rest, LGTM] >>> >>>> + >>>> +static struct platform_driver cam_cc_sm8550_driver = { >>>> + .probe = cam_cc_sm8550_probe, >>>> + .driver = { >>>> + .name = "cam_cc-sm8550", >>>> + .of_match_table = cam_cc_sm8550_match_table, >>>> + }, >>>> +}; >>>> + >>>> +static int __init cam_cc_sm8550_init(void) >>>> +{ >>>> + return platform_driver_register(&cam_cc_sm8550_driver); >>>> +} >>>> +subsys_initcall(cam_cc_sm8550_init); >>> >>> As it was pointed out, this driver is built as a module by default. >>> Please perform the tesing and cleanup before sending the driver and >>> use module_platform_driver. >>> >> >> We want clock drivers to be probed early in the bootup to avoid any >> probe deferrals of consumer drivers. If there is any scenario where >> clock drivers are built statically into kernel, then subsys_initcall() >> will ensure clock drivers are probed earlier. When built as module, >> subsys_initcall() will fallback to module_init() which is same as >> module_platform_driver(). > > Consumer driver probe deferrals are nowadays significantly prevented > by using devlink rather than depending on the initialisation level. > And I think both GKI and defconfig build camcc as modules. > Will use module_platform_driver() in next series. Thanks, Jagadeesh >> >> Thanks, >> Jagadeesh >> >>>> + >>>> +static void __exit cam_cc_sm8550_exit(void) >>>> +{ >>>> + platform_driver_unregister(&cam_cc_sm8550_driver); >>>> +} >>>> +module_exit(cam_cc_sm8550_exit); >>>> + >>>> +MODULE_DESCRIPTION("QTI CAMCC SM8550 Driver"); >>>> +MODULE_LICENSE("GPL"); >>>> -- >>>> 2.40.1 >>>> >>> >>> > > >
On 6/14/2023 5:47 PM, Dmitry Baryshkov wrote: > On Wed, 14 Jun 2023 at 14:58, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 6/9/2023 6:24 PM, Konrad Dybcio wrote: >>> >>> >>> On 9.06.2023 13:50, Jagadeesh Kona wrote: >>>> Add bindings, driver and devicetree node for camera clock controller on >>>> SM8550. >>>> >>>> Jagadeesh Kona (4): >>>> dt-bindings: clock: qcom: Add SM8550 camera clock controller >>>> clk: qcom: camcc-sm8550: Add camera clock controller driver for SM8550 >>>> clk: qcom: camcc-sm8550: Add support for qdss, sleep and xo clocks >>>> arm64: dts: qcom: sm8550: Add camera clock controller >>> What's the final verdict on RINGOSC_L etc.? >>> >>> Konrad >> >> We would like to pass RINGOSC_CAL_L field directly in config->l value >> itself and reuse existing code rather than adding a separate function >> for lucid ole pll configure. > > As I wrote in another email, it doesn't sound like a good approach. > Will avoid this approach and use separate clk_lucid_ole_pll_configure() to configure lucid ole PLL's in next series. Thanks, Jagadeesh >> >> Thanks, >> Jagadeesh >> >>>> >>>> .../bindings/clock/qcom,sm8450-camcc.yaml | 8 +- >>>> arch/arm64/boot/dts/qcom/sm8550.dtsi | 15 + >>>> drivers/clk/qcom/Kconfig | 7 + >>>> drivers/clk/qcom/Makefile | 1 + >>>> drivers/clk/qcom/camcc-sm8550.c | 3585 +++++++++++++++++ >>>> include/dt-bindings/clock/qcom,sm8550-camcc.h | 187 + >>>> 6 files changed, 3801 insertions(+), 2 deletions(-) >>>> create mode 100644 drivers/clk/qcom/camcc-sm8550.c >>>> create mode 100644 include/dt-bindings/clock/qcom,sm8550-camcc.h >>>> > > >
On 6/14/2023 5:45 PM, Dmitry Baryshkov wrote: > On Wed, 14 Jun 2023 at 14:56, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 6/9/2023 6:22 PM, Konrad Dybcio wrote: >>> >>> >>> On 9.06.2023 13:50, Jagadeesh Kona wrote: >>>> Add device node for camera clock controller on Qualcomm >>>> SM8550 platform. >>>> >>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>> --- >>>> Changes since V3: >>>> - No changes >>>> Changes since V2: >>>> - No changes >>>> Changes since V1: >>>> - Padded non-zero address part to 8 hex digits >>>> >>>> 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 75cd374943eb..4d2d610fc66a 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi >>>> @@ -5,6 +5,7 @@ >>>> >>>> #include <dt-bindings/clock/qcom,rpmh.h> >>>> #include <dt-bindings/clock/qcom,sm8450-videocc.h> >>>> +#include <dt-bindings/clock/qcom,sm8550-camcc.h> >>>> #include <dt-bindings/clock/qcom,sm8550-gcc.h> >>>> #include <dt-bindings/clock/qcom,sm8550-gpucc.h> >>>> #include <dt-bindings/clock/qcom,sm8550-tcsr.h> >>>> @@ -2419,6 +2420,20 @@ videocc: clock-controller@aaf0000 { >>>> #power-domain-cells = <1>; >>>> }; >>>> >>>> + camcc: clock-controller@ade0000 { >>>> + compatible = "qcom,sm8550-camcc"; >>>> + reg = <0 0x0ade0000 0 0x20000>; >>>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >>>> + <&bi_tcxo_div2>, >>>> + <&bi_tcxo_ao_div2>, >>>> + <&sleep_clk>; >>>> + power-domains = <&rpmhpd SM8550_MMCX>; >>> I see that both MMCX ("mmcx.lvl") and MXC ("mxc.lvl") (and MX, FWIW) >>> are consumed on msm-5.15, with the latter one powering camcc PLLs.. >>> >>> How are they related? Is that resolved internally or does it need >>> manual intervention? >>> >>> Konrad >> >> These are just different voltage rails, camcc clocks are powered by MMCX >> rail and camcc pll's are powered by MXC rail. Consumer drivers need to >> take care of voting on these rails properly based on the frequency of >> clocks requested. > > Which rail powers registers of the camcc? Which rail is required to > read PLL registers? > MMCX rail is required to access camcc registers, both MMCX and MXC are required to read PLL registers. MXC rail should be left ON from bootloaders during bootup and hence does not require explicit voting. Thanks, Jagadeesh >> >> Thanks, >> Jagadeesh >> >>>> + 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>; > > >
On 23.06.2023 18:45, Jagadeesh Kona wrote: > > > On 6/14/2023 5:45 PM, Dmitry Baryshkov wrote: >> On Wed, 14 Jun 2023 at 14:56, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>> >>> >>> >>> On 6/9/2023 6:22 PM, Konrad Dybcio wrote: >>>> >>>> >>>> On 9.06.2023 13:50, Jagadeesh Kona wrote: >>>>> Add device node for camera clock controller on Qualcomm >>>>> SM8550 platform. >>>>> >>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>>> --- >>>>> Changes since V3: >>>>> - No changes >>>>> Changes since V2: >>>>> - No changes >>>>> Changes since V1: >>>>> - Padded non-zero address part to 8 hex digits >>>>> >>>>> 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 75cd374943eb..4d2d610fc66a 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi >>>>> @@ -5,6 +5,7 @@ >>>>> >>>>> #include <dt-bindings/clock/qcom,rpmh.h> >>>>> #include <dt-bindings/clock/qcom,sm8450-videocc.h> >>>>> +#include <dt-bindings/clock/qcom,sm8550-camcc.h> >>>>> #include <dt-bindings/clock/qcom,sm8550-gcc.h> >>>>> #include <dt-bindings/clock/qcom,sm8550-gpucc.h> >>>>> #include <dt-bindings/clock/qcom,sm8550-tcsr.h> >>>>> @@ -2419,6 +2420,20 @@ videocc: clock-controller@aaf0000 { >>>>> #power-domain-cells = <1>; >>>>> }; >>>>> >>>>> + camcc: clock-controller@ade0000 { >>>>> + compatible = "qcom,sm8550-camcc"; >>>>> + reg = <0 0x0ade0000 0 0x20000>; >>>>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >>>>> + <&bi_tcxo_div2>, >>>>> + <&bi_tcxo_ao_div2>, >>>>> + <&sleep_clk>; >>>>> + power-domains = <&rpmhpd SM8550_MMCX>; >>>> I see that both MMCX ("mmcx.lvl") and MXC ("mxc.lvl") (and MX, FWIW) >>>> are consumed on msm-5.15, with the latter one powering camcc PLLs.. >>>> >>>> How are they related? Is that resolved internally or does it need >>>> manual intervention? >>>> >>>> Konrad >>> >>> These are just different voltage rails, camcc clocks are powered by MMCX >>> rail and camcc pll's are powered by MXC rail. Consumer drivers need to >>> take care of voting on these rails properly based on the frequency of >>> clocks requested. >> >> Which rail powers registers of the camcc? Which rail is required to >> read PLL registers? >> > MMCX rail is required to access camcc registers, both MMCX and MXC are required to read PLL registers. MXC rail should be left ON from bootloaders during bootup and hence does not require explicit voting. That's a bad approach. We have a sync_state callback in rpmhpd that kills unused-from-linux-POV power rails, so Linux should be made aware of any and all requirements there. Konrad > > Thanks, > Jagadeesh > >>> >>> Thanks, >>> Jagadeesh >>> >>>>> + 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>; >> >> >>