[PATCH 1/8] clk/qcom: Add SM6350 clock driver

[Luca Weiss]

Hi Neil,

On Fri Jun 20, 2025 at 11:07 AM CEST, Luca Weiss wrote:
> On Fri Jun 20, 2025 at 10:54 AM CEST, Neil Armstrong wrote:
>> On 18/06/2025 16:25, Luca Weiss wrote:
>>> Add Clock driver for the GCC block found in the SM6350 SoC.
>>> 
>>> Signed-off-by: Luca Weiss <luca.weiss at fairphone.com>
>>> ---
>
> <snip>
>
>>> +static const struct freq_tbl ftbl_gcc_sdcc2_apps_clk_src[] = {
>>> +	F(400000, CFG_CLK_SRC_CXO, 12, 1, 4),
>>> +	F(9600000, CFG_CLK_SRC_CXO, 2, 0, 0),
>>> +	F(19200000, CFG_CLK_SRC_CXO, 1, 0, 0),
>>> +	F(25000000, CFG_CLK_SRC_GPLL0_ODD, 8, 0, 0),
>>> +	F(50000000, CFG_CLK_SRC_GPLL0_ODD, 4, 0, 0),
>>> +	F(100000000, CFG_CLK_SRC_GPLL0_ODD, 2, 0, 0),
>>> +	F(202000000, CFG_CLK_SRC_GPLL7, 4, 0, 0),
>>> +	{}
>>> +};
>>> +
>>> +static struct pll_vote_clk gpll7_vote_clk = {
>>> +	.status = APCS_GPLL7_STATUS,
>>> +	.status_bit = BIT(31),
>>> +	.ena_vote = APCS_GPLLX_ENA_REG,
>>> +	.vote_bit = BIT(7),
>>> +};
>>> +
>>> +static ulong sm6350_set_rate(struct clk *clk, ulong rate)
>>> +{
>>> +	struct msm_clk_priv *priv = dev_get_priv(clk->dev);
>>> +	const struct freq_tbl *freq;
>>> +
>>> +	if (clk->id < priv->data->num_clks)
>>> +		debug("%s: %s, requested rate=%ld\n", __func__,
>>> +		      priv->data->clks[clk->id].name, rate);
>>> +
>>> +	switch (clk->id) {
>>> +	case GCC_QUPV3_WRAP1_S3_CLK: /*UART9*/
>>> +		freq = qcom_find_freq(ftbl_gcc_qupv3_wrap1_s3_clk_src, rate);
>>> +		clk_rcg_set_rate_mnd(priv->base, GCC_SE12_UART_RCG_REG,
>>> +				     freq->pre_div, freq->m, freq->n, freq->src,
>>> +				     16);
>>> +
>>> +		return freq->freq;
>>> +	case GCC_SDCC2_APPS_CLK:
>>> +		/* Enable GPLL7 so that we can point SDCC2_APPS_CLK_SRC at it */
>>> +		clk_enable_gpll0(priv->base, &gpll7_vote_clk);
>>> +		freq = qcom_find_freq(ftbl_gcc_sdcc2_apps_clk_src, rate);
>>> +		printf("%s: got freq %u\n", __func__, freq->freq);
>>> +		WARN(freq->src != CFG_CLK_SRC_GPLL7,
>>> +		     "SDCC2_APPS_CLK_SRC not set to GPLL7, requested rate %lu\n",
>>> +		     rate);
>>
>> Why not enabling GPLL7 only if freq->src == CFG_CLK_SRC_GPLL7 ??
>
> I honestly just copy-pasted this part, I didn't think too much about
> this code. But also doesn't the code do the wrong thing when pointing to
> any freq apart from 202 MHz? The warn and the clk_rcg_set_rate_mnd call
> below will complain about/set the wrong parent?
>
> Seems that construct is in clock drivers for qcm2290, sm6115 and sm8250.
>
> Also other clk drivers like clock-sc7280.c don't have any set_rate
> handling for this clock either and SD card supposedly works fine on the
> Shiftphone 8.

Do you have any feedback how the clock drivers would be correct?

Regards
Luca