[PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Ohad Ben-Cohen]

[I should really ditch my mailer, I know. hope it's the last attempt. Sorry x 2]
---
From: Ohad Ben-Cohen <ohad@wizery.com>

To allow the usage of MMC_VDD_165_195, host capability
MMC_CAP_VDD_165_195 is introduced. This is necessary
because MMC_VDD_165_195 is currently reserved/undefined.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/sdio.c  |    7 +++++++
 include/linux/mmc/host.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index cdb845b..a9f3ed6 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -494,6 +494,13 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 		ocr &= ~0x7F;
 	}
 
+	if ((ocr & MMC_VDD_165_195) && !(host->caps & MMC_CAP_VDD_165_195)) {
+		printk(KERN_WARNING "%s: SDIO card claims to support the "
+			"incompletely defined 'low voltage range'. This "
+			"will be ignored.\n", mmc_hostname(host));
+		ocr &= ~MMC_VDD_165_195;
+	}
+
 	host->ocr = mmc_select_voltage(host, ocr);
 
 	/*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 81bb423..5fa95b3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -150,6 +150,7 @@ struct mmc_host {
 #define MMC_CAP_DISABLE		(1 << 7)	/* Can the host be disabled */
 #define MMC_CAP_NONREMOVABLE	(1 << 8)	/* Nonremovable e.g. eMMC */
 #define MMC_CAP_WAIT_WHILE_BUSY	(1 << 9)	/* Waits while card is busy */
+#define MMC_CAP_VDD_165_195	(1 << 10)	/* Accepts MMC_VDD_165_195 */
 
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
-- 
1.5.4.3

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Matt Fleming]

