All of lore.kernel.org
 help / color / mirror / Atom feed
* UHS-I voltage switching on OLPC XO-1.75
@ 2012-10-30 21:11 Daniel Drake
  2012-11-02 14:35 ` Johan Rudholm
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Daniel Drake @ 2012-10-30 21:11 UTC (permalink / raw)
  To: Johan Rudholm; +Cc: Chris Ball, linux-mmc

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-10-30 21:11 UHS-I voltage switching on OLPC XO-1.75 Daniel Drake
@ 2012-11-02 14:35 ` Johan Rudholm
  2012-11-02 17:27   ` Philip Rakity
                     ` (2 more replies)
  2012-11-05 12:49 ` Philip Rakity
  2012-11-05 12:50 ` Philip Rakity
  2 siblings, 3 replies; 16+ messages in thread
From: Johan Rudholm @ 2012-11-02 14:35 UTC (permalink / raw)
  To: Daniel Drake; +Cc: Chris Ball, linux-mmc

[-- Attachment #1: Type: text/plain, Size: 4538 bytes --]

Hi Daniel,

2012/10/30 Daniel Drake <dsd@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

[-- Attachment #2: 0001-Clock-and-power-control.patch --]
[-- Type: application/octet-stream, Size: 3347 bytes --]

From a521de4e5196bdb4b81c825d5ff3841513e92869 Mon Sep 17 00:00:00 2001
From: Johan Rudholm <johan.rudholm@stericsson.com>
Date: Fri, 2 Nov 2012 15:22:17 +0100
Subject: [PATCH] Clock and power control

---
 drivers/mmc/core/core.c  |   11 +++++------
 drivers/mmc/core/sd.c    |    6 ++----
 drivers/mmc/host/sdhci.c |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index dcfd451..e1e797a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1044,6 +1044,8 @@ u32 mmc_select_voltage(struct mmc_host *host, u32 ocr)
 	return ocr;
 }
 
+extern void clock_control(struct mmc_host *mmc, int on);
+
 int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11)
 {
 	struct mmc_command cmd = {0};
@@ -1076,20 +1078,17 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, bool cmd11
 		mmc_host_clk_hold(host);
 		clock = host->ios.clock;
 		if (cmd11) {
-			host->ios.clock = 0;
-			mmc_set_ios(host);
+			clock_control(host, 0);
 		}
 
 		err = host->ops->start_signal_voltage_switch(host, &host->ios);
 
 		if (err && cmd11) {
-			host->ios.clock = clock;
-			mmc_set_ios(host);
+			clock_control(host, 1);
 		} else if (cmd11) {
 			/* Stop clock for at least 5 ms according to spec */
 			mmc_delay(5);
-			host->ios.clock = clock;
-			mmc_set_ios(host);
+			clock_control(host, 1);
 
 			/* Wait for at least 1 ms according to spec */
 			mmc_delay(1);
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index dd4509f..8523c4f 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -720,6 +720,7 @@ struct device_type sd_type = {
 	.groups = sd_attr_groups,
 };
 
+extern void power_cycle(struct mmc_host *mmc);
 /*
  * Fetch CID from card.
  */
@@ -783,10 +784,7 @@ try_again:
 			pr_warning("%s: Signal voltage switch failed, "
 				"power cycling card (retries = %d)\n",
 				mmc_hostname(host), retries);
-			mmc_power_off(host);
-			/* Wait at least 1 ms according to spec */
-			mmc_delay(1);
-			mmc_power_up(host);
+			power_cycle(host);
 			retries--;
 			goto try_again;
 		} else if (err) {
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f4b8b4d..bc29a04 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1677,6 +1677,38 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		return 0;
 }
 
+void power_cycle(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	u8 pwr;
+
+	pwr = sdhci_readb(host, SDHCI_POWER_CONTROL);
+	pwr &= ~SDHCI_POWER_ON;
+	sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
+
+	/* Wait for 1ms as per the spec */
+	usleep_range(1000, 1500);
+	pwr |= SDHCI_POWER_ON;
+	sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
+}
+
+void clock_control(struct mmc_host *mmc, int on)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	u16 clk;
+
+	if (on) {
+		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		clk |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+	} else {
+		/* Stop SDCLK */
+		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		clk &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+	}
+}
+
 static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 	struct mmc_ios *ios)
 {
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-02 14:35 ` Johan Rudholm
@ 2012-11-02 17:27   ` Philip Rakity
  2012-11-08 17:48   ` Daniel Drake
  2012-11-18 17:42   ` Daniel Drake
  2 siblings, 0 replies; 16+ messages in thread
From: Philip Rakity @ 2012-11-02 17:27 UTC (permalink / raw)
  To: Johan Rudholm; +Cc: Daniel Drake, Chris Ball, linux-mmc


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

> Hi Daniel,
> 
> 2012/10/30 Daniel Drake <dsd@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 recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-10-30 21:11 UHS-I voltage switching on OLPC XO-1.75 Daniel Drake
  2012-11-02 14:35 ` Johan Rudholm
@ 2012-11-05 12:49 ` Philip Rakity
  2012-11-05 13:11   ` Chris Ball
  2012-11-05 12:50 ` Philip Rakity
  2 siblings, 1 reply; 16+ messages in thread
From: Philip Rakity @ 2012-11-05 12:49 UTC (permalink / raw)
  To: Daniel Drake, Chris Ball; +Cc: Johan Rudholm, linux-mmc


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 <dsd@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 majordomo@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 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.
-----------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-10-30 21:11 UHS-I voltage switching on OLPC XO-1.75 Daniel Drake
  2012-11-02 14:35 ` Johan Rudholm
  2012-11-05 12:49 ` Philip Rakity
@ 2012-11-05 12:50 ` Philip Rakity
  2 siblings, 0 replies; 16+ messages in thread
From: Philip Rakity @ 2012-11-05 12:50 UTC (permalink / raw)
  To: Daniel Drake, Chris Ball; +Cc: Johan Rudholm, linux-mmc


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 <dsd@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 majordomo@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 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.
-----------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-05 12:49 ` Philip Rakity
@ 2012-11-05 13:11   ` Chris Ball
  2012-11-05 13:20     ` Philip Rakity
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Ball @ 2012-11-05 13:11 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Daniel Drake, Johan Rudholm, linux-mmc

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 <kliu5@marvell.com>

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

Reviewed-by: Philip Rakity <prakity@marvell.com>
Signed-off-by: Bin Wang <binw@marvell.com>
Signed-off-by: Kevin Liu <kliu5@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, 1800000, 1800000))
 		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   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-05 13:11   ` Chris Ball
@ 2012-11-05 13:20     ` Philip Rakity
  2012-11-05 13:46       ` Chris Ball
  0 siblings, 1 reply; 16+ messages in thread
From: Philip Rakity @ 2012-11-05 13:20 UTC (permalink / raw)
  To: Chris Ball; +Cc: Daniel Drake, Johan Rudholm, linux-mmc



On Nov 5, 2012, at 1:11 PM, Chris Ball <cjb@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 <kliu5@marvell.com>
> 
> regulator_get() returns NULL when CONFIG_REGULATOR not defined,
> which should not print out the warning.
> 
> Reviewed-by: Philip Rakity <prakity@marvell.com>
> Signed-off-by: Bin Wang <binw@marvell.com>
> Signed-off-by: Kevin Liu <kliu5@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, 1800000, 1800000))
> 		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   <cjb@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.
-----------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-05 13:20     ` Philip Rakity
@ 2012-11-05 13:46       ` Chris Ball
  0 siblings, 0 replies; 16+ messages in thread
From: Chris Ball @ 2012-11-05 13:46 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Daniel Drake, Johan Rudholm, linux-mmc, Kevin Liu

Hi, adding Kevin,

On Mon, Nov 05 2012, Philip Rakity wrote:
> On Nov 5, 2012, at 1:11 PM, Chris Ball <cjb@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   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-02 14:35 ` Johan Rudholm
  2012-11-02 17:27   ` Philip Rakity
@ 2012-11-08 17:48   ` Daniel Drake
  2012-11-18 17:42   ` Daniel Drake
  2 siblings, 0 replies; 16+ messages in thread
From: Daniel Drake @ 2012-11-08 17:48 UTC (permalink / raw)
  To: Johan Rudholm; +Cc: Chris Ball, linux-mmc

On Fri, Nov 2, 2012 at 8:35 AM, Johan Rudholm <jrudholm@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-02 14:35 ` Johan Rudholm
  2012-11-02 17:27   ` Philip Rakity
  2012-11-08 17:48   ` Daniel Drake
