[PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Laurent Pinchart]

The tmio_mmc_set_ios() function configures the MMC power, clock and bus
width. When the MMC controller gets powered off, runtime PM switches the
MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
fails and prints an error message to the kernel log.

As configuring the bus width is pointless when the interface gets
powered down, skip the operation when power is off.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/mmc/host/tmio_mmc_pio.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 9a7996a..de1226d 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -816,13 +816,15 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		tmio_mmc_clk_stop(host);
 	}
 
-	switch (ios->bus_width) {
-	case MMC_BUS_WIDTH_1:
-		sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
-	break;
-	case MMC_BUS_WIDTH_4:
-		sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
-	break;
+	if (host->power) {
+		switch (ios->bus_width) {
+		case MMC_BUS_WIDTH_1:
+			sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
+		break;
+		case MMC_BUS_WIDTH_4:
+			sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
+		break;
+		}
 	}
 
 	/* Let things settle. delay taken from winCE driver */
-- 
1.7.3.4

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Guennadi Liakhovetski]

Hi Laurent

On Tue, 12 Jun 2012, Laurent Pinchart wrote:

> The tmio_mmc_set_ios() function configures the MMC power, clock and bus
> width. When the MMC controller gets powered off, runtime PM switches the
> MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
> fails and prints an error message to the kernel log.
> 
> As configuring the bus width is pointless when the interface gets
> powered down, skip the operation when power is off.

The patch looks good - thanks! But maybe we can make the commit message a 
bit more precise. I think, ios->power_mode refers to powering the card 
down and up, not the controller. We use this information to also try to 
put the controller in a power-saving mode. Depending on the kernel and 
board configuration this can be a NOP, it can switch the MSTP clock off, 
as you correctly notice, or it can power the whole PM domain, to which the 
controller belongs, down. So, maybe something like this would be better:

The tmio_mmc_set_ios() function configures the MMC power, clock and bus 
width. When the mmc core requests the driver to power off the card, we 
inform runtime PM, that the controller can be suspended. This can lead to 
the MSTP clock being turned off. Writing to to CTL_SD_MEM_CARD_OPT 
register afterwards fails and prints an error message to the kernel log.

As conf... (continue as above)

Feel free to further improve the above :)

Thanks
Guennadi

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/mmc/host/tmio_mmc_pio.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 9a7996a..de1226d 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -816,13 +816,15 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  		tmio_mmc_clk_stop(host);
>  	}
>  
> -	switch (ios->bus_width) {
> -	case MMC_BUS_WIDTH_1:
> -		sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
> -	break;
> -	case MMC_BUS_WIDTH_4:
> -		sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
> -	break;
> +	if (host->power) {
> +		switch (ios->bus_width) {
> +		case MMC_BUS_WIDTH_1:
> +			sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
> +		break;
> +		case MMC_BUS_WIDTH_4:
> +			sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
> +		break;
> +		}
>  	}
>  
>  	/* Let things settle. delay taken from winCE driver */
> -- 
> 1.7.3.4
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 12 June 2012 15:31:49 Guennadi Liakhovetski wrote:
> On Tue, 12 Jun 2012, Laurent Pinchart wrote:
> > The tmio_mmc_set_ios() function configures the MMC power, clock and bus
> > width. When the MMC controller gets powered off, runtime PM switches the
> > MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
> > fails and prints an error message to the kernel log.
> > 
> > As configuring the bus width is pointless when the interface gets
> > powered down, skip the operation when power is off.
> 
> The patch looks good - thanks! But maybe we can make the commit message a
> bit more precise. I think, ios->power_mode refers to powering the card
> down and up, not the controller. We use this information to also try to
> put the controller in a power-saving mode. Depending on the kernel and
> board configuration this can be a NOP, it can switch the MSTP clock off,
> as you correctly notice, or it can power the whole PM domain, to which the
> controller belongs, down. So, maybe something like this would be better:
> 
> The tmio_mmc_set_ios() function configures the MMC power, clock and bus
> width. When the mmc core requests the driver to power off the card, we
> inform runtime PM, that the controller can be suspended. This can lead to
> the MSTP clock being turned off. Writing to to CTL_SD_MEM_CARD_OPT
> register afterwards fails and prints an error message to the kernel log.
> 
> As conf... (continue as above)
> 
> Feel free to further improve the above :)

Thank you for the feedback. I'll send a v2 that incorporates a second related 
fix.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Magnus Damm]

Hey Laurent,

[CC Rafael]

