Re: [PATCH 5/8] clk: qcom: add driver for SM8150 SoC

Hi Caleb,

Caleb Connolly caleb.connolly@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_babchuk@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?