Re: UHS-I voltage switching on OLPC XO-1.75

2012-11-25 Thread Daniel Drake
On Tue, Nov 20, 2012 at 2:53 AM, Johan Rudholm jrudh...@gmail.com wrote:
 On the other hand, we may have a good basis to add a quirk, triggered
 by the device tree, for when the hardware physically does not have
 1.8v capabilities.

 This also seems proper, if we know we can't get 1.8V, then we
 shouldn't even try for it. In this way the detection will also be
 faster (no 10 retries).

I've gone for this approach in the patches just posted.
sdhci: add quirk for lack of 1.8v support
mmc: add no-1-8-v device tree flag

 If this is the case, the driver could have another heuristic: if there
 is no vmmc regulator, there is no way of cutting the card power,
 therefore we could avoid even trying 1.8v on the basis that we know we
 can't recover if things go wrong.

 So the driver could for instance drop the MMC_CAP_UHS_DDR50 cap if
 there is no way to power cycle the card? I think that sounds
 reasonable and according to spec, although also a little bit hard
 since there probably are cards out there that never require a power
 cycle to perform a proper voltage switch?

Thats true, and actually, after some more investigation it doesn't
seem like we have a solid basis to add such behaviour. It looks like
whether or not the voltage supply is supplied by the same chip that
provides the SDHCI interface, or whether it is a purely external
factor, varies from board to board. So we can't assume a lack of
regulator means a lack of ability to switch voltages.

Thanks
Daniel
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-20 Thread Johan Rudholm
Hi!

2012/11/18 Daniel Drake d...@laptop.org:
 On Fri, Nov 2, 2012 at 8:35 AM, Johan Rudholm jrudh...@gmail.com wrote:
 Good question. I’d guess that mmc_power_off/up does not work as
 expected here, that the card is not at all power cycled.

 Before going further on the find a way to quirk it route, there is
 something else we could look into.

 According to our hardware engineers, the external SD card power has
 been always-on until now. It is actually controlled by our embedded
 controller, separate from the CPU.

 In a test firmware, I can now control the SD card power via our OLPC
 EC interface, I call into that from mmc_power_up and mmc_power_down.
 And, with your hacky patch to make the voltage switch failure
 detection work, this fixes it: it tries 10 times at 1.8v then falls
 back to 3.3 successfully. No more problems with the power cycle.

Ah, great! Then we know what the issue was at least. Then I guess that
this code did not work with your driver:

host-ios.clock = 0;
mmc_set_ios(host);

so with my original patch ([RFC/PATCH,v2] mmc: core: Fixup signal
voltage switch), the clock was actually never stopped during the
signal voltage switch? I guess this will need some further
investigation also.

 So we have the option of fixing it that way: if we can fix the voltage
 switch failure detection, we could implement a custom vmmc regulator
 driver that uses our EC interface to enable and disable the SD power
 appropriately, solving our ability to power cycle.

Being able to power cycle the SD-card might also come in handy in
other situations, perhaps if a poor SD-card misbehaves in some way?
SD-cards have no reset cord like eMMCs, so being able to power cycle
the card feels like a good thing.

 On the other hand, we may have a good basis to add a quirk, triggered
 by the device tree, for when the hardware physically does not have
 1.8v capabilities.

This also seems proper, if we know we can't get 1.8V, then we
shouldn't even try for it. In this way the detection will also be
faster (no 10 retries).

 I'm also curious if there is a 3rd option. It seems like in the case
 of our SoC, the SoC design mandates that the SD card power is separate
 from the SDHCI interface - requiring either a GPIO or some other
 mechanism (e.g. OLPC EC) to be able to control it.

 I'm wondering if this is the same for all sdhci-pxa devices. And the
 same for all sdhci devices? Maybe the SDHCI specs would help here, but
 I guess they aren't public.

 If this is the case, the driver could have another heuristic: if there
 is no vmmc regulator, there is no way of cutting the card power,
 therefore we could avoid even trying 1.8v on the basis that we know we
 can't recover if things go wrong.

So the driver could for instance drop the MMC_CAP_UHS_DDR50 cap if
there is no way to power cycle the card? I think that sounds
reasonable and according to spec, although also a little bit hard
since there probably are cards out there that never require a power
cycle to perform a proper voltage switch?

Kind regards, Johan
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-18 Thread Daniel Drake
On Fri, Nov 2, 2012 at 8:35 AM, Johan Rudholm jrudh...@gmail.com wrote:
 Good question. I’d guess that mmc_power_off/up does not work as
 expected here, that the card is not at all power cycled.

Before going further on the find a way to quirk it route, there is
something else we could look into.

According to our hardware engineers, the external SD card power has
been always-on until now. It is actually controlled by our embedded
controller, separate from the CPU.

In a test firmware, I can now control the SD card power via our OLPC
EC interface, I call into that from mmc_power_up and mmc_power_down.
And, with your hacky patch to make the voltage switch failure
detection work, this fixes it: it tries 10 times at 1.8v then falls
back to 3.3 successfully. No more problems with the power cycle.

So we have the option of fixing it that way: if we can fix the voltage
switch failure detection, we could implement a custom vmmc regulator
driver that uses our EC interface to enable and disable the SD power
appropriately, solving our ability to power cycle.

On the other hand, we may have a good basis to add a quirk, triggered
by the device tree, for when the hardware physically does not have
1.8v capabilities.