On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> The tmio_mmc_set_ios() function configures the MMC power, clock and bus
> width. When the MMC controller gets powered off, runtime PM switches the
> MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
> fails and prints an error message to the kernel log.
>
> As configuring the bus width is pointless when the interface gets
> powered down, skip the operation when power is off.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

First of all, thanks for reporting this issue and coming up with a fix!

Can you please explain a bit more about when this triggers? Is this
related to suspend-to-ram perhaps? Which hardware platform? Is
CONFIG_PM_RUNTIME=y set?

I suspect that this may be a side effect of the current PM code used
on the A1 SoC (which is hooked up on the armadillo board).

In short, the A1 SoC does not yet make use of PM domains, but AP4 and
the mackerel board (sh7372 based) is using PM domains. This difference
may result in different state of clocks at suspend-to-ram timing. So
the SDHI driver may work just fine on the mackerel board, but not on
the armadillo board. If that's the case then perhaps we shouldn't fix
the driver, but instead look at the platform level.

> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 9a7996a..de1226d 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -816,13 +816,15 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                tmio_mmc_clk_stop(host);
>        }
>
> -       switch (ios->bus_width) {
> -       case MMC_BUS_WIDTH_1:
> -               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
> -       break;
> -       case MMC_BUS_WIDTH_4:
> -               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
> -       break;
> +       if (host->power) {
> +               switch (ios->bus_width) {
> +               case MMC_BUS_WIDTH_1:
> +                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
> +               break;
> +               case MMC_BUS_WIDTH_4:
> +                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
> +               break;
> +               }
>        }
>
>        /* Let things settle. delay taken from winCE driver */
> --
> 1.7.3.4

Is it possible that you get here through tmio_mmc_host_suspend() and
mmc_suspend_host()?

Just guessing. =)

Thanks,

/ magnus

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Laurent Pinchart]

Hi Magnus,

On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
> On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
> > The tmio_mmc_set_ios() function configures the MMC power, clock and bus
> > width. When the MMC controller gets powered off, runtime PM switches the
> > MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
> > fails and prints an error message to the kernel log.
> > 
> > As configuring the bus width is pointless when the interface gets
> > powered down, skip the operation when power is off.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> First of all, thanks for reporting this issue and coming up with a fix!

You're welcome. You can expect more of them ;-)

> Can you please explain a bit more about when this triggers? Is this
> related to suspend-to-ram perhaps? Which hardware platform? Is
> CONFIG_PM_RUNTIME=y set?

I've noticed the problem on the Armadillo board with CONFIG_PM_RUNTIME=y. The 
driver spits out "timeout waiting for SD bus idle" error messages more or less 
continuously.

> I suspect that this may be a side effect of the current PM code used
> on the A1 SoC (which is hooked up on the armadillo board).
> 
> In short, the A1 SoC does not yet make use of PM domains, but AP4 and
> the mackerel board (sh7372 based) is using PM domains.

Does this mean that runtime PM is a no-op on A1 ? That would surprise me, as 
turning CONFIG_PM_RUNTIME off gets rid of the problem, so runtime PM is 
somehow involved. Even if the power domain does not get turned off, can't the 
MSTP clock be turned off ?

> This difference may result in different state of clocks at suspend-to-ram
> timing. So the SDHI driver may work just fine on the mackerel board, but not
> on the armadillo board. If that's the case then perhaps we shouldn't fix the
> driver, but instead look at the platform level.

I still believe there's a bug in the driver, please see below.

> > diff --git a/drivers/mmc/host/tmio_mmc_pio.c
> > b/drivers/mmc/host/tmio_mmc_pio.c index 9a7996a..de1226d 100644
> > --- a/drivers/mmc/host/tmio_mmc_pio.c
> > +++ b/drivers/mmc/host/tmio_mmc_pio.c
> > @@ -816,13 +816,15 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc,
> > struct mmc_ios *ios) tmio_mmc_clk_stop(host);
> >        }
> > 
> > -       switch (ios->bus_width) {
> > -       case MMC_BUS_WIDTH_1:
> > -               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
> > -       break;
> > -       case MMC_BUS_WIDTH_4:
> > -               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
> > -       break;
> > +       if (host->power) {
> > +               switch (ios->bus_width) {
> > +               case MMC_BUS_WIDTH_1:
> > +                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT,
> > 0x80e0);
> > +               break;
> > +               case MMC_BUS_WIDTH_4:
> > +                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT,
> > 0x00e0);
> > +               break;
> > +               }
> >        }
> > 
> >        /* Let things settle. delay taken from winCE driver */
> > --
> > 1.7.3.4
> 
> Is it possible that you get here through tmio_mmc_host_suspend() and
> mmc_suspend_host()?
> 
> Just guessing. =)