@ 2012-11-18 17:42   ` Daniel Drake
  2012-11-18 20:25     ` Philip Rakity
  2012-11-20  8:53     ` Johan Rudholm
  2 siblings, 2 replies; 16+ messages in thread
From: Daniel Drake @ 2012-11-18 17:42 UTC (permalink / raw)
  To: Johan Rudholm; +Cc: Chris Ball, linux-mmc

On Fri, Nov 2, 2012 at 8:35 AM, Johan Rudholm <jrudholm@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-18 17:42   ` Daniel Drake
@ 2012-11-18 20:25     ` Philip Rakity
  2012-11-20  8:53     ` Johan Rudholm
  1 sibling, 0 replies; 16+ messages in thread
From: Philip Rakity @ 2012-11-18 20:25 UTC (permalink / raw)
  To: Daniel Drake; +Cc: Johan Rudholm, Chris Ball, linux-mmc



On Nov 18, 2012, at 5:42 PM, Daniel Drake <dsd@laptop.org> wrote:

> On Fri, Nov 2, 2012 at 8:35 AM, Johan Rudholm <jrudholm@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?

You can use a notifier to get control when voltage switch is called in your low level board file.
I did this originally to control PAD settings for 3.3 and 1.8v.  I had the receiver of the notifier in my low level board file and I registered it my sdhci-xxx.c file.
The notified is called at the end of the voltage switch by the regulator code but maybe there is now a case to add notifier control at the beginning of the routines.

