diff mbox

[v3,12/13] mmc: mmci: add explicit clk control

Message ID 1400849578-7585-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Srinivas Kandagatla May 23, 2014, 12:52 p.m. UTC
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 and cclk_is_mclk flags in variant structure giving
more flexibility to the driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

Comments

Ulf Hansson May 26, 2014, 2:21 p.m. UTC | #1
On 23 May 2014 14:52,  <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 and cclk_is_mclk flags in variant structure giving
> more flexibility to the driver.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 5cbf644..f6dfd24 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>   * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>   * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
>   *                      are not ignored.
> + * @explicit_mclk_control: enable explicit mclk control in driver.
> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
>   */
>  struct variant_data {
>         unsigned int            clkreg;
> @@ -94,6 +96,8 @@ struct variant_data {
>         bool                    busy_detect;
>         bool                    pwrreg_nopower;
>         bool                    mclk_delayed_writes;
> +       bool                    explicit_mclk_control;
> +       bool                    cclk_is_mclk;

I can't see why you need to have both these new configurations. Aren't
"cclk_is_mclk" just a fact when you use "explicit_mclk_control".

I also believe I would prefer something like "qcom_clkdiv" instead.

>  };
>
>  static struct variant_data variant_arm = {
> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>          * for 3 clk cycles.
>          */
>         .mclk_delayed_writes    = true,
> +       .explicit_mclk_control  = true,
> +       .cclk_is_mclk   = true,
>  };
>
>  static inline u32 mmci_readl(struct mmci_host *host, u32 off)
> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>         host->cclk = 0;
>
>         if (desired) {
> -               if (desired >= host->mclk) {
> +               if (variant->cclk_is_mclk) {
> +                       host->cclk = host->mclk;
> +               } else if (desired >= host->mclk) {
>                         clk = MCI_CLK_BYPASS;
>                         if (variant->st_clkdiv)
>                                 clk |= MCI_ST_UX500_NEG_EDGE;
> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>         if (!ios->clock && variant->pwrreg_clkgate)
>                 pwr &= ~MCI_PWR_ON;
>
> +       if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {

I suggest you should clarify the statement by adding a pair of extra
parentheses. Additionally it seems like a good idea to reverse the
order of the statements, to clarify this is for qcom clock handling
only.

More important, what I think you really want to do is to compare
"ios->clock" with it's previous value it had when ->set_ios were
invoked. Then let a changed value act as the trigger to set a new clk
rate. Obvoiusly you need to cache the clock rate in the struct mmci
host to handle this.

> +               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);

So here you actually find out the new clk rate, but shouldn't you
update "host->mclk" within the spin_lock? Or it might not matter?

> +               }
> +       }
> +
>         spin_lock_irqsave(&host->lock, flags);
>
>         mmci_set_clkreg(host, ios->clock);
> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>          * 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);
> +       if (!host->variant->explicit_mclk_control) {
> +               if (mmc->f_max)
> +                       mmc->f_max = min(host->mclk, mmc->f_max);
> +               else
> +                       mmc->f_max = min(host->mclk, fmax);
> +       }

This means your mmc->f_max value will either be zero or the one you
provided through DT. And since zero won't work, that means you
_require_ to get the value from DT. According to the documentation of
this DT binding, f_max is optional.

So unless you fine another way of dynamically at runtime figure out
the value of f_max (using the clk API), you need to update the DT
documentation for mmci.

Additionally, this makes me wonder about f_min. I haven't seen
anywhere in this patch were that value is being set to proper value,
right?

>         dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>
>         /* Get regulators and the supported OCR mask */
> --
> 1.9.1
>

Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson May 26, 2014, 2:28 p.m. UTC | #2
On 26 May 2014 16:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 May 2014 14:52,  <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 and cclk_is_mclk flags in variant structure giving
>> more flexibility to the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 5cbf644..f6dfd24 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>>   * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>>   * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
>>   *                      are not ignored.
>> + * @explicit_mclk_control: enable explicit mclk control in driver.
>> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
>>   */
>>  struct variant_data {
>>         unsigned int            clkreg;
>> @@ -94,6 +96,8 @@ struct variant_data {
>>         bool                    busy_detect;
>>         bool                    pwrreg_nopower;
>>         bool                    mclk_delayed_writes;
>> +       bool                    explicit_mclk_control;
>> +       bool                    cclk_is_mclk;
>
> I can't see why you need to have both these new configurations. Aren't
> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>
> I also believe I would prefer something like "qcom_clkdiv" instead.
>
>>  };
>>
>>  static struct variant_data variant_arm = {
>> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>>          * for 3 clk cycles.
>>          */
>>         .mclk_delayed_writes    = true,
>> +       .explicit_mclk_control  = true,
>> +       .cclk_is_mclk   = true,
>>  };
>>
>>  static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>>         host->cclk = 0;
>>
>>         if (desired) {
>> -               if (desired >= host->mclk) {
>> +               if (variant->cclk_is_mclk) {
>> +                       host->cclk = host->mclk;
>> +               } else if (desired >= host->mclk) {
>>                         clk = MCI_CLK_BYPASS;
>>                         if (variant->st_clkdiv)
>>                                 clk |= MCI_ST_UX500_NEG_EDGE;
>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>         if (!ios->clock && variant->pwrreg_clkgate)
>>                 pwr &= ~MCI_PWR_ON;
>>
>> +       if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {
>
> I suggest you should clarify the statement by adding a pair of extra
> parentheses. Additionally it seems like a good idea to reverse the
> order of the statements, to clarify this is for qcom clock handling
> only.
>
> More important, what I think you really want to do is to compare
> "ios->clock" with it's previous value it had when ->set_ios were
> invoked. Then let a changed value act as the trigger to set a new clk
> rate. Obvoiusly you need to cache the clock rate in the struct mmci
> host to handle this.
>
>> +               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);
>
> So here you actually find out the new clk rate, but shouldn't you
> update "host->mclk" within the spin_lock? Or it might not matter?
>
>> +               }
>> +       }
>> +
>>         spin_lock_irqsave(&host->lock, flags);
>>
>>         mmci_set_clkreg(host, ios->clock);
>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>          * 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);
>> +       if (!host->variant->explicit_mclk_control) {
>> +               if (mmc->f_max)
>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>> +               else
>> +                       mmc->f_max = min(host->mclk, fmax);
>> +       }
>
> This means your mmc->f_max value will either be zero or the one you
> provided through DT. And since zero won't work, that means you
> _require_ to get the value from DT. According to the documentation of
> this DT binding, f_max is optional.
>
> So unless you fine another way of dynamically at runtime figure out
> the value of f_max (using the clk API), you need to update the DT
> documentation for mmci.
>
> Additionally, this makes me wonder about f_min. I haven't seen
> anywhere in this patch were that value is being set to proper value,
> right?
>
>>         dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>>
>>         /* Get regulators and the supported OCR mask */
>> --
>> 1.9.1
>>
>
> Kind regards
> Ulf Hansson

And one more thing, just for your information. If you intend to
support UHS cards, your need to be able to gate the clock though
->set_ios function, once the clock is 0. Let's handle that in a
separate patch - if needed though.

Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla May 26, 2014, 10:39 p.m. UTC | #3
Hi Ulf,
Thankyou for the comments.

On 26/05/14 15:21, Ulf Hansson wrote:
> On 23 May 2014 14:52,  <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 and cclk_is_mclk flags in variant structure giving
>> more flexibility to the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>   drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>>   1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 5cbf644..f6dfd24 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>>    * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>>    * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
>>    *                      are not ignored.
>> + * @explicit_mclk_control: enable explicit mclk control in driver.
>> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
>>    */
>>   struct variant_data {
>>          unsigned int            clkreg;
>> @@ -94,6 +96,8 @@ struct variant_data {
>>          bool                    busy_detect;
>>          bool                    pwrreg_nopower;
>>          bool                    mclk_delayed_writes;
>> +       bool                    explicit_mclk_control;
>> +       bool                    cclk_is_mclk;
>
> I can't see why you need to have both these new configurations. Aren't
> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>


> I also believe I would prefer something like "qcom_clkdiv" instead.

There is a subtle difference between both the flags.  Am happy to change 
it to qcom_clkdiv.

>
>>   };
>>
>>   static struct variant_data variant_arm = {
>> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>>           * for 3 clk cycles.
>>           */
>>          .mclk_delayed_writes    = true,
>> +       .explicit_mclk_control  = true,
>> +       .cclk_is_mclk   = true,
>>   };
>>
>>   static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>>          host->cclk = 0;
>>
>>          if (desired) {
>> -               if (desired >= host->mclk) {
>> +               if (variant->cclk_is_mclk) {
>> +                       host->cclk = host->mclk;
>> +               } else if (desired >= host->mclk) {
>>                          clk = MCI_CLK_BYPASS;
>>                          if (variant->st_clkdiv)
>>                                  clk |= MCI_ST_UX500_NEG_EDGE;
>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>          if (!ios->clock && variant->pwrreg_clkgate)
>>                  pwr &= ~MCI_PWR_ON;
>>
>> +       if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {
>
> I suggest you should clarify the statement by adding a pair of extra
> parentheses. Additionally it seems like a good idea to reverse the
> order of the statements, to clarify this is for qcom clock handling
> only.
Yes, sure Will fix this in next version.

>
> More important, what I think you really want to do is to compare
> "ios->clock" with it's previous value it had when ->set_ios were
> invoked. Then let a changed value act as the trigger to set a new clk
> rate. Obvoiusly you need to cache the clock rate in the struct mmci
> host to handle this.

host->mclk already has this cached value.

>
>> +               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);
>
> So here you actually find out the new clk rate, but shouldn't you
> update "host->mclk" within the spin_lock? Or it might not matter?
>
I think it does not matter in this case, as this is the only place mclk 
gets modified.
>> +               }
>> +       }
>> +
>>          spin_lock_irqsave(&host->lock, flags);
>>
>>          mmci_set_clkreg(host, ios->clock);
>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>           * 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);
>> +       if (!host->variant->explicit_mclk_control) {
>> +               if (mmc->f_max)
>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>> +               else
>> +                       mmc->f_max = min(host->mclk, fmax);
>> +       }
>
> This means your mmc->f_max value will either be zero or the one you
> provided through DT. And since zero won't work, that means you
> _require_ to get the value from DT. According to the documentation of
> this DT binding, f_max is optional.
>
> So unless you fine another way of dynamically at runtime figure out
> the value of f_max (using the clk API), you need to update the DT
> documentation for mmci.
>

You are right there is a possibility of f_max to be zero.

This logic could fix it.

if (host->variant->explicit_mclk_control) {
	if (mmc->f_max)
		mmc->f_max = max(host->mclk, mmc->f_max);
	else
		mmc->f_max = max(host->mclk, fmax);
} else {
	if (mmc->f_max)
		mmc->f_max = min(host->mclk, mmc->f_max);
	else
		mmc->f_max = min(host->mclk, fmax);
}



> Additionally, this makes me wonder about f_min. I haven't seen
> anywhere in this patch were that value is being set to proper value,
> right?
>

f_min should be 400000 for qcom, I think with the default mclk frequency 
and a divider of 512 used for calculating the f_min is bringing down the 
f_min to lessthan 400Kz. Which is why its working fine.
I think the possibility of mclk default frequency being greater than 
208Mhz is very less. so I could either leave it as it is Or force this 
to 400000 all the time for qcom chips.

Am ok either way..

Thanks,
srini

>>          dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>>
>>          /* Get regulators and the supported OCR mask */
>> --
>> 1.9.1
>>
>
> Kind regards
> Ulf Hansson
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson May 27, 2014, 9:32 a.m. UTC | #4
On 27 May 2014 00:39, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> Hi Ulf,
> Thankyou for the comments.
>
>
> On 26/05/14 15:21, Ulf Hansson wrote:
>>
>> On 23 May 2014 14:52,  <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 and cclk_is_mclk flags in variant structure
>>> giving
>>> more flexibility to the driver.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>>   drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>>>   1 file changed, 25 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index 5cbf644..f6dfd24 100644
>>> --- a/drivers/mmc/host/mmci.c
>>> +++ b/drivers/mmc/host/mmci.c
>>> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>>>    * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>>>    * @mclk_delayed_writes: enable delayed writes to ensure, subsequent
>>> updates
>>>    *                      are not ignored.
>>> + * @explicit_mclk_control: enable explicit mclk control in driver.
>>> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter
>>> clock.
>>>    */
>>>   struct variant_data {
>>>          unsigned int            clkreg;
>>> @@ -94,6 +96,8 @@ struct variant_data {
>>>          bool                    busy_detect;
>>>          bool                    pwrreg_nopower;
>>>          bool                    mclk_delayed_writes;
>>> +       bool                    explicit_mclk_control;
>>> +       bool                    cclk_is_mclk;
>>
>>
>> I can't see why you need to have both these new configurations. Aren't
>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>>
>
>
>> I also believe I would prefer something like "qcom_clkdiv" instead.
>
>
> There is a subtle difference between both the flags.  Am happy to change it
> to qcom_clkdiv.
>
>
>>
>>>   };
>>>
>>>   static struct variant_data variant_arm = {
>>> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>>>           * for 3 clk cycles.
>>>           */
>>>          .mclk_delayed_writes    = true,
>>> +       .explicit_mclk_control  = true,
>>> +       .cclk_is_mclk   = true,
>>>   };
>>>
>>>   static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host,
>>> unsigned int desired)
>>>          host->cclk = 0;
>>>
>>>          if (desired) {
>>> -               if (desired >= host->mclk) {
>>> +               if (variant->cclk_is_mclk) {
>>> +                       host->cclk = host->mclk;
>>> +               } else if (desired >= host->mclk) {
>>>                          clk = MCI_CLK_BYPASS;
>>>                          if (variant->st_clkdiv)
>>>                                  clk |= MCI_ST_UX500_NEG_EDGE;
>>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc,
>>> struct mmc_ios *ios)
>>>          if (!ios->clock && variant->pwrreg_clkgate)
>>>                  pwr &= ~MCI_PWR_ON;
>>>
>>> +       if (ios->clock != host->mclk &&
>>> host->variant->explicit_mclk_control) {
>>
>>
>> I suggest you should clarify the statement by adding a pair of extra
>> parentheses. Additionally it seems like a good idea to reverse the
>> order of the statements, to clarify this is for qcom clock handling
>> only.
>
> Yes, sure Will fix this in next version.
>
>
>>
>> More important, what I think you really want to do is to compare
>> "ios->clock" with it's previous value it had when ->set_ios were
>> invoked. Then let a changed value act as the trigger to set a new clk
>> rate. Obvoiusly you need to cache the clock rate in the struct mmci
>> host to handle this.
>
>
> host->mclk already has this cached value.

There are no guarantees clk_set_rate() will manage to set the exact
requested rate as ios->clock. At least if you follow the clk API
documentation.

>
>
>>
>>> +               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);
>>
>>
>> So here you actually find out the new clk rate, but shouldn't you
>> update "host->mclk" within the spin_lock? Or it might not matter?
>>
> I think it does not matter in this case, as this is the only place mclk gets
> modified.

OK.

>
>>> +               }
>>> +       }
>>> +
>>>          spin_lock_irqsave(&host->lock, flags);
>>>
>>>          mmci_set_clkreg(host, ios->clock);
>>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>>           * 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);
>>> +       if (!host->variant->explicit_mclk_control) {
>>> +               if (mmc->f_max)
>>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>>> +               else
>>> +                       mmc->f_max = min(host->mclk, fmax);
>>> +       }
>>
>>
>> This means your mmc->f_max value will either be zero or the one you
>> provided through DT. And since zero won't work, that means you
>> _require_ to get the value from DT. According to the documentation of
>> this DT binding, f_max is optional.
>>
>> So unless you fine another way of dynamically at runtime figure out
>> the value of f_max (using the clk API), you need to update the DT
>> documentation for mmci.
>>
>
> You are right there is a possibility of f_max to be zero.
>
> This logic could fix it.
>
> if (host->variant->explicit_mclk_control) {
>         if (mmc->f_max)
>                 mmc->f_max = max(host->mclk, mmc->f_max);
>         else
>                 mmc->f_max = max(host->mclk, fmax);
> } else {
>         if (mmc->f_max)
>
>                 mmc->f_max = min(host->mclk, mmc->f_max);
>         else
>
>                 mmc->f_max = min(host->mclk, fmax);
> }
>

Hmm. Looking a bit deeper into this, we have some additional related
code to fixup. :-)

In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
due to the current variants don't support higher frequency than this.
It seems like the Qcom variant may support up to 208MHz? Now, if
that's the case, we need to add "f_max" to the struct variant_data to
store this information, so we can respect different values while doing
clk_set_rate() at ->probe().

While updating mmc->f_max for host->variant->explicit_mclk_control, we
shouldn't care about using the host->mclk as a limiter, instead, use
min(mmc->f_max, host->variant->f_max) and fallback to fmax.

Not sure how that will affect the logic. :-)

