Re: [PATCH v4 12/13] mmc: mmci: add explicit clk control

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Thanks Ulf for reviewing.

On 30/05/14 11:25, Ulf Hansson wrote:
> On 28 May 2014 15:47,  <srinivas.kandagatla@linaro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> On Controllers like Qcom SD card controller where cclk is mclk and mclk should
>> be directly controlled by the driver.
>>
>> This patch adds support to control mclk directly in the driver, and also
>> adds explicit_mclk_control flag in variant structure giving more flexibility
>> to the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/mmc/host/mmci.c | 96 ++++++++++++++++++++++++++++++++-----------------
>>   drivers/mmc/host/mmci.h |  2 ++
>>   2 files changed, 65 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 202f2d5..6eb0a29 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -72,6 +72,7 @@ static unsigned int fmax = 515633;
>>    * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
>>    * @busy_detect: true if busy detection on dat0 is supported
>>    * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>> + * @explicit_mclk_control: enable explicit mclk control in driver.
>>    */
>>   struct variant_data {
>>          unsigned int            clkreg;
>> @@ -93,6 +94,7 @@ struct variant_data {
>>          bool                    pwrreg_clkgate;
>>          bool                    busy_detect;
>>          bool                    pwrreg_nopower;
>> +       bool                    explicit_mclk_control;
>>   };
>>
>>   static struct variant_data variant_arm = {
>> @@ -199,6 +201,7 @@ static struct variant_data variant_qcom = {
>>          .datalength_bits        = 24,
>>          .pwrreg_powerup         = MCI_PWR_UP,
>>          .f_max                  = 208000000,
>> +       .explicit_mclk_control  = true,
>
> This should go in patch3 instead.

yes.. will get this in to the patch3 too.

>
>>   };
>>
>>   static int mmci_card_busy(struct mmc_host *mmc)
>> @@ -301,7 +304,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>>          host->cclk = 0;
>>
>>          if (desired) {
>> -               if (desired >= host->mclk) {
>> +               if (variant->explicit_mclk_control) {
>> +                       host->cclk = host->mclk;
>> +               } else if (desired >= host->mclk) {
>>                          clk = MCI_CLK_BYPASS;
>>                          if (variant->st_clkdiv)
>>                                  clk |= MCI_ST_UX500_NEG_EDGE;
>> @@ -1340,6 +1345,18 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>          if (!ios->clock && variant->pwrreg_clkgate)
>>                  pwr &= ~MCI_PWR_ON;
>>
>> +       if ((host->variant->explicit_mclk_control) &&
>> +           (ios->clock != host->mclk_req)) {
>> +               int rc = clk_set_rate(host->clk, ios->clock);
>> +               if (rc < 0) {
>> +                       dev_err(mmc_dev(host->mmc),
>> +                               "Error setting clock rate (%d)\n", rc);
>> +               } else {
>> +                       host->mclk = clk_get_rate(host->clk);
>> +                       host->mclk_req = ios->clock;
>> +               }
>> +       }
>> +
>>          spin_lock_irqsave(&host->lock, flags);
>>
>>          mmci_set_clkreg(host, ios->clock);
>> @@ -1490,19 +1507,6 @@ static int mmci_probe(struct amba_device *dev,
>>          host->plat = plat;
>>          host->variant = variant;
>>          host->mclk = clk_get_rate(host->clk);
>> -       /*
>> -        * According to the spec, mclk is max 100 MHz,
>> -        * so we try to adjust the clock down to this,
>> -        * (if possible).
>> -        */
>> -       if (host->mclk > host->variant->f_max) {
>> -               ret = clk_set_rate(host->clk, host->variant->f_max);
>> -               if (ret < 0)
>> -                       goto clk_disable;
>> -               host->mclk = clk_get_rate(host->clk);
>> -               dev_dbg(mmc_dev(mmc), "eventual mclk rate: %u Hz\n",
>> -                       host->mclk);
>> -       }
>
> The above code is relevant for Qcom as well, I don't think you need
> change/move it.
Yes, you are right. I will fix it in next version.
>
>>
>>          host->phybase = dev->res.start;
>>          host->base = devm_ioremap_resource(&dev->dev, &dev->res);
>> @@ -1511,25 +1515,51 @@ static int mmci_probe(struct amba_device *dev,
>>                  goto clk_disable;
>>          }
>>
>> -       /*
>> -        * The ARM and ST versions of the block have slightly different
>> -        * clock divider equations which means that the minimum divider
>> -        * differs too.
>> -        */
>> -       if (variant->st_clkdiv)
>> -               mmc->f_min = DIV_ROUND_UP(host->mclk, 257);
>
> I think you could simplify the change of fixing with f_min and f_max.
>
> Start by, just add another statement here "else if
> (variant->explicit_mclk_control)" and do the clk_round_rate()
>
>> -       else
>> -               mmc->f_min = DIV_ROUND_UP(host->mclk, 512);
>> -       /*
>> -        * If no maximum operating frequency is supplied, fall back to use
>> -        * the module parameter, which has a (low) default value in case it
>> -        * is not specified. Either value must not exceed the clock rate into
>> -        * the block, of course.
>> -        */
>> -       if (mmc->f_max)
>> -               mmc->f_max = min(host->mclk, mmc->f_max);
>> -       else
>> -               mmc->f_max = min(host->mclk, fmax);
>
> Further replace the above with:
> if (mmc->f_max)
>       mmc->f_max = variant->explicit_mclk_control ? min(mmc->f_max,
> variant->f_max) : min(host->mclk, mmc->f_max);
> else
>       mmc->f_max = variant->explicit_mclk_control ? fmax : min(host->mclk, fmax);
>
> That should do it, right?
yes, that will work. It simplifies the logic too.

>
>> +       if (variant->explicit_mclk_control) {
>> +               /* get the nearest minimum clock to 100Khz */
>> +               mmc->f_min = clk_round_rate(host->clk, 100000);
>> +
>> +               if (mmc->f_max)
>> +                       mmc->f_max = min(host->variant->f_max, mmc->f_max);
>> +               else
>> +                       mmc->f_max = min(host->variant->f_max, fmax);
>> +
>> +       } else {
>> +               /*
>> +                * According to the spec, mclk is max 100 MHz,
>> +                * so we try to adjust the clock down to this,
>> +                * (if possible).
>> +                */
>> +               if (host->mclk > host->variant->f_max) {
>> +                       ret = clk_set_rate(host->clk, host->variant->f_max);
>> +                       if (ret < 0)
>> +                               goto clk_disable;
>> +                       host->mclk = clk_get_rate(host->clk);
>> +                       dev_dbg(mmc_dev(mmc), "eventual mclk rate: %u Hz\n",
>> +                               host->mclk);
>> +               }
>> +               /*
>> +                * The ARM and ST versions of the block have slightly different
>> +                * clock divider equations which means that the minimum divider
>> +                * differs too.
>> +                */
>> +               if (variant->st_clkdiv)
>> +                       mmc->f_min = DIV_ROUND_UP(host->mclk, 257);
>> +               else
>> +                       mmc->f_min = DIV_ROUND_UP(host->mclk, 512);
>> +               /*
>> +                * If no maximum operating frequency is supplied, fall back to
>> +                * use the module parameter, which has a (low) default value in
>> +                * case it is not specified. Either value must not exceed the
>> +                * clock rate into the block, of course.
>> +                */
>> +               if (mmc->f_max)
>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>> +               else
>> +                       mmc->f_max = min(host->mclk, fmax);
>> +
>> +       }
>> +
>>          dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>>
>>          /* Get regulators and the supported OCR mask */
>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>> index 706eb513..1882e20 100644
>> --- a/drivers/mmc/host/mmci.h
>> +++ b/drivers/mmc/host/mmci.h
>> @@ -208,6 +208,8 @@ struct mmci_host {
>>          spinlock_t              lock;
>>
>>          unsigned int            mclk;
>> +       /* cached value of requested clk in set_ios */
>> +       unsigned int            mclk_req;
>
> How about "clock_cache" instead?
Sounds ok to me, I will change it in next version.

Thanks,
srini
>
>>          unsigned int            cclk;
>>          u32                     pwr_reg;
>>          u32                     pwr_reg_add;
>> --
>> 1.9.1
>>
>
> Kind regards
> Ulf Hansson
>