I haven't checked the complete call stack, but I don't think it matters that 
much.

What happens here is that the driver tried to write to a 16-bit hardware 
register after calling pm_runtime_put(). At that point runtime PM may have 
turned to power domain and/or the MSTP clock off for the device. Writing to a 
16-bit register first polls for the bus idle bit in the CTL_STATUS2 to be set. 
This never happens in this case (probably because the clock has been turned 
off), so the write operation fails.

Speaking generally, I think we should avoid writing to the device after 
calling pm_runtime_put(), it just doesn't make much sense. If we release the 
device from a PM point of view, it means we don't need to touch it anymore, so 
we should prevent writes until the next pm_runtime_get_sync() call.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Rafael J. Wysocki]

On Thursday, June 14, 2012, Laurent Pinchart wrote:
> Hi Magnus,
> 
> On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
> > On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
> > > The tmio_mmc_set_ios() function configures the MMC power, clock and bus
> > > width. When the MMC controller gets powered off, runtime PM switches the
> > > MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
> > > fails and prints an error message to the kernel log.
> > > 
> > > As configuring the bus width is pointless when the interface gets
> > > powered down, skip the operation when power is off.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > First of all, thanks for reporting this issue and coming up with a fix!
> 
> You're welcome. You can expect more of them ;-)
> 
> > Can you please explain a bit more about when this triggers? Is this
> > related to suspend-to-ram perhaps? Which hardware platform? Is
> > CONFIG_PM_RUNTIME=y set?
> 
> I've noticed the problem on the Armadillo board with CONFIG_PM_RUNTIME=y. The 
> driver spits out "timeout waiting for SD bus idle" error messages more or less 
> continuously.
> 
> > I suspect that this may be a side effect of the current PM code used
> > on the A1 SoC (which is hooked up on the armadillo board).
> > 
> > In short, the A1 SoC does not yet make use of PM domains, but AP4 and
> > the mackerel board (sh7372 based) is using PM domains.
> 
> Does this mean that runtime PM is a no-op on A1 ? That would surprise me, as 
> turning CONFIG_PM_RUNTIME off gets rid of the problem, so runtime PM is 
> somehow involved. Even if the power domain does not get turned off, can't the 
> MSTP clock be turned off ?

The problem is, most likely, that with CONFIG_PM_RUNTIME set the code in
drivers/sh/pm_runtime.c triggers and that causes default_pm_domain to be
used.

So, I don't really think we need to "fix" every driver in turn,
default_pm_domain seems to be what needs fixing.  I'm not exactly sure
how to fix it at the moment, though.

You can verify if my suspicion is correct quite easily by replacing the
(&default_pm_domain) in drivers/sh/pm_runtime.c with NULL.

Thanks,
Rafael

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Laurent Pinchart]

Hi Rafael,

On Thursday 14 June 2012 21:37:26 Rafael J. Wysocki wrote:
> On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
> > > On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
> > > > The tmio_mmc_set_ios() function configures the MMC power, clock and
> > > > bus width. When the MMC controller gets powered off, runtime PM
> > > > switches the MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT
> > > > register afterwards fails and prints an error message to the kernel
> > > > log.
> > > > 
> > > > As configuring the bus width is pointless when the interface gets
> > > > powered down, skip the operation when power is off.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > First of all, thanks for reporting this issue and coming up with a fix!
> > 
> > You're welcome. You can expect more of them ;-)
> > 
> > > Can you please explain a bit more about when this triggers? Is this
> > > related to suspend-to-ram perhaps? Which hardware platform? Is
> > > CONFIG_PM_RUNTIME=y set?
> > 
> > I've noticed the problem on the Armadillo board with CONFIG_PM_RUNTIME=y.
> > The driver spits out "timeout waiting for SD bus idle" error messages
> > more or less continuously.
> > 
> > > I suspect that this may be a side effect of the current PM code used
> > > on the A1 SoC (which is hooked up on the armadillo board).
> > > 
> > > In short, the A1 SoC does not yet make use of PM domains, but AP4 and
> > > the mackerel board (sh7372 based) is using PM domains.
> > 
> > Does this mean that runtime PM is a no-op on A1 ? That would surprise me,
> > as turning CONFIG_PM_RUNTIME off gets rid of the problem, so runtime PM
> > is somehow involved. Even if the power domain does not get turned off,
> > can't the MSTP clock be turned off ?
> 
> The problem is, most likely, that with CONFIG_PM_RUNTIME set the code in
> drivers/sh/pm_runtime.c triggers and that causes default_pm_domain to be
> used.
> 
> So, I don't really think we need to "fix" every driver in turn,
> default_pm_domain seems to be what needs fixing.  I'm not exactly sure
> how to fix it at the moment, though.