>
>
>> Additionally, this makes me wonder about f_min. I haven't seen
>> anywhere in this patch were that value is being set to proper value,
>> right?
>>
>
> f_min should be 400000 for qcom, I think with the default mclk frequency and
> a divider of 512 used for calculating the f_min is bringing down the f_min
> to lessthan 400Kz. Which is why its working fine.
> I think the possibility of mclk default frequency being greater than 208Mhz
> is very less. so I could either leave it as it is Or force this to 400000
> all the time for qcom chips.

No, this seems like a wrong approach.

I think you would like to do use the clk_round_rate() find out the
lowest possible rate. Or just use a fixed fallback value somehow.

Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla May 27, 2014, 12:43 p.m. UTC | #5
On 27/05/14 10:32, Ulf Hansson wrote:
> On 27 May 2014 00:39, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> Hi Ulf,
>> Thankyou for the comments.
>>
>>
>> On 26/05/14 15:21, Ulf Hansson wrote:
>>>
>>> On 23 May 2014 14:52,  <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.
>>>>
...

>>>>            * for 3 clk cycles.
>>>>            */
>>>>           .mclk_delayed_writes    = true,
>>>> +       .explicit_mclk_control  = true,
>>>> +       .cclk_is_mclk   = true,
>>>>    };
>>>>
>>>>    static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>>>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host,
>>>> unsigned int desired)
>>>>           host->cclk = 0;
>>>>
>>>>           if (desired) {
>>>> -               if (desired >= host->mclk) {
>>>> +               if (variant->cclk_is_mclk) {
>>>> +                       host->cclk = host->mclk;
>>>> +               } else if (desired >= host->mclk) {
>>>>                           clk = MCI_CLK_BYPASS;
>>>>                           if (variant->st_clkdiv)
>>>>                                   clk |= MCI_ST_UX500_NEG_EDGE;
>>>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc,
>>>> struct mmc_ios *ios)
>>>>           if (!ios->clock && variant->pwrreg_clkgate)
>>>>                   pwr &= ~MCI_PWR_ON;
>>>>
>>>> +       if (ios->clock != host->mclk &&
>>>> host->variant->explicit_mclk_control) {
>>>
>>>
>>> I suggest you should clarify the statement by adding a pair of extra
>>> parentheses. Additionally it seems like a good idea to reverse the
>>> order of the statements, to clarify this is for qcom clock handling
>>> only.
>>
>> Yes, sure Will fix this in next version.
>>
>>
>>>
>>> More important, what I think you really want to do is to compare
>>> "ios->clock" with it's previous value it had when ->set_ios were
>>> invoked. Then let a changed value act as the trigger to set a new clk
>>> rate. Obvoiusly you need to cache the clock rate in the struct mmci
>>> host to handle this.
>>
>>
>> host->mclk already has this cached value.
>
> There are no guarantees clk_set_rate() will manage to set the exact
> requested rate as ios->clock. At least if you follow the clk API
> documentation.
>

Yes, I agree. caching ios->clock looks like the best option.

>>
>>
>>>

...

>>
>>>> +               }
>>>> +       }
>>>> +
>>>>           spin_lock_irqsave(&host->lock, flags);
>>>>
>>>>           mmci_set_clkreg(host, ios->clock);
>>>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>>>            * 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);
>>>> +       if (!host->variant->explicit_mclk_control) {
>>>> +               if (mmc->f_max)
>>>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>>>> +               else
>>>> +                       mmc->f_max = min(host->mclk, fmax);
>>>> +       }
>>>
>>>
>>> This means your mmc->f_max value will either be zero or the one you
>>> provided through DT. And since zero won't work, that means you
>>> _require_ to get the value from DT. According to the documentation of
>>> this DT binding, f_max is optional.
>>>
>>> So unless you fine another way of dynamically at runtime figure out
>>> the value of f_max (using the clk API), you need to update the DT
>>> documentation for mmci.
>>>
>>
>> You are right there is a possibility of f_max to be zero.
>>
>> This logic could fix it.
>>
>> if (host->variant->explicit_mclk_control) {
>>          if (mmc->f_max)
>>                  mmc->f_max = max(host->mclk, mmc->f_max);
>>          else
>>                  mmc->f_max = max(host->mclk, fmax);
>> } else {
>>          if (mmc->f_max)
>>
>>                  mmc->f_max = min(host->mclk, mmc->f_max);
>>          else
>>
>>                  mmc->f_max = min(host->mclk, fmax);
>> }
>>
>
> Hmm. Looking a bit deeper into this, we have some additional related
> code to fixup. :-)
>
> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
> due to the current variants don't support higher frequency than this.
> It seems like the Qcom variant may support up to 208MHz? Now, if
> that's the case, we need to add "f_max" to the struct variant_data to
> store this information, so we can respect different values while doing
> clk_set_rate() at ->probe().
>
Yes, qcom SOCs support more than 100Hhz clock.

