Re: [PATCH resend 1/3] mmc: sunxi: fix timeout in sunxi_mmc_oclk_onoff

Re: [PATCH resend 1/3] mmc: sunxi: fix timeout in sunxi_mmc_oclk_onoff

[Hans de Goede]

Hi,

On 12-08-15 14:23, michal.suchanek@ruk.cuni.cz wrote:
> The 250ms timeout is too short.
>
> On my system enabling the oclk takes under 50ms and disabling slightly
> over 100ms when idle. Under load disabling the clock can take over
> 350ms.
>
> This does not make mmc clock gating look like good option to have on
> sunxi but the system should not crash with mmc clock gating enabled
> nonetheless.
>
> This patch sets the timeout to 750ms and adds debug prints which show
> how long enabling/disabling the clock took so more data can be collected
> from other systems.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

This is a big patch for just changing a timeout, most of this is in
extra verbosity which IMHO has little value, in the error path w
know we will have waited aprox 750 ms, so printing the waiting
time there is not worth all the extra code.

As for adding the warning I'm even less of a big fan of that,
if we need higher timeouts, we need higher timeouts, spamming the
kernel logs with that we are actually hitting the higher timeouts
is not productive IMHO.

Can you please resend this as a one-liner just changing the timeout ?

Regards,

Hans


>
> --
>
>   - fix formatting
> ---
>   drivers/mmc/host/sunxi-mmc.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 4d3e1ff..f808a02 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -595,7 +595,7 @@ static irqreturn_t sunxi_mmc_handle_manual_stop(int irq, void *dev_id)
>
>   static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>   {
> -	unsigned long expire = jiffies + msecs_to_jiffies(250);
> +	unsigned long start, end;
>   	u32 rval;
>
>   	rval = mmc_readl(host, REG_CLKCR);
> @@ -604,6 +604,8 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>   	if (oclk_en)
>   		rval |= SDXC_CARD_CLOCK_ON;
>
> +	start = jiffies;
> +	end = start + msecs_to_jiffies(750);
>   	mmc_writel(host, REG_CLKCR, rval);
>
>   	rval = SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER;
> @@ -611,15 +613,29 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>
>   	do {
>   		rval = mmc_readl(host, REG_CMDR);
> -	} while (time_before(jiffies, expire) && (rval & SDXC_START));
> +	} while (time_before(jiffies, end) && (rval & SDXC_START));
> +	end = jiffies;
>
>   	/* clear irq status bits set by the command */
>   	mmc_writel(host, REG_RINTR,
>   		   mmc_readl(host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT);
>
>   	if (rval & SDXC_START) {
> -		dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
> +		dev_err(mmc_dev(host->mmc),
> +			"fatal err update oclk timeout. Could not %s in %ims.\n",
> +			oclk_en ? "enable" : "disable",
> +			jiffies_to_msecs(end - start));
>   		return -EIO;
> +	} else {
> +		int msecs = jiffies_to_msecs(end - start);
> +		const char *ing = oclk_en ? "enabling" : "disabling";
> +
> +		if ((msecs > 150) || (oclk_en && (msecs > 50)))
> +			dev_warn(mmc_dev(host->mmc),
> +				 "%s oclk took %ims", ing, msecs);
> +		else
> +			dev_dbg(mmc_dev(host->mmc),
> +				 "%s oclk took %ims", ing, msecs);
>   	}
>
>   	return 0;
>

Re: [linux-sunxi] Re: [PATCH resend 1/3] mmc: sunxi: fix timeout in sunxi_mmc_oclk_onoff

[Olliver Schinagl]

While I can't speak for Michal obviously,

I left the debugging bit (in my v2 that i sent 2 minutes ago) as both 
you and Hans where content with it back then and both acked it.

Michal, feel free to send the v3 without the debug info, unless you want 
me to do it ;)

Olliver