But isn't there still an issue in the driver itself ? If my understanding is 
correct, calling pm_runtime_put() essentially means "I won't need to touch the 
device from now on, it can be suspended it 
needed/useful/appropriate/whatever". The runtime PM core is then free to turn 
the power domain and/or clock off synchronously or with a delay, or do 
nothing. After signaling that it won't need to touch the device, the driver 
should really avoid touching the device until the next pm_runtime_get_sync() 
call.

> You can verify if my suspicion is correct quite easily by replacing the
> (&default_pm_domain) in drivers/sh/pm_runtime.c with NULL.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Rafael J. Wysocki]

On Thursday, June 14, 2012, Laurent Pinchart wrote:
> Hi Rafael,
> 
> On Thursday 14 June 2012 21:37:26 Rafael J. Wysocki wrote:
> > On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > > On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
> > > > On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
> > > > > The tmio_mmc_set_ios() function configures the MMC power, clock and
> > > > > bus width. When the MMC controller gets powered off, runtime PM
> > > > > switches the MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT
> > > > > register afterwards fails and prints an error message to the kernel
> > > > > log.
> > > > > 
> > > > > As configuring the bus width is pointless when the interface gets
> > > > > powered down, skip the operation when power is off.
> > > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > First of all, thanks for reporting this issue and coming up with a fix!
> > > 
> > > You're welcome. You can expect more of them ;-)
> > > 
> > > > Can you please explain a bit more about when this triggers? Is this
> > > > related to suspend-to-ram perhaps? Which hardware platform? Is
> > > > CONFIG_PM_RUNTIME=y set?
> > > 
> > > I've noticed the problem on the Armadillo board with CONFIG_PM_RUNTIME=y.
> > > The driver spits out "timeout waiting for SD bus idle" error messages
> > > more or less continuously.
> > > 
> > > > I suspect that this may be a side effect of the current PM code used
> > > > on the A1 SoC (which is hooked up on the armadillo board).
> > > > 
> > > > In short, the A1 SoC does not yet make use of PM domains, but AP4 and
> > > > the mackerel board (sh7372 based) is using PM domains.
> > > 
> > > Does this mean that runtime PM is a no-op on A1 ? That would surprise me,
> > > as turning CONFIG_PM_RUNTIME off gets rid of the problem, so runtime PM
> > > is somehow involved. Even if the power domain does not get turned off,
> > > can't the MSTP clock be turned off ?
> > 
> > The problem is, most likely, that with CONFIG_PM_RUNTIME set the code in
> > drivers/sh/pm_runtime.c triggers and that causes default_pm_domain to be
> > used.
> > 
> > So, I don't really think we need to "fix" every driver in turn,
> > default_pm_domain seems to be what needs fixing.  I'm not exactly sure
> > how to fix it at the moment, though.
> 
> But isn't there still an issue in the driver itself ? If my understanding is 
> correct, calling pm_runtime_put() essentially means "I won't need to touch the 
> device from now on, it can be suspended it 
> needed/useful/appropriate/whatever". The runtime PM core is then free to turn 
> the power domain and/or clock off synchronously or with a delay, or do 
> nothing. After signaling that it won't need to touch the device, the driver 
> should really avoid touching the device until the next pm_runtime_get_sync() 
> call.

That's correct, the hardware shouldn't be touched after runtime suspend.

Thanks,
Rafael

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Rafael J. Wysocki]