Probe and clk_set_rate/set_ios should respect this.

On the other thought, Should probe actually care about clocks which are 
explicitly controlled? It should not even attempt to set any frequency 
to start with. mmc-core would set the right frequency depending on the 
mmc state-machine respecting f_min and f_max.
I think for qcom, probe should just check the if f_max and f_min are 
valid and set them to defaults if any in the same lines as existing code.

> While updating mmc->f_max for host->variant->explicit_mclk_control, we
> shouldn't care about using the host->mclk as a limiter, instead, use
> min(mmc->f_max, host->variant->f_max) and fallback to fmax.
>
Yes, that's correct, mclk should not be used as limiter. Adding f_max to 
the variant looks useful.

> Not sure how that will affect the logic. :-)
>
>>
>>
>>> Additionally, this makes me wonder about f_min. I haven't seen
>>> anywhere in this patch were that value is being set to proper value,
>>> right?
>>>
>>
>> f_min should be 400000 for qcom, I think with the default mclk frequency and
>> a divider of 512 used for calculating the f_min is bringing down the f_min
>> to lessthan 400Kz. Which is why its working fine.
>> I think the possibility of mclk default frequency being greater than 208Mhz
>> is very less. so I could either leave it as it is Or force this to 400000
>> all the time for qcom chips.
>
> No, this seems like a wrong approach.
>
> I think you would like to do use the clk_round_rate() find out the
> lowest possible rate. Or just use a fixed fallback value somehow.

