Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-04 Thread Alex Dubov 
I think the trivial fix will do (after all, there's nothing that should matter 
to the controller
in the R6 response; I don't know about R7). I don't have any SDHC cards so I 
can't test this.

--- tifm_sd.c.orig  2006-12-11 01:39:28.0 +1100
+++ tifm_sd.c   2007-01-04 23:40:48.441724000 +1100
@@ -179,6 +179,8 @@
case MMC_RSP_R1B:
rc |= TIFM_MMCSD_RSP_BUSY; // deliberate fall-through
case MMC_RSP_R1:
+   case MMC_RSP_R6:
+   case MMC_RSP_R7:
rc |= TIFM_MMCSD_RSP_R1;
break;
case MMC_RSP_R2:
@@ -187,9 +189,6 @@
case MMC_RSP_R3:
rc |= TIFM_MMCSD_RSP_R3;
break;
-   case MMC_RSP_R6:
-   rc |= TIFM_MMCSD_RSP_R6;
-   break;
default:
BUG();
}


__
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-04 Thread Alex Dubov 
I think the trivial fix will do (after all, there's nothing that should matter 
to the controller
in the R6 response; I don't know about R7). I don't have any SDHC cards so I 
can't test this.

--- tifm_sd.c.orig  2006-12-11 01:39:28.0 +1100
+++ tifm_sd.c   2007-01-04 23:40:48.441724000 +1100
@@ -179,6 +179,8 @@
case MMC_RSP_R1B:
rc |= TIFM_MMCSD_RSP_BUSY; // deliberate fall-through
case MMC_RSP_R1:
+   case MMC_RSP_R6:
+   case MMC_RSP_R7:
rc |= TIFM_MMCSD_RSP_R1;
break;
case MMC_RSP_R2:
@@ -187,9 +189,6 @@
case MMC_RSP_R3:
rc |= TIFM_MMCSD_RSP_R3;
break;
-   case MMC_RSP_R6:
-   rc |= TIFM_MMCSD_RSP_R6;
-   break;
default:
BUG();
}


__
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Pierre Ossman 
Philip Langdale wrote:
> Pierre Ossman wrote:
>   
>> Amen to that. All hw vendors that implement this particular form of
>> brain damage should be dragged out and shot.
>>
>> I'll fix a patch for this later on.
>> 
>
> See my updated Take 3 patch. I've implemented a uniqueness fix by
> adding additional RSP flags do make R6 and R7 unique. I don't know
> if this is what you wanted, but it works without being too ugly.
>
>   

NAK. If two response types look the same over the wire, then they should
have the same definition. Hardware that uses type codes is simply
broken. There are a lot of sinners unfortunately...

> However, also note my caveat that it's not clear if tifm or imxmmc
> can ever be made to work with SD 2.0 cards. *sigh*
>   

They probably can. They just need a fix for their switch statements.

Rgds

-- 
 -- Pierre Ossman

  Linux kernel, MMC maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Philip Langdale 
Pierre Ossman wrote:
> 
> Amen to that. All hw vendors that implement this particular form of
> brain damage should be dragged out and shot.
> 
> I'll fix a patch for this later on.

See my updated Take 3 patch. I've implemented a uniqueness fix by
adding additional RSP flags do make R6 and R7 unique. I don't know
if this is what you wanted, but it works without being too ugly.

However, also note my caveat that it's not clear if tifm or imxmmc
can ever be made to work with SD 2.0 cards. *sigh*

--phil
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Pierre Ossman 
Philip Langdale wrote:
> This is a bug. The MMC_RSP_R? #defines do not fully characterise the
> responses (specically, the way that the response is parsed is not
> characterised) and consequently there is no guarantee of uniqueness.
> Given this reality - the way that the tifm_sd driver works is unsafe.
>
>   

Amen to that. All hw vendors that implement this particular form of
brain damage should be dragged out and shot.

I'll fix a patch for this later on.

Rgds

-- 
 -- Pierre Ossman

  Linux kernel, MMC maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Philip Langdale 
Andrew Morton wrote:
> On Mon, 01 Jan 2007 07:29:55 -0800
> Philip Langdale <[EMAIL PROTECTED]> wrote:
> 
>>  #define MMC_RSP_R1B 
>> (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY)
>>  #define MMC_RSP_R2  (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC)
>>  #define MMC_RSP_R3  (MMC_RSP_PRESENT)
>> -#define MMC_RSP_R6  (MMC_RSP_PRESENT|MMC_RSP_CRC)
>> +#define MMC_RSP_R6  (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
>> +#define MMC_RSP_R7  (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
> 
> This gives MMC_RSP_R1 and MMC_RSP_R6 the same value, so
> 
> drivers/mmc/tifm_sd.c: In function 'tifm_sd_op_flags':
> drivers/mmc/tifm_sd.c:190: error: duplicate case value
> drivers/mmc/tifm_sd.c:181: error: previously used here