On Thursday, June 14, 2012, Rafael J. Wysocki wrote:
> On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > Hi Rafael,
> > 
> > On Thursday 14 June 2012 21:37:26 Rafael J. Wysocki wrote:
> > > On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > > > On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
> > > > > On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
> > > > > > The tmio_mmc_set_ios() function configures the MMC power, clock and
> > > > > > bus width. When the MMC controller gets powered off, runtime PM
> > > > > > switches the MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT
> > > > > > register afterwards fails and prints an error message to the kernel
> > > > > > log.
> > > > > > 
> > > > > > As configuring the bus width is pointless when the interface gets
> > > > > > powered down, skip the operation when power is off.
> > > > > > 
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > 
> > > > > First of all, thanks for reporting this issue and coming up with a fix!
> > > > 
> > > > You're welcome. You can expect more of them ;-)
> > > > 
> > > > > Can you please explain a bit more about when this triggers? Is this
> > > > > related to suspend-to-ram perhaps? Which hardware platform? Is
> > > > > CONFIG_PM_RUNTIME=y set?
> > > > 
> > > > I've noticed the problem on the Armadillo board with CONFIG_PM_RUNTIME=y.
> > > > The driver spits out "timeout waiting for SD bus idle" error messages
> > > > more or less continuously.
> > > > 
> > > > > I suspect that this may be a side effect of the current PM code used
> > > > > on the A1 SoC (which is hooked up on the armadillo board).
> > > > > 
> > > > > In short, the A1 SoC does not yet make use of PM domains, but AP4 and
> > > > > the mackerel board (sh7372 based) is using PM domains.
> > > > 
> > > > Does this mean that runtime PM is a no-op on A1 ? That would surprise me,
> > > > as turning CONFIG_PM_RUNTIME off gets rid of the problem, so runtime PM
> > > > is somehow involved. Even if the power domain does not get turned off,
> > > > can't the MSTP clock be turned off ?
> > > 
> > > The problem is, most likely, that with CONFIG_PM_RUNTIME set the code in
> > > drivers/sh/pm_runtime.c triggers and that causes default_pm_domain to be
> > > used.
> > > 
> > > So, I don't really think we need to "fix" every driver in turn,
> > > default_pm_domain seems to be what needs fixing.  I'm not exactly sure
> > > how to fix it at the moment, though.
> > 
> > But isn't there still an issue in the driver itself ? If my understanding is 
> > correct, calling pm_runtime_put() essentially means "I won't need to touch the 
> > device from now on, it can be suspended it 
> > needed/useful/appropriate/whatever". The runtime PM core is then free to turn 
> > the power domain and/or clock off synchronously or with a delay, or do 
> > nothing. After signaling that it won't need to touch the device, the driver 
> > should really avoid touching the device until the next pm_runtime_get_sync() 
> > call.
> 
> That's correct, the hardware shouldn't be touched after runtime suspend.

On a second thought, runtime suspend should only happen when it is _known_
that the hardware won't be accessed.  So, if the hardware _is_ accessed
after runtime suspend, the runtime suspend shouldn't have happened in the
first place.

That's why I don't think it is correct to "harden" code paths that shouldn't
be entered after runtime suspend in the first place.

Thanks,
Rafael

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Laurent Pinchart]

Hi Rafael,

On Friday 15 June 2012 12:03:02 Rafael J. Wysocki wrote:
> On Thursday, June 14, 2012, Rafael J. Wysocki wrote:
> > On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > > On Thursday 14 June 2012 21:37:26 Rafael J. Wysocki wrote:
> > > > On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > > > > On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
> > > > > > On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
> > > > > > > The tmio_mmc_set_ios() function configures the MMC power, clock
> > > > > > > and bus width. When the MMC controller gets powered off, runtime
> > > > > > > PM switches the MSTP clock off. Writing to to
> > > > > > > CTL_SD_MEM_CARD_OPT register afterwards fails and prints an
> > > > > > > error message to the kernel log.
> > > > > > > 
> > > > > > > As configuring the bus width is pointless when the interface
> > > > > > > gets powered down, skip the operation when power is off.
> > > > > > > 
> > > > > > > Signed-off-by: Laurent Pinchart
> > > > > > > <laurent.pinchart@ideasonboard.com>
> > > > > > 
> > > > > > First of all, thanks for reporting this issue and coming up with a
> > > > > > fix!
> > > > > 
> > > > > You're welcome. You can expect more of them ;-)
> > > > > 
> > > > > > Can you please explain a bit more about when this triggers? Is
> > > > > > this related to suspend-to-ram perhaps? Which hardware platform?
> > > > > > Is CONFIG_PM_RUNTIME=y set?
> > > > > 
> > > > > I've noticed the problem on the Armadillo board with
> > > > > CONFIG_PM_RUNTIME=y.
> > > > > The driver spits out "timeout waiting for SD bus idle" error
> > > > > messages more or less continuously.
> > > > > 
> > > > > > I suspect that this may be a side effect of the current PM code
> > > > > > used on the A1 SoC (which is hooked up on the armadillo board).
> > > > > > 
> > > > > > In short, the A1 SoC does not yet make use of PM domains, but AP4
> > > > > > and the mackerel board (sh7372 based) is using PM domains.
> > > > > 
> > > > > Does this mean that runtime PM is a no-op on A1 ? That would
> > > > > surprise me, as turning CONFIG_PM_RUNTIME off gets rid of the
> > > > > problem, so runtime PM is somehow involved. Even if the power domain
> > > > > does not get turned off, can't the MSTP clock be turned off ?
> > > > 
> > > > The problem is, most likely, that with CONFIG_PM_RUNTIME set the code
> > > > in drivers/sh/pm_runtime.c triggers and that causes default_pm_domain
> > > > to be used.
> > > > 
> > > > So, I don't really think we need to "fix" every driver in turn,
> > > > default_pm_domain seems to be what needs fixing.  I'm not exactly sure
> > > > how to fix it at the moment, though.
> > > 
> > > But isn't there still an issue in the driver itself ? If my
> > > understanding is correct, calling pm_runtime_put() essentially means "I
> > > won't need to touch the device from now on, it can be suspended it
> > > needed/useful/appropriate/whatever". The runtime PM core is then free to
> > > turn the power domain and/or clock off synchronously or with a delay,
> > > or do nothing. After signaling that it won't need to touch the device,
> > > the driver should really avoid touching the device until the next
> > > pm_runtime_get_sync() call.
> > 
> > That's correct, the hardware shouldn't be touched after runtime suspend.
> 
> On a second thought, runtime suspend should only happen when it is _known_
> that the hardware won't be accessed.  So, if the hardware _is_ accessed
> after runtime suspend, the runtime suspend shouldn't have happened in the
> first place.
> 
> That's why I don't think it is correct to "harden" code paths that shouldn't
> be entered after runtime suspend in the first place.