clk_round_rate(mclk, 0) might be more generic, instead of fixed 
fallback. Let the mmc-core figure out what frequency would be best from 
its table starting from f_min.

thanks,
srini
>
> Kind regards
> Ulf Hansson
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson May 27, 2014, 2:07 p.m. UTC | #6
>> Hmm. Looking a bit deeper into this, we have some additional related
>> code to fixup. :-)
>>
>> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
>> due to the current variants don't support higher frequency than this.
>> It seems like the Qcom variant may support up to 208MHz? Now, if
>> that's the case, we need to add "f_max" to the struct variant_data to
>> store this information, so we can respect different values while doing
>> clk_set_rate() at ->probe().
>>
> Yes, qcom SOCs support more than 100Hhz clock.
>
> Probe and clk_set_rate/set_ios should respect this.
>
> On the other thought, Should probe actually care about clocks which are
> explicitly controlled? It should not even attempt to set any frequency to
> start with.

The 100 MHz is related to constraints set by the specification of the
IP block, not the MMC/SD/SDIO spec.

Thus at ->probe() we must perform the clk_set_rate().

> mmc-core would set the right frequency depending on the mmc
> state-machine respecting f_min and f_max.
> I think for qcom, probe should just check the if f_max and f_min are valid
> and set them to defaults if any in the same lines as existing code.
>
>
>> While updating mmc->f_max for host->variant->explicit_mclk_control, we
>> shouldn't care about using the host->mclk as a limiter, instead, use
>> min(mmc->f_max, host->variant->f_max) and fallback to fmax.
>>
> Yes, that's correct, mclk should not be used as limiter. Adding f_max to the
> variant looks useful.
>
>
>> Not sure how that will affect the logic. :-)
>>
>>>
>>>
>>>> Additionally, this makes me wonder about f_min. I haven't seen
>>>> anywhere in this patch were that value is being set to proper value,
>>>> right?
>>>>
>>>
>>> f_min should be 400000 for qcom, I think with the default mclk frequency
>>> and
>>> a divider of 512 used for calculating the f_min is bringing down the
>>> f_min
>>> to lessthan 400Kz. Which is why its working fine.
>>> I think the possibility of mclk default frequency being greater than
>>> 208Mhz
>>> is very less. so I could either leave it as it is Or force this to 400000
>>> all the time for qcom chips.
>>
>>
>> No, this seems like a wrong approach.
>>
>> I think you would like to do use the clk_round_rate() find out the
>> lowest possible rate. Or just use a fixed fallback value somehow.
>
>
> clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback.
> Let the mmc-core figure out what frequency would be best from its table
> starting from f_min.