On 12-08-15 14:32, Hans de Goede wrote:
> Hi,
>
> On 12-08-15 14:23, michal.suchanek@ruk.cuni.cz wrote:
>> The 250ms timeout is too short.
>>
>> On my system enabling the oclk takes under 50ms and disabling slightly
>> over 100ms when idle. Under load disabling the clock can take over
>> 350ms.
>>
>> This does not make mmc clock gating look like good option to have on
>> sunxi but the system should not crash with mmc clock gating enabled
>> nonetheless.
>>
>> This patch sets the timeout to 750ms and adds debug prints which show
>> how long enabling/disabling the clock took so more data can be collected
>> from other systems.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> This is a big patch for just changing a timeout, most of this is in
> extra verbosity which IMHO has little value, in the error path w
> know we will have waited aprox 750 ms, so printing the waiting
> time there is not worth all the extra code.
>
> As for adding the warning I'm even less of a big fan of that,
> if we need higher timeouts, we need higher timeouts, spamming the
> kernel logs with that we are actually hitting the higher timeouts
> is not productive IMHO.
>
> Can you please resend this as a one-liner just changing the timeout ?
>
> Regards,
>
> Hans
>
>
>>
>> -- 
>>
>>   - fix formatting
>> ---
>>   drivers/mmc/host/sunxi-mmc.c | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index 4d3e1ff..f808a02 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -595,7 +595,7 @@ static irqreturn_t 
>> sunxi_mmc_handle_manual_stop(int irq, void *dev_id)
>>
>>   static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 
>> oclk_en)
>>   {
>> -    unsigned long expire = jiffies + msecs_to_jiffies(250);
>> +    unsigned long start, end;
>>       u32 rval;
>>
>>       rval = mmc_readl(host, REG_CLKCR);
>> @@ -604,6 +604,8 @@ static int sunxi_mmc_oclk_onoff(struct 
>> sunxi_mmc_host *host, u32 oclk_en)
>>       if (oclk_en)
>>           rval |= SDXC_CARD_CLOCK_ON;
>>
>> +    start = jiffies;
>> +    end = start + msecs_to_jiffies(750);
>>       mmc_writel(host, REG_CLKCR, rval);
>>
>>       rval = SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER;
>> @@ -611,15 +613,29 @@ static int sunxi_mmc_oclk_onoff(struct 
>> sunxi_mmc_host *host, u32 oclk_en)
>>
>>       do {
>>           rval = mmc_readl(host, REG_CMDR);
>> -    } while (time_before(jiffies, expire) && (rval & SDXC_START));
>> +    } while (time_before(jiffies, end) && (rval & SDXC_START));
>> +    end = jiffies;
>>
>>       /* clear irq status bits set by the command */
>>       mmc_writel(host, REG_RINTR,
>>              mmc_readl(host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT);
>>
>>       if (rval & SDXC_START) {
>> -        dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
>> +        dev_err(mmc_dev(host->mmc),
>> +            "fatal err update oclk timeout. Could not %s in %ims.\n",
>> +            oclk_en ? "enable" : "disable",
>> +            jiffies_to_msecs(end - start));
>>           return -EIO;
>> +    } else {
>> +        int msecs = jiffies_to_msecs(end - start);
>> +        const char *ing = oclk_en ? "enabling" : "disabling";
>> +
>> +        if ((msecs > 150) || (oclk_en && (msecs > 50)))
>> +            dev_warn(mmc_dev(host->mmc),
>> +                 "%s oclk took %ims", ing, msecs);
>> +        else
>> +            dev_dbg(mmc_dev(host->mmc),
>> +                 "%s oclk took %ims", ing, msecs);
>>       }
>>
>>       return 0;
>>
>

Re: [linux-sunxi] Re: [PATCH resend 1/3] mmc: sunxi: fix timeout in sunxi_mmc_oclk_onoff

[Michal Suchanek]

On 12 August 2015 at 14:40, Olliver Schinagl <oliver+list@schinagl.nl> wrote:

>
> On 12-08-15 14:32, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 12-08-15 14:23, michal.suchanek@ruk.cuni.cz wrote:
>>>
>>> The 250ms timeout is too short.
>>>
>>> On my system enabling the oclk takes under 50ms and disabling slightly
>>> over 100ms when idle. Under load disabling the clock can take over
>>> 350ms.
>>>
>>> This does not make mmc clock gating look like good option to have on
>>> sunxi but the system should not crash with mmc clock gating enabled
>>> nonetheless.
>>>
>>> This patch sets the timeout to 750ms and adds debug prints which show
>>> how long enabling/disabling the clock took so more data can be collected
>>> from other systems.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>
>>
>> This is a big patch for just changing a timeout, most of this is in
>> extra verbosity which IMHO has little value, in the error path w
>> know we will have waited aprox 750 ms, so printing the waiting
>> time there is not worth all the extra code.
>>
>> As for adding the warning I'm even less of a big fan of that,
>> if we need higher timeouts, we need higher timeouts, spamming the
>> kernel logs with that we are actually hitting the higher timeouts
>> is not productive IMHO.
>>
>> Can you please resend this as a one-liner just changing the timeout ?
>>

If we expect the timeout to be short and it isn't that means something is wrong.

The user is probably experiencing degraded performance in that case so
it's good to have a diagnostic for it IMHO.

Yes, printing of the timeout in the error case does not have that much
value but since it's calculated anyway I just pasted it there.

> While I can't speak for Michal obviously,
>
> I left the debugging bit (in my v2 that i sent 2 minutes ago) as both you
> and Hans where content with it back then and both acked it.
>
> Michal, feel free to send the v3 without the debug info, unless you want me
> to do it ;)
>