I'm also curious if there is a 3rd option. It seems like in the case
of our SoC, the SoC design mandates that the SD card power is separate
from the SDHCI interface - requiring either a GPIO or some other
mechanism (e.g. OLPC EC) to be able to control it.

I'm wondering if this is the same for all sdhci-pxa devices. And the
same for all sdhci devices? Maybe the SDHCI specs would help here, but
I guess they aren't public.

If this is the case, the driver could have another heuristic: if there
is no vmmc regulator, there is no way of cutting the card power,
therefore we could avoid even trying 1.8v on the basis that we know we
can't recover if things go wrong.

Similarly, for our next product we are looking at adding 1.8v
capabilities. However, it seems that again, the SoC design (or
something more fundamental?) requires that this power is controlled
via some external mechanism - we're planning to use a GPIO to switch
between 1.8v and 3.3v, which can be exposed as a regulator. So again,
would it be fair for sdhci-pxa or sdhci to drop the UHS-I support when
no regulator is present? Or am I over-generalizing?

Thanks,
Daniel
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-08 Thread Daniel Drake
On Fri, Nov 2, 2012 at 8:35 AM, Johan Rudholm jrudh...@gmail.com wrote:
 An initial question, would it be possible to solve this by disabling
 the UHS support in the host cap, or through a quirk? Maybe this kind
 of solution is not applicable in this scenario? I wonder this because
 I guess it will be very hard to know how a card will react if we first
 tell it that we're going to switch voltages and then don't, since this
 is out of spec.

That would definitely be possible.

I'm not convinced this is outside the spec - the spec talks heavily
about power cycling, which you would  think would handle such cases
(i.e. a full state reset). However, I think I have generated enough
evidence in my last mail to show that we are not able to fully power
cycle, giving further support for some kind of quirk.

 Good question. I’d guess that mmc_power_off/up does not work as
 expected here, that the card is not at all power cycled. But it seems
 that the power cycle code in sdhci_do_start_signal_voltage_switch
 works? What if we export a couple of debug functions from sdhci.c
 which allow us to power cycle and control the clock just like sdhci
 does, and call these functions from core.c and sd.c? If this works
 (and the failure is detected properly by the core layer, and 10
 retries are made etc), then we know that the problems most likely
 depend on how mmc_set_ios and mmc_power_off/up work with sdhci. I've
 attached a patch suggesting this, if you'd like to give it a try
 (warning: the patch has not been tested). The patch does not do
 anything about pm_runtime, but perhaps we don't need to worry about
 this here.

Your patch fixes the detection that the voltage switch has failed.

It doesn't fix the reset though - it still returns the -110 error
after trying to power cycle.

I mentioned in an earlier mail that we had this working in 3.0. I went
back to that configuration to investigate. Actually, it is maybe only
reliable 50% of the time - the other 50% we get the same error. When
it does work, here's what happens:

- mmc_rescan_try_freq is called to try the 400khz frequency
- in mmc_sd_get_cid(), rocr has value  c1ff8000, so we try the 1.8
switch, this fails
- we go back to 3.3v, then we receive the -110 timeout error immediately after
- mmc_rescan_try_freq is called to try the 300khz frequency
- the card is now alive for some reason
- in mmc_sd_get_cid(), rocr has value  c0ff8000, so we stick at 3.3V

Rather odd. It seems that 3.0 is more successful at power cycling the
card than 3.5, but it is far from reliable. It also appears that the
card I have here might have some logic to realise that 1.8V failed so
then it stops advertising the capability?

Anyway, with all the unreliability, unknowns, and power cycle
difficulty, I guess we should go for the quirk approach.
Do you have any suggestions for how this should be implemented? Should
I go for a new SDHCI quirk triggered by a new sdhci,disable-1-8v
device tree property?

Thanks
Daniel
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-08 Thread Daniel Drake
On Thu, Nov 8, 2012 at 12:48 PM, Philip Rakity prak...@nvidia.com wrote:
 This is already there.  Use broken caps and in the board code clear out the
 nits in caps2.

There is no board code; we're working with device tree. So I'm still
not exactly clear on the best way to work this.

Maybe something like this: sdhci,disable-1-8v property in the device
tree, would trigger the sdhci-pxa2 code to:
1. sdhci_readl() to retrieve caps and caps1
2. Drop the 1.8v flag from caps1
3. Set the BROKEN_CAPS quirk

It would work but I'm not too fond of it, and I can't really think of
an approach I do like (except maybe having a specific 1.8v broken
sdhci quirk).

Daniel
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-08 Thread Daniel Drake
On Thu, Nov 8, 2012 at 1:03 PM, Philip Rakity prak...@nvidia.com wrote:
 Don't know enough about the device tree to help.  Originally I suggested a
 quirk simular to what you are suggesting but it was felt to be too specific
 since the bits in caps2 achieve the same effect.  Maybe your board specific
 code can check if the regulator is capable of switching and can be turned on
 and off.  Not sure how to do the later.  Checking the usage count does not
 help.  You want exclusive access.  Mark Brown might know how to do this else
 perhaps the regulator framework needs extending.

We don't have a regulator in this case. If we did we'd probably be in
a different situation.