I don't think we disagree is. The problem with the TMIO driver is that it 
entered after runtime suspend a code path that should not be entered. When the 
driver is notified by the MMC core that access to the card is not needed 
anymore, it correctly signal to the runtime PM core that the device can be 
suspended, but then tried to setup the device nonetheless. There's no point in 
setting up the device after we get told that we don't need it anymore, it will 
be setup when resumed anyway. That's the code path that was wrong, and is 
fixed by my patch.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Rafael J. Wysocki]

On Friday, June 15, 2012, Laurent Pinchart wrote:
> Hi Rafael,
> 
> On Friday 15 June 2012 12:03:02 Rafael J. Wysocki wrote:
> > On Thursday, June 14, 2012, Rafael J. Wysocki wrote:
> > > On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > > > On Thursday 14 June 2012 21:37:26 Rafael J. Wysocki wrote:
> > > > > On Thursday, June 14, 2012, Laurent Pinchart wrote:
> > > > > > On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
> > > > > > > On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
> > > > > > > > The tmio_mmc_set_ios() function configures the MMC power, clock
> > > > > > > > and bus width. When the MMC controller gets powered off, runtime
> > > > > > > > PM switches the MSTP clock off. Writing to to
> > > > > > > > CTL_SD_MEM_CARD_OPT register afterwards fails and prints an
> > > > > > > > error message to the kernel log.
> > > > > > > > 
> > > > > > > > As configuring the bus width is pointless when the interface
> > > > > > > > gets powered down, skip the operation when power is off.
> > > > > > > > 
> > > > > > > > Signed-off-by: Laurent Pinchart
> > > > > > > > <laurent.pinchart@ideasonboard.com>
> > > > > > > 
> > > > > > > First of all, thanks for reporting this issue and coming up with a
> > > > > > > fix!
> > > > > > 
> > > > > > You're welcome. You can expect more of them ;-)
> > > > > > 
> > > > > > > Can you please explain a bit more about when this triggers? Is
> > > > > > > this related to suspend-to-ram perhaps? Which hardware platform?
> > > > > > > Is CONFIG_PM_RUNTIME=y set?
> > > > > > 
> > > > > > I've noticed the problem on the Armadillo board with
> > > > > > CONFIG_PM_RUNTIME=y.
> > > > > > The driver spits out "timeout waiting for SD bus idle" error
> > > > > > messages more or less continuously.
> > > > > > 
> > > > > > > I suspect that this may be a side effect of the current PM code
> > > > > > > used on the A1 SoC (which is hooked up on the armadillo board).
> > > > > > > 
> > > > > > > In short, the A1 SoC does not yet make use of PM domains, but AP4
> > > > > > > and the mackerel board (sh7372 based) is using PM domains.
> > > > > > 
> > > > > > Does this mean that runtime PM is a no-op on A1 ? That would
> > > > > > surprise me, as turning CONFIG_PM_RUNTIME off gets rid of the
> > > > > > problem, so runtime PM is somehow involved. Even if the power domain
> > > > > > does not get turned off, can't the MSTP clock be turned off ?
> > > > > 
> > > > > The problem is, most likely, that with CONFIG_PM_RUNTIME set the code
> > > > > in drivers/sh/pm_runtime.c triggers and that causes default_pm_domain
> > > > > to be used.
> > > > > 
> > > > > So, I don't really think we need to "fix" every driver in turn,
> > > > > default_pm_domain seems to be what needs fixing.  I'm not exactly sure
> > > > > how to fix it at the moment, though.
> > > > 
> > > > But isn't there still an issue in the driver itself ? If my
> > > > understanding is correct, calling pm_runtime_put() essentially means "I
> > > > won't need to touch the device from now on, it can be suspended it
> > > > needed/useful/appropriate/whatever". The runtime PM core is then free to
> > > > turn the power domain and/or clock off synchronously or with a delay,
> > > > or do nothing. After signaling that it won't need to touch the device,
> > > > the driver should really avoid touching the device until the next
> > > > pm_runtime_get_sync() call.
> > > 
> > > That's correct, the hardware shouldn't be touched after runtime suspend.
> > 
> > On a second thought, runtime suspend should only happen when it is _known_
> > that the hardware won't be accessed.  So, if the hardware _is_ accessed
> > after runtime suspend, the runtime suspend shouldn't have happened in the
> > first place.
> > 
> > That's why I don't think it is correct to "harden" code paths that shouldn't
> > be entered after runtime suspend in the first place.
> 
> I don't think we disagree is. The problem with the TMIO driver is that it 
> entered after runtime suspend a code path that should not be entered. When the 
> driver is notified by the MMC core that access to the card is not needed 
> anymore, it correctly signal to the runtime PM core that the device can be 
> suspended, but then tried to setup the device nonetheless.

