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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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