> 
> Thanks,
> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@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 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.
-----------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-18 17:42   ` Daniel Drake
  2012-11-18 20:25     ` Philip Rakity
@ 2012-11-20  8:53     ` Johan Rudholm
  2012-11-25 18:22       ` Daniel Drake
  1 sibling, 1 reply; 16+ messages in thread
From: Johan Rudholm @ 2012-11-20  8:53 UTC (permalink / raw)
  To: Daniel Drake; +Cc: Chris Ball, linux-mmc

Hi!

2012/11/18 Daniel Drake <dsd@laptop.org>:
> On Fri, Nov 2, 2012 at 8:35 AM, Johan Rudholm <jrudholm@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
  2012-11-20  8:53     ` Johan Rudholm
@ 2012-11-25 18:22       ` Daniel Drake
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Drake @ 2012-11-25 18:22 UTC (permalink / raw)
  To: Johan Rudholm; +Cc: Chris Ball, linux-mmc

On Tue, Nov 20, 2012 at 2:53 AM, Johan Rudholm <jrudholm@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
       [not found] <5817E66E-3B94-42E1-AB06-699935D83379@nvidia.com>
  2012-11-08 19:10 ` Daniel Drake
@ 2012-11-08 22:23 ` Philip Rakity
  1 sibling, 0 replies; 16+ messages in thread
From: Philip Rakity @ 2012-11-08 22:23 UTC (permalink / raw)
  To: Daniel Drake; +Cc: Johan Rudholm, Chris Ball, linux-mmc


On Nov 8, 2012, at 7:03 PM, Philip Rakity <prakity@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" <dsd@laptop.org>
> To: "Philip Rakity" <prakity@nvidia.com>
> Cc: "Johan Rudholm" <jrudholm@gmail.com>, "Chris Ball" <cjb@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 <prakity@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.
-----------------------------------------------------------------------------------

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
       [not found] <5817E66E-3B94-42E1-AB06-699935D83379@nvidia.com>
@ 2012-11-08 19:10 ` Daniel Drake
  2012-11-08 22:23 ` Philip Rakity
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Drake @ 2012-11-08 19:10 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Johan Rudholm, Chris Ball, linux-mmc

On Thu, Nov 8, 2012 at 1:03 PM, Philip Rakity <prakity@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: UHS-I voltage switching on OLPC XO-1.75
       [not found] <6CB855B4-CBBE-4AA1-8D07-E276160FCE8D@nvidia.com>
@ 2012-11-08 18:52 ` Daniel Drake
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Drake @ 2012-11-08 18:52 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Johan Rudholm, Chris Ball, linux-mmc

On Thu, Nov 8, 2012 at 12:48 PM, Philip Rakity <prakity@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-11-25 18:22 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-30 21:11 UHS-I voltage switching on OLPC XO-1.75 Daniel Drake
2012-11-02 14:35 ` Johan Rudholm
2012-11-02 17:27   ` Philip Rakity
2012-11-08 17:48   ` Daniel Drake
2012-11-18 17:42   ` Daniel Drake
2012-11-18 20:25     ` Philip Rakity
2012-11-20  8:53     ` Johan Rudholm
2012-11-25 18:22       ` Daniel Drake
2012-11-05 12:49 ` Philip Rakity
2012-11-05 13:11   ` Chris Ball
2012-11-05 13:20     ` Philip Rakity
2012-11-05 13:46       ` Chris Ball
2012-11-05 12:50 ` Philip Rakity
     [not found] <6CB855B4-CBBE-4AA1-8D07-E276160FCE8D@nvidia.com>
2012-11-08 18:52 ` Daniel Drake
     [not found] <5817E66E-3B94-42E1-AB06-699935D83379@nvidia.com>
2012-11-08 19:10 ` Daniel Drake
2012-11-08 22:23 ` Philip Rakity

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.