Hi Caleb,
Caleb Connolly <caleb.conno...@linaro.org> writes: > On 29/02/2024 14:21, Volodymyr Babchuk wrote: >> Add clock, reset and power domain driver for SM8150. Driver code is >> based on the similar U-Boot drivers. All constants are taken from the >> corresponding Linux driver. >> >> This driver supports clock rate setting only for the debug UART and >> RGMII/Ethernet modules, because this is all I can test right now. >> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babc...@epam.com> >> --- >> >> drivers/clk/qcom/Kconfig | 8 ++ >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/clock-qcom.h | 1 + >> drivers/clk/qcom/clock-sm8150.c | 234 ++++++++++++++++++++++++++++++++ >> 4 files changed, 244 insertions(+) >> create mode 100644 drivers/clk/qcom/clock-sm8150.c >> >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig >> index 0df0d1881a..18ccf6a45e 100644 >> --- a/drivers/clk/qcom/Kconfig >> +++ b/drivers/clk/qcom/Kconfig >> @@ -47,6 +47,14 @@ config CLK_QCOM_SDM845 >> on the Snapdragon 845 SoC. This driver supports the clocks >> and resets exposed by the GCC hardware block. >> >> +config CLK_QCOM_SM8150 >> + bool "Qualcomm SM8150 GCC" >> + select CLK_QCOM >> + help >> + Say Y here to enable support for the Global Clock Controller >> + on the Snapdragon 8150 SoC. This driver supports the clocks >> + and resets exposed by the GCC hardware block. >> + >> endmenu >> >> endif >> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile >> index cb179fdac5..12c09ba19e 100644 >> --- a/drivers/clk/qcom/Makefile >> +++ b/drivers/clk/qcom/Makefile >> @@ -8,3 +8,4 @@ obj-$(CONFIG_CLK_QCOM_APQ8016) += clock-apq8016.o >> obj-$(CONFIG_CLK_QCOM_APQ8096) += clock-apq8096.o >> obj-$(CONFIG_CLK_QCOM_IPQ4019) += clock-ipq4019.o >> obj-$(CONFIG_CLK_QCOM_QCS404) += clock-qcs404.o >> +obj-$(CONFIG_CLK_QCOM_SM8150) += clock-sm8150.o >> diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h >> index 12a1eaec2b..41107df216 100644 >> --- a/drivers/clk/qcom/clock-qcom.h >> +++ b/drivers/clk/qcom/clock-qcom.h >> @@ -9,6 +9,7 @@ >> >> #define CFG_CLK_SRC_CXO (0 << 8) >> #define CFG_CLK_SRC_GPLL0 (1 << 8) >> +#define CFG_CLK_SRC_GPLL7_MAIN (3 << 8) > Please follow the existing scheme and remove the _MAIN suffix. Ah, yes, planned to do this, but looks like lost it when created a final version of the patch. > I'd also be fine with converting all of these definitions to follow how > the Linux drivers name them. This should be done in the separate patch, I think. [...] >> +static int sm8150_clk_enable(struct clk *clk) >> +{ >> + struct msm_clk_priv *priv = dev_get_priv(clk->dev); >> + >> + clk_enable_gpll0(priv->base, &gpll7_vote_clk); > This will write to the GPLL for every single clock, I think you're > missing a switch case block here. Yes, you are right. >> + qcom_gate_clk_en(priv, clk->id); >> + >> + return 0; >> +} >> + >> +static const struct qcom_reset_map sm8150_gcc_resets[] = { >> + [GCC_EMAC_BCR] = { 0x6000 }, >> + [GCC_GPU_BCR] = { 0x71000 }, >> + [GCC_MMSS_BCR] = { 0xb000 }, >> + [GCC_NPU_BCR] = { 0x4d000 }, >> + [GCC_PCIE_0_BCR] = { 0x6b000 }, >> + [GCC_PCIE_0_PHY_BCR] = { 0x6c01c }, >> + [GCC_PCIE_1_BCR] = { 0x8d000 }, >> + [GCC_PCIE_1_PHY_BCR] = { 0x8e01c }, >> + [GCC_PCIE_PHY_BCR] = { 0x6f000 }, >> + [GCC_PDM_BCR] = { 0x33000 }, >> + [GCC_PRNG_BCR] = { 0x34000 }, >> + [GCC_QSPI_BCR] = { 0x24008 }, >> + [GCC_QUPV3_WRAPPER_0_BCR] = { 0x17000 }, >> + [GCC_QUPV3_WRAPPER_1_BCR] = { 0x18000 }, >> + [GCC_QUPV3_WRAPPER_2_BCR] = { 0x1e000 }, >> + [GCC_QUSB2PHY_PRIM_BCR] = { 0x12000 }, >> + [GCC_QUSB2PHY_SEC_BCR] = { 0x12004 }, >> + [GCC_USB3_PHY_PRIM_BCR] = { 0x50000 }, >> + [GCC_USB3_DP_PHY_PRIM_BCR] = { 0x50008 }, >> + [GCC_USB3_PHY_SEC_BCR] = { 0x5000c }, >> + [GCC_USB3PHY_PHY_SEC_BCR] = { 0x50010 }, >> + [GCC_SDCC2_BCR] = { 0x14000 }, >> + [GCC_SDCC4_BCR] = { 0x16000 }, >> + [GCC_TSIF_BCR] = { 0x36000 }, >> + [GCC_UFS_CARD_BCR] = { 0x75000 }, >> + [GCC_UFS_PHY_BCR] = { 0x77000 }, >> + [GCC_USB30_PRIM_BCR] = { 0xf000 }, >> + [GCC_USB30_SEC_BCR] = { 0x10000 }, >> + [GCC_USB_PHY_CFG_AHB2PHY_BCR] = { 0x6a000 }, >> +}; >> + >> +static const struct qcom_power_map sm8150_gcc_power_domains[] = { > This is really nice! Certainly better than my approach [1]. > Please reorder the patches so that the power domain driver is added > before this so we don't break future bisects. power domain driver is added in the previous patch: "[PATCH 4/8] clk: qcom: add support for power domains uclass" >> + [EMAC_GDSC] = { 0x6004 }, >> + [PCIE_0_GDSC] = { 0x6b004 }, >> + [PCIE_1_GDSC] = { 0x8d004 }, >> + [UFS_CARD_GDSC] = { 0x75004 }, >> + [UFS_PHY_GDSC] = { 0x7704 }, >> + [USB30_PRIM_GDSC] = { 0x6004 }, >> + [USB30_SEC_GDSC] = { 0x10004 }, >> +}; >> + >> + >> +static struct msm_clk_data sm8150_clk_data = { >> + .resets = sm8150_gcc_resets, >> + .num_resets = ARRAY_SIZE(sm8150_gcc_resets), >> + .clks = sm8150_clks, >> + .num_clks = ARRAY_SIZE(sm8150_clks), >> + .power_domains = sm8150_gcc_power_domains, >> + .num_power_domains = ARRAY_SIZE(sm8150_gcc_power_domains), >> + >> + .enable = sm8150_clk_enable, >> + .set_rate = sm8150_clk_set_rate, >> +}; >> + >> +static const struct udevice_id gcc_sm8150_of_match[] = { >> + { >> + .compatible = "qcom,gcc-sm8150", >> + .data = (ulong)&sm8150_clk_data, >> + }, >> + { } >> +}; >> + >> +U_BOOT_DRIVER(gcc_sm8150) = { >> + .name = "gcc_sm8150", >> + .id = UCLASS_NOP, >> + .of_match = gcc_sm8150_of_match, >> + .bind = qcom_cc_bind, >> + .flags = DM_FLAG_PRE_RELOC, >> +}; > > Just in case it's useful to you, I have some work in progress patches > for dumping the clock configuration, I hope to upstream these eventually > but in the mean time you can find them here: This is what bothers me now. Looks like I jumped in the middle of your USB series and Sumit's Qulacom device tree binding series. I don't want to interfere with the ongoing work, so probably I postpone posting my v2 unless at least Sumit's series are taken into the mainline. On other hand, there are couple of patches that are not directly related to the SM8155P-ADP, like my fix to the clock driver or already mentioned "clk: qcom: add support for power domains uclass", which you can reuse. So I can post them separately or you can include them into your series if you want. What is your opinion? -- WBR, Volodymyr