Re: [PATCH 2/5 V2] mmc: core: call pm_runtime_put_noidle in pm_runtime_get_sync failed case
On Mon, Apr 8, 2013 at 4:36 AM, Li Fei wrote: > Even in failed case of pm_runtime_get_sync, the usage_count > is incremented. In order to keep the usage_count with correct > value and runtime power management to behave correctly, call > pm_runtime_put_noidle in such case. > > Signed-off-by Liu Chuansheng > Signed-off-by: Li Fei Acked-by: Ohad Ben-Cohen BTW, Li, could you please move to _noidle in those other places where your previous patch was already applied? I think we have at least the 12xx driver (cc'ing Luca). Thanks! Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] mmc: core: call pm_runtime_put_sync in pm_runtime_get_sync failed case
Hi Li, On Thu, Feb 28, 2013 at 9:44 AM, Li Fei wrote: > Even in failed case of pm_runtime_get_sync, the usage_count > is incremented. In order to keep the usage_count with correct > value and runtime power management to behave correctly, call > pm_runtime_put(_sync) in such case. As with the remoteproc case, it is probably better to call the put_noidle variant here. This way you are sure not to erroneously invoke any underlying pm handler where your only intention is to fix usage_count. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: Removing the unnecessary runtime_get/put in sdio_bus_remove()
On Wed, Dec 19, 2012 at 4:51 PM, Chuansheng Liu wrote: > > The runtime_get_sync() is called during sdio_bus_probe(), then the device > will be kept in active runtime state Unless, of course, the driver powered it down. >, so not neccessary to call > runtime_get_sync/put_noidle() again in sdio_bus_remove(). See comment above... -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()
On Mon, Nov 19, 2012 at 10:01 AM, Liu, Chuansheng wrote: >> > Rechecked these codes, the trace event runtime_pm_status is added newly, >> this is different with vanilla >> > Linux. >> >> Not sure I'm following - can you point out which tree are you working with ? > Sorry, it is added internally for debugging purpose. Maybe keep this patch internally too then ? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()
On Mon, Nov 19, 2012 at 7:57 AM, Liu, Chuansheng wrote: > Rechecked these codes, the trace event runtime_pm_status is added newly, this > is different with vanilla > Linux. Not sure I'm following - can you point out which tree are you working with ? > So I still think that calling pm_runtime_set_active is not safe when dev_name > is NULL. > If you agree this point, I can refine the code that moving "init the dev_name > " from mmc_add_card > to mmc_sdio_init_card. This sounds more reasonable. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()
Hi Liu, On Fri, Nov 16, 2012 at 2:54 PM, Chuansheng Liu wrote: > So when calling pm_runtime_set_active(), it will hit the strlen(devname==0) > which trigger the panic. Can you please point to the exact line of code that triggers this panic ? > Here before calling pm_runtime_set_active(), set the dev name, although > it is duplicated with mmc_add_card(), but it do not break the original > design(commit 81968561b). Normally we'd like to avoid such a solution, so let's first make sure we understand the problem. Have you tried thinking how come this shows up only now - has any of the relevant code been changed lately ? Are you using a vanilla Linux tree ? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC_CAP_POWER_OFF_CARD, MMC_PM_KEEP_POWER - when to set
On Fri, May 25, 2012 at 12:15 AM, Guennadi Liakhovetski wrote: > I think it'd be better to go with driver-assisted PM straight away instead > of implementing black-/white-lists to later find out about cards with the > same IDs and different behaviour. I'm not sure - we usually prefer to evolve as real use cases show up instead of aiming for the final design in the first place (which, as practice shows, most likely anyway have to change when the real use case show up). This usually proves itself as the minimum effort for nailing the existing requirements (less code to develop, debug and maintain). We also want to support powering off the cards even if their driver isn't at all loaded (as I wrote previously). > Thanks:-) It was important for me in the first place to understand the > problem. Now that the problem has become clearer we can think about > whether, when and how to solve it and who shall do it:-) Oh I thought you volunteered ! ;) Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC_CAP_POWER_OFF_CARD, MMC_PM_KEEP_POWER - when to set
On Thu, May 24, 2012 at 10:08 PM, Ohad Ben-Cohen wrote: > Feel free to propose a solution, but please back it up with real > hardware and show that it satisfies the MMC_CAP_POWER_OFF_CARD issues > we had in the past. The solution should also still support powering off the card even if a driver for it isn't loaded (at least for chips that support it). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC_CAP_POWER_OFF_CARD, MMC_PM_KEEP_POWER - when to set
Hi Guennadi, On Thu, May 24, 2012 at 6:42 PM, Guennadi Liakhovetski wrote: > Yes, exactly, in certain cases. And when it's not hard-wired? We don't support this yet. Daniel suggested using either black or white listing (based on vendor/device id presumably) so we can support regular cards without breaking others, but I'm not sure if Daniel had any free cycles to look into it. > Because with pluggable cards one flag is not enough. I think, you need (1) > board capability to switch power, and (2) driver's allowance to power the > card down. ... > The problem of hot-pluggable cards. One card / driver combination will > support powering down at runtime, another one will not. Ah ok now I understand you, thanks for the explanation. Whether or not to involve the driver here probably depends on the actual hardware requirements. E.g., if the vendor/device id is enough information, then we might be able to implement this below the drivers without involving them. But in case only the driver could tell between variants of specific chips, then a generic white/black listing might not be enough. Feel free to propose a solution, but please back it up with real hardware and show that it satisfies the MMC_CAP_POWER_OFF_CARD issues we had in the past. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC_CAP_POWER_OFF_CARD, MMC_PM_KEEP_POWER - when to set
Hi Guennadi, On Thu, May 24, 2012 at 5:06 PM, Guennadi Liakhovetski wrote: > Sorry, I mean - you don't know what card will be plugged in, so, you > cannot know, whether that specific card and its driver can be safely > powered off. We do know in certain cases, most commonly with wifi SDIO cards which are hardwired to the board. > Ok, I think, it's this: MMC_PM_KEEP_POWER is for system-wide suspend / > resume, whereas MMC_CAP_POWER_OFF_CARD is for runtime PM. Am I right? MMC_CAP_POWER_OFF_CARD is used by SDIO runtime PM, but I won't classify it as "runtime PM" specific. It describes the hardware, and not inherently limited to a specific use case. > that case, I think, they also should function similar Can you explain why ? > - wouldn't it make > sense to use MMC_CAP_POWER_OFF_CARD similar to MMC_PM_KEEP_POWER - as a > static capability flag (meaning, whether the board is physically able to > switch power) and a dynamic flag, that, say, an SDIO function driver could > set if it knows, that it cannot handle runtime power switching properly? I'm not sure I'm following the reasoning. Can you explain what are you trying to solve ? Is there a specific problem you have in mind ? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC_CAP_POWER_OFF_CARD, MMC_PM_KEEP_POWER - when to set
Hi Guennadi, On Thu, May 24, 2012 at 4:07 PM, Guennadi Liakhovetski wrote: > How do you know, if this is a normal sd-card slot? Not sure I follow the question. But if you ask whether we can dynamically set MMC_CAP_POWER_OFF_CARD, then we can't (with what we know today). This CAP is dealing with issues of specific boards which, unfortunately, we have no way (today) to dynamically probe. >> No, MMC_PM_KEEP_POWER is different: it is a dynamic user decision >> controlling whether the card should stay on while the system is >> suspended, > > Isn't it both? Yes, you're right - we need host support to keep the card powered while the system is suspended. > A complete board reset in this case will differ from the card power-off > only in the state of the rest of the SD-card ping, e.g., the clock. So, > additionally switching off the clock in this case should be equivalent to > the board reset? Not really - consider the example I gave you, where you are required to toggle a card reset pin after you power cycle it, but there's no way to control that pin. In that case, only a complete board reset can help you. > Given the above, wouldn't it be sufficient to test (mmc->pm_flags & > MMC_PM_KEEP_POWER) to decide, whether the card can be suspended? No, MMC_PM_KEEP_POWER is about the ability (and desire) to keep the card powered while the system is suspended. MMC_CAP_POWER_OFF_CARD is about the ability to power off the card without killing it completely until the next board reset. These two flags really address completely different problems, despite the similar nomenclature. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC_CAP_POWER_OFF_CARD, MMC_PM_KEEP_POWER - when to set
Hi Guennadi,
On Thu, May 24, 2012 at 2:18 PM, Guennadi Liakhovetski
wrote:
> Some sources
> indicate, that this flag means, that the host / board are _capable_ of
> powering down the slot. Others seem to suggest, that it also means, that
> the card can handle being powered off.
Take a look how MMC_CAP_POWER_OFF_CARD is described in the commit log
which added it:
commit ed919b0125b26dcc052e44836f66e7e1f5c49c7e
Author: Ohad Ben-Cohen
Date: Fri Nov 19 09:29:09 2010 +0200
mmc: sdio: fix runtime PM anomalies by introducing MMC_CAP_POWER_OFF_CARD
Some board/card/host configurations are not capable of powering off the
card after boot.
To support such configurations, and to allow smoother transition to
runtime PM behavior, MMC_CAP_POWER_OFF_CARD is added, so hosts need to
explicitly indicate whether it's OK to power off their cards after boot.
SDIO core will enable runtime PM for a card only if that cap is set.
As a result, the card will be powered down after boot, and will only
be powered up again when a driver is loaded (and then it's up to the
driver to decide whether power will be kept or not).
This will prevent sdio_bus_probe() failures with setups that do not
support powering off the card.
Reported-and-tested-by: Daniel Drake
Reported-and-tested-by: Arnd Hannemann
Signed-off-by: Ohad Ben-Cohen
Signed-off-by: Chris Ball
> The former meaning would make sense
> to me, but then - why would one want to check it when deciding whether or
> not to use runtime PM with the host?
Because if a certain board/host/card setup doesn't support it, then we
should not power off the card, because doing so means we'll have to
reset the entire board if we want the card to work again.
> Isn't it exactly what the MMC_PM_KEEP_POWER flag is for?
No, MMC_PM_KEEP_POWER is different: it is a dynamic user decision
controlling whether the card should stay on while the system is
suspended, while MMC_CAP_POWER_OFF_CARD is a static capability of the
hardware.
> If MMC_CAP_POWER_OFF_CARD does indeed mean,
> that the host is able to switch the power off and on, then it can be
> safely set almost always. E.g., if the slot is supplied by a regulator,
> this flag can be enabled automatically?
Not always.
For example, consider a (non-fictional) configuration where you can
power off the card, but after you power it on again, you must toggle
some card reset pin for it to work, but your board doesn't have that
reset pin hardwired to any gpio.
In this case, despite the existing regulator, you must not power off
the card, because doing so requires you to reset your entire board if
you want the card to work again.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: add runtime PM handlers
Hi Tony, On Tue, Jan 10, 2012 at 7:44 AM, Lin Tony-B19295 wrote: > [Tony Lin]does it mean Runtime PM is not fully supported by SD/MMC stack? SDIO runtime PM is fully supported (there were also some initial MMC runtime PM patches circulating at some point, but they were not merged afaik). It isn't enabled by default though: you'd have to flip a cap flag in order to enable it for your chip. > I really saw some problems using Runtime PM, so I just want a confirmation. The problems you saw are all related to the sd8686, with which it is required to toggle an external gpio on some setups whenever power is switched off and brought back again. This isn't implemented yet, so we're not enabling SDIO runtime PM by default at this point, because that would break those needy sd8686 setups. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
On Mon, Dec 19, 2011 at 11:10 AM, Ohad Ben-Cohen wrote: > (it seems there are a few different hw revisions of it, some > of which do require toggling the reset pin before sending another init > sequence) (it might just be different setups and not really different hw revisions of the 8686) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.
Hello Bing, On Thu, Dec 15, 2011 at 9:35 AM, Bing Zhao wrote: >> I wonder why the sdio reset command isn't helpful for you - it did >> seem to resolve some issues for Daniel. Maybe you have two different >> hardware revisions of the 8686 which behave differently in this >> respect ? > > CMD52 Card Reset (writing bit3=1 to CCCR 0x06) may not reset the SD8686 > properly for some systems. The reliable way to reset SD8686 is via RESETn > toggling. Thanks a lot for confirming this. Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
Hi Chris, On Wed, Nov 30, 2011 at 4:29 PM, Ohad Ben-Cohen wrote: > On Wed, Nov 30, 2011 at 4:12 PM, Daniel Drake wrote: >> My patch simply sets a flag to enable a whole load of code written by >> you - code which I understand little about. I am now available to >> help, but given that my understanding of mmc is limited, I didn't >> write the code in question, and I also lack expert insight into 8686 >> powerup, I'm not sure how useful I can be. > > Let's revert 08da834 then. We're slowly progressing towards understanding the issues we have with the 8686 (it seems there are a few different hw revisions of it, some of which do require toggling the reset pin before sending another init sequence), but it might take some more time until a solution fully materializes. Since 3.2 is just around the corner, I suggest we should now proceed with the revert (see patch below), and later revisit this once we have a complete solution in hand. Thanks, Ohad. > > When SDIO runtime PM was originally introduced, we immediately faced > two regressions with two different chipsets, and in response decided > not to enable it by default. > > With your work on the 8686 we hoped we found all the gotchas, so > 08da834 did make sense (at least experimentally). > > Unfortunately we now see that some setups out there still refuse to > work when SDIO runtime PM is enabled by default, and obviously we > can't live with these kind of regressions. > > commit fd9fec7a00f58a16803e37a99a014ef82543ef6f > Author: Ohad Ben-Cohen > Date: Wed Nov 30 16:22:13 2011 +0200 > > Revert "mmc: enable runtime PM by default" > > When SDIO runtime PM was originally introduced, we immediately faced > two regressions with two different chipsets, and in response decided > not to enable it by default. > > With the recent work on the 8686 we hoped we found all the gotchas, > so 08da834 did make sense (at least experimentally). > > Unfortunately we now see that some setups out there still refuse to work > when SDIO runtime PM is enabled by default (see > http://www.spinics.net/lists/linux-mmc/msg11161.html), and obviously we > can't live with these kind of regressions. > > This reverts commit 08da834a24312157f512224691ad1fddd11c1073. > > Signed-off-by: Ohad Ben-Cohen > Cc: Daniel Drake > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index e8a5eb3..d31c78b 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -302,17 +302,6 @@ struct mmc_host *mmc_alloc_host(int extra, struct > device *dev) > host->max_blk_size = 512; > host->max_blk_count = PAGE_CACHE_SIZE / 512; > > - /* > - * Enable runtime power management by default. This flag was added due > - * to runtime power management causing disruption for some users, but > - * the power on/off code has been improved since then. > - * > - * We'll enable this flag by default as an experiment, and if no > - * problems are reported, we will follow up later and remove the flag > - * altogether. > - */ > - host->caps = MMC_CAP_POWER_OFF_CARD; > - > return host; > > free: -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.
On Sat, Dec 10, 2011 at 9:15 AM, NeilBrown wrote: > When I force it to hold off suspend for a little while I see (starting at the > same place): > > [ 656.189697] bus: 'sdio': driver_probe_device: matched device mmc1:0001:1 > with driver libertas_sdio > [ 656.212768] bus: 'sdio': really_probe: probing driver libertas_sdio with > device mmc1:0001:1 > [ 656.247741] libertas_sdio mmc1:0001:1: firmware: requesting > libertas/sd8686_v9_helper.bin > [ 656.257537] device: 'mmc1:0001:1': device_add > [ 656.263580] PM: Adding info for No Bus:mmc1:0001:1 > [ 656.298187] device: 'mmc1:0001:1': device_unregister > [ 656.303375] PM: Removing info for No Bus:mmc1:0001:1 > [ 656.322967] libertas_sdio mmc1:0001:1: firmware: requesting > libertas/sd8686_v8_helper.bin > [ 656.344207] device: 'mmc1:0001:1': device_add > [ 656.357147] PM: Adding info for No Bus:mmc1:0001:1 > [ 656.395782] device: 'mmc1:0001:1': device_unregister > [ 656.407409] PM: Removing info for No Bus:mmc1:0001:1 > [ 656.418579] libertas_sdio mmc1:0001:1: firmware: requesting > sd8686_helper.bin > [ 656.435699] device: 'mmc1:0001:1': device_add > [ 656.446655] PM: Adding info for No Bus:mmc1:0001:1 > [ 656.504974] device: 'mmc1:0001:1': device_unregister > [ 656.511749] PM: Removing info for No Bus:mmc1:0001:1 > [ 656.521148] libertas_sdio mmc1:0001:1: firmware: requesting sd8686.bin > [ 656.529205] device: 'mmc1:0001:1': device_add > [ 656.535095] PM: Adding info for No Bus:mmc1:0001:1 > [ 656.584625] device: 'mmc1:0001:1': device_unregister > [ 656.591369] PM: Removing info for No Bus:mmc1:0001:1 > [ 657.384063] libertas_sdio mmc1:0001:1: (unregistered net_device): > 00:19:88:3d:ff:f0, fw 9.70.3p24, cap 0x0303 > [ 657.454467] libertas_sdio mmc1:0001:1: (unregistered net_device): > PREP_CMD: command 0x00a3 failed: 2 > [ 657.464080] device: 'phy0': device_add > > > But shortly there after the extra tracing I put in shows that mmc_power_off > is called, Probably right after libertas' if_sdio_probe() returns ? > then mmc_sdio_power_restore calls mmc_send_io_op_cond Is that as a result of libertas' if_sdio_power_restore() being called (i.e. someone/something called 'ifconfig up') ? > which again > returns -110, but now it isn't a problem and the wifi chip keeps working. if_sdio_power_restore doesn't check the return value of pm_runtime_get_sync, so it won't error out, but I wonder how come the chip still works. > So maybe the fact that we error-out in the first case is a problem?? It might be nice if Marvell could comment on this, though we can probably empirically deduce this too. > I found that if I pull the reset line down and then let it back up then it > all works. Nice. Joe, did this work out for you too ? > So I have run out of ideas. I can make it work by reseting the chip during > mmc_power_up but I have no idea what is causing the chip to need a reset. I wonder why the sdio reset command isn't helpful for you - it did seem to resolve some issues for Daniel. Maybe you have two different hardware revisions of the 8686 which behave differently in this respect ? > However for now I think I'll go with my 'remux' hack. Feel free to post it so we can take a look. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.
On Mon, Dec 5, 2011 at 9:06 PM, NeilBrown wrote: > Challenge accepted. cool :) > Even if I don't find a better solution, this seems backwards. Sure the > default should be that PM is enabled, but individual board can request no > PM on MMC interfaces where it is a problem. I somewhat tend to pick a white listing approach, so we don't keep on inflicting further pain on random sdio users (at least until we learn more about the issues at hand), but I wouldn't object black listing. > The chip has a requirement that the reset line is held down during power-on, > and raised shortly after (I don't know exactly how short). So if you just > remove power and give it back, the chip doesn't come up properly. Well, not according to Bing Zhao from Marvell: http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08257.html Though I agree that if we do find this reset line helping (and get it to work cleanly with the help of some regulator code), the solution might be much more robust. > "It *should be* perfectly fine ..." :-) heh, yeah :) > We just have to make sure the bug powers up the card properly. > Maybe I need to create a virtual regulator that powers on the real regulator, > then raises the reset line. I wonder how hard that is. Btw did you try toggling the reset line manually and see if it really helps ? > I'll spend some more time on this and get back to you - probably next week. Very cool, thanks a lot. Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.
Hi Neil, On Mon, Dec 5, 2011 at 3:54 AM, NeilBrown wrote: > I've been chasing down a problem with the SDIO-attached wifi card in the > GTA04 (www.gta04.org). 8686, right ? > The problem seems very similar to that described in > > http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg04289.html Don't go that far back, Joe just reported this exact problem in : http://comments.gmane.org/gmane.linux.kernel.mmc/11231 > commit 08da834a24312157f512224691ad1fddd11c1073 > Author: Daniel Drake > Date: Wed Jul 20 17:39:22 2011 +0100 > > mmc: enable runtime PM by default > > > and if I revert that commit so that runtime PM is not enabled the problem > goes away. And this is most likely what we're going to do, unless someone with the 8686 can further look into the problem. > (The wifi chip is a libertas_sdio supported chip connected to an mmc > interface on an omap3, and has a separate regulator for power supply, plus a > GPIO pin for reset, just like in the email thread. The problem is > exemplified by: > [ 64.643981] libertas_sdio: probe of mmc1:0001:1 failed with error -16 > ). Yes, the same issue that Joe is seeing. > I finally managed to track down exactly what was happening - runtime PM was > putting the interface to sleep before the SDIO functions were probed so > initialising them failed. Yeah, but the question is why it fails. It's perfectly fine to power the card down before the functions are probed, because just before they are probed the bus is responsible to power the card up again. > From: NeilBrown > Date: Mon, 5 Dec 2011 11:34:47 +1100 > Subject: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until > probe is finished. You can't tell when probe is finished. In fact, in can happen very far in the future or even never at all (e.g. when the function driver isn't loaded). > mmc_attach_sdio currently enables runtime power management on > the mmc interface before it completes the probe of functions. > This means that it might be asleep during the probe and the probe > will fail. No - the sdio bus powers the card up before probing the drivers. See sdio_bus_probe. > In particular, if 'drivers_autoprobe' is enabled on the mmc bus, then > device_attach() will be called by bus_probe_device() from device_add() > and it will pm_runtime_get_noresume()/pm_runtime_put_sync(). > > As runtime power management this the device is running > (pm_runtime_set_active() in mmc_attach_sdio()) and rtpm is enabled > (pm_runtime_enable()), the pm_runtime_put_sync() call puts the mmc > interface to sleep, This is fine > so function probing fails. The question is why. Again - sdio_bus_probe should power up the card. For some reason, it (or something else) fails to do so with the 8686 on some setups. Other chips might have similar issues, too - we just don't know (all we know is that it works great with the wl12xx, and with the Daniel's 8686 setup). > So this patch moves the call to pm_runtime_enable to after all the > calls to sdio_add_func. Looks like this will have the undesired side-effect of keeping the chip powered up, even if it doesn't have any driver loaded for it. And this doesn't really address the problem: we still don't know why the 8686 fails to power up at this point. Can you please tell us where exactly is the first failure coming from ? is this from libertas own probe function ? is this sdio_bus_probe getting an error from calling pm_runtime_get_sync ? Thanks, Ohad -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
On Wed, Nov 30, 2011 at 5:00 PM, Joe Woodward wrote: > It seems CMD5 fails (which seems to be what the workarounds were aiming to > fix). Right; you get a timeout (-110), which means the 8686 didn't send any response. This is unexpected because the sdio reset (the CMD52 you see before the init sequence) should be enough to reset the device (we confirmed this with the Marvell folks on this list awhile ago). > I'm afraid I may not be of much more use as Gumstix (annoyingly) don't > release schematics of their COMs, so actually probing any signals isn't > really possible. Yes, it makes it difficult to debug. You can try to verify (in the code) that all the text book prerequisites are there (e.g. all SDIO lines are pulled up and muxed correctly. though since stuff work for you without SDIO runtime PM, I assume these are ok), and maybe even play a bit with stuff like delays, sdio resets and power cycles and see if things change. If you have access to the other inputs of the 8686 (it has several of them which are related to power and resets. Neither me nor Daniel have intimate knowledge about what any of those really do, but you could dig some info from emails send by Marvell folks on this list), then you can also try to play with them and see if things change. With luck, you could learn more about the problem and can hopefully get closer to finding a solution. It could be great if a solution will be found, but if not, we'll probably have to proceed with the revert... Good luck! Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
On Wed, Nov 30, 2011 at 4:57 PM, Daniel Drake wrote: > If we go ahead with the revert, can you help me come up with a way to > whitelist it for OLPC? Sure thing. > Would a DMI-based quirk in sdhci be acceptable for you? I wasn't following sdhci development so much as I'm not developing on such hardware, so I'll have to take a look. AFAIK though sdhci is maintained by Wolfram, so you need his consent. Whatever approach Wolfram and you will agree on is of course fine with me. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
On Wed, Nov 30, 2011 at 4:12 PM, Daniel Drake wrote: > My patch simply sets a flag to enable a whole load of code written by > you - code which I understand little about. I am now available to > help, but given that my understanding of mmc is limited, I didn't > write the code in question, and I also lack expert insight into 8686 > powerup, I'm not sure how useful I can be. Let's revert 08da834 then. When SDIO runtime PM was originally introduced, we immediately faced two regressions with two different chipsets, and in response decided not to enable it by default. With your work on the 8686 we hoped we found all the gotchas, so 08da834 did make sense (at least experimentally). Unfortunately we now see that some setups out there still refuse to work when SDIO runtime PM is enabled by default, and obviously we can't live with these kind of regressions. commit fd9fec7a00f58a16803e37a99a014ef82543ef6f Author: Ohad Ben-Cohen Date: Wed Nov 30 16:22:13 2011 +0200 Revert "mmc: enable runtime PM by default" When SDIO runtime PM was originally introduced, we immediately faced two regressions with two different chipsets, and in response decided not to enable it by default. With the recent work on the 8686 we hoped we found all the gotchas, so 08da834 did make sense (at least experimentally). Unfortunately we now see that some setups out there still refuse to work when SDIO runtime PM is enabled by default (see http://www.spinics.net/lists/linux-mmc/msg11161.html), and obviously we can't live with these kind of regressions. This reverts commit 08da834a24312157f512224691ad1fddd11c1073. Signed-off-by: Ohad Ben-Cohen Cc: Daniel Drake diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index e8a5eb3..d31c78b 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -302,17 +302,6 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev) host->max_blk_size = 512; host->max_blk_count = PAGE_CACHE_SIZE / 512; - /* -* Enable runtime power management by default. This flag was added due -* to runtime power management causing disruption for some users, but -* the power on/off code has been improved since then. -* -* We'll enable this flag by default as an experiment, and if no -* problems are reported, we will follow up later and remove the flag -* altogether. -*/ - host->caps = MMC_CAP_POWER_OFF_CARD; - return host; free: -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
Hi Daniel, On Tue, Nov 29, 2011 at 11:42 PM, Daniel Drake wrote: > Even though I have the pleasure of working with 8686 quite a bit (Is there a subtle cynicism here ? :) > So first thing to do is to investigate the source of this error, as before: > [ 34.637481] libertas_sdio: probe of mmc1:0001:1 failed with error -16 > > which is likely bubbling up from somewhere inside the mmc/sdio layer. Well, it sounds like Joe might need a hand here. Since this regression is directly caused by commit 08da834 "mmc: enable runtime PM by default", which you authored and have interest in, it does make sense that you help Joe here. In case you're short of time, then I suggest to revert 08da834 (which anyway was experimental), and get back to it later, when you're more available. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
On Tue, Nov 29, 2011 at 1:17 PM, Joe Woodward wrote: > Any more feedback on this? Daniel, can you please take a look ? Joe, Daniel's patch is long term the right thing to do, so I hope we could understand and solve the issue you have and still go on with Daniel's long term plan. At the same time, though, we're very much determined not to compromise any existing functionality. So IMHO it's either fixing your regression or reverting the change (and then possibly introducing white/black listing as Daniel suggested), but we definitely aren't going to have you maintain the fix for this out-of-tree. Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
On Fri, Nov 18, 2011 at 12:00 PM, Joe Woodward wrote: > I'm assuming these changes have already made it to the mainline, and > therefore are in the 3.2rc2 build I was testing... Most probably, yeah. > Is there anything I can do to debug further the problems I'm seeing? Let's wait for Daniel to take a look, as he got libertas and sd8686 working with SDIO runtime PM. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: sdio: runtime PM and 8686 problems
Hi Joe, On Fri, Nov 18, 2011 at 10:53 AM, Joe Woodward wrote: > Are these workarounds expected to be enough for runtime PM to just "work"? It definitely works for 12xx, and with Daniel's recent work merged I think it also works fine for the sd8686 (Daniel, PCMIIW). Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] hwspinlock changes for 3.2
On Sun, Oct 30, 2011 at 10:06 PM, Ohad Ben-Cohen wrote:
> Please pull from
>
> git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock.git for-linus
...
> All of the patches have been tested in linux-next and currently there
> are no merge conflicts, though I expect one to happen with the arm-sco
> tree, as reported (and fixed) by Stephen earlier this month (see
> http://www.spinics.net/lists/linux-next/msg16931.html).
Now that arm-soc is merged, I fixed the conflict with the hwspinlock
tree and pushed it (just for reference) at:
git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock.git
for-linus-merged
There also seems to be a (trivial) omap_hsmmc build issue which I've
fixed and pushed to that branch (Chris I'm also attaching it below in
case you prefer to take it through the mmc tree).
> the hwspinlock
> tree and its upstream path have only materialized quite late in the
> cycle (see https://lkml.org/lkml/2011/9/21/249)
Linus, in the aforementioned thread, Arnd, Tony and Linus W preferred
that I send you direct pull requests for hwspinlock changes, but were
also ok with other upstream paths (e.g. through the arm-soc or the
omap trees), so please express your preference :) I don't expect much
hwspinlock activity anytime soon (at least not until we'll get either
c6x support or additional u8500 functionality).
Thanks,
Ohad.
commit f6e49189a11622f8675bd386db560309a05e581a
Author: Ohad Ben-Cohen
Date: Wed Nov 2 08:57:23 2011 +0200
mmc: omap_hsmmc: fix build
Fix omap_hsmmc build issue that was introduced by commit a3c76eb
"mmc: replace printk with appropriate display macro".
Signed-off-by: Ohad Ben-Cohen
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index e8ff123..101cd31 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1270,7 +1270,7 @@ static void omap_hsmmc_protect_card(struct omap_hsmmc_host
}
} else {
if (!host->protect_card) {
- pr_info"%s: cover is open, "
+ pr_info("%s: cover is open, "
"card is now inaccessible\n",
mmc_hostname(host->mmc));
host->protect_card = 1;
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: add a short delay in mmc_power_off
On Fri, Sep 16, 2011 at 6:33 PM, Daniel Drake wrote: > Do you know the source of that info? No, not more than what the comments say. It also seems that the first delay was originally 1ms and at some point it was increased to 2ms (commit f9996aee36921e8f1d499de1b2ea380855cf6d97 "mmc: increase power up delay"). Later on both delays were increased to 10ms (commit 79bccc5aefb4e64e651abe04f78c3e6bf8acd6f0 "mmc: increase power up delay"). > It probably also justifies what we need here. Maybe. I was wondering why nobody cared so far about putting delays at the power down path then, but I guess no one was really powering down their controllers ? I know Marvell did, using some out-of-tree code, but IIUC Marvell utilizes the sd8686's reset gpios (much like we do with the 12xx) to ensure hardware sanity. IIRC you guys don't toggle them at all ? > If short delays are needed when messing with power in the powerup path > (i.e. "we just touched power state, give power lines a chance to reach > that state"), it seems perfectly reasonable for similar (if not the > exact same) things to apply during power down. Sounds reasonable. Though I wonder why this is not abstracted in the regulator code: it only seems reasonable that a power up/down handler will return after the hardware is ready. > I have asked our hardware guys about tracing, and they weren't sure > exactly what I'm asking (and nor am I). Can you be more specific on > exactly how they should diagnose this issue at the hardware level? I > am not familiar with such tasks. I'm interested how does the hardware reacts to the sdio reset command that is sent at this state. There's no response whatsover ? > They were also not surprised that a delay is needed when playing with > power state in this way. Quickly bouncing rails can lead to > indeterminate hardware behaviour. I'm fine with your change. Feel free to add my Ack. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: add a short delay in mmc_power_off
On Wed, Sep 7, 2011 at 10:03 PM, Daniel Drake wrote: > On Wed, Sep 7, 2011 at 12:44 PM, Ohad Ben-Cohen wrote: >> No, I wouldn't write a driver for this... >> >> I suggest first to understand exactly what's the bug, and that really >> involves using a scope here. > > I'll see what can be done, but no promises. It would be nice if you can at least ask your hardware guys about it. The delay is benign and no one would notice it but if you can at least get some explanation of what's going on it'd really help, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: add a short delay in mmc_power_off
On Wed, Sep 7, 2011 at 10:03 PM, Daniel Drake wrote: >> Adding random sleeps without understanding what exactly do they solve >> generally feels wrong... > > Well, if you remove the existing sleeps from mmc_power_up do things > keep working for you? I wouldn't call them random - they're pretty much explained. OTOH, the delay you're suggesting to add isn't: we don't know what is it solving exactly and why is it needed. IMHO mainline kernel code should be well explained, otherwise it'd become impossible to maintain. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: enable runtime PM by default
On Sun, Jul 17, 2011 at 6:39 PM, Daniel Drake wrote: > Now that we have improved the runtime power management powerup/powerdown > code, we believe that MMC_CAP_POWER_OFF_CARD is no longer necessary: > runtime PM should now work everywhere. Let's also add: The only hard evidence for introducing MMC_CAP_POWER_OFF_CARD was the Marvell sd8686 wifi chip, which was believed to require external gpio manipulation which wasn't supported by some boards. After further investigation it was realized (and confirmed by Marvell folks) that sd8686 requirements can be fulfilled by changing the reset sequence itself, even if no external gpio is manipulated. For further information, see the following thread: http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg04289.html > Enable this trivially for a release or two. If no problems are reported, > we will follow up with a more extensive patch to remove this flag > altogether. If problems are reported, we can look at whitelist/blacklist > possibilities as before. This is exactly how I planed to take this too, thanks. Acked-by: Ohad Ben-Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: -ENOSYS suspend-powerdown regression
On Fri, Jul 8, 2011 at 6:38 PM, Daniel Drake wrote: > I tested vanilla and realised that this bug exists there too I'm aware of it - it's caused (indirectly) by another PM core change, which since then was already changed again in 3.1. Btw sorry for not being responsive - I've been mostly offline lately due to family getting bigger ;) Thanks for pushing the patches forward. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Wed, Jun 29, 2011 at 12:19 PM, zhangfei gao wrote: > Enable wlan: # ifconfig up mlan0 -> power up the chip via runtime PM > -> wlan_probe download the firmware > Disable wlan: # ifconfig down mlan0 -> power down the chip via runtime > PM -> wlan_remove ? Sounds all good, besides the part where you mention the "probe" and "remove" names. I'm not sure what you mean exactly, but generally, the driver's probe and remove functions should be called only once during the lifetime of the device it controls. You may want to refactor the driver a bit, so you don't need those handlers to be called whenever you power up/down the card. > So every time ifconfig up/down, the chip is power up/down, and > firmware reloaded? Yes, this is one way to go (I know some other out-of-tree WLAN drivers behave differently, but this is the way mac80211 works, and it's quite nice). -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Wed, Jun 29, 2011 at 11:43 AM, zhangfei gao wrote: > However still not fully understand how to call ->remove to power off > wlan, using suspend system looks to me is only test method, which > counting on bus_ops->suspend returns -ENOSYS. Please take a look at mac80211 and wl12xx. We're not using ->remove, ->probe or -ENOSYS at all. When the user brings up the interface, we then power up the chip and download the firmware. Likewise, when the interface is brought down, we just power off the chip. > 2. Explicitly call mmc_detect_change, > when power off, mmc_rescan -> bus_ops->detect(host) -> mmc_select_card > fail -> mmc_sdio_remove; > when power on, mmc_rescan -> mmc_attach_sdio->wlan probe. This is awkward. Basically what you're saying is that your driver can only power on the chip when it is probed. IMHO you need to refactor your driver, so it will power on/off the device according to interface status changes, without requiring the system to remove and re-detect the device. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: -ENOSYS suspend-powerdown regression
On Wed, Jun 29, 2011 at 12:54 AM, Daniel Drake wrote:
> Too many patches floating around, just trying to be sure of what I'm doing!
Ok, let's start from fresh :)
Let's take 3.0-rc5, it already includes our work:
297c7f2 mmc: sdio: fix runtime PM path during driver removal
c6e633a mmc: sdio: reset card during power_restore
Then apply these hunks:
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index d29b9c3..73dc3c2 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -167,11 +167,8 @@ static int sdio_bus_remove(struct device *dev)
int ret = 0;
/* Make sure card is powered before invoking ->remove() */
- if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD) {
- ret = pm_runtime_get_sync(dev);
- if (ret < 0)
- goto out;
- }
+ if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
+ pm_runtime_get_sync(dev);
drv->remove(func);
@@ -191,7 +188,6 @@ static int sdio_bus_remove(struct device *dev)
if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
pm_runtime_put_noidle(dev);
-out:
return ret;
}
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 58d5436..ce3e2e2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2488,7 +2488,7 @@ int sdhci_add_host(struct sdhci_host *host)
} else
mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
- mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
+ mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23 |
MMC_CAP_POWER_OFF_CARD;
if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
host->flags |= SDHCI_AUTO_CMD12;
I think this should be enough (I don't remember anything else pending).
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: -ENOSYS suspend-powerdown regression
On Tue, Jun 28, 2011 at 11:59 PM, Daniel Drake wrote: > On 28 June 2011 06:55, Ohad Ben-Cohen wrote: >> Obviously the second hunk is necessary, but I'd like to know whether >> the first one really is too or not. Can you please retest this without >> that hunk (try to suspend/resume while the chip is powered off, and >> again while it is powered on, but wol isn't used) ? > > Exactly which kernel should I run this test on? Latest (isn't that what you've been working with all this time ?). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
Hi Zhangfei, On Tue, Jun 28, 2011 at 12:13 PM, zhangfei gao wrote: > One question :( Np, feel free to ask ! (but it's usually better to start a new thread, rather then forking an existing one under the same subject) > Under pm_runtime mechanism, how to dynamically open/close wlan. Actually it should be the other way around: when you enable/disable WLAN (e.g. Ifconfig up/down), you then use the runtime PM API to power up/down the device. > If the wlan chip really power off, the firmware should be reloaded, > since the firmware is download to ram and disappear after power off. Right. > So wlan probe function should be called for re-downloading, is it be > achieved by calling pm_runtime_get_sync and mmc_power_restore_host? Not really; I assume you refer to libertas_sdio, which AFAICT, is built to power on the device (and configure it) on ->probe(), and then power it off on ->remove(). It then seems that this model was extended to support suspend/resume by asking the MMC core to ->remove() the card on suspend, and then relying on it to re-detect and re-add the card on resume, which will trigger libertas' ->probe() again. IMHO this model is a little awkward; as you can see, powering on/off the chip requires re-probing the driver. Take a look how mac80211 drivers (and wl12xx in particular) behave: they are powered on (and firmware is downloaded) when the user brings the wlan0 interface up, and then powered off when the wlan interface is brought down. The runtime PM API is only being used to control the power to the device, but downloading the firmware and doing driver-specific configuration is up to the driver to do. > From Daniel's test log, "echo mem > /sys/power/state" is used to > restart wlan, is this standard method? That command was only used to trigger system suspend - Daniel was testing libertas & runtime PM behavior in that scenario. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: -ENOSYS suspend-powerdown regression
Hi Daniel, On Sun, Jun 5, 2011 at 3:31 PM, Daniel Drake wrote: > On 5 June 2011 00:08, Ohad Ben-Cohen wrote: >> We can stop checking the return value of pm_runtime_get_sync in >> sdio_bus_remove, like pci_device_remove is doing, but I don't think >> that's enough: I guess libertas' if_sdio_remove won't be so happy >> dealing with a powered off card. >> >> Can you try the following please (untested): > > Thanks, that seems to be working. That patch had two hunks. The first one, in mmc_sdio_remove, made sure libertas' ->remove() handler will be called powered on, even if the chip was powered off beforehand. The second one, in sdio_bus_remove, directly took care of the problem you saw by ignoring rumtime PM's return value (which is a valid error in your scenario). Obviously the second hunk is necessary, but I'd like to know whether the first one really is too or not. Can you please retest this without that hunk (try to suspend/resume while the chip is powered off, and again while it is powered on, but wol isn't used) ? If the second hunk is sufficient (everything works as expected + no scary error messages coming from libertas), I would like to avoid the first one. I'm not sure at all there's any reason to power up the card if it is being removed, and a quick glance shows libertas_sdio has this 'user_rmmod' flag which should prevent scary error messages in this exact scenario. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Sat, Jun 25, 2011 at 9:23 PM, Daniel Drake wrote: > Now we just need you to submit the remaining parts of > http://dev.laptop.org/~dsd/20110609/sd-pwr-debug6.patch - can you take > care of that? I'm not sure if/how it should be split up or how to > write the commit message. Sure, i'll post it soon. > Then we can get rid of MMC_CAP_POWER_OFF_CARD, which would be good to > do ASAP to maximize testing before Linux 3.1. I'll take care of that. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Sat, Jun 25, 2011 at 9:20 PM, Daniel Drake wrote: > mmc_sdio_power_restore() skips some steps that are performed in other > power-related codepaths which are necessary to fully reset the card. > Without this, runtime PM fails for SD8686 SDIO wifi on OLPC XO-1.5. > > Signed-off-by: Daniel Drake Thanks for your work, Daniel. Acked-by: Ohad Ben-Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: fix runtime PM path during driver removal
Hi Chris, On Fri, Jun 10, 2011 at 2:40 AM, Ohad Ben-Cohen wrote: > After commit e1866b3 "PM / Runtime: Rework runtime PM handling > during driver removal" was introduced, the driver core stopped > incrementing the runtime PM usage counter of the device during > the invocation of the ->remove() callback. > > This indirectly broke SDIO's runtime PM path during driver removal, > because no one calls _put_sync() anymore after ->remove() completes. > > This means that the power of runtime-PM-managed SDIO cards is kept > high after their driver is removed (even if it was powered down > beforehand). > > Fix that by directly calling _put_sync() when the last usage > counter is downref'ed by the SDIO bus. Can you please take this 1-liner into 3.0-rc ? It fixes SDIO runtime PM after a breakage was introduced in 3.0. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
Hi Daniel, On Sun, Jun 19, 2011 at 1:33 PM, Daniel Drake wrote: > So, as I found here: http://article.gmane.org/gmane.linux.kernel.mmc/8605 > This is an OLPC-specific issue due to lack of power clamping on the > motherboard's power supply, which might not be shared by other sd8686 > users, and a long delay is expected. > > I have been testing further and I have seen a couple of times that > msleep(300) is not enough, or at least the card failed to do the cmd5 > reset in those cases. The sdio_reset() method seems to be 100% > reliable however, I haven't seen a single failure with that yet. Then sdio_reset() it is. We're taking a risk of covering up bugs like the one we just found here: http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08371.html So people will be happy that everything (seem to) work, without even knowing their card might not have been powered off at all. But I still think it's generally better, given the alternatives. And since this should not have any bad effect for anyone else, I'm also OK with not wrapping the sdio_reset and the CMD5 arg=0 you're adding with a quirk. After that's in place, I plan to completely get rid of MMC_CAP_POWER_OFF_CARD, since the only evidence we had when we added it was the sd8686, and today we know it's not necessary at all. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Fri, Jun 17, 2011 at 4:58 PM, Daniel Drake wrote: > Ignore the probe error - just observe the fact that the card was > detected and powered on, powered off, powered on and *then* probed. This is the normal behavior today when drivers are probed: we power up the card before probing a driver, and if the probe fails, we power it off. To be exact, we're not even doing that deterministically. We are powering off the *SDIO function*, and not the card. I once changed this to be deterministic, but didn't pursue this eventually, because frankly it's not hugely important. > That's the same thing that happens in the resume path. You mean on *libertas'* resume path, which doesn't really do suspend/resume - it just removes the card, and let it be re-probed later. > Couldn't the > power have just been maintained from when the card was detected, given > that the driver was loaded and ready to go? Most cards wouldn't care too much: we're not loading/unloading drivers too frequently, and the advantage of the current solution is simplicity. But you are welcome to change this if you need to. I don't see how it can hurt anyone, and anyway it can also happen today too in some cases. > Thinking more, I think I prefer sdio_reset() as a quirk/workaround > instead of the sleep, because the sleep is quite long. What do you > think? I agree that a 250ms delay on the power up path is way too long. But let's first try to understand what's the real problem it is solving before picking a solution. This usually pays off, and eventually you'd be happier with the end result. > Any suggestions for how I go about doing that? Unfortunately we have a > backlog of support requests with Marvell which have not yet been > responded to. Use the mailing list :) The Marvell guys were very nice and replied my questions pretty fast. > Yes, a regulator could work. But, aren't regulators linked to the > platform rather than to the card? I would imagine that this quirk > needs to be applied to all users of the card, therefore a card quirk > could be more appropriate? Also, if you think the sdio_reset() option > is preferable to the sleep, can that be done from a regulator? I'd really prefer to understand what's wrong first. If we have a rigid hardware requirement that demands waiting before powering it up again, then we better obey it. But I somehow doubt that a 250ms delay is required. That's way too much... Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Thu, Jun 16, 2011 at 8:27 PM, Daniel Drake wrote: > Why is there so much power flipping going on during resume? There isn't. You see this with libertas, because it doesn't really suspend/resume cleanly. Instead, it uses a pretty awkward (in my opinion) mechanism to remove the card on suspend, and then let the system re-probe it on resume. > But even if that gets fixed, we still need to fix the case where the > network interface is brought down then up immediately (another way to > trigger the issue). Could you also trigger this with the previous power off/on mechanism you used (an rfkill driver IIRC) ? if not, what's the difference ? did you have just enough delay there which didn't trigger this problem ? > Would you suggest a card quirk for that? Adding > another 250ms to the already-slow libertas powerup routine would be a > bit painful, would you support the added complexity needed to make the > 250ms delay only occur when >250ms has passed since it was powered > off? First, I suggest to understand where does this requirement come from. Is this a rigid hardware requirement ? Does it always require waiting 250ms before powering it on again ? If yes, then why not let the relevant regulator take care of it ? this way you never have to care. you just power it off and on as you please, and things just work. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Fri, Jun 10, 2011 at 7:15 PM, Daniel Drake wrote: > Done. Without bringing suspend/resume into the picture, this is > working fine. Power is removed when the interface goes down, and is > restored when the interface is brought up. ios values look fine at > every stage. Wireless card works fine after several power cycles. That sounds great. > Patches here: http://dev.laptop.org/~dsd/20110610/ > The most relevant ones for this discussion are 6 and 7. > > What next? We need to debug the suspend/resume path. Now that we have the other runtime pm paths working, we can pretty much tell there's no hw issue at hand. I definitely plan to help you nail this, but I'm a little tied up with some tight schedule for a little while. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
On Sat, Jun 11, 2011 at 5:33 AM, zhangfei gao wrote: > If you power down and up the device, then IO reset is not needed and > 8686 can process host init sequence correctly. Thanks, that's what I thought. Daniel, this ultimately means that whenever sending a reset is required to re-init the 8686, we can surely say that the chip wasn't really powered off beforehand, and that something went wrong in the software, leading us to think the chip is powered off when it is really not. But I think we also demonstrated this with the simple insmod/rmmod/insmod scenario, where every insmod successfully re-initialized the chip without sending an sdio reset. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
Hi Zhangfei, On Fri, Jun 10, 2011 at 5:02 AM, zhangfei gao wrote: > Here is answer got from the sd8686 maintainer. > > For 8686, the SDIO state machine can only handle init sequence (CMD5, > 5, 3, 7) from host once. If host sends another init sequence, it will > not be able to handle CMD5 and causes the SDIO block to hang. Chips > that are newer than 8686 will be able to handle multiple init sequence > from host. Thanks for the reply ! > So yes, for 8686, an IO reset is needed before host can send a new set > of init sequence. But if we're powering down and up the device first, then the init sequence is considered the first one, and then we don't need an IO reset, right ? That was what we wondered about. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: sdio: fix runtime PM path during driver removal
After commit e1866b3 "PM / Runtime: Rework runtime PM handling during driver removal" was introduced, the driver core stopped incrementing the runtime PM usage counter of the device during the invocation of the ->remove() callback. This indirectly broke SDIO's runtime PM path during driver removal, because no one calls _put_sync() anymore after ->remove() completes. This means that the power of runtime-PM-managed SDIO cards is kept high after their driver is removed (even if it was powered down beforehand). Fix that by directly calling _put_sync() when the last usage counter is downref'ed by the SDIO bus. Reported-and-tested-by: Daniel Drake Signed-off-by: Ohad Ben-Cohen --- drivers/mmc/core/sdio_bus.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c index d29b9c3..d2565df 100644 --- a/drivers/mmc/core/sdio_bus.c +++ b/drivers/mmc/core/sdio_bus.c @@ -189,7 +189,7 @@ static int sdio_bus_remove(struct device *dev) /* Then undo the runtime PM settings in sdio_bus_probe() */ if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD) - pm_runtime_put_noidle(dev); + pm_runtime_put_sync(dev); out: return ret; -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Thu, Jun 9, 2011 at 10:55 PM, Daniel Drake wrote: > On 9 June 2011 19:25, Ohad Ben-Cohen wrote: >> let's update that patch. I'd send you an updated one, but I have to go >> for awhile, so here's the quick change you need to do. > > Unfortunately it doesn't help. That's ok. Let's go back to small steps now. We have cyclic rmmod/insmod working, and that's good: it means there's no hw issue. The next step to have working now is really the ifconfig up/down/up scenario. That should come before suspend/resume. And this step-by-step approach is even more relevant to libertas_sdio, because I suspect there's some if_sdio.c refactoring needed to get it right, but let's see. The expected result you want to get is: - boot, power is off - insmod, power is still off - ifconfig up, power is on - ifconfig down or rmmod, power is back off -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Thu, Jun 9, 2011 at 8:56 PM, Daniel Drake wrote:
> Done, the rmmod issue remains fixed - the card gets powered down during rmmod.
great !
> Next up is the problem where on this setup, if I suspend after loading
> the module, and the module asks the card to shut down during suspend,
> it fails to get brought up during resume.
we need to take into account that driver core change I spotted.
> Note that I have now rolled in a patch you made in another thread to
> correctly call into the driver's remove routine during suspend when
> powering down the card.
let's update that patch. I'd send you an updated one, but I have to go
for awhile, so here's the quick change you need to do.
replace the put_noidle() call in the hunk below with a put_sync():
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 4d0c15b..e23888a 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -540,6 +540,13 @@ static void mmc_sdio_remove(struct mmc_host *host)
BUG_ON(!host);
BUG_ON(!host->card);
+ /*
+* if this card is managed by runtime pm, make sure it is powered on
+* before invoking its SDIO functions' ->remove() handler
+*/
+ if (host->caps & MMC_CAP_POWER_OFF_CARD)
+ pm_runtime_get_sync(&host->card->dev);
+
for (i = 0;i < host->card->sdio_funcs;i++) {
if (host->card->sdio_func[i]) {
sdio_remove_func(host->card->sdio_func[i]);
@@ -547,6 +554,9 @@ static void mmc_sdio_remove(struct mmc_host *host)
}
}
+ if (host->caps & MMC_CAP_POWER_OFF_CARD)
+ pm_runtime_put_noidle(&host->card->dev); //<==
use
pm_runtime_put_sync here
+
mmc_remove_card(host->card);
host->card = NULL;
}
tell me how it goes...
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Thu, Jun 9, 2011 at 7:44 PM, Daniel Drake wrote: > Note that during the insmod/rmmod calls, runtime PM did not touch the > power state - the card remained powered ever since the first insmod. > Not sure if this is in-line with your expectations. It definitely isn't. And that may very well be our culprit here. Let's focus now on just this one sequence: - boot - cat /sys/.../ios (power should be off) - insmod - cat /sys/.../ios (power should be on) - rmmod - cat /sys/.../ios (power should be off again) You will see the problem now. And I think I know why - there was a driver core change that indirectly caused this. Please try to revert e1866b33b1e89f077b7132daae3dfd9a594e9a1a "PM / Runtime: Rework runtime PM handling during driver removal" and tell me if it helped. If it did, un-revert that change, and try this one patch, which adopts SDIO runtime PM to that change: diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c index d29b9c3..d2565df 100644 --- a/drivers/mmc/core/sdio_bus.c +++ b/drivers/mmc/core/sdio_bus.c @@ -189,7 +189,7 @@ static int sdio_bus_remove(struct device *dev) /* Then undo the runtime PM settings in sdio_bus_probe() */ if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD) - pm_runtime_put_noidle(dev); + pm_runtime_put_sync(dev); out: return ret; Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Thu, Jun 9, 2011 at 7:21 PM, Daniel Drake wrote: > With this version of the patch: > http://dev.laptop.org/~dsd/20110609/sd-pwr-debug2.patch > > bash-4.1# mount -t debugfs none /sys/kernel/debug > bash-4.1# cat /sys/kernel/debug/mmc1/ios > clock: 0 Hz > vdd: 0 (invalid) > bus mode: 1 (open drain) > chip select: 0 (don't care) > power mode: 0 (off) > bus width: 0 (1 bits) > timing spec: 0 (legacy) > bash-4.1# > bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko > [ 53.917466] libertas_sdio: Libertas SDIO driver > [ 53.922718] libertas_sdio: Copyright Pierre Ossman > [ 54.839032] libertas_sdio mmc1:0001:1: (unregistered net_device): > 00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x03a3 > [ 54.855479] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter > bash-4.1# [ 54.941099] udev[985]: renamed network interface wlan0 to eth0 > [ 54.997656] ieee80211 phy0: assoc: bss (null) not in scan results > [ 56.310846] ieee80211 phy0: assoc: bss (null) not in scan results > [ 56.360840] cfg80211: Calling CRDA for country: EU Looks good. At this point everything works ? (can you bring up the interface and scan/connect) Can you now do a series of insmod-rmmod-insmod.. and see if things always work (with no runtime pm errors) after you insmod ? If yes, we're good. > Note that the patch includes the mmc_select_voltage() call Good, keep that one please. > With a version of the patch that just does the reset, the post-powerup > "vdd" figure does change: > http://dev.laptop.org/~dsd/20110609/sd-pwr-debug3.patch ... > bash-4.1# cat /sys/kernel/debug/mmc1/ios > clock: 2500 Hz > vdd: 21 (3.3 ~ 3.4 V) Without even looking exactly why it happens, it doesn't look too good. I don't see a reason to stick to that version. Let's use the mmc_select_voltage. > > For reference, here is the equivalent test performed without runtime > PM enabled (i.e. all changes reverted) you mean runtime PM disabled, right ? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Thu, Jun 9, 2011 at 6:51 PM, Daniel Drake wrote: > bash-4.1# mount -t debugfs none /sys/kernel/debug > bash-4.1# cat /sys/kernel/debug/mmc1/ios > clock: 0 Hz > vdd: 0 (invalid) > bus mode: 1 (open drain) > chip select: 0 (don't care) > power mode: 0 (off) > bus width: 0 (1 bits) > timing spec: 0 (legacy) > bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko > [ 67.917748] libertas_sdio: Libertas SDIO driver > [ 67.922976] libertas_sdio: Copyright Pierre Ossman > [ 67.972073] libertas_sdio: probe of mmc1:0001:1 failed with error -16 > bash-4.1# cat /sys/kernel/debug/mmc1/ios > clock: 40 Hz > vdd: 21 (3.3 ~ 3.4 V) > bus mode: 1 (open drain) > chip select: 0 (don't care) > power mode: 2 (on) > bus width: 0 (1 bits) > timing spec: 0 (legacy) Ok, cool. Now can you please repeat this, but this time add your original patch (which only added the CMD5 arg=0 cmd, no sdio reset yet) ? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Wed, Jun 8, 2011 at 11:58 PM, Daniel Drake wrote: >> Please reboot, and immediately after booting (without insmoding the >> driver) tell me what's the output of : >> >> mount -t debugfs none /sys/kernel/debug >> cat /sys/kernel/debug/mmc1/ios > > Which base kernel setup should I run these tests on? Vanilla, with your temporary patch below. Without this patch, mmc1 (I assume this what drives sd8686 in your setup) should be powered on. With it, it should be powered off. diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 58d5436..ce3e2e2 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2488,7 +2488,7 @@ int sdhci_add_host(struct sdhci_host *host) } else mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200; - mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23; + mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23 | MMC_CAP_POWER_OFF_CARD; if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) host->flags |= SDHCI_AUTO_CMD12; -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Wed, Jun 8, 2011 at 5:21 PM, Daniel Drake wrote: >> 1. How come power off+on works for you in the first time, but doesn't >> work in the second time ? > > I assume it is because a different codepath was taken in order to > power up the device the first time, before it then got powered down, > compared to the codepath that failed to power it up the 2nd time. My > efforts so far have been based around eliminating the differences in > those codepaths, and this approach seems to be successful. We have 3 power-on paths here: on boot, on insmod, and and resume. I understand the first one succeeds (naturally), and the last one failed, but now I'm confused what was the outcome of the 2nd (without sending a reset cmd) ? >> 3. what if you do a series of insmod+rmmod ? does this work ? (power >> should be taken down and up every time) > > rmmod doesn't appear to take down the power. Let's nail this one first. if we get it right, the rest will immediately follow. Please reboot, and immediately after booting (without insmoding the driver) tell me what's the output of : mount -t debugfs none /sys/kernel/debug cat /sys/kernel/debug/mmc1/ios Then insmod the driver, and tell me again what's the output of /sys/.../mmc1/ios. Please send a contiguous terminal session of these steps (incl. the boot messages), thanks ! -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
Hi Bing, On Sat, Jun 4, 2011 at 1:52 AM, Ohad Ben-Cohen wrote: > On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao wrote: >> "CMD5 Arg=0" refers to the very first CMD5 sent from host during >> initialization sequence. >> This is required because our state machine always expects two CMD5 from host >> (5, 5, 3, 7, ...). > > Great, thanks for confirming this ! I have another question please. Does the sd8686 require an SDIO I/O reset (CMD52 setting bit 3 of address 0x6 of the CCCR) to it after powering it on ? Daniel (cc'ed) is trying to power it off and back on, and it does seem to work in the first time, without sending a reset. In the second time, though, the card doesn't answer CMD5 anymore, unless Daniel is sending it an SDIO I/O reset. I was wondering whether this is an sd8686 requirement, or whether we have some other issue at hand. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Wed, Jun 8, 2011 at 4:36 PM, Daniel Drake wrote: > If I add the reset (shown commented-out in the patch), it matches the > behaviour of other known-good codepaths and also solves the problem. Yeah, but we don't know why. And we may very well be covering a different bug if we just take it as-is. Don't get me wrong, I have no objection to have this if it's needed, but we shouldn't do this just "because it works". So I'll try to look at the logs you provided a bit more later, but the main questions I have are: 1. How come power off+on works for you in the first time, but doesn't work in the second time ? 2. Was the card really powered off in this 2nd-time failed scenario ? 3. what if you do a series of insmod+rmmod ? does this work ? (power should be taken down and up every time) 4. Are you also interested in powering off the card when the wlan interface is down (i.e. coupling the power of the card with the state of the interface) too ? it should be easy to debug. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Wed, Jun 8, 2011 at 12:20 PM, Daniel Drake wrote: > In the first patch I sent I had only tested basic power-up after > probe, and found it to be working. Then I tested the other scenarios > listed in my last mail - basically powering it off and on *again* - > and found them to be broken, but fixed by further mirroring what > existing non-rpm code would do in order to power up a SD card. Sounds like you didn't power the card off then in the driver after probe ? Can you show the diff with which you did this experiment ? There is no difference between powering the card up in sdio_bus_probe to powering it up later in the driver. If the card was indeed powered off beforehand, then the latter should work too. Your reset command strongly suggests that wasn't the case. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Tue, Jun 7, 2011 at 7:41 PM, Daniel Drake wrote: > On 5 June 2011 14:48, Ohad Ben-Cohen wrote: >> Sending a reset is necessary only if we are re-initing a powered on >> card, but in this case, we know that we just powered the card up, so >> this is not needed. > > Don't ask me for an explanation, but this is required here Uhm, strange. Maybe this has something to do with the sd8686's reset line which you are not toggling ? Btw, the previous patch which you sent didn't have this reset command, and the patch did work for you. What has changed in this respect ? > Are you suggesting that a quirk is used to enable this reset, or > to disable it? To disable the CMD5 arg=0. it's not mandatory by the spec, and some cards don't need it. It's not harmful either way, but it's just cleaner for those cards (and it's really just adding 1 'if' statement..). -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
On Tue, Jun 7, 2011 at 7:41 PM, Daniel Drake wrote: > Also, the most frustrating thing when working on this issue is that > the codepaths for powering up a SD card between the following 3 > situations are significantly different: > 1. Power up and probe during boot > 2. Resume from suspend > 3. Power up after runtime suspend > > It feels like there should be a lot more in common here. Definitely agree. I also suggested this in my first reply to you on our other thread: http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08193.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
On Tue, Jun 7, 2011 at 5:34 PM, Arnd Hannemann wrote: > Sorry, I don't have the hardware anymore so I'm not able to check this. Ok, thanks for the reply ! -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: -ENOSYS suspend-powerdown regression
On Sun, Jun 5, 2011 at 3:31 PM, Daniel Drake wrote: > On 5 June 2011 00:08, Ohad Ben-Cohen wrote: >> We can stop checking the return value of pm_runtime_get_sync in >> sdio_bus_remove, like pci_device_remove is doing, but I don't think >> that's enough: I guess libertas' if_sdio_remove won't be so happy >> dealing with a powered off card. >> >> Can you try the following please (untested): > > Thanks, that seems to be working. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdio: reset card during power_restore
Hi Daniel, On Sun, Jun 5, 2011 at 3:38 PM, Daniel Drake wrote: > @@ -706,10 +706,25 @@ static int mmc_sdio_power_restore(struct mmc_host *host) > BUG_ON(!host->card); > > mmc_claim_host(host); > + > + /* > + * Reset the card by performing the same steps that are taken by > + * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe > + */ > + sdio_reset(host); Sending a reset is necessary only if we are re-initing a powered on card, but in this case, we know that we just powered the card up, so this is not needed. + ret = mmc_send_io_op_cond(host, 0, NULL); + if (ret) + goto out; Can you please add this under a card quirk ? The spec does not require this for embedded sdio cards, and we would like to skip this for wl12xx. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: -ENOSYS suspend-powerdown regression
... going back to the call trace you provided ...
On Sat, Jun 4, 2011 at 1:18 PM, Daniel Drake wrote:
> Here is the call trace:
>
> mmc_suspend_host
> mmc_sdio_remove
> sdio_remove_func
> device_del
This eventually disables runtime PM for your SDIO function.
We can stop checking the return value of pm_runtime_get_sync in
sdio_bus_remove, like pci_device_remove is doing, but I don't think
that's enough: I guess libertas' if_sdio_remove won't be so happy
dealing with a powered off card.
Can you try the following please (untested):
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 4d0c15b..8af3330 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -540,6 +540,13 @@ static void mmc_sdio_remove(struct mmc_host *host)
BUG_ON(!host);
BUG_ON(!host->card);
+ /*
+* if this card is managed by runtime pm, make sure it is powered on
+* before invoking its SDIO functions' ->remove() handler
+*/
+ if (host->caps & MMC_CAP_POWER_OFF_CARD)
+ pm_runtime_get_sync(&host->card->dev);
+
for (i = 0;i < host->card->sdio_funcs;i++) {
if (host->card->sdio_func[i]) {
sdio_remove_func(host->card->sdio_func[i]);
@@ -547,6 +554,9 @@ static void mmc_sdio_remove(struct mmc_host *host)
}
}
+ if (host->caps & MMC_CAP_POWER_OFF_CARD)
+ pm_runtime_put_noidle(&host->card->dev);
+
mmc_remove_card(host->card);
host->card = NULL;
}
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index d29b9c3..73dc3c2 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -167,11 +167,8 @@ static int sdio_bus_remove(struct device *dev)
int ret = 0;
/* Make sure card is powered before invoking ->remove() */
- if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD) {
- ret = pm_runtime_get_sync(dev);
- if (ret < 0)
- goto out;
- }
+ if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
+ pm_runtime_get_sync(dev);
drv->remove(func);
@@ -191,7 +188,6 @@ static int sdio_bus_remove(struct device *dev)
if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
pm_runtime_put_noidle(dev);
-out:
return ret;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: -ENOSYS suspend-powerdown regression
On Sat, Jun 4, 2011 at 1:18 PM, Daniel Drake wrote: > Digging further, in the pm_runtime_get_sync() call we reach > rpm_resume() in drivers/base/power where we hit: > > else if (dev->power.disable_depth > 0) > retval = -EAGAIN; > > Not sure what this means. Any thoughts? It means runtime PM is disabled for your device. Which doesn't make sense, assuming you had MMC_CAP_POWER_OFF_CARD configured (if you didn't, you wouldn't end up calling runtime PM api at all). If you post your entire diff, I might be able to spot what's wrong. As a side note, if the additional CMD5 arg=0 fixes both sd8686's and b43's issues, we might consider removing MMC_CAP_POWER_OFF_CARD entirely. But let's not rush with that yet. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
(cc'ing Arnd Hannermann) On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao wrote: > "CMD5 Arg=0" refers to the very first CMD5 sent from host during > initialization sequence. > This is required because our state machine always expects two CMD5 from host > (5, 5, 3, 7, ...). Great, thanks for confirming this ! Arnd, you have reported SDIO runtime pm issues with the b43 in the past. If you're still interested and have some free cycles, you might want to check out Daniel's patch and see if that fixes the issues you had too. With Daniel's patch we're now following the spec more strictly, and the change is particularly relevant for removable SDIO cards like the one you were using. Daniel, please go ahead and submit your patch when you're ready. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
On Thu, Jun 2, 2011 at 11:39 AM, Bing Zhao wrote: >> If we power down the sd8686, and power it up again, without toggling >> the reset pin at all (e.g. keep it always high), will the card work ? > > Yes. The card should work without toggling the reset pin. Thanks. Just for closure-sake, can you please confirm that the sd8686 requires sending a CMD5 arg=0 as part of the initialization sequence after powering it on ? (we'd probably add it anyway, but it'd be nice to get a confirmation about this from you guys) Btw, it will be nice to allow cards to avoid sending this if not required; a simple card quirk will do. wl12xx will definitely use such a quirk. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
Hi Zhangfei, Bing, On Mon, May 30, 2011 at 2:04 PM, zhangfei gao wrote: > sd8686 requires two pins to control power, PDn and RESETn. > To enable sd8686, PDn and RESETn should both set high. > To disable sd8686, PDn should be set low. If we power down the sd8686, and power it up again, without toggling the reset pin at all (e.g. keep it always high), will the card work ? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
On Mon, May 30, 2011 at 10:01 AM, Daniel Drake wrote: > On 30 May 2011 07:52, Ohad Ben-Cohen wrote: >> Last we talked, we found out runtime PM didn't work because the sd8686 >> required an additional manipulation of an external reset gpio line, >> and that the only reason OLPC could power it down/up was this patch: >> >> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0 > > My further investigation here suggests that this change is not > necessary. It was added in response to a separate (hard-to-reproduce) > issue and it was never known if it would actually fix that issue, it > was more of a guess. We don't have any convincing evidence that it > helps, so it will be dropped in future. > > Anyway, just to be sure, I tried combining this hack with runtime PM, > and also as a regulator, and it didn't help anything. runtime PM still > fails to power up the card. > > Sorry for leading you down the wrong path there. ... >> does mmc_stop_host+mmc_start_host >> work for you without manipulating that reset gpio ? > > Yes. Hm. I still don't entirely get it, because we had others (Mike, cc'd) saying too that the sd8686 requires manipulating an external reset gpio after bringing the power back up. Maybe someone from Marvell can comment on this (cc'ing Zhangfei Gao) ? > You didn't comment on the added mmc_select_voltage() call. Is that one > also sensible? I guess. if we're reading the I/O OCR, might as well use it. This way our runtime PM power up path will also be identical to the one induced by mmc_attach_sdio. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MMC runtime PM patches break libertas probe
Hi Daniel, On Sun, May 29, 2011 at 7:21 PM, Daniel Drake wrote: > It's certainly possible that there's something weird about the > hardware in question, but we *are* able to successfully power down and > up the card with a hacky rfkill driver that calls mmc_stop_host / > mmc_start_host. Are we talking about the XO-1.5 and the sd8686 ? Last we talked, we found out runtime PM didn't work because the sd8686 required an additional manipulation of an external reset gpio line, and that the only reason OLPC could power it down/up was this patch: http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0 One of the suggested solutions back then was to use a regulator for this. Has anything changed since then ? does mmc_stop_host+mmc_start_host work for you without manipulating that reset gpio ? > err = mmc_send_io_op_cond(host, 0, &ocr); > if (err) > return err; This part actually makes sense. While sending a CMD5 arg=0 is not mandatory when initializing embedded SDIO cards (like the wl12xx is), some cards might still require it, and it is actually anyway required by the spec for removal cards. I just tried it briefly with the wl12xx, and things seems ok. > printk(KERN_WARNING "%s: card claims to support voltages " > "below the defined range. These will be ignored.\n", > mmc_hostname(host)); Please drop this warning though. It was already displayed when mmc_attach_sdio() executed, so the user already knows his card has this thing. With the wl12xx, you'd see this everytime you bring up the interface, so it's really unnecessarily too noisy. > Any thoughts? One other thing to consider is system resume. when wol is disabled, and your chip is powered off during system suspend, you'd need this CMD5 arg=0 in the resume path as well. Some code refactoring should be considered to avoid duplicating this snippet three times though. Otherwise, submit :) Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch "mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume" has been added to the 2.6.32-longterm tree
On Fri, May 6, 2011 at 12:54 AM, wrote: > > This is a note to let you know that I've just added the patch titled > > mmc: fix all hangs related to mmc/sd card insert/removal during > suspend/resume > > to the 2.6.32-longterm tree That patch introduced a suspend/resume regression, so please also add the one that fixed it: commit 1c8cf9c997a4a6b36e907c7ede5f048aeaab1644 Author: Ohad Ben-Cohen Date: Wed Oct 13 09:31:56 2010 +0200 mmc: sdio: fix SDIO suspend/resume regression Fix SDIO suspend/resume regression introduced by 4c2ef25fe0b "mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume": PM: Syncing filesystems ... done. Freezing user space processes ... (elapsed 0.01 seconds) done. Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done. Suspending console(s) (use no_console_suspend to debug) pm_op(): platform_pm_suspend+0x0/0x5c returns -38 PM: Device pxa2xx-mci.0 failed to suspend: error -38 PM: Some devices failed to suspend 4c2ef25fe0b moved the card removal/insertion mechanism out of MMC's suspend/resume path and into pm notifiers (mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host() will remove the card, and squash the error, in case -ENOSYS is returned from the bus suspend handler (mmc_sdio_suspend() in this case). mmc_sdio_suspend() is using this whenever at least one of the card's SDIO function drivers does not have suspend/resume handlers - in that case it is agreed to force removal of the entire card. This patch fixes this regression by trivially bringing back that part of mmc_suspend_host(), which was removed by 4c2ef25fe0b. Reported-and-tested-by: Sven Neumann Signed-off-by: Ohad Ben-Cohen Cc: Maxim Levitsky Cc: Acked-by: Nicolas Pitre Signed-off-by: Chris Ball Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()
On Tue, Apr 19, 2011 at 4:23 PM, Guennadi Liakhovetski
wrote:
> Seeing a "struct dev_pm_ops" instance with .runtime_suspend(),
> .runtime_resume(), and .runtime_idle() methods I understand, that
> "suspend" and "resume" are two counterparts, that balance each other. Now
> with "idle" I am not sure which method should balance it. With platform
> devices in the generic case idle ends up calling
> pm_generic_runtime_idle(), which then calls pm_runtime_suspend(). So,
> there should be a balancing pm_runtime_resume() somewhere?
There are many ways with which the dev_pm_ops handlers get called, but
none of them include imbalance.
E.g. take a look how pm_runtime_{get,put}_sync balance each other,
while involving all three runtime pm handlers that you've specified
(suspend/resume/idle).
Can you point out an existing device/flow that demonstrates a runtime
pm imbalance ?
>> More specifically, without having this ->runtime_idle() handler, the
>> last user giving up its power usage_count (e.g. by calling
>> pm_runtime_put{_sync}) will not end up powering down the MMC card.
>
> How do they work then? Who does the pm_runtime_resume() to undo the
> effects of the pm_runtime_suspend()
Let's take SDIO for example; note how sdio_bus_probe and
sdio_bus_remove balance each other with respect to runtime PM api
invocations.
> or is it the platform runtime-pm that is implementing the "idle" method
> wrongly?
I'm not following what's wrong exactly. If you point out specific code
and specify exactly the issues you witness, it might be easier to
help.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()
On Tue, Apr 19, 2011 at 1:46 PM, Guennadi Liakhovetski
wrote:
> MMC bus PM operations implement a .runtime_idle() method, which calls
> pm_runtime_suspend(), but this call is not balanced by a resume
> counterpart,
What's the exact flow you refer to ?
> which causes problems with repeated card-plug and driver-load
> cycles.
Can you please be more specific ? What are you trying to achieve, what
are the problems you encounter ?
> Removing this method fixes the disbalance.
I'm not sure which disbalance you're referring to, but if you'll
remove this method you will break MMC/SDIO runtime PM.
More specifically, without having this ->runtime_idle() handler, the
last user giving up its power usage_count (e.g. by calling
pm_runtime_put{_sync}) will not end up powering down the MMC card.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] mmc: add MMC_QUIRK_DISABLE_CD
006ebd5d introduced sdio_disable_cd(), which disconnects the pull-up
resistor on CD/DAT[3] (pin 1) of the card.
Make it possible to start using sdio_disable_cd() by introducing
MMC_QUIRK_DISABLE_CD.
Signed-off-by: Ohad Ben-Cohen
---
Chris, I've posted this one before. Sending it again, this time with a
follow-up patch that uses the new mmc quirks facility. thanks!
drivers/mmc/core/sdio.c |2 +-
include/linux/mmc/card.h |6 ++
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index c3c2bd0..a5840c0 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -187,7 +187,7 @@ static int sdio_disable_cd(struct mmc_card *card)
int ret;
u8 ctrl;
- if (!card->cccr.disable_cd)
+ if (!mmc_card_disable_cd(card))
return 0;
ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_IF, 0, &ctrl);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index c7f1532..4e03be8 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -126,6 +126,7 @@ struct mmc_card {
/* (missing CIA registers) */
#define MMC_QUIRK_BROKEN_CLK_GATING (1<<3) /* clock gating the sdio bus
will make card fail */
#define MMC_QUIRK_NONSTD_FUNC_IF (1<<4)/* SDIO card has nonstd
function interfaces */
+#define MMC_QUIRK_DISABLE_CD (1<<5) /* disconnect CD/DAT[3]
resistor */
unsigned interase_size; /* erase size in sectors */
unsigned interase_shift;/* if erase unit is power 2 */
@@ -181,6 +182,11 @@ static inline int mmc_blksz_for_byte_mode(const struct
mmc_card *c)
return c->quirks & MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
}
+static inline int mmc_card_disable_cd(const struct mmc_card *c)
+{
+ return c->quirks & MMC_QUIRK_DISABLE_CD;
+}
+
static inline int mmc_card_nonstd_func_interface(const struct mmc_card *c)
{
return c->quirks & MMC_QUIRK_NONSTD_FUNC_IF;
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] mmc: quirks: wl1271 is MMC_QUIRK_DISABLE_CD
The wl12xx device supports disconnecting the pull-up resistor on
CD/DAT[3] (pin 1) of the card.
Tell SDIO core to disconnect that resistor during card init,
since we don't need it at that point (and anyway all
hosts shall provide pull-up resistors on all data lines DAT[3:0]
as described in section 6 of the SD physical specification).
As a result, this may save some power, but it's also generally healthy
since it prevents both ends from pulling up that pin, which
results in undesirable asymmetric physical bus.
Signed-off-by: Ohad Ben-Cohen
---
drivers/mmc/core/quirks.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/quirks.c b/drivers/mmc/core/quirks.c
index 1957398..a4c42ed 100644
--- a/drivers/mmc/core/quirks.c
+++ b/drivers/mmc/core/quirks.c
@@ -66,6 +66,8 @@ static const struct mmc_fixup mmc_fixup_methods[] = {
remove_quirk, MMC_QUIRK_BROKEN_CLK_GATING },
{ SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
add_quirk, MMC_QUIRK_NONSTD_FUNC_IF },
+ { SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+ add_quirk, MMC_QUIRK_DISABLE_CD },
{ 0 }
};
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] mmc: quirks: wl1271 is MMC_QUIRK_NONSTD_FUNC_IF
Tell SDIO core to ignore the standard SDIO function interface
codes indicated by the wl1271. This is required because the
wl1271 erroneously indicates its first function as a standard
Bluetooth SDIO interface, and that drives btsdio mad.
Signed-off-by: Ohad Ben-Cohen
---
drivers/mmc/core/quirks.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/quirks.c b/drivers/mmc/core/quirks.c
index 8b7..1957398 100644
--- a/drivers/mmc/core/quirks.c
+++ b/drivers/mmc/core/quirks.c
@@ -64,6 +64,8 @@ static const struct mmc_fixup mmc_fixup_methods[] = {
add_quirk_for_sdio_devices, MMC_QUIRK_BROKEN_CLK_GATING },
{ SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
remove_quirk, MMC_QUIRK_BROKEN_CLK_GATING },
+ { SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271,
+ add_quirk, MMC_QUIRK_NONSTD_FUNC_IF },
{ 0 }
};
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] mmc: add MMC_QUIRK_NONSTD_FUNC_IF
Introduce MMC_QUIRK_NONSTD_FUNC_IF to ignore the "SDIO Standard Function
interface code" as indicated by the card's FBR, and instead treat all
functions as non-standard interfaces.
This is required to prevent standard drivers from facing
errors when trying to communicate with SDIO cards that erroneously
indicate standard function interface codes.
Signed-off-by: Ohad Ben-Cohen
---
Chris, I've posted this one before. Sending it again, this time with a
follow-up patch that uses the new mmc quirks facility. thanks!
drivers/mmc/core/sdio.c |6 ++
include/linux/mmc/card.h |6 ++
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 4221670..c3c2bd0 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include "core.h"
#include "bus.h"
@@ -31,6 +32,11 @@ static int sdio_read_fbr(struct sdio_func *func)
int ret;
unsigned char data;
+ if (mmc_card_nonstd_func_interface(func->card)) {
+ func->class = SDIO_CLASS_NONE;
+ return 0;
+ }
+
ret = mmc_io_rw_direct(func->card, 0, 0,
SDIO_FBR_BASE(func->num) + SDIO_FBR_STD_IF, 0, &data);
if (ret)
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index adb4888..c7f1532 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -125,6 +125,7 @@ struct mmc_card {
#define MMC_QUIRK_NONSTD_SDIO (1<<2) /* non-standard SDIO card
attached */
/* (missing CIA registers) */
#define MMC_QUIRK_BROKEN_CLK_GATING (1<<3) /* clock gating the sdio bus
will make card fail */
+#define MMC_QUIRK_NONSTD_FUNC_IF (1<<4)/* SDIO card has nonstd
function interfaces */
unsigned interase_size; /* erase size in sectors */
unsigned interase_shift;/* if erase unit is power 2 */
@@ -180,6 +181,11 @@ static inline int mmc_blksz_for_byte_mode(const struct
mmc_card *c)
return c->quirks & MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
}
+static inline int mmc_card_nonstd_func_interface(const struct mmc_card *c)
+{
+ return c->quirks & MMC_QUIRK_NONSTD_FUNC_IF;
+}
+
#define mmc_card_name(c) ((c)->cid.prod_name)
#define mmc_card_id(c) (dev_name(&(c)->dev))
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] mmc: do not switch to 1-bit mode if not required
6b5eda36 followed SDIO spec part E1 section 8, which states that
in case SDIO interrupts are being used to wake up a suspended host,
then it is required to switch to 1-bit mode before stopping the clock.
Before switching to 1-bit mode (or back to 4-bit mode on resume),
make sure that SDIO interrupts are really being used to wake the host.
This is helpful for devices which have an external irq line (e.g.
wl1271), and do not use SDIO interrupts to wake up the host.
In this case, switching to 1-bit mode (and back to 4-bit mode on resume)
is not necessary.
Reported-by: Eliad Peller
Signed-off-by: Ohad Ben-Cohen
---
drivers/mmc/core/sdio.c |4 ++--
include/linux/mmc/host.h |4
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 0f7d436..4221670 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -625,7 +625,7 @@ static int mmc_sdio_suspend(struct mmc_host *host)
}
}
- if (!err && mmc_card_keep_power(host)) {
+ if (!err && mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
mmc_claim_host(host);
sdio_disable_wide(host->card);
mmc_release_host(host);
@@ -648,7 +648,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
err = mmc_sdio_init_card(host, host->ocr, host->card,
mmc_card_keep_power(host));
- else if (mmc_card_keep_power(host)) {
+ else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
/* We may have switched to 1-bit mode during suspend */
err = sdio_enable_4bit_bus(host->card);
if (err > 0) {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index cccb5cf..4f705eb 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -326,5 +326,9 @@ static inline int mmc_card_keep_power(struct mmc_host *host)
return host->pm_flags & MMC_PM_KEEP_POWER;
}
+static inline int mmc_card_wake_sdio_irq(struct mmc_host *host)
+{
+ return host->pm_flags & MMC_PM_WAKE_SDIO_IRQ;
+}
#endif
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] mmc: mmc_card_keep_power cleanups
mmc_card_is_powered_resumed is a mouthful; instead, simply use
mmc_card_keep_power, which also better explains the purpose of
the macro.
Employ mmc_card_keep_power() where possible.
Signed-off-by: Ohad Ben-Cohen
---
Chris, these were already posted previously, i'm just pinging with a rebased
version, thanks!
drivers/mmc/core/core.c |4 ++--
drivers/mmc/core/sdio.c | 10 +-
include/linux/mmc/host.h |2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 1f453ac..c2350e4 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1746,7 +1746,7 @@ int mmc_suspend_host(struct mmc_host *host)
}
mmc_bus_put(host);
- if (!err && !(host->pm_flags & MMC_PM_KEEP_POWER))
+ if (!err && !mmc_card_keep_power(host))
mmc_power_off(host);
return err;
@@ -1764,7 +1764,7 @@ int mmc_resume_host(struct mmc_host *host)
mmc_bus_get(host);
if (host->bus_ops && !host->bus_dead) {
- if (!(host->pm_flags & MMC_PM_KEEP_POWER)) {
+ if (!mmc_card_keep_power(host)) {
mmc_power_up(host);
mmc_select_voltage(host, host->ocr);
/*
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index db0f0b4..0f7d436 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -625,7 +625,7 @@ static int mmc_sdio_suspend(struct mmc_host *host)
}
}
- if (!err && host->pm_flags & MMC_PM_KEEP_POWER) {
+ if (!err && mmc_card_keep_power(host)) {
mmc_claim_host(host);
sdio_disable_wide(host->card);
mmc_release_host(host);
@@ -645,10 +645,10 @@ static int mmc_sdio_resume(struct mmc_host *host)
mmc_claim_host(host);
/* No need to reinitialize powered-resumed nonremovable cards */
- if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
+ if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
err = mmc_sdio_init_card(host, host->ocr, host->card,
-(host->pm_flags & MMC_PM_KEEP_POWER));
- else if (mmc_card_is_powered_resumed(host)) {
+ mmc_card_keep_power(host));
+ else if (mmc_card_keep_power(host)) {
/* We may have switched to 1-bit mode during suspend */
err = sdio_enable_4bit_bus(host->card);
if (err > 0) {
@@ -691,7 +691,7 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
mmc_claim_host(host);
ret = mmc_sdio_init_card(host, host->ocr, host->card,
- (host->pm_flags & MMC_PM_KEEP_POWER));
+ mmc_card_keep_power(host));
if (!ret && host->sdio_irqs)
mmc_signal_sdio_irq(host);
mmc_release_host(host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index bcb793e..cccb5cf 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -321,7 +321,7 @@ static inline int mmc_card_is_removable(struct mmc_host
*host)
return !(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable;
}
-static inline int mmc_card_is_powered_resumed(struct mmc_host *host)
+static inline int mmc_card_keep_power(struct mmc_host *host)
{
return host->pm_flags & MMC_PM_KEEP_POWER;
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] MMC: fix mmc_pm_notify bus_ops->remove deadlock.
On Mon, Apr 4, 2011 at 4:27 PM, Andrei Warkentin wrote: > On Mon, Apr 4, 2011 at 9:00 AM, Andrei Warkentin wrote: >> This resolves the deadlock issue with suspend. There is no >> need to claim host before the remove op. >> >> Signed-off-by: Andrei Warkentin ... > I think this means we can take > 1c8cf9c997a4a6b36e907c7ede5f048aeaab1644 out (mmc: sdio: fix SDIO > suspend/resume regression). I don't see how is this related ? 1c8cf9c997a4a6b36e907c7ede5f048aeaab1644 makes sure the -ENOSYS semantics of mmc_sdio_suspend() are followed. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 2/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode
On Fri, Mar 25, 2011 at 7:08 PM, Wilson Loi wrote: > Therefore, there is a case that we power save for the host CPU and let the > firmware handle the wlan state. > WLAN card will wake up the host by SDIO IRQ or GPIO if there is a new > incoming packet later. Then you're talking about keeping the card powered during system suspend, which is already supported (as mentioned by Nicolas and Chris), but your patches really changed SDIO's runtime PM path (which is not needed since anyway the driver fully controls it). -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 2/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode
On Fri, Mar 25, 2011 at 6:00 AM, Wilson Loi wrote: > Another patch file > > > diff -ruN a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c Please follow Documentation/SubmittingPatches when submitting patches, or at least just add -p to diff, otherwise it's a bit difficult to follow. thanks. > mmc_bus_put(host); > > - mmc_power_off(host); > + if (!ret && !(host->pm_flags & MMC_PM_KEEP_POWER)) > + mmc_power_off(host); ... > > - mmc_power_up(host); > + if (!(host->pm_flags & MMC_PM_KEEP_POWER)) > + mmc_power_up(host); > ret = host->bus_ops->power_restore(host); I have the same question here: if you driver doesn't want its card to be powered down, why does it invoke (or let runtime PM invoke on its behalf) mmc_power_save_host() in the first place ? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode
Hello Wilson,
On Fri, Mar 25, 2011 at 5:58 AM, Wilson Loi wrote:
> The current power_restore handler only support cut off mode.
> It is better to support keep power mode too.
This doesn't make much sense really;
mmc_power_save_host() and mmc_power_restore_host() are invoked by
runtime PM when it is time to power off/on the card.
If your driver doesn't want the power to go, it should just maintain a
positive runtime PM usage_count (e.g. in case it's an SDIO driver,
don't call pm_runtime_put{_sync} after being probed).
> static const struct mmc_bus_ops mmc_sdio_ops = {
> .remove = mmc_sdio_remove,
> .detect = mmc_sdio_detect,
> .suspend = mmc_sdio_suspend,
> .resume = mmc_sdio_resume,
> - .power_restore = mmc_sdio_power_restore,
> + .power_save = mmc_sdio_suspend,
> + .power_restore = mmc_sdio_resume,
This can break SDIO runtime PM.
While -ENOSYS is a valid return value for mmc_sdio_suspend (it tells
mmc_suspend_host to remove the entire MMC card on system suspend), it
will be treated as an error by runtime PM.
In addition, this can really yield unexpected results, since drivers
do not expect their suspend/resume handlers to be called during
runtime PM transitions.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: fix CONFIG_MMC_UNSAFE_RESUME regression
On Tue, Mar 8, 2011 at 11:56 PM, Chris Ball wrote: > I have removed the stable@ tag, because the patch causing this > regression was introduced in .38-rc1 -- it has not been part of > any released kernel. So, as long as we fix it before .38 releases > in a week, there's no need to involve stable@. Thanks, Chris. I misread git describe; v2.6.37-3748-g30201e7 is of course post-2.6.37 material. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: fix CONFIG_MMC_UNSAFE_RESUME regression
30201e7 allowed skipping detection of nonremovable cards on mmc_rescan(). The intention was to only skip detection of hardwired cards that cannot be removed, so make sure this is indeed the case by directly checking for (lack of) MMC_CAP_NONREMOVABLE, instead of using mmc_card_is_removable(), which is overloaded with CONFIG_MMC_UNSAFE_RESUME semantics. Reported-and-tested-by: Dmitry Shmidt Reported-and-tested-by: Maxim Levitsky Signed-off-by: Ohad Ben-Cohen Cc: --- Based on linux-2.6.git master, as this should probably go to Linus. drivers/mmc/core/core.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 6625c05..150b5f3 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1529,7 +1529,7 @@ void mmc_rescan(struct work_struct *work) * still present */ if (host->bus_ops && host->bus_ops->detect && !host->bus_dead - && mmc_card_is_removable(host)) + && !(host->caps & MMC_CAP_NONREMOVABLE)) host->bus_ops->detect(host); /* -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression
On Tue, Mar 8, 2011 at 7:02 PM, Maxim Levitsky wrote: >> Maxim, does this work for you too ? > > Yep, just tested. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression
On Mon, Mar 7, 2011 at 9:01 PM, Dmitry Shmidt wrote: > On Sat, Mar 5, 2011 at 7:03 AM, Ohad Ben-Cohen wrote: >> On Fri, Mar 4, 2011 at 4:22 AM, Maxim Levitsky >> wrote: >>> This patch breaks the CONFIG_MMC_UNSAFE_RESUME >> >> Does the below work for you (untested) ? >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 6625c05..150b5f3 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -1529,7 +1529,7 @@ void mmc_rescan(struct work_struct *work) >> * still present >> */ >> if (host->bus_ops && host->bus_ops->detect && !host->bus_dead >> - && mmc_card_is_removable(host)) >> + && !(host->caps & MMC_CAP_NONREMOVABLE)) >> host->bus_ops->detect(host); >> >> /* >> > This works for me, Thanks. Maxim, does this work for you too ? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression
On Fri, Mar 4, 2011 at 4:22 AM, Maxim Levitsky wrote: > On Mon, 2010-12-20 at 00:11 +0200, Ohad Ben-Cohen wrote: >> Hi Chris, >> >> On Fri, Dec 17, 2010 at 2:51 AM, Chris Ball wrote: >> ... >> > Thanks, pushed to mmc-next with trivial cleanup as below: >> >> Looks good, thanks. >> >> I just briefly tested mmc-next and it looks fine. > > This patch breaks the CONFIG_MMC_UNSAFE_RESUME Does the below work for you (untested) ? diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 6625c05..150b5f3 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1529,7 +1529,7 @@ void mmc_rescan(struct work_struct *work) * still present */ if (host->bus_ops && host->bus_ops->detect && !host->bus_dead - && mmc_card_is_removable(host)) + && !(host->caps & MMC_CAP_NONREMOVABLE)) host->bus_ops->detect(host); /* -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] omap: panda: wlan board muxing
Hi Pandu, On Mon, Feb 14, 2011 at 9:19 AM, wrote: > + /* WLAN IRQ - GPIO 53 */ > + OMAP4_MUX(GPMC_NCS3, OMAP_MUX_MODE3 | OMAP_PIN_INPUT_PULLUP), Actually we don't need to pull up the IRQ line - it is always held by the 1271 device, and there's no need for the host to pull it up (wasting some power while doing it). We have that on the ZOOM, but that's not necessary, and will be fixed there, too. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Setting MMC_CAP_POWER_OFF_CARD on mmc2 leads to filesystem problems on mmc1
On Wed, Feb 2, 2011 at 2:11 AM, Tony Lindgren wrote: > * Ohad Ben-Cohen [110122 07:47]: >> On Sat, Jan 22, 2011 at 5:30 PM, Koen Kooi >> wrote: >> > That was indeed the problem, not I get: >> > >> > [ 35.417053] wl1271: firmware booted (Rev 6.1.0.0.343) >> > >> > and >> > >> > root@beagleboard-xm-next:~# ifconfig wlan0 hw eth 00:11:22:33:44:55 >> > root@beagleboard-xm-next:~# iwlist wlan0 scanning | grep ESSID | wc -l >> > 13 >> > >> > That get's me a lot further! >> >> Cool ! > > Hmm, do we need to still patch something for the -rc cycle for this? I don't think so; the problem wasn't in the existing mainline code, but in the new functionality developed (adding support for the beagletoy expansion board), so this is not -rc material. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] sdio: skip initialization on powered resume
On Tue, Jan 25, 2011 at 9:11 AM, Ohad Ben-Cohen wrote: > One thing we can do is letting drivers register a wake-up function > within the SDIO core, just before suspending. Then, upon resume, if > such a wake-up function was registered, SDIO core would invoke it just > before it tries to read the CIS. I guess that would be non-trivial, since at that point the driver is not resumed yet, and you might need its facilities to receive some wake-up ACK from the chip before continuing. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] sdio: skip initialization on powered resume
Hello Zhangfei, On Tue, Jan 25, 2011 at 5:10 AM, zhangfei gao wrote: > Could we remove mmc_card_is_removable(host) condition, the skip is not > related with whether card is removable or not, do you think so? But, as Nicolas pointed out before, what if the card is removed (or replaced) while the system was suspended ? It seems that this won't be detected, and your driver, once resumed, will assume the card is still in place and will just fail. Are you ok with it ? Can you please explain what's required to wake up mrvl8787 from this deep sleep ? One thing we can do is letting drivers register a wake-up function within the SDIO core, just before suspending. Then, upon resume, if such a wake-up function was registered, SDIO core would invoke it just before it tries to read the CIS. This way your card stays in deep sleep while the system is suspended, and when the system wakes up, SDIO core has a chance to make sure the card was not removed/replaced. If the card is replaced, the wake-up function will probably just fail, and then SDIO core will have to reinitialize that new card. I'm not sure that's ideal, but those errors would happen anyway if we just skip the reinitialization and let the driver assume the card wasn't replaced. What do you think ? Would that work for you ? Btw we have the same thing with the wl1271. Since it's mostly nonremovable, we're ok with 3cfc33a, but that may change. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
On Sun, Jan 23, 2011 at 7:20 AM, Nicolas Pitre wrote: > First case: you want to stop the clock _to_ the _card_ when not talking > to it. That has nothing to do with power saving performed within the > host controller. This is for reducing power consumption by the _card_ > (and possibly by the clock generator). The runtime PM stuff has no > business here as the decision to gate the clock on the card require > MMC/SD protocol knowledge. But why can't it be implemented using the runtime PM framework ? It's just plumbing; the decision and knowledge stays at the MMC core. It's possible to do it without making any compromises of use cases and requirements. And if something is missing in the runtime PM framework, it can be changed. It just needs someone who cares. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: add per device quirk placeholder
On Sun, Jan 9, 2011 at 6:26 PM, Pierre Tardy wrote: > From: Pierre Tardy > > Some cards have quirks valid for every platforms > using current platform quirk hooks leads to a lot > of code and debug duplication. > > So we inspire a bit from what exists in PCI subsystem > and do our own per vendorid/deviceid quirk > We still drop the complexity of the pci quirk system > (with special section tables, and so on) > That can be added later if needed. I like this. It's exactly what we need for the wl12xx, so thanks for pushing this. I'm not sure about the necessity of the function hooks though - it just looks like complexity that we don't really need, and I would rather have this framework directly set the required quirks like USB is doing. I do see the benefit it provides you with the gating framework and the SDIO set/unset issue, and I don't have a cleaner alternative approach, so I guess we can live with it. Acked-by: Ohad Ben-Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] sdio: skip initialization on powered resume
On Fri, Jan 21, 2011 at 11:07 AM, zhangfei gao wrote: > Do you have any updated patch to skip mmc_sdio_init_card in resume back. > We need such patch in enable host sleep feature for mrvl8787. Is mrvl8787 a removable card ? I'm asking because we already skip mmc_sdio_init_card() for powered-resumed nonremovable cards (check out commit 3cfc33a "mmc: sdio: don't reinitialize nonremovable powered-resumed cards"). I'm not familiar with marvell's cards, but I do remember a thread mentioning they have dedicated reset GPIOs, and that may suggest they are nonremovables. If that's the case, simply setting MMC_CAP_NONREMOVABLE on the relevant slot should do the trick for you. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
On Thu, Jan 20, 2011 at 7:08 AM, Chris Ball wrote: > On Wed, Jan 19, 2011 at 09:45:57AM +, Tardy, Pierre wrote: >> Chris, >> (Sorry for top posting) >> Seems we have a intel intern disagreement. >> >> Could we have maintainer opinion on this ? > > Linus W and Ohad, any input here? Thanks, Personally I prefer the runtime PM approach. The MMC aggressive clock gating framework is great, but it ended up duplicating plumbing that already existed in the runtime PM framework (workqueue, locking, usage refcount, "get/put" API). Moving to runtime PM should reduce a fair amount of code from the MMC core, make the code more readable and easier to maintain, and let us take advantage of standard tools and knobs that are based on runtime PM's sysfs entries. But surely we don't want two frameworks doing the same; that would be too messy. And we want to be sure that any runtime PM approach would satisfy everyone's requirement. Chuanxiao, do you think you can come up with a runtime PM-based patch that provides the aggressive clock gating framework's functionality ? If functionality and requirements are preserved, I don't think anyone would complain about moving to an established and well-maintained kernel subsystem. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Setting MMC_CAP_POWER_OFF_CARD on mmc2 leads to filesystem problems on mmc1
On Sat, Jan 22, 2011 at 5:30 PM, Koen Kooi wrote: > That was indeed the problem, not I get: > > [ 35.417053] wl1271: firmware booted (Rev 6.1.0.0.343) > > and > > root@beagleboard-xm-next:~# ifconfig wlan0 hw eth 00:11:22:33:44:55 > root@beagleboard-xm-next:~# iwlist wlan0 scanning | grep ESSID | wc -l > 13 > > That get's me a lot further! Cool ! -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Setting MMC_CAP_POWER_OFF_CARD on mmc2 leads to filesystem problems on mmc1
Hi Koen,
On Fri, Jan 21, 2011 at 4:49 PM, Luciano Coelho wrote:
>> My patch basically does:
>>
>> --- a/arch/arm/mach-omap2/board-omap3beagle.c
>> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
>> @@ -270,7 +270,7 @@ static struct omap2_hsmmc_info mmc[] = {
>> {
>> .name = "wl1271",
>> .mmc = 2,
>> - .caps = MMC_CAP_4_BIT_DATA,
>> + .caps = MMC_CAP_4_BIT_DATA |
>> MMC_CAP_POWER_OFF_CARD,
>>
>> And does NOT touch mmc1. But after adding MMC_CAP_POWER_OFF_CARD I get tons
>> of:
Hmm. The snippet above looks different in your patch.
It seems that you're adding a new mmcbbt array, along with the
existing mmc one, but still using unchanged board-omap3beagle code,
and I suspect you have some unhealthy mmc/mmcbbt references.
Particularly, look at this:
@@ -384,7 +445,14 @@ static int beagle_twl_gpio_setup(struct device *dev,
}
/* gpio + 0 is "mmc0_cd" (input/IRQ) */
mmc[0].gpio_cd = gpio + 0;
+#if defined(CONFIG_WL1271) || defined(CONFIG_WL1271_MODULE)
+ if(!strcmp(expansionboard_name, "fixme-beagletoy"))
+ omap2_hsmmc_init(mmcbbt);
+ else
+ omap2_hsmmc_init(mmc);
+#else
omap2_hsmmc_init(mmc);
+#endif
/* link regulators to MMC adapters */
beagle_vmmc1_supply.dev = mmc[0].dev;
When WL1271 is configured, and you have your "fixme-beagletoy"
expansionboard around, you're only initializing mmcbbt, but still
using mmc for regulators references.
Care to check if fixing that makes your issues go away ?
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] mmc: add MMC_QUIRK_DISABLE_CD
On Wed, Jan 5, 2011 at 1:04 AM, David Vrabel wrote: > This would cause regressions on boards that aren't properly designed and > have omitted the pull-up. Are there boards like this? I'm not sure there are. All hosts shall provide pull-up resistors on all data lines DAT[3:0] as described in section 6 of the SD physical specification. A board that doesn't pull up DAT3, and instead relies on the card's internal pull-up, sound very quirky to me. So we might really be able to unconditionally disable the card's DAT3 internal pull-up. We can do that for cards that specifically want it, or, we can even consider a bolder approach where we do that for all the nonremovable SDIO cards (which therefore don't need the card detection functionality anyway). What do you think ? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] mmc: add MMC_QUIRK_NONSTD_FUNC_IF
Hi Pierre,
On Wed, Jan 5, 2011 at 2:28 PM, Pierre Tardy wrote:
> Well... TI is selling quirky hw, would make sense that you handle this. :-)
Heh I'd happily take over - just say the word :)
> Subject: [PATCH] mmc: add per device quirk placeholder
>
> Some cards have quirks valid for every platforms
> using current platform quirk hooks leads to a lot
> of code and debug duplication.
>
> So we inspire a bit from what exists in PCI subsystem
> and do out own per vendorid/deviceid quirk
> We still drop the complexity of the pci quirk system
> (with special section tables, and so on)
> That can be added later if needed.
I like the idea. It would allow us to handle card quirks without
involving board code (and obviously we can't rely on drivers for that
because that may be too late).
> +struct mmc_fixup {
> + u16 vendor, device; /* You can use SDIO_ANY_ID here of course */
> + void (*hook)(struct mmc_card *card);
Do we really need to hook up code with quirky cards ? why not just
directly indicate the exact MMC_QUIRK_* instead (just like USB is
doing) ? that may simplify the whole thing.
So instead of the ->hook() function pointer, we can just put a uint
quirks member,
> +};
> +
> +static const struct mmc_fixup mmc_fixup_methods[] = {
> + { 0 }
Fill it up here,
> +};
> +
> +void mmc_fixup_device(struct mmc_card *card)
> +{
> + const struct mmc_fixup *f;
> +
> + for (f = mmc_fixup_methods; f->hook; f++) {
> + if ((f->vendor == card->cis.vendor || f->vendor == (u16)
> SDIO_ANY_ID) &&
> + (f->device == card->cis.device || f->device == (u16)
> SDIO_ANY_ID)) {
> + dev_dbg(&card->dev, "calling %pF\n", f->hook);
> + f->hook(card);
And then here instead of calling a dedicated ->hook() function, we can
just do something like this:
card->quirks |= f->quirks;
This way we don't need to add a function for every quirky card.
And if we will happen to need it in the future we can always bring it
back in, but right now we really only need to specify the relevant
QUIRK(s).
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
