[PATCH] mmc: mmci: Gate the clock when freq is 0

[PATCH] mmc: mmci: Gate the clock when freq is 0

[Ulf Hansson]

From: Johan Rudholm <johan.rudholm@stericsson.com>

In the ST Micro variant, the MMCI_CLOCK register should not be used to
gate the clock. Use MMCI_POWER to do this.

Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index edc3e9b..bf07823 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1147,6 +1147,15 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		}
 	}
 
+	/*
+	 * If clock = 0 and the block needs a certain value in the clreg
+	 * to function, we need to gate the clock by removing MCI_PWR_ON.
+	 * This is a special case for ST Micro variants, since the power
+	 * register in the ARM legacy case is used for powering the cards.
+	 */
+	if (!ios->clock && variant->clkreg)
+		pwr &= ~MCI_PWR_ON;
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	mmci_set_clkreg(host, ios->clock);
-- 
1.7.10

[PATCH] mmc: mmci: Gate the clock when freq is 0

[Linus Walleij]

On Wed, Dec 12, 2012 at 4:50 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:

> From: Johan Rudholm <johan.rudholm@stericsson.com>
>
> In the ST Micro variant, the MMCI_CLOCK register should not be used to
> gate the clock. Use MMCI_POWER to do this.
>
> Signed-off-by: Johan Rudholm <johan.rudholm@stericsson.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

I don't know if I'm qualified to ACK anything more today after
misunderstanding so many patches, but this seems correct to
me and won't affect any non-ST IP, so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH] mmc: mmci: Gate the clock when freq is 0

[Russell King - ARM Linux]

On Wed, Dec 12, 2012 at 04:50:21PM +0100, Ulf Hansson wrote:
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index edc3e9b..bf07823 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1147,6 +1147,15 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  		}
>  	}
>  
> +	/*
> +	 * If clock = 0 and the block needs a certain value in the clreg
> +	 * to function, we need to gate the clock by removing MCI_PWR_ON.
> +	 * This is a special case for ST Micro variants, since the power
> +	 * register in the ARM legacy case is used for powering the cards.

I wish you'd stop using "legacy" when talking about the ARM primecell.
How can it be legacy when ARMs current development boards still use this
primecell?

> +	 */
> +	if (!ios->clock && variant->clkreg)
> +		pwr &= ~MCI_PWR_ON;

This is horrid.  You're overloading the clkreg default value with another
meaning here - "if we have any bits set in the default value for the
MCICLOCK register, the MCIPOWER register has special meanings for the
power control bits".

When you think about it as I've described it above, do you think this is
an acceptable solution to this problem?

[PATCH] mmc: mmci: Gate the clock when freq is 0

[Ulf Hansson]

On 21 December 2012 17:21, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Dec 12, 2012 at 04:50:21PM +0100, Ulf Hansson wrote:
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index edc3e9b..bf07823 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -1147,6 +1147,15 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>               }
>>       }
>>
>> +     /*
>> +      * If clock = 0 and the block needs a certain value in the clreg
>> +      * to function, we need to gate the clock by removing MCI_PWR_ON.
>> +      * This is a special case for ST Micro variants, since the power
>> +      * register in the ARM legacy case is used for powering the cards.
>
> I wish you'd stop using "legacy" when talking about the ARM primecell.
> How can it be legacy when ARMs current development boards still use this
> primecell?
>
>> +      */
>> +     if (!ios->clock && variant->clkreg)
>> +             pwr &= ~MCI_PWR_ON;
>
> This is horrid.  You're overloading the clkreg default value with another
> meaning here - "if we have any bits set in the default value for the
> MCICLOCK register, the MCIPOWER register has special meanings for the
> power control bits".

I was hesitating when we decided to use the clkreg value due to what
you states here. Just to give you some more background.

By using clkreg default value you will get an implicit sequential
dependency that you will write the default value to MMCICLOCK reg,
_before_ writing the "power on" bit to the MMCIPOWER reg. Otherwise
you will not be able to update MMCICLK reg after the "power on" bit in
the MMCIPOWER reg has been set. That was the whole reason to why the
clkreg default value was invented.

Anyway, I still think your comment is valid so I suggest we add
another variant data which meaning will indicate whether the MMCIPOWER
register is used to control the actual power to the card. If that is
the case we must not use the MMCIPOWER to gate the clock. Does that
make sense?

>
> When you think about it as I've described it above, do you think this is
> an acceptable solution to this problem?

Kind regards
Ulf Hansson

[PATCH] mmc: mmci: Gate the clock when freq is 0

[Ulf Hansson]

On 7 January 2013 11:29, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 21 December 2012 17:21, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Dec 12, 2012 at 04:50:21PM +0100, Ulf Hansson wrote:
>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index edc3e9b..bf07823 100644
>>> --- a/drivers/mmc/host/mmci.c
>>> +++ b/drivers/mmc/host/mmci.c
>>> @@ -1147,6 +1147,15 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>>               }
>>>       }
>>>
>>> +     /*
>>> +      * If clock = 0 and the block needs a certain value in the clreg
>>> +      * to function, we need to gate the clock by removing MCI_PWR_ON.
>>> +      * This is a special case for ST Micro variants, since the power
>>> +      * register in the ARM legacy case is used for powering the cards.
>>
>> I wish you'd stop using "legacy" when talking about the ARM primecell.
>> How can it be legacy when ARMs current development boards still use this
>> primecell?
>>
>>> +      */
>>> +     if (!ios->clock && variant->clkreg)
>>> +             pwr &= ~MCI_PWR_ON;
>>
>> This is horrid.  You're overloading the clkreg default value with another
>> meaning here - "if we have any bits set in the default value for the
>> MCICLOCK register, the MCIPOWER register has special meanings for the
>> power control bits".
>
> I was hesitating when we decided to use the clkreg value due to what
> you states here. Just to give you some more background.
>
> By using clkreg default value you will get an implicit sequential
> dependency that you will write the default value to MMCICLOCK reg,
> _before_ writing the "power on" bit to the MMCIPOWER reg. Otherwise
> you will not be able to update MMCICLK reg after the "power on" bit in
> the MMCIPOWER reg has been set. That was the whole reason to why the
> clkreg default value was invented.
>
> Anyway, I still think your comment is valid so I suggest we add
> another variant data which meaning will indicate whether the MMCIPOWER
> register is used to control the actual power to the card. If that is
> the case we must not use the MMCIPOWER to gate the clock. Does that
> make sense?
>
>>
>> When you think about it as I've described it above, do you think this is
>> an acceptable solution to this problem?

I just sent out a new patch to handle the clock gating for the ST
variants. I decided to add a new variant data which is only indicating
whether the power register should be used to gate the clock. It felt
like the most proper way of doing this. Please have look when you have
the time.

Thanks!
Ulf Hansson