This is a bug. The MMC_RSP_R? #defines do not fully characterise the
responses (specically, the way that the response is parsed is not
characterised) and consequently there is no guarantee of uniqueness.
Given this reality - the way that the tifm_sd driver works is unsafe.

If R6 had not been incorrectly defined (the missing RSP_OPCODE should
always have been there), then this code would not have worked. As things
currently stand, it is necessary to also check the command number to
decide on the correct response type - that's suboptimal and it's probably
good to uniquely identify the response in the mmc_command in some other
fashion.

I'm going to remove the R6 fix from my next diff to keep these things
distinct but this needs to be resolved.

--phil
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Andrew Morton 
On Mon, 01 Jan 2007 07:29:55 -0800
Philip Langdale <[EMAIL PROTECTED]> wrote:

>  #define MMC_RSP_R1B  
> (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY)
>  #define MMC_RSP_R2   (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC)
>  #define MMC_RSP_R3   (MMC_RSP_PRESENT)
> -#define MMC_RSP_R6   (MMC_RSP_PRESENT|MMC_RSP_CRC)
> +#define MMC_RSP_R6   (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
> +#define MMC_RSP_R7   (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)

This gives MMC_RSP_R1 and MMC_RSP_R6 the same value, so

drivers/mmc/tifm_sd.c: In function 'tifm_sd_op_flags':
drivers/mmc/tifm_sd.c:190: error: duplicate case value
drivers/mmc/tifm_sd.c:181: error: previously used here
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Pierre Ossman 
Philip Langdale wrote:
> @@ -1386,10 +1420,37 @@
>* all get the idea that they should be ready for CMD2.
>* (My SanDisk card seems to need this.)
>*/
> - if (host->mode == MMC_MODE_SD)
> - mmc_send_app_op_cond(host, host->ocr, NULL);
> - else
> + if (host->mode == MMC_MODE_SD) {
> + struct mmc_command cmd;
> + int err, ocr = host->ocr;
> + static const u8 test_pattern = 0xAA;
> + 
> + /*
> + * To support SD 2.0 cards, we must always invoke SD_SEND_IF_COND
> + * before SD_APP_OP_COND. This command will harmlessly fail for
> + * SD 1.0 cards.
> + */
> + cmd.opcode = SD_SEND_IF_COND;
> + cmd.arg = ((host->ocr & 0xFF8000) != 0) << 8 | test_pattern;
> + cmd.flags = MMC_RSP_R7 | MMC_CMD_BCR;
> + 
> + err = mmc_wait_for_cmd(host, , 0);
> + if (err != MMC_ERR_NONE ||
> + (cmd.resp[0] & 0xFF) == test_pattern) {
> + if (err == MMC_ERR_NONE) {
> + /*
> +  * If SD_SEND_IF_COND succeeded, we are dealing
> +  * with an SD 2.0 compliant card and we should
> +  * set bit 30 of the ocr to indicate that we
> +  * can handle block-addressed SDHC cards.
> +  */
> + ocr |= 1 << 30;
> + }
> + mmc_send_app_op_cond(host, ocr, NULL);
> + }
> + } else {
>   mmc_send_op_cond(host, host->ocr, NULL);
> + }
>
>   mmc_discover_cards(host);
>
>   

Nah... I think a mmc_send_if_cond() would be cleaner. The setup routine
should contain the sequence of events needed, while we abstract the
really low level grunt work into functions.

(more on that subject as soon as kernel.org has finished mirroring ;))

Rgds

-- 
 -- Pierre Ossman

  Linux kernel, MMC maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Pierre Ossman 
Philip Langdale wrote:
 @@ -1386,10 +1420,37 @@