I don't really care either way so long as the boards do not crash.

Thanks

Michal

Re: [linux-sunxi] [PATCH v3 1/3] mmc: sunxi: fix timeout in sunxi_mmc_oclk_onoff

[Hans de Goede]

Hi,

On 08/12/2015 03:29 PM, Michal Suchanek wrote:
> The 250ms timeout is too short.
>
> On my system enabling the oclk takes under 50ms and disabling slightly
> over 100ms when idle. Under load disabling the clock can take over
> 350ms.
>
> This does not make mmc clock gating look like good option to have on
> sunxi but the system should not crash with mmc clock gating enabled
> nonetheless.
>
> This patch sets the timeout to 750ms.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> ---
> v3
>   - remove debug message

Thanks, this one looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Ulf, can you apply this one please?

Thanks & Regards,

Hans


> v2
>   - fix formatting
> ---
>   drivers/mmc/host/sunxi-mmc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 4d3e1ff..a7b7a67 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -595,7 +595,7 @@ static irqreturn_t sunxi_mmc_handle_manual_stop(int irq, void *dev_id)
>
>   static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>   {
> -	unsigned long expire = jiffies + msecs_to_jiffies(250);
> +	unsigned long expire = jiffies + msecs_to_jiffies(750);
>   	u32 rval;
>
>   	rval = mmc_readl(host, REG_CLKCR);
>

Re: [linux-sunxi] [PATCH v3 1/3] mmc: sunxi: fix timeout in sunxi_mmc_oclk_onoff

[Ulf Hansson]

On 12 August 2015 at 16:49, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 08/12/2015 03:29 PM, Michal Suchanek wrote:
>>
>> The 250ms timeout is too short.
>>
>> On my system enabling the oclk takes under 50ms and disabling slightly
>> over 100ms when idle. Under load disabling the clock can take over
>> 350ms.
>>
>> This does not make mmc clock gating look like good option to have on
>> sunxi but the system should not crash with mmc clock gating enabled
>> nonetheless.
>>
>> This patch sets the timeout to 750ms.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>
>> ---
>> v3
>>   - remove debug message
>
>
> Thanks, this one looks good to me:
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
>
> Ulf, can you apply this one please?
>
> Thanks & Regards,
>
> Hans
>

Thanks, applied for next!

Kind regards
Uffe

>
>
>> v2
>>   - fix formatting
>> ---
>>   drivers/mmc/host/sunxi-mmc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index 4d3e1ff..a7b7a67 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -595,7 +595,7 @@ static irqreturn_t sunxi_mmc_handle_manual_stop(int
>> irq, void *dev_id)
>>
>>   static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32
>> oclk_en)
>>   {
>> -       unsigned long expire = jiffies + msecs_to_jiffies(250);
>> +       unsigned long expire = jiffies + msecs_to_jiffies(750);
>>         u32 rval;
>>
>>         rval = mmc_readl(host, REG_CLKCR);
>>
>

Re: [linux-sunxi] [PATCH 3/3] mmc: sunxi: use controller automatic clock gating.

[Hans de Goede]

Hi,

On 12-08-15 14:23, Michal Suchanek wrote:
> When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable
> automatic hardware controlled clock gating on the mmc interface.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

In general this looks good, but I wonder how intensively this has
been tested ? Also given the long latencies when using manual
clock on/off support, have you done any testing to check what
sort of latencies this adds, e.g. Both with and without
the patch, dump all the filesystem caches:

echo 3 > /proc/sys/vm/drop_caches

And then do:

time cat /some/small/file/on/the/sdcard

Do this at least 3 time both with and without the patch, and see
if there are any noticable differences ?

Regards,

Hans



> ---
>   drivers/mmc/host/sunxi-mmc.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index f808a02..443cab5 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -601,8 +601,13 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>   	rval = mmc_readl(host, REG_CLKCR);
>   	rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
>
> -	if (oclk_en)
> +	if (oclk_en) {
>   		rval |= SDXC_CARD_CLOCK_ON;
> +		if (!host->mmc->card ||
> +		     !(host->mmc->card->quirks & MMC_QUIRK_BROKEN_CLK_GATING))
> +
> +			rval |= SDXC_LOW_POWER_ON;
> +	}
>
>   	start = jiffies;
>   	end = start + msecs_to_jiffies(750);
>

Re: [linux-sunxi] [PATCH 3/3] mmc: sunxi: use controller automatic clock gating.

[Michal Suchanek]

On 12 August 2015 at 14:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 12-08-15 14:23, Michal Suchanek wrote:
>>
>> When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable
>> automatic hardware controlled clock gating on the mmc interface.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
>
> In general this looks good, but I wonder how intensively this has
> been tested ?

Unlike the wrong timeout it does not cause the boards to crash frequently ;-)