Agree!

clk_round_rate(mclk, 100KHz), might be better though - since that is
actually the lowest request frequency whatsoever.

Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla May 27, 2014, 2:14 p.m. UTC | #7
On 27/05/14 15:07, Ulf Hansson wrote:
>>> Hmm. Looking a bit deeper into this, we have some additional related
>>> code to fixup. :-)
>>>
>>> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
>>> due to the current variants don't support higher frequency than this.
>>> It seems like the Qcom variant may support up to 208MHz? Now, if
>>> that's the case, we need to add "f_max" to the struct variant_data to
>>> store this information, so we can respect different values while doing
>>> clk_set_rate() at ->probe().
>>>
>> Yes, qcom SOCs support more than 100Hhz clock.
>>
>> Probe and clk_set_rate/set_ios should respect this.
>>
>> On the other thought, Should probe actually care about clocks which are
>> explicitly controlled? It should not even attempt to set any frequency to
>> start with.
>
> The 100 MHz is related to constraints set by the specification of the
> IP block, not the MMC/SD/SDIO spec.
>
> Thus at ->probe() we must perform the clk_set_rate().
I agree its valid for controllers which have this constraints.

>
>> mmc-core would set the right frequency depending on the mmc
>> state-machine respecting f_min and f_max.
>> I think for qcom, probe should just check the if f_max and f_min are valid
>> and set them to defaults if any in the same lines as existing code.
>>
>>
>>> While updating mmc->f_max for host->variant->explicit_mclk_control, we
>>> shouldn't care about using the host->mclk as a limiter, instead, use
>>> min(mmc->f_max, host->variant->f_max) and fallback to fmax.
>>>
>> Yes, that's correct, mclk should not be used as limiter. Adding f_max to the
>> variant looks useful.
>>
>>
>>> Not sure how that will affect the logic. :-)
>>>
>>>>
>>>>
>>>>> Additionally, this makes me wonder about f_min. I haven't seen
>>>>> anywhere in this patch were that value is being set to proper value,
>>>>> right?
>>>>>
>>>>
>>>> f_min should be 400000 for qcom, I think with the default mclk frequency
>>>> and
>>>> a divider of 512 used for calculating the f_min is bringing down the
>>>> f_min
>>>> to lessthan 400Kz. Which is why its working fine.
>>>> I think the possibility of mclk default frequency being greater than
>>>> 208Mhz
>>>> is very less. so I could either leave it as it is Or force this to 400000
>>>> all the time for qcom chips.
>>>
>>>
>>> No, this seems like a wrong approach.
>>>
>>> I think you would like to do use the clk_round_rate() find out the
>>> lowest possible rate. Or just use a fixed fallback value somehow.
>>
>>
>> clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback.
>> Let the mmc-core figure out what frequency would be best from its table
>> starting from f_min.
>
> Agree!
>
> clk_round_rate(mclk, 100KHz), might be better though - since that is
> actually the lowest request frequency whatsoever.
>
Perfect.