An option closer to overriding caps/caps1 in the board code is to put
the caps/caps1 overrides in the device tree directly. Doesn't sound
pleasing either, and I'm not sure that it makes sense; really the
hardware's reporting of its own capabilities is not wrong, it just
does not consider the capabilities of the surrounding components on
the motherboard. So a quirk feels more natural to me...

Daniel
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-08 Thread Philip Rakity

On Nov 8, 2012, at 7:03 PM, Philip Rakity prak...@nvidia.com wrote:

 Don't know enough about the device tree to help.  Originally I suggested a 
 quirk simular to what you are suggesting but it was felt to be too specific 
 since the bits in caps2 achieve the same effect.  Maybe your board specific 
 code can check if the regulator is capable of switching and can be turned on 
 and off.  Not sure how to do the later.  Checking the usage count does not 
 help.  You want exclusive access.  Mark Brown might know how to do this else 
 perhaps the regulator framework needs extending.

upon reflection you can configure the regulator supply with the name of the 
specific device.  That should make it exclusive to the mmc subsystem.  In the 
general case I do not know how one can test for this (in say sdhci.c).
 
 - Reply message -
 From: Daniel Drake d...@laptop.org
 To: Philip Rakity prak...@nvidia.com
 Cc: Johan Rudholm jrudh...@gmail.com, Chris Ball c...@laptop.org, 
 linux-mmc@vger.kernel.org linux-mmc@vger.kernel.org
 Subject: UHS-I voltage switching on OLPC XO-1.75
 Date: Thu, Nov 8, 2012 19:52
 
 
 
 On Thu, Nov 8, 2012 at 12:48 PM, Philip Rakity prak...@nvidia.com wrote:
  This is already there.  Use broken caps and in the board code clear out the
  nits in caps2.
 
 There is no board code; we're working with device tree. So I'm still
 not exactly clear on the best way to work this.
 
 Maybe something like this: sdhci,disable-1-8v property in the device
 tree, would trigger the sdhci-pxa2 code to:
 1. sdhci_readl() to retrieve caps and caps1
 2. Drop the 1.8v flag from caps1
 3. Set the BROKEN_CAPS quirk
 
 It would work but I'm not too fond of it, and I can't really think of
 an approach I do like (except maybe having a specific 1.8v broken
 sdhci quirk).
 
 Daniel

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-05 Thread Philip Rakity

Hi Daneil, Chris,

I reviewed kevin's patch in September which fixes this issue.  Chris -- can we 
pull it into mmc-next ?  This patch is okay as a standalone change.

From: Philip Rakity prakity at marvell.com
Subject: Re: [PATCH v2 5/8] mmc: sdhci: fix null return check of regulator_get
Newsgroups: gmane.linux.kernel.mmc
Date: 2012-09-24 15:02:34 GMT (5 weeks, 6 days, 21 hours and 44 minutes ago)

Philip

On Oct 30, 2012, at 9:11 PM, Daniel Drake d...@laptop.org wrote:

 Hi,
 
 Following on from the recent thread
  [PATCH 2/2] mmc: core: Power cycle card on voltage switch fail
 I have tried to get a better grip on the problems being faced.
 
 Test setup:
 OLPC XO-1.75 laptop, based on Marvell MMP2 ARM SoC (includes sdhci-pxa
 interface)
 32GB Sandisk Ultra SD card being inserted into external SD slot
 
 The SoC apparently has support for 1.8V, but we physically do not have
 the right power lines wired on the motherboard, so we need to detect
 the 1.8V failure and go back to 3.3V mode.
 
 Before patching anything, inserting this card results in:
  sdhci: Switching to 1.8V signalling voltage failed, retrying with
 S18R set to 0
  mmc0: error -110 whilst initialising SD card
 
 Now, working on Linux-3.5.4, I add these patches:
 
mmc: core: reset sigal voltage on power up
[RFC/PATCH,v2] mmc: core: Fixup signal voltage switch
regulator: add missing defintion regulator_is_supported_voltage
mmc: sdhci: Add regulator support for vccq (voltage regualor)
mmc: sdhci: Let core handle UHS switch failure
 
 I also tweaked the sdhci code so that UHS-I flags are not
 unconditionally disabled by the vccq commit (as explained in sdhci
 vccq regulator support drops UHS-I flags).
 
 
 Under this setup, the first problem encountered is that the system no
 longer identifies the fact that the 1.8V voltage failed. Things are
 not happy afterwards. The card_busy check in sdhci is saying that the
 card is not busy.
 Full kernel logs: http://dev.laptop.org/~dsd/20121030/mmc1.log
 
 I tracked down what causes this regression. In order to get the
 failure detection working again, I have to make the following change
 in mmc_set_signal_voltage():