It would be nice if somebody actively using some board atm tested this.

> Also given the long latencies when using manual
> clock on/off support, have you done any testing to check what

I suspect the long latency is due to the controller flushing data to
the card or something like that since it happens only under load.

> sort of latencies this adds, e.g. Both with and without
> the patch, dump all the filesystem caches:
>
> echo 3 > /proc/sys/vm/drop_caches
>
> And then do:
>
> time cat /some/small/file/on/the/sdcard
>
> Do this at least 3 time both with and without the patch, and see
> if there are any noticable differences ?

Yes, it would be good idea to test.

Thanks

Michal

Re: [linux-sunxi] [PATCH 3/3] mmc: sunxi: use controller automatic clock gating.

[Olliver Schinagl]

Hey,

On 12-08-15 14:35, Hans de Goede wrote:
> Hi,
>
> On 12-08-15 14:23, Michal Suchanek wrote:
>> When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable
>> automatic hardware controlled clock gating on the mmc interface.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> In general this looks good, but I wonder how intensively this has
> been tested ?
It doesn't matter actually, it took some time longer, but the mmc still 
craps out even with Michal's 3 patches. I'll revert hans's earlier patch 
again and do a bit more extensive testing.
> Also given the long latencies when using manual
> clock on/off support, have you done any testing to check what
> sort of latencies this adds, e.g. Both with and without
> the patch, dump all the filesystem caches:
>
> echo 3 > /proc/sys/vm/drop_caches
>
> And then do:
>
> time cat /some/small/file/on/the/sdcard
>
> Do this at least 3 time both with and without the patch, and see
> if there are any noticable differences ?
>
> Regards,
>
> Hans
>
>
>
>> ---
>>   drivers/mmc/host/sunxi-mmc.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index f808a02..443cab5 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -601,8 +601,13 @@ static int sunxi_mmc_oclk_onoff(struct 
>> sunxi_mmc_host *host, u32 oclk_en)
>>       rval = mmc_readl(host, REG_CLKCR);
>>       rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
>>
>> -    if (oclk_en)
>> +    if (oclk_en) {
>>           rval |= SDXC_CARD_CLOCK_ON;
>> +        if (!host->mmc->card ||
>> +             !(host->mmc->card->quirks & MMC_QUIRK_BROKEN_CLK_GATING))
>> +
>> +            rval |= SDXC_LOW_POWER_ON;
>> +    }
>>
>>       start = jiffies;
>>       end = start + msecs_to_jiffies(750);
>>
>

Re: [linux-sunxi] [PATCH 3/3] mmc: sunxi: use controller automatic clock gating.

[Michal Suchanek]

On 12 August 2015 at 15:19, Olliver Schinagl <oliver+list@schinagl.nl> wrote:
> Hey,
>
> On 12-08-15 14:35, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 12-08-15 14:23, Michal Suchanek wrote:
>>>
>>> When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable
>>> automatic hardware controlled clock gating on the mmc interface.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>
>>
>> In general this looks good, but I wonder how intensively this has
>> been tested ?
>
> It doesn't matter actually, it took some time longer, but the mmc still
> craps out even with Michal's 3 patches. I'll revert hans's earlier patch
> again and do a bit more extensive testing.

