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.we...@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