* all get the idea that they should be ready for CMD2.
* (My SanDisk card seems to need this.)
*/
 - if (host-mode == MMC_MODE_SD)
 - mmc_send_app_op_cond(host, host-ocr, NULL);
 - else
 + if (host-mode == MMC_MODE_SD) {
 + struct mmc_command cmd;
 + int err, ocr = host-ocr;
 + static const u8 test_pattern = 0xAA;
 + 
 + /*
 + * To support SD 2.0 cards, we must always invoke SD_SEND_IF_COND
 + * before SD_APP_OP_COND. This command will harmlessly fail for
 + * SD 1.0 cards.
 + */
 + cmd.opcode = SD_SEND_IF_COND;
 + cmd.arg = ((host-ocr  0xFF8000) != 0)  8 | test_pattern;
 + cmd.flags = MMC_RSP_R7 | MMC_CMD_BCR;
 + 
 + err = mmc_wait_for_cmd(host, cmd, 0);
 + if (err != MMC_ERR_NONE ||
 + (cmd.resp[0]  0xFF) == test_pattern) {
 + if (err == MMC_ERR_NONE) {
 + /*
 +  * If SD_SEND_IF_COND succeeded, we are dealing
 +  * with an SD 2.0 compliant card and we should
 +  * set bit 30 of the ocr to indicate that we
 +  * can handle block-addressed SDHC cards.
 +  */
 + ocr |= 1  30;
 + }
 + mmc_send_app_op_cond(host, ocr, NULL);
 + }
 + } else {
   mmc_send_op_cond(host, host-ocr, NULL);
 + }

   mmc_discover_cards(host);

   

Nah... I think a mmc_send_if_cond() would be cleaner. The setup routine
should contain the sequence of events needed, while we abstract the
really low level grunt work into functions.

(more on that subject as soon as kernel.org has finished mirroring ;))

Rgds

-- 
 -- Pierre Ossman

  Linux kernel, MMC maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Andrew Morton 
On Mon, 01 Jan 2007 07:29:55 -0800
Philip Langdale [EMAIL PROTECTED] wrote:

  #define MMC_RSP_R1B  
 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY)
  #define MMC_RSP_R2   (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC)
  #define MMC_RSP_R3   (MMC_RSP_PRESENT)
 -#define MMC_RSP_R6   (MMC_RSP_PRESENT|MMC_RSP_CRC)
 +#define MMC_RSP_R6   (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 +#define MMC_RSP_R7   (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)

This gives MMC_RSP_R1 and MMC_RSP_R6 the same value, so

drivers/mmc/tifm_sd.c: In function 'tifm_sd_op_flags':
drivers/mmc/tifm_sd.c:190: error: duplicate case value
drivers/mmc/tifm_sd.c:181: error: previously used here
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Philip Langdale 
Andrew Morton wrote:
 On Mon, 01 Jan 2007 07:29:55 -0800
 Philip Langdale [EMAIL PROTECTED] wrote:
 
  #define MMC_RSP_R1B 
 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY)
  #define MMC_RSP_R2  (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC)
  #define MMC_RSP_R3  (MMC_RSP_PRESENT)
 -#define MMC_RSP_R6  (MMC_RSP_PRESENT|MMC_RSP_CRC)
 +#define MMC_RSP_R6  (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 +#define MMC_RSP_R7  (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 
 This gives MMC_RSP_R1 and MMC_RSP_R6 the same value, so
 
 drivers/mmc/tifm_sd.c: In function 'tifm_sd_op_flags':
 drivers/mmc/tifm_sd.c:190: error: duplicate case value
 drivers/mmc/tifm_sd.c:181: error: previously used here

This is a bug. The MMC_RSP_R? #defines do not fully characterise the
responses (specically, the way that the response is parsed is not
characterised) and consequently there is no guarantee of uniqueness.
Given this reality - the way that the tifm_sd driver works is unsafe.

If R6 had not been incorrectly defined (the missing RSP_OPCODE should
always have been there), then this code would not have worked. As things
currently stand, it is necessary to also check the command number to
decide on the correct response type - that's suboptimal and it's probably
good to uniquely identify the response in the mmc_command in some other
fashion.

I'm going to remove the R6 fix from my next diff to keep these things
distinct but this needs to be resolved.

--phil
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.6.19] mmc: Add support for SDHC cards (Take 2)

2007-01-03 Thread Pierre Ossman 
Philip Langdale wrote:
 This is a bug. The MMC_RSP_R? #defines do not fully characterise the
 responses (specically, the way that the response is parsed is not
 characterised) and consequently there is no guarantee of uniqueness.
 Given this reality - the way that the tifm_sd driver works is unsafe.

   

Amen to that. All hw vendors that implement this particular form of
brain damage should be dragged out and shot.

I'll fix a patch for this later on.

Rgds

-- 
 -- Pierre Ossman

  Linux kernel, MMC maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/