Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Ritesh Harjani <riteshh@codeaurora.org>]

Hi Adrian,


On 8/22/2016 11:50 AM, Adrian Hunter wrote:
> On 19/08/16 16:31, Ritesh Harjani wrote:
>> Hi,
>>
>>
>> On 8/19/2016 6:34 PM, Adrian Hunter wrote:
>>> On 19/08/16 07:36, Ritesh Harjani wrote:
>>>> sdhci-msm controller may have different clk-rates for each
>>>> bus speed mode. Thus implement set_clock callback for
>>>> sdhci-msm driver.
>>>>
>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>> ---
>>>>  drivers/mmc/host/sdhci-msm.c | 103
>>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>> index 7c032c3..c0ad9c2 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>>>      struct mmc_host *mmc;
>>>>      bool use_14lpp_dll_reset;
>>>>      struct sdhci_msm_pltfm_data *pdata;
>>>> +    u32 clk_rate;
>>>>  };
>>>>
>>>>  /* Platform specific tuning */
>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct
>>>> sdhci_host *host)
>>>>      return msm_host->pdata->clk_table[0];
>>>>  }
>>>>
>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>>>> +                    u32 req_clk)
>>>> +{
>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>> +    int count = msm_host->pdata->clk_table_sz;
>>>> +    unsigned int sel_clk = -1;
>>>> +    int cnt;
>>>> +
>>>> +    if (req_clk < sdhci_msm_get_min_clock(host)) {
>>>> +        sel_clk = sdhci_msm_get_min_clock(host);
>>>> +        return sel_clk;
>>>> +    }
>>>> +
>>>> +    for (cnt = 0; cnt < count; cnt++) {
>>>> +        if (msm_host->pdata->clk_table[cnt] > req_clk) {
>>>> +            break;
>>>> +        } else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>> +            break;
>>>> +        } else {
>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>> +        }
>>>> +    }
>>>
>>> 'else' is not needed after 'break' but can't this be simpler e.g.
>>>
>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host
>>> *msm_host, u32 req_clk)
>>> {
>>>     int count = msm_host->pdata->clk_table_sz;
>>>     unsigned int sel_clk = -1;
>>>
>>>     while (count--) {
>>>         sel_clk = msm_host->pdata->clk_table[count];
>>>         if (req_clk >= sel_clk)
>>>             return sel_clk;
>>>     }
>>>
>>>     return sel_clk;
>>> }
>>
>> Ok, sure I will check and get back on this.
>>
>>
>>>
>>>
>>>> +    return sel_clk;
>>>> +}
>>>
>>> Blank line needed
>> Ok done.
>>
>>>
>>>> +/**
>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>> + *
>>>> + * Description:
>>>> + * Implement MSM version of sdhci_set_clock.
>>>> + * This is required since MSM controller does not
>>>> + * use internal divider and instead directly control
>>>> + * the GCC clock as per HW recommendation.
>>>> + **/
>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>> +{
>>>> +    u16 clk;
>>>> +    unsigned long timeout;
>>>> +
>>>> +    host->mmc->actual_clock = 0;
>>>> +
>>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +    if (clock == 0)
>>>> +        return;
>>>
>>> Should set host->mmc->actual_clock to the actual rate somewhere.
>> Since MSM controller does not uses divider then there is no need of having
>> actual clock since it should be same as host->clock.
>
> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate?
So I was assuming we need host->mmc->actual_clock rate if host->clock is 
not the actual rate.
Ok, so I will update actual_clock to host->clock itself.

pls, let me know if any concerns.
>
>>
>> That's why I had kept it 0 intentionally. But I will add a comment
>> here then.
>>
>>>
>>>> +
>>>> +    /*
>>>> +     * MSM controller do not use clock divider.
>>>> +     * Thus read SDHCI_CLOCK_CONTROL and only enable
>>>> +     * clock with no divider value programmed.
>>>> +     */
>>>> +    clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +    clk |= SDHCI_CLOCK_INT_EN;
>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +    /* Wait max 20 ms */
>>>> +    timeout = 20;
>>>> +    while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>>> +        & SDHCI_CLOCK_INT_STABLE)) {
>>>> +        if (timeout == 0) {
>>>> +            pr_err("%s: Internal clock never stabilised.\n",
>>>> +                   mmc_hostname(host->mmc));
>>>> +            return;
>>>> +        }
>>>> +        timeout--;
>>>> +        mdelay(1);
>>>> +    }
>>>> +
>>>> +    clk |= SDHCI_CLOCK_CARD_EN;
>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>> +}
>>>> +
>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int
>>>> clock)
>>>> +{
>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>> +    u32 msm_clock = 0;
>>>> +    int rc = 0;
>>>> +
>>>> +    if (!clock)
>>>> +        goto out;
>>>> +
>>>> +    if (clock != msm_host->clk_rate) {
>>>> +        msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>>>> +        rc = clk_set_rate(msm_host->clk, msm_clock);
>>>> +        if (rc) {
>>>> +            pr_err("%s: failed to set clock at rate %u, requested clock
>>>> rate %u\n",
>>>> +                mmc_hostname(host->mmc), msm_clock, clock);
>>>> +            goto out;
>>>> +        }
>>>> +        msm_host->clk_rate = clock;
>>>> +        pr_debug("%s: setting clock at rate %lu\n",
>>>> +            mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>>> +    }
>>>> +out:
>>>> +    __sdhci_msm_set_clock(host, clock);
>>>> +}
>>>> +
>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>      {},
>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>      .reset = sdhci_reset,
>>>> -    .set_clock = sdhci_set_clock,
>>>> +    .set_clock = sdhci_msm_set_clock,
>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>      .set_bus_width = sdhci_set_bus_width,
>>>>
>>>
>>
>