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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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?

-- 
WBR, Volodymyr