--srini
> Kind regards
> Ulf Hansson
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij May 28, 2014, 8:02 a.m. UTC | #8
On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> On 26/05/14 15:21, Ulf Hansson wrote:
>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:

>>>
>>> +       bool                    explicit_mclk_control;
>>> +       bool                    cclk_is_mclk;
>>
>> I can't see why you need to have both these new configurations. Aren't
>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>
>> I also believe I would prefer something like "qcom_clkdiv" instead.
>
> There is a subtle difference between both the flags.  Am happy to change it
> to qcom_clkdiv.

I think this was due to me wanting the variant variables to be more about
the actual technical difference they indicate rather than pointing to
a certain vendor or variant where that difference occurs.

It's a very minor thing though, if you prefer it this way, go for it.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla May 28, 2014, 8:28 a.m. UTC | #9
On 28/05/14 09:02, Linus Walleij wrote:
> On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> On 26/05/14 15:21, Ulf Hansson wrote:
>>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>
>>>>
>>>> +       bool                    explicit_mclk_control;
>>>> +       bool                    cclk_is_mclk;
>>>
>>> I can't see why you need to have both these new configurations. Aren't
>>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>>
>>> I also believe I would prefer something like "qcom_clkdiv" instead.
>>
>> There is a subtle difference between both the flags.  Am happy to change it
>> to qcom_clkdiv.
>
> I think this was due to me wanting the variant variables to be more about
> the actual technical difference they indicate rather than pointing to
> a certain vendor or variant where that difference occurs.
>
Yes, that's correct, I think having these two variables seems to be more 
generic than qcom_clkdiv.