On Mon, Sep 28, 2009 at 07:58:34PM +0200, Ohad Ben-Cohen wrote:
> [I should really ditch my mailer, I know. hope it's the last attempt. Sorry x 2]
> ---
> From: Ohad Ben-Cohen <ohad@wizery.com>
> 
> To allow the usage of MMC_VDD_165_195, host capability
> MMC_CAP_VDD_165_195 is introduced. This is necessary
> because MMC_VDD_165_195 is currently reserved/undefined.
> 

Have you got patches that add this capability to the TI 127x and ZOOM2
board setup files?

> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
>  drivers/mmc/core/sdio.c  |    7 +++++++
>  include/linux/mmc/host.h |    1 +
>  2 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index cdb845b..a9f3ed6 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -494,6 +494,13 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>  		ocr &= ~0x7F;
>  	}
>  
> +	if ((ocr & MMC_VDD_165_195) && !(host->caps & MMC_CAP_VDD_165_195)) {
> +		printk(KERN_WARNING "%s: SDIO card claims to support the "
> +			"incompletely defined 'low voltage range'. This "
> +			"will be ignored.\n", mmc_hostname(host));
> +		ocr &= ~MMC_VDD_165_195;
> +	}
> +
>  	host->ocr = mmc_select_voltage(host, ocr);
>  
>  	/*


I know you copied this warning from the old code but does anyone think
it's worth making this warning a bit clearer? e.g,

> +		printk(KERN_WARNING "%s: SDIO card claims to support the "
> +			"incompletely defined 'low voltage range', but the "
> +			"host controller does not. This voltage range"
> +			"will be ignored.\n", mmc_hostname(host));

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Ohad Ben-Cohen]

Hi Matt,

On Mon, Sep 28, 2009 at 8:10 PM, Matt Fleming <matt@console-pimps.org> wrote:
> Have you got patches that add this capability to the TI 127x and ZOOM2
> board setup files?

Sure, but I planned on waiting for the discussion on this to conclude.
Seems like it is moot now.

Thanks,
Ohad.

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Andrew Morton]

On Mon, 28 Sep 2009 19:10:48 +0100
Matt Fleming <matt@console-pimps.org> wrote:

> >  
> > +	if ((ocr & MMC_VDD_165_195) && !(host->caps & MMC_CAP_VDD_165_195)) {
> > +		printk(KERN_WARNING "%s: SDIO card claims to support the "
> > +			"incompletely defined 'low voltage range'. This "
> > +			"will be ignored.\n", mmc_hostname(host));
> > +		ocr &= ~MMC_VDD_165_195;
> > +	}
> > +
> >  	host->ocr = mmc_select_voltage(host, ocr);
> >  
> >  	/*
> 
> 
> I know you copied this warning from the old code

It'd be better to avoid copying anything at all.  Are we missing
code-sharing opportunities here?

> but does anyone think
> it's worth making this warning a bit clearer? e.g,
> 
> > +		printk(KERN_WARNING "%s: SDIO card claims to support the "
> > +			"incompletely defined 'low voltage range', but the "
> > +			"host controller does not. This voltage range"
> > +			"will be ignored.\n", mmc_hostname(host));

That looks better to me.

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Matt Fleming]

On Mon, Sep 28, 2009 at 03:59:00PM -0700, Andrew Morton wrote:
> On Mon, 28 Sep 2009 19:10:48 +0100
> Matt Fleming <matt@console-pimps.org> wrote:
> 
> > >  
> > > +	if ((ocr & MMC_VDD_165_195) && !(host->caps & MMC_CAP_VDD_165_195)) {
> > > +		printk(KERN_WARNING "%s: SDIO card claims to support the "
> > > +			"incompletely defined 'low voltage range'. This "
> > > +			"will be ignored.\n", mmc_hostname(host));
> > > +		ocr &= ~MMC_VDD_165_195;
> > > +	}
> > > +
> > >  	host->ocr = mmc_select_voltage(host, ocr);
> > >  
> > >  	/*
> > 
> > 
> > I know you copied this warning from the old code
> 
> It'd be better to avoid copying anything at all.  Are we missing
> code-sharing opportunities here?
> 

No, Ohad's commit 27cce39f555def6f5ebe7f03d69ccc44ab25f0b2 "sdio: do not
ignore MMC_VDD_165_195" deleted this warning and this commit is just
bringing it back from the dead.

It seems that Philip and David have other ideas on how they want to
handle this voltage range so this patch probably doesn't need picking
up.

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Pierre Ossman]

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

On Wed, 14 Oct 2009 11:34:42 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> 
> It can be done per-card, the switch to the lower voltage just needs to
> be deferred.  Initially set the voltage to a standard one that's
> supported by the card and host.  After the card is fully initialized and
> enumerated, have a hook for per-card fixups.  For the particular
> non-standard card in question, this would then reduce the voltage to 1.8V.
> 

In theory. But it would require a bit of redesign of the code as it
would have to restart the whole init again.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[David Vrabel]

Pierre Ossman wrote:
> On Wed, 14 Oct 2009 09:56:28 +0200
> Ohad Ben-Cohen <ohad@wizery.com> wrote:
> 
>> On Tue, Oct 13, 2009 at 4:39 AM, Philip Langdale <philipl@overt.org> wrote:
>>> Interesting. But that means that Ohad's patch doesn't make much sense;
>>> his uses the MMC low voltage OCR bit in an SDIO context. So either, the
>>> patch is wrong, or he's dealing with out-of-spec hardware.
>> Yes, the hardware is out-of-spec. It uses the undefined low voltage
>> OCR bit to achieve 1.8V SDIO voltage. By removing the MMC_VDD_165_195
>> restriction, commit 27cce39f555def6f5ebe7f03d69ccc44ab25f0b2 makes it
>> possible for the hardware to work with unpatched kernels.
>>
>> Philip, David, Pierre - would you like to remove the MMC_VDD_165_195
>> restriction differently ? maybe to revive the MMC_CAP_VDD_165_195 host
>> capability patch in some way (see
>> http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg00386.html) ?
>>
> 
> Since this is out-of-spec and therefore possibly dangerous behaviour,
> I'd like it to be opt-in for the user. And since it's so early in the
> init process, we can't automate it based on card id.

It can be done per-card, the switch to the lower voltage just needs to
be deferred.  Initially set the voltage to a standard one that's
supported by the card and host.  After the card is fully initialized and
enumerated, have a hook for per-card fixups.  For the particular
non-standard card in question, this would then reduce the voltage to 1.8V.

Alternatively, if this is for a chip hardwired to the controller then
some board-specific data for the SD controller can be used to always set
the voltage correctly. e.g., always run at 1.8V regardless of what the
stack says.  Obviously, this strategy won't work with removable cards.

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Pierre Ossman]

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

On Wed, 14 Oct 2009 09:56:28 +0200
Ohad Ben-Cohen <ohad@wizery.com> wrote:

> On Tue, Oct 13, 2009 at 4:39 AM, Philip Langdale <philipl@overt.org> wrote:
> > Interesting. But that means that Ohad's patch doesn't make much sense;
> > his uses the MMC low voltage OCR bit in an SDIO context. So either, the
> > patch is wrong, or he's dealing with out-of-spec hardware.
> 
> Yes, the hardware is out-of-spec. It uses the undefined low voltage
> OCR bit to achieve 1.8V SDIO voltage. By removing the MMC_VDD_165_195
> restriction, commit 27cce39f555def6f5ebe7f03d69ccc44ab25f0b2 makes it
> possible for the hardware to work with unpatched kernels.
> 
> Philip, David, Pierre - would you like to remove the MMC_VDD_165_195
> restriction differently ? maybe to revive the MMC_CAP_VDD_165_195 host
> capability patch in some way (see
> http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg00386.html) ?
> 

Since this is out-of-spec and therefore possibly dangerous behaviour,
I'd like it to be opt-in for the user. And since it's so early in the
init process, we can't automate it based on card id.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Ohad Ben-Cohen]

On Tue, Oct 13, 2009 at 4:39 AM, Philip Langdale <philipl@overt.org> wrote:
> Interesting. But that means that Ohad's patch doesn't make much sense;
> his uses the MMC low voltage OCR bit in an SDIO context. So either, the
> patch is wrong, or he's dealing with out-of-spec hardware.

Yes, the hardware is out-of-spec. It uses the undefined low voltage
OCR bit to achieve 1.8V SDIO voltage. By removing the MMC_VDD_165_195
restriction, commit 27cce39f555def6f5ebe7f03d69ccc44ab25f0b2 makes it
possible for the hardware to work with unpatched kernels.

Philip, David, Pierre - would you like to remove the MMC_VDD_165_195
restriction differently ? maybe to revive the MMC_CAP_VDD_165_195 host
capability patch in some way (see
http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg00386.html) ?

Thank you for the comments and help,
Ohad.

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Philip Langdale]

On Mon, 12 Oct 2009 14:11:29 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> 
> In the 3.00 spec, OCR bit 24 is S18R (Signalling 1.8V request).  This
> is outside of the VDD region (bits 0:23).  So I think we can support
> non standard DS/HS (Default Speed/High Speed) cards that report 1.8V
> operation (in the VDD region) and new UHS-I (Ultra High Speed phase I)
> cards without any confusion.
> 
> On a UHS-I capable controller, the software would send ACMD41 with
> S18R set and see if the card responses with it set.  If so, it tells
> the card to switch to 1.8V signalling (CMD11) and tells the
> controller to switch to 1.8V signalling and SDR12 mode.  The
> controller still supplies 3.3V to the card even after the switch.
> Later on the host software will query the card to find out what UHS-I
> modes are supported and switch card and controller to a faster mode
> (if possible).

Interesting. But that means that Ohad's patch doesn't make much sense;
his uses the MMC low voltage OCR bit in an SDIO context. So either, the
patch is wrong, or he's dealing with out-of-spec hardware.

Ohad?

--phil

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[David Vrabel]

Philip Langdale wrote:
> On Thu, 8 Oct 2009 20:38:26 +0200
> Pierre Ossman <pierre@ossman.eu> wrote:
> 
>>> In practice, I expect that the timings are close enough that this
>>> will work anyway, but I think the situation is analogous to HS-MMC
>>> vs HS-SD. There the timings are slightly different and you felt it
>>> was enough to justify a separate host cap for each one.
>>>
>> It's difficult to say without seeing the spec. But if things are not
>> backwards compatible, then we should probably add either a new timing
>> mode, or a new bus mode (where we have open drain and push pull
>> today).
>>
>>> In fact, thinking about it in those terms, it suggests we need to
>>> retroactively introduce a reduced-voltage MMC host flag too, just in
>>> case SDHCI 3.0 controllers barf on those cards...
>>>
>> Maybe. Again, it's difficult to say without seeing the specifics of
>> the new specification.
> 
> Make sense. So, I guess the question is do we assume the best case (in
> which case there is no additional work to do - the code that is now
> in allows low voltage SDIO cards to try and run) or the worst case and
> add pre-emptive mode flags?
> 
> David, you've actually seen the new spec right? What's your opinion?

In the 3.00 spec, OCR bit 24 is S18R (Signalling 1.8V request).  This is
outside of the VDD region (bits 0:23).  So I think we can support non
standard DS/HS (Default Speed/High Speed) cards that report 1.8V
operation (in the VDD region) and new UHS-I (Ultra High Speed phase I)
cards without any confusion.

On a UHS-I capable controller, the software would send ACMD41 with S18R
set and see if the card responses with it set.  If so, it tells the card
 to switch to 1.8V signalling (CMD11) and tells the controller to switch
to 1.8V signalling and SDR12 mode.  The controller still supplies 3.3V
to the card even after the switch.  Later on the host software will
query the card to find out what UHS-I modes are supported and switch
card and controller to a faster mode (if possible).

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Philip Langdale]

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

On Thu, 8 Oct 2009 20:38:26 +0200
Pierre Ossman <pierre@ossman.eu> wrote:

> > 
> > In practice, I expect that the timings are close enough that this
> > will work anyway, but I think the situation is analogous to HS-MMC
> > vs HS-SD. There the timings are slightly different and you felt it
> > was enough to justify a separate host cap for each one.
> > 
> 
> It's difficult to say without seeing the spec. But if things are not
> backwards compatible, then we should probably add either a new timing
> mode, or a new bus mode (where we have open drain and push pull
> today).
> 
> > In fact, thinking about it in those terms, it suggests we need to
> > retroactively introduce a reduced-voltage MMC host flag too, just in
> > case SDHCI 3.0 controllers barf on those cards...
> > 
> 
> Maybe. Again, it's difficult to say without seeing the specifics of
> the new specification.

Make sense. So, I guess the question is do we assume the best case (in
which case there is no additional work to do - the code that is now
in allows low voltage SDIO cards to try and run) or the worst case and
add pre-emptive mode flags?

David, you've actually seen the new spec right? What's your opinion?

--phil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Pierre Ossman]

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

On Tue, 29 Sep 2009 23:10:13 -0700
Philip Langdale <philipl@overt.org> wrote:

> On Tue, 29 Sep 2009 23:37:32 +0200
> Pierre Ossman <pierre@ossman.eu> wrote:
> 
> > I must have missed that part of discussion. If the voltage fully
> > overlaps with the MMC definition, then I don't see the controllers
> > having to be designed explicitly for SD 3.0. If not, then we probably
> > need a new voltage bit for the hosts. In that case separating
> > supporting from non-supporting should sort itself out easily.
> 
> The reason I think we need is a host cap is that low voltage operations
> apparently implies different signal timings. This is second hand from
> David as there are no public specs for 3.0 available yet. So, the
> failure case is a controller that publishes support for low voltage,
> but only expects MMC cards to use it.
> 
> In practice, I expect that the timings are close enough that this will
> work anyway, but I think the situation is analogous to HS-MMC vs HS-SD.
> There the timings are slightly different and you felt it was enough to
> justify a separate host cap for each one.
> 

It's difficult to say without seeing the spec. But if things are not
backwards compatible, then we should probably add either a new timing
mode, or a new bus mode (where we have open drain and push pull today).

> In fact, thinking about it in those terms, it suggests we need to
> retroactively introduce a reduced-voltage MMC host flag too, just in
> case SDHCI 3.0 controllers barf on those cards...
> 

Maybe. Again, it's difficult to say without seeing the specifics of the
new specification.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Philip Langdale]

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

On Tue, 29 Sep 2009 23:37:32 +0200
Pierre Ossman <pierre@ossman.eu> wrote:

> I must have missed that part of discussion. If the voltage fully
> overlaps with the MMC definition, then I don't see the controllers
> having to be designed explicitly for SD 3.0. If not, then we probably
> need a new voltage bit for the hosts. In that case separating
> supporting from non-supporting should sort itself out easily.

The reason I think we need is a host cap is that low voltage operations
apparently implies different signal timings. This is second hand from
David as there are no public specs for 3.0 available yet. So, the
failure case is a controller that publishes support for low voltage,
but only expects MMC cards to use it.

In practice, I expect that the timings are close enough that this will
work anyway, but I think the situation is analogous to HS-MMC vs HS-SD.
There the timings are slightly different and you felt it was enough to
justify a separate host cap for each one.

In fact, thinking about it in those terms, it suggests we need to
retroactively introduce a reduced-voltage MMC host flag too, just in
case SDHCI 3.0 controllers barf on those cards...

Am I being too doom-and-gloom here? David, do you have an opinion on
this one?

> 
> Btw, you really need to whip some sense into the line handling of your
> email client. :)

Indeed. Let's see if it's any better this time :-)

Thanks for taking the time to chime in on this one.

--phil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Pierre Ossman]

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

On Tue, 29 Sep 2009 16:20:38 -0400
Philip Langdale <philipl@overt.org> wrote:

> 
> My understanding from the previous discussion was that SD 3.0 (and
> presumably
> a matching SDHCI 3.0) fully define the low voltage range. As such, a
> controller
> that is documented to conform to this spec, or is otherwise documented to
> implement the functionality, can be safely allowed to run SD cards that
> also
> claim to support the bit.
> 

I must have missed that part of discussion. If the voltage fully
overlaps with the MMC definition, then I don't see the controllers
having to be designed explicitly for SD 3.0. If not, then we probably
need a new voltage bit for the hosts. In that case separating
supporting from non-supporting should sort itself out easily.

> Yes, there is a danger of pre 3.0 cards claiming to suport the low voltage
> range,
> but I think there's a credible chance that no such cards actually exist,
> and if
> they do, I think they're obscure enough to ignore - if they were a problem,
> the
> SD association would have had to abandon using the same bit as MMC uses.
> 

Agreed.

Btw, you really need to whip some sense into the line handling of your
email client. :)

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Philip Langdale]

On Tue, 29 Sep 2009 20:28:33 +0200, Pierre Ossman <pierre@ossman.eu> wrote:
> On Mon, 28 Sep 2009 19:25:48 -0700
> Philip Langdale <philipl@overt.org> wrote:
> 
>> 
>> Hi David,
>> 
>> Ok, that sounds reasonable, but my concern is a controller that
>> publishes support for MMC_VDD_165_195 for mmc cards but doesn't
>> claim support for SDIO cards - particularly considering the
>> signalling implications you mentioned. Now, maybe you don't see
>> this happening in the wild, but it seems to me that it has to
>> be possible. It seems that to guard against this, you'd need a
>> host cap that says "165_195 for SD" and if it's not present,
>> mask it out of the OCR when dealing with SD/IO cards.
>> 
>> Am I being too paranoid?
>> 
> 
> There is no way the vendor of the SD controller can know what this bit
> means to the vendor of the SD/SDIO card as it is undefined. I'm afraid
> I see no safe way of supporting this bit. The only thing we can do is
> interpret it as being the same as the MMC bit, but then only with an
> opt-in configuration as we might be killing hardware with this.

My understanding from the previous discussion was that SD 3.0 (and
presumably
a matching SDHCI 3.0) fully define the low voltage range. As such, a
controller
that is documented to conform to this spec, or is otherwise documented to
implement the functionality, can be safely allowed to run SD cards that
also
claim to support the bit.

Yes, there is a danger of pre 3.0 cards claiming to suport the low voltage
range,
but I think there's a credible chance that no such cards actually exist,
and if
they do, I think they're obscure enough to ignore - if they were a problem,
the
SD association would have had to abandon using the same bit as MMC uses.

If you're really paranoid, you could also cross-check the SD card version
but
that would have to happen after establishing basic communication so you'd
have
to go back and re-negotiate the voltage after starting at 3.3V.

It seems a reasonable compromise to me to assume that a card which claims
low
voltage support knows what its talking about, and then have host support be
opt
in, either by explicitly reporting SDHCI 3.0 compliance or because the
controller
docs state that it supports SD 3.0 cards.

--phil

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Pierre Ossman]

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

On Mon, 28 Sep 2009 19:25:48 -0700
Philip Langdale <philipl@overt.org> wrote:

> 
> Hi David,
> 
> Ok, that sounds reasonable, but my concern is a controller that
> publishes support for MMC_VDD_165_195 for mmc cards but doesn't
> claim support for SDIO cards - particularly considering the
> signalling implications you mentioned. Now, maybe you don't see
> this happening in the wild, but it seems to me that it has to
> be possible. It seems that to guard against this, you'd need a
> host cap that says "165_195 for SD" and if it's not present,
> mask it out of the OCR when dealing with SD/IO cards.
> 
> Am I being too paranoid?
> 

There is no way the vendor of the SD controller can know what this bit
means to the vendor of the SD/SDIO card as it is undefined. I'm afraid
I see no safe way of supporting this bit. The only thing we can do is
interpret it as being the same as the MMC bit, but then only with an
opt-in configuration as we might be killing hardware with this.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Philip Langdale]

On Mon, 28 Sep 2009 19:09:52 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> Ohad Ben-Cohen wrote:
> > From: Ohad Ben-Cohen <ohad@wizery.com>
> > 
> > To allow the usage of MMC_VDD_165_195, host capability
> > MMC_CAP_VDD_165_195 is introduced. This is necessary
> > because MMC_VDD_165_195 is currently reserved/undefined.
> 
> The host already reports what voltages it supports (in
> mmc_host::ocr_avail) so a seperate MMC_CAP_* isn't needed.
> 
> This interpretation of the reserved bits in the OCR should only done
> for certain cards where the bits actually do mean 1.8 V operation
> (with v2.0 signalling) is possible.
> 
> That's a fair amount of work so perhaps in the interim something like
> this:
> 
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -494,6 +494,9 @@ int mmc_attach_sdio(struct mmc_host *host, u32
> ocr) ocr &= ~0x7F;
>  	}
> 
> +	if (ocr & MMC_VDD_165_195)
> +		printk(KERN_WARNING "%s: warning: card claims
> non-standard 1.65-1.95 V support"
> +
>  	host->ocr = mmc_select_voltage(host, ocr);
> 
>  	/*
> 
> And revisit this if these bits either: a) gain a (different) standard
> meaning; or b) some other card uses these bits in a different
> non-standard way.  Neither seems likely.

Hi David,

Ok, that sounds reasonable, but my concern is a controller that
publishes support for MMC_VDD_165_195 for mmc cards but doesn't
claim support for SDIO cards - particularly considering the
signalling implications you mentioned. Now, maybe you don't see
this happening in the wild, but it seems to me that it has to
be possible. It seems that to guard against this, you'd need a
host cap that says "165_195 for SD" and if it's not present,
mask it out of the OCR when dealing with SD/IO cards.

Am I being too paranoid?

--phil

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Ohad Ben-Cohen]

Hi David,

On Mon, Sep 28, 2009 at 8:09 PM, David Vrabel <david.vrabel@csr.com> wrote:

...

> That's a fair amount of work so perhaps in the interim something like this:
>
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -494,6 +494,9 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>                ocr &= ~0x7F;
>        }
>
> +       if (ocr & MMC_VDD_165_195)
> +               printk(KERN_WARNING "%s: warning: card claims non-standard 1.65-1.95
> V support"
> +
>        host->ocr = mmc_select_voltage(host, ocr);
>
>        /*


Thanks, that would work for us just fine.

FWIW,

Ack-by: Ohad Ben-Cohen <ohad@wizery.com>


Regards,
Ohad.

Re: [PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[David Vrabel]

Ohad Ben-Cohen wrote:
> From: Ohad Ben-Cohen <ohad@wizery.com>
> 
> To allow the usage of MMC_VDD_165_195, host capability
> MMC_CAP_VDD_165_195 is introduced. This is necessary
> because MMC_VDD_165_195 is currently reserved/undefined.

The host already reports what voltages it supports (in
mmc_host::ocr_avail) so a seperate MMC_CAP_* isn't needed.

This interpretation of the reserved bits in the OCR should only done for
certain cards where the bits actually do mean 1.8 V operation (with v2.0
signalling) is possible.

That's a fair amount of work so perhaps in the interim something like this:

--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -494,6 +494,9 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 		ocr &= ~0x7F;
 	}

+	if (ocr & MMC_VDD_165_195)
+		printk(KERN_WARNING "%s: warning: card claims non-standard 1.65-1.95
V support"
+
 	host->ocr = mmc_select_voltage(host, ocr);

 	/*

And revisit this if these bits either: a) gain a (different) standard
meaning; or b) some other card uses these bits in a different
non-standard way.  Neither seems likely.

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

[PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Ohad Ben-Cohen]

[resending due to mailer issues - sorry]
---
From: Ohad Ben-Cohen <ohad@wizery.com>

To allow the usage of MMC_VDD_165_195, host capability
MMC_CAP_VDD_165_195 is introduced. This is necessary
because MMC_VDD_165_195 is currently reserved/undefined.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/sdio.c  |    7 +++++++
 include/linux/mmc/host.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index cdb845b..a9f3ed6 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -494,6 +494,13 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 		ocr &= ~0x7F;
 	}
 
+	if ((ocr & MMC_VDD_165_195) && !(host->caps & MMC_CAP_VDD_165_195)) {
+		printk(KERN_WARNING "%s: SDIO card claims to support the "
+			"incompletely defined 'low voltage range'. This "
+			"will be ignored.\n", mmc_hostname(host));
+		ocr &= ~MMC_VDD_165_195;
+	}
+
 	host->ocr = mmc_select_voltage(host, ocr);
 
 	/*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 81bb423..5fa95b3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -150,6 +150,7 @@ struct mmc_host {
 #define MMC_CAP_DISABLE		(1 << 7)	/* Can the host be disabled */
 #define MMC_CAP_NONREMOVABLE	(1 << 8)	/* Nonremovable e.g. eMMC */
 #define MMC_CAP_WAIT_WHILE_BUSY	(1 << 9)	/* Waits while card is busy */