Does the oclk switch timeout even after 750ms?

In some earlier tests I tried to enable/disable the clock repeatedly
when it failed but it seemed to have little effect on the total time
it took to disable the clock in the end.

Maybe it would be worh trying to set the timeout to some insanely long
value and test stability with that. I picked 750 as around twice the
maximum time it ever took on my board.

Thanks

Michal

Re: [linux-sunxi] [PATCH 3/3] mmc: sunxi: use controller automatic clock gating.

[Olliver Schinagl]

The thing is, I never saw the prints in my console (i assume that it 
prints on the console by default with earlyprintk/debug stuff enabled as 
per sunxi-usual)?

P.S. what I do as a basic test, is on our very very minimal empty debian 
image (think < 150 mb) i do a apt-get build-essential that installs 
about 150mb worth of packages.

Without these patches, everything works as before. All packages 
successfully installed. I can do more intense testing later, but I 
figured that this is pretty intense.

Olliver

On 12-08-15 15:37, Michal Suchanek wrote:
> On 12 August 2015 at 15:19, Olliver Schinagl <oliver+list@schinagl.nl> wrote:
>> Hey,
>>
>> On 12-08-15 14:35, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 12-08-15 14:23, Michal Suchanek wrote:
>>>> When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable
>>>> automatic hardware controlled clock gating on the mmc interface.
>>>>
>>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>>
>>> In general this looks good, but I wonder how intensively this has
>>> been tested ?
>> It doesn't matter actually, it took some time longer, but the mmc still
>> craps out even with Michal's 3 patches. I'll revert hans's earlier patch
>> again and do a bit more extensive testing.
> Does the oclk switch timeout even after 750ms?
>
> In some earlier tests I tried to enable/disable the clock repeatedly
> when it failed but it seemed to have little effect on the total time
> it took to disable the clock in the end.
>
> Maybe it would be worh trying to set the timeout to some insanely long
> value and test stability with that. I picked 750 as around twice the
> maximum time it ever took on my board.
>
> Thanks
>
> Michal
>

Re: [PATCH 2/3] mmc: dw_mmc: simplify the SDMMC_CLKEN_LOW_PWR logic

[Jaehoon Chung]

Hi, Michal.

On 08/12/2015 09:23 PM, Michal Suchanek wrote:
> The driver has open-coded test for SDIO cards. Use the mmc core provided
> MMC_QUIRK_BROKEN_CLK_GATING flag instead.

Did you use the clock-gating for SDIO cards?
Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken?
Could you explain to me more?

Best Regards,
Jaehoon Chung

> 
> As a bonus this may enable clock gating on SDIO cards that are known to
> work with it.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 40e9d8e..3bc9fd7 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>  	 * description of the CLKENA register we should disable low power mode
>  	 * for SDIO cards if we need SDIO interrupts to work.
>  	 */
> -	if (mmc->caps & MMC_CAP_SDIO_IRQ) {
> -		const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> -		u32 clk_en_a_old;
> -		u32 clk_en_a;
> +	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> +	u32 clk_en_a_old;
> +	u32 clk_en_a;
>  
> -		clk_en_a_old = mci_readl(host, CLKENA);
> +	clk_en_a_old = mci_readl(host, CLKENA);
>  
> -		if (card->type == MMC_TYPE_SDIO ||
> -		    card->type == MMC_TYPE_SD_COMBO) {
> -			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> -			clk_en_a = clk_en_a_old & ~clken_low_pwr;
> -		} else {
> -			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> -			clk_en_a = clk_en_a_old | clken_low_pwr;
> -		}
> +	if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
> +		set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +		clk_en_a = clk_en_a_old & ~clken_low_pwr;
> +	} else {
> +		clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +		clk_en_a = clk_en_a_old | clken_low_pwr;
> +	}
>  
> -		if (clk_en_a != clk_en_a_old) {
> -			mci_writel(host, CLKENA, clk_en_a);
> -			mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> -				     SDMMC_CMD_PRV_DAT_WAIT, 0);
> -		}
> +	if (clk_en_a != clk_en_a_old) {
> +		mci_writel(host, CLKENA, clk_en_a);
> +		mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> +				SDMMC_CMD_PRV_DAT_WAIT, 0);
>  	}
>  }
>  
> 