if (cmd11) {
host-ios.clock = 0;
 -   mmc_set_ios(host);
 +   //mmc_set_ios(host);
}
 (i.e. don't mess with ios before asking sdhci to do the voltage switch)
 
 and at the top of sdhci_do_1_8v_signal_voltage_switch:
 +   /* Stop SDCLK */
 +   clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 +   clk = ~SDHCI_CLOCK_CARD_EN;
 +   sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 (i.e. add the equivalent clock disabling code here)
 
 
 With that change, I now get:
  mmc2: Signal voltage switch failed, power cycling card (retries = 10)
  mmc2: error -110 whilst initialising SD card
 
 The -110 error comes from mmc_send_app_op_cond(), but its worth noting
 that mmc_send_if_cond() (called immediately above) silently failed
 with -110 too. Neither of these failed the first time around before
 1.8V was tried.
 
 Full patch that I'm testing with: 
 http://dev.laptop.org/~dsd/20121030/mmc.patch
 
 So, now the question is: why is the card not responding to
 if_cond/app_op_cond after being powered down and up again? Is the
 power down/up sequence OK?
 I tried inserting the mmc_power_off, mmc_power_up, mmc_send_if_cond
 sequence into various points in the codepaths being discussed.
 Measuring success as the card responding to mmc_send_if_cond(0x30080)
 without error (0x30080 is a known good value from when before 1.8V is
 tried), I find that this reset sequence works just fine up until the
 point when CMD11 is run.
 CMD11 is sent and returns successfully without error, but from this
 point, running the reset sequence will fail (send_if_cond will fail
 with -110).
 
 Is this kind of test valid? Does this suggest that something is wrong
 with the host controller hardware? My assumption is that whatever
 state is modified by CMD11 should be erased here, and of course the
 hope is that mmc_power_off and mmc_power_up will do a full power cycle
 anyway. But whatever is happening, it looks like the effects of CMD11
 are persisting past the reset sequence and are breaking later
 communication.
 
 In the mail last week, I think we found some kind of configuration (on
 Linux-3.0) where this same setup successfully detected the 1.8V
 switching failure, and successfully switched back to 3.3V. I plan to
 go back and test that, maybe there is some kind of sequence that
 doesn't make the hardware die.
 
 Daniel
 --
 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

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information. 

Re: UHS-I voltage switching on OLPC XO-1.75

2012-11-05 Thread Philip Rakity

Hi Daneil, Chris,

I reviewed kevin's patch in September which fixes this issue.  Chris -- can we 
pull it into mmc-next ?  This patch is okay as a standalone change.

From: Philip Rakity prakity at marvell.com
Subject: Re: [PATCH v2 5/8] mmc: sdhci: fix null return check of regulator_get
Newsgroups: gmane.linux.kernel.mmc
Date: 2012-09-24 15:02:34 GMT (5 weeks, 6 days, 21 hours and 44 minutes ago)

Philip

On Oct 30, 2012, at 9:11 PM, Daniel Drake d...@laptop.org wrote:

 Hi,
 
 Following on from the recent thread
 [PATCH 2/2] mmc: core: Power cycle card on voltage switch fail
 I have tried to get a better grip on the problems being faced.
 
 Test setup:
 OLPC XO-1.75 laptop, based on Marvell MMP2 ARM SoC (includes sdhci-pxa
 interface)
 32GB Sandisk Ultra SD card being inserted into external SD slot
 
 The SoC apparently has support for 1.8V, but we physically do not have
 the right power lines wired on the motherboard, so we need to detect
 the 1.8V failure and go back to 3.3V mode.
 
 Before patching anything, inserting this card results in:
 sdhci: Switching to 1.8V signalling voltage failed, retrying with
 S18R set to 0
 mmc0: error -110 whilst initialising SD card
 
 Now, working on Linux-3.5.4, I add these patches:
 
  mmc: core: reset sigal voltage on power up
  [RFC/PATCH,v2] mmc: core: Fixup signal voltage switch
  regulator: add missing defintion regulator_is_supported_voltage
  mmc: sdhci: Add regulator support for vccq (voltage regualor)
  mmc: sdhci: Let core handle UHS switch failure
 
 I also tweaked the sdhci code so that UHS-I flags are not
 unconditionally disabled by the vccq commit (as explained in sdhci
 vccq regulator support drops UHS-I flags).
 
 
 Under this setup, the first problem encountered is that the system no
 longer identifies the fact that the 1.8V voltage failed. Things are
 not happy afterwards. The card_busy check in sdhci is saying that the
 card is not busy.
 Full kernel logs: http://dev.laptop.org/~dsd/20121030/mmc1.log
 
 I tracked down what causes this regression. In order to get the
 failure detection working again, I have to make the following change
 in mmc_set_signal_voltage():
  if (cmd11) {
  host-ios.clock = 0;
 -   mmc_set_ios(host);
 +   //mmc_set_ios(host);
  }
 (i.e. don't mess with ios before asking sdhci to do the voltage switch)
 
 and at the top of sdhci_do_1_8v_signal_voltage_switch:
 +   /* Stop SDCLK */
 +   clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 +   clk = ~SDHCI_CLOCK_CARD_EN;
 +   sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 (i.e. add the equivalent clock disabling code here)
 
 
 With that change, I now get:
 mmc2: Signal voltage switch failed, power cycling card (retries = 10)
 mmc2: error -110 whilst initialising SD card
 
 The -110 error comes from mmc_send_app_op_cond(), but its worth noting
 that mmc_send_if_cond() (called immediately above) silently failed
 with -110 too. Neither of these failed the first time around before
 1.8V was tried.
 
 Full patch that I'm testing with: 
 http://dev.laptop.org/~dsd/20121030/mmc.patch
 
 So, now the question is: why is the card not responding to
 if_cond/app_op_cond after being powered down and up again? Is the
 power down/up sequence OK?
 I tried inserting the mmc_power_off, mmc_power_up, mmc_send_if_cond
 sequence into various points in the codepaths being discussed.
 Measuring success as the card responding to mmc_send_if_cond(0x30080)
 without error (0x30080 is a known good value from when before 1.8V is
 tried), I find that this reset sequence works just fine up until the
 point when CMD11 is run.
 CMD11 is sent and returns successfully without error, but from this
 point, running the reset sequence will fail (send_if_cond will fail
 with -110).
 
 Is this kind of test valid? Does this suggest that something is wrong
 with the host controller hardware? My assumption is that whatever
 state is modified by CMD11 should be erased here, and of course the
 hope is that mmc_power_off and mmc_power_up will do a full power cycle
 anyway. But whatever is happening, it looks like the effects of CMD11
 are persisting past the reset sequence and are breaking later
 communication.
 
 In the mail last week, I think we found some kind of configuration (on
 Linux-3.0) where this same setup successfully detected the 1.8V
 switching failure, and successfully switched back to 3.3V. I plan to
 go back and test that, maybe there is some kind of sequence that
 doesn't make the hardware die.
 
 Daniel
 --
 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

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized 

Re: UHS-I voltage switching on OLPC XO-1.75

2012-11-05 Thread Chris Ball
Hi,

On Mon, Nov 05 2012, Philip Rakity wrote:
 Hi Daneil, Chris,

 I reviewed kevin's patch in September which fixes this issue.  Chris
 -- can we pull it into mmc-next ?  This patch is okay as a standalone
 change.

 From: Philip Rakity prakity at marvell.com
 Subject: Re: [PATCH v2 5/8] mmc: sdhci: fix null return check of regulator_get
 Newsgroups: gmane.linux.kernel.mmc
 Date: 2012-09-24 15:02:34 GMT (5 weeks, 6 days, 21 hours and 44 minutes ago)

It sounds like I misread this patch, then -- I understood it as only
affecting whether a pr_info() call is made, which would not fix a
voltage switching bug.  What am I missing?  (Dan, here's a copy of
Kevin's patch.)


From: Kevin Liu kl...@marvell.com

regulator_get() returns NULL when CONFIG_REGULATOR not defined,
which should not print out the warning.

Reviewed-by: Philip Rakity prak...@marvell.com
Signed-off-by: Bin Wang b...@marvell.com
Signed-off-by: Kevin Liu kl...@marvell.com
---
 drivers/mmc/host/sdhci.c |   18 --
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8e6a6f0..0104ae9 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2845,9 +2845,12 @@ int sdhci_add_host(struct sdhci_host *host)
 
/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
host-vqmmc = regulator_get(mmc_dev(mmc), vqmmc);
-   if (IS_ERR(host-vqmmc)) {
-   pr_info(%s: no vqmmc regulator found\n, mmc_hostname(mmc));
-   host-vqmmc = NULL;
+   if (IS_ERR_OR_NULL(host-vqmmc)) {
+   if (PTR_ERR(host-vqmmc)  0) {
+   pr_info(%s: no vqmmc regulator found\n,
+   mmc_hostname(mmc));
+   host-vqmmc = NULL;
+   }
}
else if (regulator_is_supported_voltage(host-vqmmc, 180, 180))
regulator_enable(host-vqmmc);
@@ -2903,9 +2906,12 @@ int sdhci_add_host(struct sdhci_host *host)
ocr_avail = 0;
 
host-vmmc = regulator_get(mmc_dev(mmc), vmmc);
-   if (IS_ERR(host-vmmc)) {
-   pr_info(%s: no vmmc regulator found\n, mmc_hostname(mmc));
-   host-vmmc = NULL;
+   if (IS_ERR_OR_NULL(host-vmmc)) {
+   if (PTR_ERR(host-vmmc)  0) {
+   pr_info(%s: no vmmc regulator found\n,
+   mmc_hostname(mmc));
+   host-vmmc = NULL;
+   }
} else
regulator_enable(host-vmmc);
 
-- 
Chris Ball   c...@laptop.org   http://printf.net/
One Laptop Per Child
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-05 Thread Philip Rakity


On Nov 5, 2012, at 1:11 PM, Chris Ball c...@laptop.org wrote:

 Hi,
 
 On Mon, Nov 05 2012, Philip Rakity wrote:
 Hi Daneil, Chris,
 
 I reviewed kevin's patch in September which fixes this issue.  Chris
 -- can we pull it into mmc-next ?  This patch is okay as a standalone
 change.

That was the original intent.

The question is what to do if no regulator.  regulator_get was returning NULL 
in Daniel;s case.
IS_ERR patch was not taken so UHS support was removed.  
The intent of the original code was to remove UHS support if there was a 
regulator but it could not support voltage switching.

Phlip

 
 From: Philip Rakity prakity at marvell.com
 Subject: Re: [PATCH v2 5/8] mmc: sdhci: fix null return check of 
 regulator_get
 Newsgroups: gmane.linux.kernel.mmc
 Date: 2012-09-24 15:02:34 GMT (5 weeks, 6 days, 21 hours and 44 minutes ago)
 
 It sounds like I misread this patch, then -- I understood it as only
 affecting whether a pr_info() call is made, which would not fix a
 voltage switching bug.  What am I missing?  (Dan, here's a copy of
 Kevin's patch.)
 
 
 From: Kevin Liu kl...@marvell.com
 
 regulator_get() returns NULL when CONFIG_REGULATOR not defined,
 which should not print out the warning.
 
 Reviewed-by: Philip Rakity prak...@marvell.com
 Signed-off-by: Bin Wang b...@marvell.com
 Signed-off-by: Kevin Liu kl...@marvell.com
 ---
 drivers/mmc/host/sdhci.c |   18 --
 1 files changed, 12 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
 index 8e6a6f0..0104ae9 100644
 --- a/drivers/mmc/host/sdhci.c
 +++ b/drivers/mmc/host/sdhci.c
 @@ -2845,9 +2845,12 @@ int sdhci_add_host(struct sdhci_host *host)
 
   /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
   host-vqmmc = regulator_get(mmc_dev(mmc), vqmmc);
 - if (IS_ERR(host-vqmmc)) {
 - pr_info(%s: no vqmmc regulator found\n, mmc_hostname(mmc));
 - host-vqmmc = NULL;
 + if (IS_ERR_OR_NULL(host-vqmmc)) {
 + if (PTR_ERR(host-vqmmc)  0) {
 + pr_info(%s: no vqmmc regulator found\n,
 + mmc_hostname(mmc));
 + host-vqmmc = NULL;
 + }
   }
   else if (regulator_is_supported_voltage(host-vqmmc, 180, 180))
   regulator_enable(host-vqmmc);
 @@ -2903,9 +2906,12 @@ int sdhci_add_host(struct sdhci_host *host)
   ocr_avail = 0;
 
   host-vmmc = regulator_get(mmc_dev(mmc), vmmc);
 - if (IS_ERR(host-vmmc)) {
 - pr_info(%s: no vmmc regulator found\n, mmc_hostname(mmc));
 - host-vmmc = NULL;
 + if (IS_ERR_OR_NULL(host-vmmc)) {
 + if (PTR_ERR(host-vmmc)  0) {
 + pr_info(%s: no vmmc regulator found\n,
 + mmc_hostname(mmc));
 + host-vmmc = NULL;
 + }
   } else
   regulator_enable(host-vmmc);
 
 -- 
 Chris Ball   c...@laptop.org   http://printf.net/
 One Laptop Per Child

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-05 Thread Chris Ball
Hi, adding Kevin,

On Mon, Nov 05 2012, Philip Rakity wrote:
 On Nov 5, 2012, at 1:11 PM, Chris Ball c...@laptop.org wrote:

 Hi,
 
 On Mon, Nov 05 2012, Philip Rakity wrote:
 Hi Daneil, Chris,
 
 I reviewed kevin's patch in September which fixes this issue.  Chris
 -- can we pull it into mmc-next ?  This patch is okay as a standalone
 change.

 That was the original intent.

 The question is what to do if no regulator.  regulator_get was
 returning NULL in Daniel;s case.
 IS_ERR patch was not taken so UHS support was removed.  
 The intent of the original code was to remove UHS support if there was
 a regulator but it could not support voltage switching.

So this patch does fix a real bug, other than the pr_info -- by affecting
whether we disable UHS in the else clause -- and the commit message
doesn't mention the existence of that real bug at all, and performs a
cosmetic fix for vmmc at the same time as a semantic fix for vqmmc.
That's terrible.

I'll merge Kevin's patch after adding a commit message that explains
what's actually going on.

Thanks,

- Chris.
-- 
Chris Ball   c...@laptop.org   http://printf.net/
One Laptop Per Child
--
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: UHS-I voltage switching on OLPC XO-1.75

2012-11-02 Thread Johan Rudholm
Hi Daniel,

2012/10/30 Daniel Drake d...@laptop.org:
 Hi,

 Following on from the recent thread
   [PATCH 2/2] mmc: core: Power cycle card on voltage switch fail
 I have tried to get a better grip on the problems being faced.

 Test setup:
 OLPC XO-1.75 laptop, based on Marvell MMP2 ARM SoC (includes sdhci-pxa
 interface)
 32GB Sandisk Ultra SD card being inserted into external SD slot

 The SoC apparently has support for 1.8V, but we physically do not have
 the right power lines wired on the motherboard, so we need to detect
 the 1.8V failure and go back to 3.3V mode.

An initial question, would it be possible to solve this by disabling
the UHS support in the host cap, or through a quirk? Maybe this kind
of solution is not applicable in this scenario? I wonder this because
I guess it will be very hard to know how a card will react if we first
tell it that we're going to switch voltages and then don't, since this
is out of spec.

 I tracked down what causes this regression. In order to get the
 failure detection working again, I have to make the following change
 in mmc_set_signal_voltage():
 if (cmd11) {
 host-ios.clock = 0;
 -   mmc_set_ios(host);
 +   //mmc_set_ios(host);
 }
 (i.e. don't mess with ios before asking sdhci to do the voltage switch)

 and at the top of sdhci_do_1_8v_signal_voltage_switch:
 +   /* Stop SDCLK */
 +   clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 +   clk = ~SDHCI_CLOCK_CARD_EN;
 +   sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 (i.e. add the equivalent clock disabling code here)

Ok, so this suggests that the SDHCI driver does not stop the clock
when we set ios.clock = 0 and call mmc_set_ios?

 With that change, I now get:
   mmc2: Signal voltage switch failed, power cycling card (retries = 10)
   mmc2: error -110 whilst initialising SD card

 The -110 error comes from mmc_send_app_op_cond(), but its worth noting
 that mmc_send_if_cond() (called immediately above) silently failed
 with -110 too. Neither of these failed the first time around before
 1.8V was tried.

 Full patch that I'm testing with: 
 http://dev.laptop.org/~dsd/20121030/mmc.patch

 So, now the question is: why is the card not responding to
 if_cond/app_op_cond after being powered down and up again? Is the
 power down/up sequence OK?

Good question. I’d guess that mmc_power_off/up does not work as
expected here, that the card is not at all power cycled. But it seems
that the power cycle code in sdhci_do_start_signal_voltage_switch
works? What if we export a couple of debug functions from sdhci.c
which allow us to power cycle and control the clock just like sdhci
does, and call these functions from core.c and sd.c? If this works
(and the failure is detected properly by the core layer, and 10
retries are made etc), then we know that the problems most likely
depend on how mmc_set_ios and mmc_power_off/up work with sdhci. I've
attached a patch suggesting this, if you'd like to give it a try
(warning: the patch has not been tested). The patch does not do
anything about pm_runtime, but perhaps we don't need to worry about
this here.

 I tried inserting the mmc_power_off, mmc_power_up, mmc_send_if_cond
 sequence into various points in the codepaths being discussed.
 Measuring success as the card responding to mmc_send_if_cond(0x30080)
 without error (0x30080 is a known good value from when before 1.8V is
 tried), I find that this reset sequence works just fine up until the
 point when CMD11 is run.
 CMD11 is sent and returns successfully without error, but from this
 point, running the reset sequence will fail (send_if_cond will fail
 with -110).

 Is this kind of test valid? Does this suggest that something is wrong
 with the host controller hardware? My assumption is that whatever
 state is modified by CMD11 should be erased here, and of course the
 hope is that mmc_power_off and mmc_power_up will do a full power cycle
 anyway. But whatever is happening, it looks like the effects of CMD11
 are persisting past the reset sequence and are breaking later
 communication.

I think that the power cycle has simply failed here, I find it hard to
believe that CMD11 has persistent effects :) If mmc_power_off/up
actually works, then perhaps the 1ms udelay is too small for this
case?

Kind regards, Johan


0001-Clock-and-power-control.patch
Description: Binary data


Re: UHS-I voltage switching on OLPC XO-1.75

2012-11-02 Thread Philip Rakity

On Nov 2, 2012, at 2:35 PM, Johan Rudholm jrudh...@gmail.com wrote:

 Hi Daniel,
 
 2012/10/30 Daniel Drake d...@laptop.org:
 Hi,
 
 Following on from the recent thread
  [PATCH 2/2] mmc: core: Power cycle card on voltage switch fail
 I have tried to get a better grip on the problems being faced.
 
 Test setup:
 OLPC XO-1.75 laptop, based on Marvell MMP2 ARM SoC (includes sdhci-pxa
 interface)
 32GB Sandisk Ultra SD card being inserted into external SD slot
 
 The SoC apparently has support for 1.8V, but we physically do not have
 the right power lines wired on the motherboard, so we need to detect
 the 1.8V failure and go back to 3.3V mode.
 
 An initial question, would it be possible to solve this by disabling
 the UHS support in the host cap, or through a quirk? Maybe this kind
 of solution is not applicable in this scenario? I wonder this because
 I guess it will be very hard to know how a card will react if we first
 tell it that we're going to switch voltages and then don't, since this
 is out of spec.
 

I submitted a patch a while ago that allows the board specific code to change 
host-caps2
The controller may indicate support for UHS but the board design can prohibit 
this.

 I tracked down what causes this regression. In order to get the
 failure detection working again, I have to make the following change
 in mmc_set_signal_voltage():
if (cmd11) {
host-ios.clock = 0;
 -   mmc_set_ios(host);
 +   //mmc_set_ios(host);
}
 (i.e. don't mess with ios before asking sdhci to do the voltage switch)
 
 and at the top of sdhci_do_1_8v_signal_voltage_switch:
 +   /* Stop SDCLK */
 +   clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 +   clk = ~SDHCI_CLOCK_CARD_EN;
 +   sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 (i.e. add the equivalent clock disabling code here)
 
 Ok, so this suggests that the SDHCI driver does not stop the clock
 when we set ios.clock = 0 and call mmc_set_ios?
 
 With that change, I now get:
  mmc2: Signal voltage switch failed, power cycling card (retries = 10)
  mmc2: error -110 whilst initialising SD card
 
 The -110 error comes from mmc_send_app_op_cond(), but its worth noting
 that mmc_send_if_cond() (called immediately above) silently failed
 with -110 too. Neither of these failed the first time around before
 1.8V was tried.
 
 Full patch that I'm testing with: 
 http://dev.laptop.org/~dsd/20121030/mmc.patch
 
 So, now the question is: why is the card not responding to
 if_cond/app_op_cond after being powered down and up again? Is the
 power down/up sequence OK?
 
 Good question. I’d guess that mmc_power_off/up does not work as
 expected here, that the card is not at all power cycled. But it seems
 that the power cycle code in sdhci_do_start_signal_voltage_switch
 works? What if we export a couple of debug functions from sdhci.c
 which allow us to power cycle and control the clock just like sdhci
 does, and call these functions from core.c and sd.c? If this works
 (and the failure is detected properly by the core layer, and 10
 retries are made etc), then we know that the problems most likely
 depend on how mmc_set_ios and mmc_power_off/up work with sdhci. I've
 attached a patch suggesting this, if you'd like to give it a try
 (warning: the patch has not been tested). The patch does not do
 anything about pm_runtime, but perhaps we don't need to worry about
 this here.
 
 I tried inserting the mmc_power_off, mmc_power_up, mmc_send_if_cond
 sequence into various points in the codepaths being discussed.
 Measuring success as the card responding to mmc_send_if_cond(0x30080)
 without error (0x30080 is a known good value from when before 1.8V is
 tried), I find that this reset sequence works just fine up until the
 point when CMD11 is run.
 CMD11 is sent and returns successfully without error, but from this
 point, running the reset sequence will fail (send_if_cond will fail
 with -110).
 
 Is this kind of test valid? Does this suggest that something is wrong
 with the host controller hardware? My assumption is that whatever
 state is modified by CMD11 should be erased here, and of course the
 hope is that mmc_power_off and mmc_power_up will do a full power cycle
 anyway. But whatever is happening, it looks like the effects of CMD11
 are persisting past the reset sequence and are breaking later
 communication.
 
 I think that the power cycle has simply failed here, I find it hard to
 believe that CMD11 has persistent effects :) If mmc_power_off/up
 actually works, then perhaps the 1ms udelay is too small for this
 case?
 
 Kind regards, Johan
 0001-Clock-and-power-control.patch

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended 

UHS-I voltage switching on OLPC XO-1.75

2012-10-30 Thread Daniel Drake
Hi,

Following on from the recent thread
  [PATCH 2/2] mmc: core: Power cycle card on voltage switch fail
I have tried to get a better grip on the problems being faced.

Test setup:
OLPC XO-1.75 laptop, based on Marvell MMP2 ARM SoC (includes sdhci-pxa
interface)
32GB Sandisk Ultra SD card being inserted into external SD slot

The SoC apparently has support for 1.8V, but we physically do not have
the right power lines wired on the motherboard, so we need to detect
the 1.8V failure and go back to 3.3V mode.

Before patching anything, inserting this card results in:
  sdhci: Switching to 1.8V signalling voltage failed, retrying with
S18R set to 0
  mmc0: error -110 whilst initialising SD card

Now, working on Linux-3.5.4, I add these patches:

mmc: core: reset sigal voltage on power up
[RFC/PATCH,v2] mmc: core: Fixup signal voltage switch
regulator: add missing defintion regulator_is_supported_voltage
mmc: sdhci: Add regulator support for vccq (voltage regualor)
mmc: sdhci: Let core handle UHS switch failure

I also tweaked the sdhci code so that UHS-I flags are not
unconditionally disabled by the vccq commit (as explained in sdhci
vccq regulator support drops UHS-I flags).


Under this setup, the first problem encountered is that the system no
longer identifies the fact that the 1.8V voltage failed. Things are
not happy afterwards. The card_busy check in sdhci is saying that the
card is not busy.
Full kernel logs: http://dev.laptop.org/~dsd/20121030/mmc1.log

I tracked down what causes this regression. In order to get the
failure detection working again, I have to make the following change
in mmc_set_signal_voltage():
if (cmd11) {
host-ios.clock = 0;
-   mmc_set_ios(host);
+   //mmc_set_ios(host);
}
(i.e. don't mess with ios before asking sdhci to do the voltage switch)

and at the top of sdhci_do_1_8v_signal_voltage_switch:
+   /* Stop SDCLK */
+   clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+   clk = ~SDHCI_CLOCK_CARD_EN;
+   sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
(i.e. add the equivalent clock disabling code here)


With that change, I now get:
  mmc2: Signal voltage switch failed, power cycling card (retries = 10)
  mmc2: error -110 whilst initialising SD card

The -110 error comes from mmc_send_app_op_cond(), but its worth noting
that mmc_send_if_cond() (called immediately above) silently failed
with -110 too. Neither of these failed the first time around before
1.8V was tried.

Full patch that I'm testing with: http://dev.laptop.org/~dsd/20121030/mmc.patch

So, now the question is: why is the card not responding to
if_cond/app_op_cond after being powered down and up again? Is the
power down/up sequence OK?
I tried inserting the mmc_power_off, mmc_power_up, mmc_send_if_cond
sequence into various points in the codepaths being discussed.
Measuring success as the card responding to mmc_send_if_cond(0x30080)
without error (0x30080 is a known good value from when before 1.8V is
tried), I find that this reset sequence works just fine up until the
point when CMD11 is run.
CMD11 is sent and returns successfully without error, but from this
point, running the reset sequence will fail (send_if_cond will fail
with -110).

Is this kind of test valid? Does this suggest that something is wrong
with the host controller hardware? My assumption is that whatever
state is modified by CMD11 should be erased here, and of course the
hope is that mmc_power_off and mmc_power_up will do a full power cycle
anyway. But whatever is happening, it looks like the effects of CMD11
are persisting past the reset sequence and are breaking later
communication.

In the mail last week, I think we found some kind of configuration (on
Linux-3.0) where this same setup successfully detected the 1.8V
switching failure, and successfully switched back to 3.3V. I plan to
go back and test that, maybe there is some kind of sequence that
doesn't make the hardware die.

Daniel
--
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