I will keep it as it is and fix other comments from Ulf in next version.

> It's a very minor thing though, if you prefer it this way, go for it.
>

Thanks,
sirni
> Yours,
> Linus Walleij
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson May 28, 2014, 10:17 a.m. UTC | #10
On 28 May 2014 10:28, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
> On 28/05/14 09:02, Linus Walleij wrote:
>>
>> On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla
>> <srinivas.kandagatla@linaro.org> wrote:
>>>
>>> On 26/05/14 15:21, Ulf Hansson wrote:
>>>>
>>>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>>>>>
>>>>> +       bool                    explicit_mclk_control;
>>>>> +       bool                    cclk_is_mclk;
>>>>
>>>>
>>>> I can't see why you need to have both these new configurations. Aren't
>>>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>>>
>>>
>>>> I also believe I would prefer something like "qcom_clkdiv" instead.
>>>
>>>
>>> There is a subtle difference between both the flags.  Am happy to change
>>> it
>>> to qcom_clkdiv.
>>
>>
>> I think this was due to me wanting the variant variables to be more about
>> the actual technical difference they indicate rather than pointing to
>> a certain vendor or variant where that difference occurs.
>>
> Yes, that's correct, I think having these two variables seems to be more
> generic than qcom_clkdiv.
>
> I will keep it as it is and fix other comments from Ulf in next version.
>