Re: [PATCH 2/3] mmc: dw_mmc: simplify the SDMMC_CLKEN_LOW_PWR logic

[Michal Suchanek]

Hello,

On 17 August 2015 at 03:55, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi, Michal.
>
> On 08/12/2015 09:23 PM, Michal Suchanek wrote:
>> The driver has open-coded test for SDIO cards. Use the mmc core provided
>> MMC_QUIRK_BROKEN_CLK_GATING flag instead.
>
> Did you use the clock-gating for SDIO cards?
> Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken?
> Could you explain to me more?

The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.

It may coincide with MMC_CAP_SDIO_IRQ but that's different flag for
different purpose.

MMC_QUIRK_BROKEN_CLK_GATING is currently set for all SDIO cards except
for cards on a whitelist.

I don't know any particular SDIO card that has problems when the clock
is off and does not use an IRQ.

Thanks

Michal

>
> Best Regards,
> Jaehoon Chung
>
>>
>> As a bonus this may enable clock gating on SDIO cards that are known to
>> work with it.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>> ---
>>  drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
>>  1 file changed, 15 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 40e9d8e..3bc9fd7 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>        * description of the CLKENA register we should disable low power mode
>>        * for SDIO cards if we need SDIO interrupts to work.
>>        */
>> -     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
>> -             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>> -             u32 clk_en_a_old;
>> -             u32 clk_en_a;
>> +     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>> +     u32 clk_en_a_old;
>> +     u32 clk_en_a;
>>
>> -             clk_en_a_old = mci_readl(host, CLKENA);
>> +     clk_en_a_old = mci_readl(host, CLKENA);
>>
>> -             if (card->type == MMC_TYPE_SDIO ||
>> -                 card->type == MMC_TYPE_SD_COMBO) {
>> -                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>> -                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
>> -             } else {
>> -                     clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>> -                     clk_en_a = clk_en_a_old | clken_low_pwr;
>> -             }
>> +     if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
>> +             set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>> +             clk_en_a = clk_en_a_old & ~clken_low_pwr;
>> +     } else {
>> +             clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>> +             clk_en_a = clk_en_a_old | clken_low_pwr;
>> +     }
>>
>> -             if (clk_en_a != clk_en_a_old) {
>> -                     mci_writel(host, CLKENA, clk_en_a);
>> -                     mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>> -                                  SDMMC_CMD_PRV_DAT_WAIT, 0);
>> -             }
>> +     if (clk_en_a != clk_en_a_old) {
>> +             mci_writel(host, CLKENA, clk_en_a);
>> +             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>> +                             SDMMC_CMD_PRV_DAT_WAIT, 0);
>>       }
>>  }
>>
>>
>

Re: [PATCH 2/3] mmc: dw_mmc: simplify the SDMMC_CLKEN_LOW_PWR logic

[Jaehoon Chung]

On 08/17/2015 02:52 PM, Michal Suchanek wrote:
> Hello,
> 
> On 17 August 2015 at 03:55, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi, Michal.
>>
>> On 08/12/2015 09:23 PM, Michal Suchanek wrote:
>>> The driver has open-coded test for SDIO cards. Use the mmc core provided
>>> MMC_QUIRK_BROKEN_CLK_GATING flag instead.
>>
>> Did you use the clock-gating for SDIO cards?
>> Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken?
>> Could you explain to me more?
> 
> The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.

I understood your intention. And i read the comment into mmc/core/quirks.c
I will test SDIO card with this patch. Thanks.

Best Regards,
Jaehoon Chung