I see.

> There's no point in 
> setting up the device after we get told that we don't need it anymore, it will 
> be setup when resumed anyway. That's the code path that was wrong, and is 
> fixed by my patch.

OK, then.

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Magnus Damm]

Hi Laurent,

On Fri, Jun 15, 2012 at 4:12 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Thursday 14 June 2012 20:12:33 Magnus Damm wrote:
>> On Tue, Jun 12, 2012 at 9:57 PM, Laurent Pinchart wrote:
>> > The tmio_mmc_set_ios() function configures the MMC power, clock and bus
>> > width. When the MMC controller gets powered off, runtime PM switches the
>> > MSTP clock off. Writing to to CTL_SD_MEM_CARD_OPT register afterwards
>> > fails and prints an error message to the kernel log.
>> >
>> > As configuring the bus width is pointless when the interface gets
>> > powered down, skip the operation when power is off.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> First of all, thanks for reporting this issue and coming up with a fix!
>
> You're welcome. You can expect more of them ;-)
>
>> Can you please explain a bit more about when this triggers? Is this
>> related to suspend-to-ram perhaps? Which hardware platform? Is
>> CONFIG_PM_RUNTIME=y set?
>
> I've noticed the problem on the Armadillo board with CONFIG_PM_RUNTIME=y. The
> driver spits out "timeout waiting for SD bus idle" error messages more or less
> continuously.

I see, thanks for explaining.

>> I suspect that this may be a side effect of the current PM code used
>> on the A1 SoC (which is hooked up on the armadillo board).
>>
>> In short, the A1 SoC does not yet make use of PM domains, but AP4 and
>> the mackerel board (sh7372 based) is using PM domains.
>
> Does this mean that runtime PM is a no-op on A1 ? That would surprise me, as
> turning CONFIG_PM_RUNTIME off gets rid of the problem, so runtime PM is
> somehow involved. Even if the power domain does not get turned off, can't the
> MSTP clock be turned off ?

The behavior may be different depending on how the pm domains are tied in.

