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@xxxxxxxxxxxxxxxx> > > > > > > > > > > > > > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