> 
> It may coincide with MMC_CAP_SDIO_IRQ but that's different flag for
> different purpose.
> 
> MMC_QUIRK_BROKEN_CLK_GATING is currently set for all SDIO cards except
> for cards on a whitelist.
> 
> I don't know any particular SDIO card that has problems when the clock
> is off and does not use an IRQ.
> 
> Thanks
> 
> Michal
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> As a bonus this may enable clock gating on SDIO cards that are known to
>>> work with it.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>> ---
>>>  drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
>>>  1 file changed, 15 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 40e9d8e..3bc9fd7 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>        * description of the CLKENA register we should disable low power mode
>>>        * for SDIO cards if we need SDIO interrupts to work.
>>>        */
>>> -     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
>>> -             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>> -             u32 clk_en_a_old;
>>> -             u32 clk_en_a;
>>> +     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>> +     u32 clk_en_a_old;
>>> +     u32 clk_en_a;
>>>
>>> -             clk_en_a_old = mci_readl(host, CLKENA);
>>> +     clk_en_a_old = mci_readl(host, CLKENA);
>>>
>>> -             if (card->type == MMC_TYPE_SDIO ||
>>> -                 card->type == MMC_TYPE_SD_COMBO) {
>>> -                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>> -                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
>>> -             } else {
>>> -                     clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>> -                     clk_en_a = clk_en_a_old | clken_low_pwr;
>>> -             }
>>> +     if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
>>> +             set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>> +             clk_en_a = clk_en_a_old & ~clken_low_pwr;
>>> +     } else {
>>> +             clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>> +             clk_en_a = clk_en_a_old | clken_low_pwr;
>>> +     }
>>>
>>> -             if (clk_en_a != clk_en_a_old) {
>>> -                     mci_writel(host, CLKENA, clk_en_a);
>>> -                     mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>>> -                                  SDMMC_CMD_PRV_DAT_WAIT, 0);
>>> -             }
>>> +     if (clk_en_a != clk_en_a_old) {
>>> +             mci_writel(host, CLKENA, clk_en_a);
>>> +             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>>> +                             SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>       }
>>>  }
>>>
>>>
>>
> --
> 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
> 

Re: [PATCH 2/3] mmc: dw_mmc: simplify the SDMMC_CLKEN_LOW_PWR logic

[Alim Akhtar]

HI

On Mon, Aug 17, 2015 at 4:56 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 08/17/2015 02:52 PM, Michal Suchanek wrote:
>> Hello,
>>
>> On 17 August 2015 at 03:55, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> Hi, Michal.
>>>
>>> On 08/12/2015 09:23 PM, Michal Suchanek wrote:
>>>> The driver has open-coded test for SDIO cards. Use the mmc core provided
>>>> MMC_QUIRK_BROKEN_CLK_GATING flag instead.
>>>
>>> Did you use the clock-gating for SDIO cards?
>>> Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken?
>>> Could you explain to me more?
>>
>> The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.
>
> I understood your intention. And i read the comment into mmc/core/quirks.c
> I will test SDIO card with this patch. Thanks.
>
When you test, please check if SDIO IRQ still works, we need to put
dw_mmc in low_power mode otherwise SDIO IRQ will be not be generated
by dw_mmc host controller.

> Best Regards,
> Jaehoon Chung
>
>>
>> It may coincide with MMC_CAP_SDIO_IRQ but that's different flag for
>> different purpose.
>>
>> MMC_QUIRK_BROKEN_CLK_GATING is currently set for all SDIO cards except
>> for cards on a whitelist.
>>
>> I don't know any particular SDIO card that has problems when the clock
>> is off and does not use an IRQ.
>>
>> Thanks
>>
>> Michal
>>
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>> As a bonus this may enable clock gating on SDIO cards that are known to
>>>> work with it.
>>>>
>>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>>> ---
>>>>  drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
>>>>  1 file changed, 15 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index 40e9d8e..3bc9fd7 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>>        * description of the CLKENA register we should disable low power mode
>>>>        * for SDIO cards if we need SDIO interrupts to work.
>>>>        */
>>>> -     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
>>>> -             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>>> -             u32 clk_en_a_old;
>>>> -             u32 clk_en_a;
>>>> +     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>>> +     u32 clk_en_a_old;
>>>> +     u32 clk_en_a;
>>>>
>>>> -             clk_en_a_old = mci_readl(host, CLKENA);
>>>> +     clk_en_a_old = mci_readl(host, CLKENA);
>>>>
>>>> -             if (card->type == MMC_TYPE_SDIO ||
>>>> -                 card->type == MMC_TYPE_SD_COMBO) {
>>>> -                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>>> -                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
>>>> -             } else {
>>>> -                     clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>>> -                     clk_en_a = clk_en_a_old | clken_low_pwr;
>>>> -             }
>>>> +     if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
>>>> +             set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>>> +             clk_en_a = clk_en_a_old & ~clken_low_pwr;
>>>> +     } else {
>>>> +             clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>>> +             clk_en_a = clk_en_a_old | clken_low_pwr;
>>>> +     }
>>>>
>>>> -             if (clk_en_a != clk_en_a_old) {
>>>> -                     mci_writel(host, CLKENA, clk_en_a);
>>>> -                     mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>>>> -                                  SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>> -             }
>>>> +     if (clk_en_a != clk_en_a_old) {
>>>> +             mci_writel(host, CLKENA, clk_en_a);
>>>> +             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>>>> +                             SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>>       }
>>>>  }
>>>>
>>>>
>>>
>> --
>> 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
>>
>
> --
> 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



