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 <[email protected]>
>> ---
<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.
>
> Apart that, looks good :-)
>
> Reviewed-by: Neil Armstrong <[email protected]>
Thanks!
Regards
Luca