I think this relates to the discussion we had around fetching the
f_min and f_max in ->probe(). It, just seems silly to have to check
for an extra flag there as well, because that is in principle what
this would mean, right?

So, please adjust to my proposal, I strongly think this should be only
one flag. You might want a more generic name of the flag in favour of
qcom_clkdiv, feel free to change to whatever you think make sense.

Kind regards
Ulf Hansson

>
>> It's a very minor thing though, if you prefer it this way, go for it.
>>
>
> Thanks,
> sirni
>>
>> Yours,
>> Linus Walleij
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 5cbf644..f6dfd24 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -73,6 +73,8 @@  static unsigned int fmax = 515633;
  * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
  * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
  *			 are not ignored.
+ * @explicit_mclk_control: enable explicit mclk control in driver.
+ * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
  */
 struct variant_data {
 	unsigned int		clkreg;
@@ -94,6 +96,8 @@  struct variant_data {
 	bool			busy_detect;
 	bool			pwrreg_nopower;
 	bool			mclk_delayed_writes;
+	bool			explicit_mclk_control;
+	bool			cclk_is_mclk;
 };
 
 static struct variant_data variant_arm = {
@@ -202,6 +206,8 @@  static struct variant_data variant_qcom = {
 	 * for 3 clk cycles.
 	 */
 	.mclk_delayed_writes	= true,
+	.explicit_mclk_control	= true,
+	.cclk_is_mclk	= true,
 };
 
 static inline u32 mmci_readl(struct mmci_host *host, u32 off)
@@ -317,7 +323,9 @@  static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 	host->cclk = 0;
 
 	if (desired) {
-		if (desired >= host->mclk) {
+		if (variant->cclk_is_mclk) {
+			host->cclk = host->mclk;
+		} else if (desired >= host->mclk) {
 			clk = MCI_CLK_BYPASS;
 			if (variant->st_clkdiv)
 				clk |= MCI_ST_UX500_NEG_EDGE;
@@ -1354,6 +1362,16 @@  static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (!ios->clock && variant->pwrreg_clkgate)
 		pwr &= ~MCI_PWR_ON;
 
+	if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {
+		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);
+		}
+	}
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	mmci_set_clkreg(host, ios->clock);
@@ -1540,10 +1558,12 @@  static int mmci_probe(struct amba_device *dev,
 	 * 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);
+	if (!host->variant->explicit_mclk_control) {
+		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 */