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

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

[Julian Calaby]

Hi Michal,

On Mon, May 25, 2015 at 4:07 AM, Michal Suchanek <hramrach@gmail.com> 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>
> ---
>  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..7cdeecd 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);

You might want to make the timeout value a #define so it's not buried
in the code like this.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Hans de Goede]

Hi,

On 24-05-15 20:07, 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 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>

Looks good: Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>   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..7cdeecd 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: [PATCH 2/2] mmc: sunxi: Also set SDXC_LOW_POWER_ON

[Hans de Goede]

Hi,

On 24-05-15 20:04, Michal Suchanek wrote:
> The function sunxi_mmc_oclk_onoff filters out the SDXC_LOW_POWER_ON flag
> but never sets it.
>
> Set SDXC_LOW_POWER_ON when oclk is disabled.

Nack, looking at the datasheet I do not thing this patch actually
does anything, according to the datasheet setting this bit to 1 results
in: "Turn off card clock when FSM in IDLE state", iow this does mmc clock
gating on idle automatically in hardware, since we completely disable the
clock on clock-off by clearing SDXC_CARD_CLOCK_ON setting this bit on
clock-off is a nop.

We could consider actually setting this to safe power when setting the clock
on, for doing that it would be good to look at the android code and see if
it ever sets this bit and if so when.

WRT CONFIG_MMC_CLKGATE we are probably better off using SDXC_LOW_POWER_ON
then that when it is enabled. So maybe we should set SDXC_LOW_POWER_ON
based on #ifdef WRT CONFIG_MMC_CLKGATE, and have some way to tell the
mmc core to not do clock gating on this host ?

Regards,

Hans



>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>   drivers/mmc/host/sunxi-mmc.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 7cdeecd..e957888 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -603,6 +603,8 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>
>   	if (oclk_en)
>   		rval |= SDXC_CARD_CLOCK_ON;
> +	else
> +		rval |= SDXC_LOW_POWER_ON;
>
>   	start = jiffies;
>   	end = start + msecs_to_jiffies(750);
>

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3064 bytes --]

Hi Michal,

The patch itself looks good, but there's a few style issues.

On Sun, May 24, 2015 at 08:07:32PM +0200, 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 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>
> ---
>  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..7cdeecd 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));

You should have spaces around that operator.

>  		return -EIO;
> +	} else {
> +		int msecs = jiffies_to_msecs(end-start);

Ditto.

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

And this should be aligned with the opening parenthesis.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] mmc: sunxi: Also set SDXC_LOW_POWER_ON

[Ulf Hansson]

On 25 May 2015 at 09:02, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 24-05-15 20:04, Michal Suchanek wrote:
>>
>> The function sunxi_mmc_oclk_onoff filters out the SDXC_LOW_POWER_ON flag
>> but never sets it.
>>
>> Set SDXC_LOW_POWER_ON when oclk is disabled.
>
>
> Nack, looking at the datasheet I do not thing this patch actually
> does anything, according to the datasheet setting this bit to 1 results
> in: "Turn off card clock when FSM in IDLE state", iow this does mmc clock
> gating on idle automatically in hardware, since we completely disable the
> clock on clock-off by clearing SDXC_CARD_CLOCK_ON setting this bit on
> clock-off is a nop.
>
> We could consider actually setting this to safe power when setting the clock
> on, for doing that it would be good to look at the android code and see if
> it ever sets this bit and if so when.
>
> WRT CONFIG_MMC_CLKGATE we are probably better off using SDXC_LOW_POWER_ON
> then that when it is enabled. So maybe we should set SDXC_LOW_POWER_ON
> based on #ifdef WRT CONFIG_MMC_CLKGATE, and have some way to tell the
> mmc core to not do clock gating on this host ?

I am planning to remove the entire thing for CONFIG_MMC_CLKGATE, maybe
it's time to do that now!?

Most hosts use runtime PM to deal with clock gating at idle and
browsing def configs tells me that CONFIG_MMC_CLKGATE isn't much used.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: sunxi: Also set SDXC_LOW_POWER_ON

[Hans de Goede]

Hi,

On 28-05-15 10:02, Ulf Hansson wrote:
> On 25 May 2015 at 09:02, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 24-05-15 20:04, Michal Suchanek wrote:
>>>
>>> The function sunxi_mmc_oclk_onoff filters out the SDXC_LOW_POWER_ON flag
>>> but never sets it.
>>>
>>> Set SDXC_LOW_POWER_ON when oclk is disabled.
>>
>>
>> Nack, looking at the datasheet I do not thing this patch actually
>> does anything, according to the datasheet setting this bit to 1 results
>> in: "Turn off card clock when FSM in IDLE state", iow this does mmc clock
>> gating on idle automatically in hardware, since we completely disable the
>> clock on clock-off by clearing SDXC_CARD_CLOCK_ON setting this bit on
>> clock-off is a nop.
>>
>> We could consider actually setting this to safe power when setting the clock
>> on, for doing that it would be good to look at the android code and see if
>> it ever sets this bit and if so when.
>>
>> WRT CONFIG_MMC_CLKGATE we are probably better off using SDXC_LOW_POWER_ON
>> then that when it is enabled. So maybe we should set SDXC_LOW_POWER_ON
>> based on #ifdef WRT CONFIG_MMC_CLKGATE, and have some way to tell the
>> mmc core to not do clock gating on this host ?
>
> I am planning to remove the entire thing for CONFIG_MMC_CLKGATE, maybe
> it's time to do that now!?

Removing it now sounds like a good plan to me, we would still need
to look into maybe enabling SDXC_LOW_POWER_ON on sunxi in some cases
to save power, but that is an orthogonal problem.

Regards,

Hans