>> This difference may result in different state of clocks at suspend-to-ram
>> timing. So the SDHI driver may work just fine on the mackerel board, but not
>> on the armadillo board. If that's the case then perhaps we shouldn't fix the
>> driver, but instead look at the platform level.
>
> I still believe there's a bug in the driver, please see below.
>
>> > diff --git a/drivers/mmc/host/tmio_mmc_pio.c
>> > b/drivers/mmc/host/tmio_mmc_pio.c index 9a7996a..de1226d 100644
>> > --- a/drivers/mmc/host/tmio_mmc_pio.c
>> > +++ b/drivers/mmc/host/tmio_mmc_pio.c
>> > @@ -816,13 +816,15 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc,
>> > struct mmc_ios *ios) tmio_mmc_clk_stop(host);
>> >        }
>> >
>> > -       switch (ios->bus_width) {
>> > -       case MMC_BUS_WIDTH_1:
>> > -               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
>> > -       break;
>> > -       case MMC_BUS_WIDTH_4:
>> > -               sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
>> > -       break;
>> > +       if (host->power) {
>> > +               switch (ios->bus_width) {
>> > +               case MMC_BUS_WIDTH_1:
>> > +                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT,
>> > 0x80e0);
>> > +               break;
>> > +               case MMC_BUS_WIDTH_4:
>> > +                       sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT,
>> > 0x00e0);
>> > +               break;
>> > +               }
>> >        }
>> >
>> >        /* Let things settle. delay taken from winCE driver */
>> > --
>> > 1.7.3.4
>>
>> Is it possible that you get here through tmio_mmc_host_suspend() and
>> mmc_suspend_host()?
>>
>> Just guessing. =)
>
> I haven't checked the complete call stack, but I don't think it matters that
> much.

I suppose that depends on what you are trying to fix. =)

> What happens here is that the driver tried to write to a 16-bit hardware
> register after calling pm_runtime_put(). At that point runtime PM may have
> turned to power domain and/or the MSTP clock off for the device. Writing to a
> 16-bit register first polls for the bus idle bit in the CTL_STATUS2 to be set.
> This never happens in this case (probably because the clock has been turned
> off), so the write operation fails.
>
> Speaking generally, I think we should avoid writing to the device after
> calling pm_runtime_put(), it just doesn't make much sense. If we release the
> device from a PM point of view, it means we don't need to touch it anymore, so
> we should prevent writes until the next pm_runtime_get_sync() call.

I totally agree with you on that. I'm just surprised that it triggers
at this point.

Guennadi, does this trigger on sh7372 as well? If not, why is that?

Thanks for your help!

/ magnus

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Guennadi Liakhovetski]

On Fri, 15 Jun 2012, Magnus Damm wrote:

[snip]

> Guennadi, does this trigger on sh7372 as well? If not, why is that?

No. That message is produced by the sh_mobile_sdhi_wait_idle() function, 
which is only used, if the TMIO_MMC_HAS_IDLE_WAIT flag is set, which is 
not set on mackerel (or ap4evb).

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Magnus Damm]

On Fri, Jun 15, 2012 at 4:09 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Fri, 15 Jun 2012, Magnus Damm wrote:
>
> [snip]
>
>> Guennadi, does this trigger on sh7372 as well? If not, why is that?
>
> No. That message is produced by the sh_mobile_sdhi_wait_idle() function,
> which is only used, if the TMIO_MMC_HAS_IDLE_WAIT flag is set, which is
> not set on mackerel (or ap4evb).

Ok, thanks for checking this. It seems that we are "lucky" to find
this breakage...

In the future, please take care to make sure this does not happen
again. This goes without saying, but the driver should not access the
hardware when it is Runtime PM suspended. I believe it should be
possible to verify the code paths by manual code inspection. Also, to
make the behavior more consistent you may want to make use of
"pm_runtime_put_sync()" instead of "pm_runtime_put()".

Thanks,

/ magnus

Re: [PATCH] mmc: tmio: Don't access hardware registers after stopping clocks

[Laurent Pinchart]

Hi Magnus,

On Tuesday 19 June 2012 15:53:30 Magnus Damm wrote:
> On Fri, Jun 15, 2012 at 4:09 PM, Guennadi Liakhovetski wrote:
> > On Fri, 15 Jun 2012, Magnus Damm wrote:
> > 
> > [snip]
> > 
> >> Guennadi, does this trigger on sh7372 as well? If not, why is that?
> > 
> > No. That message is produced by the sh_mobile_sdhi_wait_idle() function,
> > which is only used, if the TMIO_MMC_HAS_IDLE_WAIT flag is set, which is
> > not set on mackerel (or ap4evb).
> 
> Ok, thanks for checking this. It seems that we are "lucky" to find
> this breakage...
> 
> In the future, please take care to make sure this does not happen
> again. This goes without saying, but the driver should not access the
> hardware when it is Runtime PM suspended. I believe it should be
> possible to verify the code paths by manual code inspection. Also, to
> make the behavior more consistent you may want to make use of
> "pm_runtime_put_sync()" instead of "pm_runtime_put()".

For testing that's a good idea. That makes me wonder whether we should have a 
Kconfig option to turn all pm_runtime_put() into pm_runtime_put_sync() for 
stress-testing.

-- 
Regards,

Laurent Pinchart