Re: [PATCH v7 08/14] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Ritesh Harjani <riteshh@codeaurora.org>]

Hi,

On 11/15/2016 10:40 AM, Ritesh Harjani wrote:
> Hi Stephen/Adrian,
>
> On 11/15/2016 1:07 AM, Stephen Boyd wrote:
>> On 11/14, Ritesh Harjani wrote:
>>> @@ -577,6 +578,90 @@ static unsigned int
>>> sdhci_msm_get_min_clock(struct sdhci_host *host)
>>>      return SDHCI_MSM_MIN_CLOCK;
>>>  }
>>>
>>> +/**
>>> + * __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;
>>> +
>>> +    /*
>>> +     * Keep actual_clock as zero -
>>> +     * - since there is no divider used so no need of having
>>> actual_clock.
>>> +     * - MSM controller uses SDCLK for data timeout calculation. If
>>> +     *   actual_clock is zero, host->clock is taken for calculation.
>>> +     */
>>> +    host->mmc->actual_clock = 0;
>>> +
>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>> +
>>> +    if (clock == 0)
>>> +        return;
>>> +
>>> +    /*
>>> +     * 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);
>>
>> This is almost a copy/paste of sdhci_set_clock(). Can we make
>> sdhci_set_clock() call a __sdhci_set_clock() function that takes
>> unsigned int clock, and also a flag indicating if we want to set
>> the internal clock divider or not? Then we can call
>> __sdhci_set_clock() from sdhci_set_clock() with (clock, true) as
>> arguments and (clock, false).
Actually what you may be referring here is some sort of quirks which is 
not entertained any more for sdhci driver.
sdhci is tending towards becoming a library and hence such changes are 
restricted.

But I think we may do below changes to avoid duplication of code which 
enables the sdhci internal clock and waits for internal clock to be stable.

Adrian, could you please tell if this should be ok?
Then we may be able to call for sdhci_set_clock_enable function from
sdhci_msm_set_clock.


diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 42ef3eb..28e605c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1343,19 +1343,8 @@ u16 sdhci_calc_clk(struct sdhci_host *host, 
unsigned int clock,
  }
  EXPORT_SYMBOL_GPL(sdhci_calc_clk);

-void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+void sdhci_set_clock_enable(struct sdhci_host *host, unsigned short clk)
  {
-       u16 clk;
-       unsigned long timeout;
-
-       host->mmc->actual_clock = 0;
-
-       sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
-
-       if (clock == 0)
-               return;
-
-       clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);

         clk |= SDHCI_CLOCK_INT_EN;
         sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
@@ -1377,6 +1366,24 @@ void sdhci_set_clock(struct sdhci_host *host, 
unsigned int clock)
         clk |= SDHCI_CLOCK_CARD_EN;
         sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
  }
+EXPORT_SYMBOL_GPL(sdhci_set_clock_enable);
+
+void sdhci_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;
+
+       clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
+
+       sdhci_set_clock_enable(host, clk);
+}
  EXPORT_SYMBOL_GPL(sdhci_set_clock);

  static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char 
mode,
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 766df17..43382e1 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -681,6 +681,7 @@ static inline bool sdhci_sdio_irq_enabled(struct 
sdhci_host *host)
  u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
                    unsigned int *actual_clock);
  void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
+void sdhci_set_clock_enable(struct sdhci_host *host, unsigned short clk);
  void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
                      unsigned short vdd);
  void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,



> Adrian,
> Could you please comment here ?
>
>>
>>> +}
>>> +
>>> +/* sdhci_msm_set_clock - Called with (host->lock) spinlock held. */
>>> +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);
>>> +    int rc;
>>> +
>>> +    if (!clock) {
>>> +        msm_host->clk_rate = clock;
>>> +        goto out;
>>> +    }
>>> +
>>> +    spin_unlock_irq(&host->lock);
>>> +    if (clock != msm_host->clk_rate) {
>>
>> Why do we need to check here? Can't we call clk_set_rate()
>> Unconditionally?
> Since it may so happen that above layers may call for ->set_clock
> function with same requested clock more than once, hence we cache the
> host->clock here.
> Also, since requested clock (host->clock) can be say 400Mhz but the
> actual pltfm supported clock would be say 384MHz.
>
>
>>
>>> +        rc = clk_set_rate(msm_host->clk, clock);
>>> +        if (rc) {
>>> +            pr_err("%s: Failed to set clock at rate %u\n",
>>> +                   mmc_hostname(host->mmc), clock);
>>> +            spin_lock_irq(&host->lock);
>>> +            goto out;
>>
>> Or replace the above two lines with goto err;
> Ok, I will have another label out_lock instead of err.
>>
>>> +        }
>>> +        msm_host->clk_rate = clock;
>>> +        pr_debug("%s: Setting clock at rate %lu\n",
>>> +             mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>> +    }
>>
>> And put an err label here.
> will put the label here as out_lock;
>>
>>> +    spin_lock_irq(&host->lock);
>>> +out:
>>> +    __sdhci_msm_set_clock(host, clock);
>>> +}
>>> +
>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>      {},
>>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project