+#define MMC_CAP_VDD_165_195	(1 << 10)	/* Accepts MMC_VDD_165_195 */
 
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
-- 
1.5.4.3

[PATCH] sdio: add MMC_CAP_VDD_165_195 host capability

[Ohad Ben-Cohen]

From: Ohad Ben-Cohen <ohad@wizery.com>

To allow the usage of MMC_VDD_165_195, host capability
MMC_CAP_VDD_165_195 is introduced. This is necessary
because MMC_VDD_165_195 is currently reserved/undefined.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/sdio.c  |    7 +++++++
 include/linux/mmc/host.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index cdb845b..a9f3ed6 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -494,6 +494,13 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 		ocr &= ~0x7F;
 	}
 
+	if ((ocr & MMC_VDD_165_195) && !(host->caps & MMC_CAP_VDD_165_195)) {
+		printk(KERN_WARNING "%s: SDIO card claims to support the "
+			"incompletely defined 'low voltage range'. This "
+			"will be ignored.\n", mmc_hostname(host));
+		ocr &= ~MMC_VDD_165_195;
+	}
+
 	host->ocr = mmc_select_voltage(host, ocr);
 
 	/*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 81bb423..5fa95b3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -150,6 +150,7 @@ struct mmc_host {
 #define MMC_CAP_DISABLE		(1 << 7)	/* Can the host be disabled */
 #define MMC_CAP_NONREMOVABLE	(1 << 8)	/* Nonremovable e.g. eMMC */
 #define MMC_CAP_WAIT_WHILE_BUSY	(1 << 9)	/* Waits while card is busy */
+#define MMC_CAP_VDD_165_195	(1 << 10)	/* Accepts MMC_VDD_165_195 */
 
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
-- 
1.5.4.3