-- 
Regards,
Alim

Re: [PATCH 2/3] mmc: dw_mmc: simplify the SDMMC_CLKEN_LOW_PWR logic

[Michal Suchanek]

 Hello,

On 17 August 2015 at 16:42, Alim Akhtar <alim.akhtar@gmail.com> wrote:
> HI
>
> On Mon, Aug 17, 2015 at 4:56 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> On 08/17/2015 02:52 PM, Michal Suchanek wrote:
>>> Hello,
>>>
>>> On 17 August 2015 at 03:55, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>> Hi, Michal.
>>>>
>>>> On 08/12/2015 09:23 PM, Michal Suchanek wrote:
>>>>> The driver has open-coded test for SDIO cards. Use the mmc core provided
>>>>> MMC_QUIRK_BROKEN_CLK_GATING flag instead.
>>>>
>>>> Did you use the clock-gating for SDIO cards?
>>>> Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken?
>>>> Could you explain to me more?
>>>
>>> The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.
>>
>> I understood your intention. And i read the comment into mmc/core/quirks.c
>> I will test SDIO card with this patch. Thanks.
>>
> When you test, please check if SDIO IRQ still works, we need to put
> dw_mmc in low_power mode otherwise SDIO IRQ will be not be generated
> by dw_mmc host controller.
>

As far as I understand the logic which is removed in this patch and
the core logic which replaces it is the same -  low power by means of
clock gating is *not* enabled for SDIO cards in either case.

The original code also checks for SDIO IRQ and disables clock gating
regardless of card type which is probably redundant. If not it should
be fixed in mmc core.

My recent kernel builds which I run on a system with mwifiex card
probably include this patch.

Thanks

Michal

Re: [PATCH 2/3] mmc: dw_mmc: simplify the SDMMC_CLKEN_LOW_PWR logic

[Alim Akhtar]

Hi Michal,

On Mon, Aug 17, 2015 at 8:25 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>  Hello,
>
> On 17 August 2015 at 16:42, Alim Akhtar <alim.akhtar@gmail.com> wrote:
>> HI
>>
>> On Mon, Aug 17, 2015 at 4:56 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> On 08/17/2015 02:52 PM, Michal Suchanek wrote:
>>>> Hello,
>>>>
>>>> On 17 August 2015 at 03:55, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>> Hi, Michal.
>>>>>
>>>>> On 08/12/2015 09:23 PM, Michal Suchanek wrote:
>>>>>> The driver has open-coded test for SDIO cards. Use the mmc core provided
>>>>>> MMC_QUIRK_BROKEN_CLK_GATING flag instead.
>>>>>
>>>>> Did you use the clock-gating for SDIO cards?
>>>>> Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken?
>>>>> Could you explain to me more?
>>>>
>>>> The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.
>>>
>>> I understood your intention. And i read the comment into mmc/core/quirks.c
>>> I will test SDIO card with this patch. Thanks.
>>>
>> When you test, please check if SDIO IRQ still works, we need to put
>> dw_mmc in low_power mode otherwise SDIO IRQ will be not be generated
>> by dw_mmc host controller.
>>
>
> As far as I understand the logic which is removed in this patch and
> the core logic which replaces it is the same -  low power by means of
> clock gating is *not* enabled for SDIO cards in either case.
>
> The original code also checks for SDIO IRQ and disables clock gating
> regardless of card type which is probably redundant. If not it should
> be fixed in mmc core.
>
> My recent kernel builds which I run on a system with mwifiex card
> probably include this patch.
>
I have no objection to this patch as such, it just that some extra
testing on few other boards will be good, which I am sure Jeahoon will
take care.
Thanks!!

> Thanks
>
> Michal



-- 
Regards,
Alim