RE: RE: Bug in mmc.c == stops eMMC toshiba from working - sector check not good (PATCH)

RE: RE: Bug in mmc.c == stops eMMC toshiba from working - sector check not good (PATCH)

[Philip Rakity]

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

MMC 4.4 Spec JEDEC Standard No. 84-A44 says CMD1 response from card
has bit 30 set if card is high capacity.  (page 111 - Section 8.1)

Toshiba flash <= 2GB has EXT_CSD sectors but does not support
High Capacity but because of bad test -- card was put into sector mode
causing failure.

Signed-off-by: Philip Rakity <prakity@marvell.com>

diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
--- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-11 14:15:03.000000000 -0800
@@ -221,8 +221,6 @@
 			ext_csd[EXT_CSD_SEC_CNT + 1] << 8 |
 			ext_csd[EXT_CSD_SEC_CNT + 2] << 16 |
 			ext_csd[EXT_CSD_SEC_CNT + 3] << 24;
-		if (card->ext_csd.sectors)
-			mmc_card_set_blockaddr(card);
 	}
 
 	switch (ext_csd[EXT_CSD_CARD_TYPE]) {
@@ -306,6 +304,7 @@
 	int err;
 	u32 cid[4];
 	unsigned int max_dtr;
+	u32 rocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
@@ -319,7 +318,7 @@
 	mmc_go_idle(host);
 
 	/* The extra bit indicates that we support high capacity */
-	err = mmc_send_op_cond(host, ocr | (1 << 30), NULL);
+	err = mmc_send_op_cond(host, ocr | MMC_CARD_SECTOR_ADDR, &rocr);
 	if (err)
 		goto err;
 
@@ -407,6 +406,9 @@
 		err = mmc_read_ext_csd(card);
 		if (err)
 			goto free_card;
+
+		if(rocr & MMC_CARD_SECTOR_ADDR)
+		  mmc_card_set_blockaddr(card);
 	}
 
 	/*
diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
--- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-11 13:47:00.000000000 -0800
@@ -202,6 +202,7 @@
  * OCR bits are mostly in host.h
  */
 #define MMC_CARD_BUSY	0x80000000	/* Card Power up status bit */
+#define MMC_CARD_SECTOR_ADDR	0x40000000	/* Card supports sectors */
 
 /*
  * Card Command Classes (CCC)

[-- Attachment #2: eMMCHighCapacity.patch --]
[-- Type: application/octet-stream, Size: 1933 bytes --]

MMC 4.4 Spec JEDEC Standard No. 84-A44 says CMD1 response from card
has bit 30 set if card is high capacity.  (page 111 - Section 8.1)

Toshiba flash <= 2GB has EXT_CSD sectors but does not support
High Capacity but because of bad test -- card was put into sector mode
causing failure.

Signed-off-by: Philip Rakity <prakity@marvell.com>

diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
--- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-11 14:15:03.000000000 -0800
@@ -221,8 +221,6 @@
 			ext_csd[EXT_CSD_SEC_CNT + 1] << 8 |
 			ext_csd[EXT_CSD_SEC_CNT + 2] << 16 |
 			ext_csd[EXT_CSD_SEC_CNT + 3] << 24;
-		if (card->ext_csd.sectors)
-			mmc_card_set_blockaddr(card);
 	}
 
 	switch (ext_csd[EXT_CSD_CARD_TYPE]) {
@@ -306,6 +304,7 @@
 	int err;
 	u32 cid[4];
 	unsigned int max_dtr;
+	u32 rocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
@@ -319,7 +318,7 @@
 	mmc_go_idle(host);
 
 	/* The extra bit indicates that we support high capacity */
-	err = mmc_send_op_cond(host, ocr | (1 << 30), NULL);
+	err = mmc_send_op_cond(host, ocr | MMC_CARD_SECTOR_ADDR, &rocr);
 	if (err)
 		goto err;
 
@@ -407,6 +406,9 @@
 		err = mmc_read_ext_csd(card);
 		if (err)
 			goto free_card;
+
+		if(rocr & MMC_CARD_SECTOR_ADDR)
+		  mmc_card_set_blockaddr(card);
 	}
 
 	/*
diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
--- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-11 13:47:00.000000000 -0800
@@ -202,6 +202,7 @@
  * OCR bits are mostly in host.h
  */
 #define MMC_CARD_BUSY	0x80000000	/* Card Power up status bit */
+#define MMC_CARD_SECTOR_ADDR	0x40000000	/* Card supports sectors */
 
 /*
  * Card Command Classes (CCC)

Patch: eMMC boot partition needs to be deactivated for linux to find user partitions

[Philip Rakity]

Some eMMC chips have a boot partition that is meant to be used to load in low level boot code.
This partition is available when the chip is powered up.  Normally the boot loader would disable
access to the partition once boot code was loaded in before passing control to linux.  

if booting occurs from another device (not the eMMC chip) the partition will not be disabled by
the boot loader and control will be passed to linux.  This will cause linux to not recognize user
partitions on the chip unless access to the boot partition is deactivated.

See JEDEC Standard 84-A44 (eMMC 4.4 spec) -- Page 139

signed off by:  Philip Rakity <prakity@marvell.com>
diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
--- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
@@ -430,6 +432,13 @@
 	}
 
 	/*
+	* ensure eMMC private booting PARTITION is not enabled
+	* see JEDEC Standard No. 84-A44 - Page 139 
+	*/
+	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+		EXT_CSD_BOOT_CONFIG, 0x0);
+
+	/*
 	 * Compute bus speed.
 	 */
 	max_dtr = (unsigned int)-1;
diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
--- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
@@ -251,6 +252,7 @@
  * EXT_CSD fields
  */
 
+#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
 #define EXT_CSD_BUS_WIDTH	183	/* R/W */
 #define EXT_CSD_HS_TIMING	185	/* R/W */
 #define EXT_CSD_CARD_TYPE	196	/* RO */

Re: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions

[Adrian Hunter]

Philip Rakity wrote:
> Some eMMC chips have a boot partition that is meant to be used to load in low level boot code.
> This partition is available when the chip is powered up.  Normally the boot loader would disable
> access to the partition once boot code was loaded in before passing control to linux.  
> 
> if booting occurs from another device (not the eMMC chip) the partition will not be disabled by
> the boot loader and control will be passed to linux.  This will cause linux to not recognize user
> partitions on the chip unless access to the boot partition is deactivated.
> 
> See JEDEC Standard 84-A44 (eMMC 4.4 spec) -- Page 139

Page 139 doesn't say anything about why you need that switch command.
Please provide a more useful reference or delete this.

Boot mode is terminated by CMD1, so that switch command should not
be needed.  Please explain why it is needed in more detail.

That switch command should not be used for devices that do not
support it e.g. eMMC 4.3 and before.


> 
> signed off by:  Philip Rakity <prakity@marvell.com>
> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
> --- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
> @@ -430,6 +432,13 @@
>  	}
>  
>  	/*
> +	* ensure eMMC private booting PARTITION is not enabled
> +	* see JEDEC Standard No. 84-A44 - Page 139 
> +	*/
> +	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +		EXT_CSD_BOOT_CONFIG, 0x0);
> +
> +	/*
>  	 * Compute bus speed.
>  	 */
>  	max_dtr = (unsigned int)-1;
> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
> --- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
> @@ -251,6 +252,7 @@
>   * EXT_CSD fields
>   */
>  
> +#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
>  #define EXT_CSD_BUS_WIDTH	183	/* R/W */
>  #define EXT_CSD_HS_TIMING	185	/* R/W */
>  #define EXT_CSD_CARD_TYPE	196	/* RO */
> 
> --
> 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
> 

Features vs versions (Was: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions)

[Edward Falk]

Adrian Hunter wrote:

> That switch command should not be used for devices that do not
> support it e.g. eMMC 4.3 and before.

I've been wondering about this -- how can you tell which feature is 
available in which version of the standard?  I have 84-A44 in front of 
me now, and I can't see it.  Do I need to just obtain older versions of 
the standards doc and compare them by hand?

	-ed falk

RE: Features vs versions (Was: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions)

[Johnson, Charles F]

At the back of the document is a change list for each revision.  On the 84-A44 spec I have it is Annex B, on page 209. 

Charles Johnson
Ultra-Mobility Group
UPSG Architecture
Intel Corporation
charles.f.johnson@intel.com
 

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Edward Falk
Sent: Monday, March 15, 2010 11:33 AM
To: linux-mmc@vger.kernel.org
Subject: Features vs versions (Was: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions)

Adrian Hunter wrote:

> That switch command should not be used for devices that do not
> support it e.g. eMMC 4.3 and before.

I've been wondering about this -- how can you tell which feature is 
available in which version of the standard?  I have 84-A44 in front of 
me now, and I can't see it.  Do I need to just obtain older versions of 
the standards doc and compare them by hand?

	-ed falk
--
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

Re: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions

[Philip Rakity]

Adiran,

snip for mmc_debug below

I will forward to you under full logs.

Philip

Without mod the following error occurs
==============================

mmc1: req done (CMD6): 0: 00000900 00000000 00000000 00000000
mmc1: clock 48000000Hz busmode 2 powermode 2 cs 0 Vdd 7 width 3 timing 1
pxa-sdh [platform_set_8_bit()]: EXIT: mmc1 SD_CE_ATA_2 = 1100
mmc1: new high speed MMC card at address 0001
mmc1: starting CMD16 arg 00000200 flags 00000095
pxa-sdh [platform_specific_card_detect()]: EXIT: mmc1 card_detect_ok = 0000
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000001
mmc1: req done (CMD16): 0: 00000900 00000000 00000000 00000000
mmcblk0: mmc1:0001 M2G1DD 1.90 GiB
 mmcblk0:<7>mmc1: starting CMD18 arg 00000000 flags 000000b5
mmc1:     blksz 512 blocks 8 flags 00000200 tsac 150 ms nsac 1000
mmc1:     CMD12 arg 00000000 flags 0000049d
pxa-sdh [platform_specific_card_detect()]: EXIT: mmc1 card_detect_ok = 0000
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000001
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000002
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000001
mmc1: req done (CMD18): 0: 00000900 00000000 00000000 00000000
mmc1:     4096 bytes transferred: 0
mmc1:     (CMD12): 0: 00000b00 00000000 00000000 00000000
 unknown partition table


with the patch 
===========

mmc1: req done (CMD6): 0: 00000900 00000000 00000000 00000000
mmc1: clock 48000000Hz busmode 2 powermode 2 cs 0 Vdd 7 width 3 timing 1
pxa-sdh [platform_set_8_bit()]: EXIT: mmc1 SD_CE_ATA_2 = 1100
mmc1: new high speed MMC card at address 0001
mmc1: starting CMD16 arg 00000200 flags 00000095
pxa-sdh [platform_specific_card_detect()]: EXIT: mmc1 card_detect_ok = 0000
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000001
mmc1: req done (CMD16): 0: 00000900 00000000 00000000 00000000
mmcblk0: mmc1:0001 M2G1DD 1.90 GiB
 mmcblk0:<7>mmc1: starting CMD18 arg 00000000 flags 000000b5
mmc1:     blksz 512 blocks 8 flags 00000200 tsac 150 ms nsac 1000
mmc1:     CMD12 arg 00000000 flags 0000049d
pxa-sdh [platform_specific_card_detect()]: EXIT: mmc1 card_detect_ok = 0000
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000001
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000002
pxa-sdh [sdhci_irq()]: *** mmc1 got interrupt: 0x00000001
mmc1: req done (CMD18): 0: 00000900 00000000 00000000 00000000
mmc1:     4096 bytes transferred: 0
mmc1:     (CMD12): 0: 00000b00 00000000 00000000 00000000
 p1 p2 p3 p4




On Mar 15, 2010, at 1:08 AM, Adrian Hunter wrote:

> Philip Rakity wrote:
>> Some eMMC chips have a boot partition that is meant to be used to load in low level boot code.
>> This partition is available when the chip is powered up.  Normally the boot loader would disable
>> access to the partition once boot code was loaded in before passing control to linux.  
>> 
>> if booting occurs from another device (not the eMMC chip) the partition will not be disabled by
>> the boot loader and control will be passed to linux.  This will cause linux to not recognize user
>> partitions on the chip unless access to the boot partition is deactivated.
>> 
>> See JEDEC Standard 84-A44 (eMMC 4.4 spec) -- Page 139
> 
> Page 139 doesn't say anything about why you need that switch command.
> Please provide a more useful reference or delete this.
> 
> Boot mode is terminated by CMD1, so that switch command should not
> be needed.  Please explain why it is needed in more detail.
> 
> That switch command should not be used for devices that do not
> support it e.g. eMMC 4.3 and before.
> 
> 
>> 
>> signed off by:  Philip Rakity <prakity@marvell.com>
>> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
>> --- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
>> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
>> @@ -430,6 +432,13 @@
>> 	}
>> 
>> 	/*
>> +	* ensure eMMC private booting PARTITION is not enabled
>> +	* see JEDEC Standard No. 84-A44 - Page 139 
>> +	*/
>> +	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> +		EXT_CSD_BOOT_CONFIG, 0x0);
>> +
>> +	/*
>> 	 * Compute bus speed.
>> 	 */
>> 	max_dtr = (unsigned int)-1;
>> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
>> --- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
>> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
>> @@ -251,6 +252,7 @@
>>  * EXT_CSD fields
>>  */
>> 
>> +#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
>> #define EXT_CSD_BUS_WIDTH	183	/* R/W */
>> #define EXT_CSD_HS_TIMING	185	/* R/W */
>> #define EXT_CSD_CARD_TYPE	196	/* RO */
>> 
>> --
>> 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
>> 
> 

Re: Bug in mmc.c == stops eMMC toshiba from working - sector check not good (REPOST) (PATCH)

[Philip Rakity]

On Mar 11, 2010, at 9:17 PM, Philip Rakity wrote:

> MMC 4.4 Spec JEDEC Standard No. 84-A44 says CMD1 response from card
> has bit 30 set if card is high capacity.  (page 111 - Section 8.1)
> 
> Toshiba flash <= 2GB has EXT_CSD sectors but does not support
> High Capacity but because of bad test -- card was put into sector mode
> causing failure.
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> 
> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
> --- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-11 14:15:03.000000000 -0800
> @@ -221,8 +221,6 @@
> 			ext_csd[EXT_CSD_SEC_CNT + 1] << 8 |
> 			ext_csd[EXT_CSD_SEC_CNT + 2] << 16 |
> 			ext_csd[EXT_CSD_SEC_CNT + 3] << 24;
> -		if (card->ext_csd.sectors)
> -			mmc_card_set_blockaddr(card);
> 	}
> 
> 	switch (ext_csd[EXT_CSD_CARD_TYPE]) {
> @@ -306,6 +304,7 @@
> 	int err;
> 	u32 cid[4];
> 	unsigned int max_dtr;
> +	u32 rocr;
> 
> 	BUG_ON(!host);
> 	WARN_ON(!host->claimed);
> @@ -319,7 +318,7 @@
> 	mmc_go_idle(host);
> 
> 	/* The extra bit indicates that we support high capacity */
> -	err = mmc_send_op_cond(host, ocr | (1 << 30), NULL);
> +	err = mmc_send_op_cond(host, ocr | MMC_CARD_SECTOR_ADDR, &rocr);
> 	if (err)
> 		goto err;
> 
> @@ -407,6 +406,9 @@
> 		err = mmc_read_ext_csd(card);
> 		if (err)
> 			goto free_card;
> +
> +		if(rocr & MMC_CARD_SECTOR_ADDR)
> +		  mmc_card_set_blockaddr(card);
> 	}
> 
> 	/*
> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
> --- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-11 13:47:00.000000000 -0800
> @@ -202,6 +202,7 @@
>  * OCR bits are mostly in host.h
>  */
> #define MMC_CARD_BUSY	0x80000000	/* Card Power up status bit */
> +#define MMC_CARD_SECTOR_ADDR	0x40000000	/* Card supports sectors */
> 
> /*
>  * Card Command Classes (CCC)
> <eMMCHighCapacity.patch>

Re: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions

[Philip Rakity]

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


Adrian,

Thank you for the feedback.  I have amended the patch.  I tested the patch against mmc4.3 or earlier cards and it works.
Your comment about the switch boot config not being supported by earlier cards is handled by NOT checking for any errors. 

The reason the patch is needed is because when our  boot loader gets loaded from the private boot area, it sets the partition to the private boot area and then hands control to a secondary loader.  Normally our boot loader would disable access to the partition once boot code was loaded in before passing control to linux but if the process is stopped or the kernel is loaded from another device the partition number is not reset.  

We can fix this but it seems good practice for linux to reset the flash to a normal state.

The other concerns I believe were addressed earlier.  The trace showing what was happening with and without the patch.

regards,
Philip

signed-off-by: Philip Rakity <prakity@marvell.com>
diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
--- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
@@ -430,6 +432,13 @@
 	}
 
 	/*
+	* ensure eMMC private booting PARTITION is not enabled
+	*/
+	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+		EXT_CSD_BOOT_CONFIG, 0x0);
+
+	/*
 	 * Compute bus speed.
 	 */
 	max_dtr = (unsigned int)-1;
diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
--- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
@@ -251,6 +252,7 @@
  * EXT_CSD fields
  */
 
+#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
 #define EXT_CSD_BUS_WIDTH	183	/* R/W */
 #define EXT_CSD_HS_TIMING	185	/* R/W */
 #define EXT_CSD_CARD_TYPE	196	/* RO */



On Mar 15, 2010, at 1:08 AM, Adrian Hunter wrote:

> Philip Rakity wrote:
>> Some eMMC chips have a boot partition that is meant to be used to load in low level boot code.
>> This partition is available when the chip is powered up.  Normally the boot loader would disable
>> access to the partition once boot code was loaded in before passing control to linux.  
>> 
>> if booting occurs from another device (not the eMMC chip) the partition will not be disabled by
>> the boot loader and control will be passed to linux.  This will cause linux to not recognize user
>> partitions on the chip unless access to the boot partition is deactivated.
>> 
>> See JEDEC Standard 84-A44 (eMMC 4.4 spec) -- Page 139
> 
> Page 139 doesn't say anything about why you need that switch command.
> Please provide a more useful reference or delete this.
> 
> Boot mode is terminated by CMD1, so that switch command should not
> be needed.  Please explain why it is needed in more detail.
> 
> That switch command should not be used for devices that do not
> support it e.g. eMMC 4.3 and before.
> 
> 
>> 
>> signed off by:  Philip Rakity <prakity@marvell.com>
>> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
>> --- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
>> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
>> @@ -430,6 +432,13 @@
>> 	}
>> 
>> 	/*
>> +	* ensure eMMC private booting PARTITION is not enabled
>> +	* see JEDEC Standard No. 84-A44 - Page 139 
>> +	*/
>> +	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> +		EXT_CSD_BOOT_CONFIG, 0x0);
>> +
>> +	/*
>> 	 * Compute bus speed.
>> 	 */
>> 	max_dtr = (unsigned int)-1;
>> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
>> --- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
>> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
>> @@ -251,6 +252,7 @@
>>  * EXT_CSD fields
>>  */
>> 
>> +#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
>> #define EXT_CSD_BUS_WIDTH	183	/* R/W */
>> #define EXT_CSD_HS_TIMING	185	/* R/W */
>> #define EXT_CSD_CARD_TYPE	196	/* RO */
>> 
>> --
>> 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
>> 
> 

[-- Attachment #2: eMMCPartitionPatch.txt --]
[-- Type: text/plain, Size: 1030 bytes --]

signed-off-by: Philip Rakity <prakity@marvell.com>
diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
--- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
@@ -430,6 +432,13 @@
 	}
 
 	/*
+	* ensure eMMC private booting PARTITION is not enabled
+	*/
+	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+		EXT_CSD_BOOT_CONFIG, 0x0);
+
+	/*
 	 * Compute bus speed.
 	 */
 	max_dtr = (unsigned int)-1;
diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
--- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
+++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
@@ -251,6 +252,7 @@
  * EXT_CSD fields
  */
 
+#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
 #define EXT_CSD_BUS_WIDTH	183	/* R/W */
 #define EXT_CSD_HS_TIMING	185	/* R/W */
 #define EXT_CSD_CARD_TYPE	196	/* RO */

Re: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions

[Adrian Hunter]

Philip Rakity wrote:
> Adrian,
> 
> Thank you for the feedback.  I have amended the patch.  I tested the patch against mmc4.3 or earlier cards and it works.
> Your comment about the switch boot config not being supported by earlier cards is handled by NOT checking for any errors. 

I am not sure I see the value in doing a switch command
when it is not needed or expected.

In addition, messing with the reserved bits and bits that do
not need to be changed seems like a bad idea.

I would go for something more complicated:

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 0eac6c8..cb8b2d0 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -247,6 +247,9 @@ static int mmc_read_ext_csd(struct mmc_card *card)
 		if (sa_shift > 0 && sa_shift <= 0x17)
 			card->ext_csd.sa_timeout =
 					1 << ext_csd[EXT_CSD_S_A_TIMEOUT];
+
+		card->ext_csd.partition_access = ext_csd[EXT_CSD_BOOT_CONFIG] &
+						 MMC_BOOT_PART_ACC_MASK;
 	}
 
 out:
@@ -430,6 +433,18 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	}
 
 	/*
+	 * Ensure eMMC private booting PARTITION is not enabled.
+	 */
+	if (card->ext_csd.partition_access != 0) {
+		err = mmc_do_switch(card, MMC_SWITCH_MODE_CLEAR_BITS, 0,
+				    EXT_CSD_BOOT_CONFIG,
+				    MMC_BOOT_PART_ACC_MASK);
+		if (err)
+			goto free_card;
+		card->ext_csd.partition_access = 0;
+	}
+
+	/*
 	 * Compute bus speed.
 	 */
 	max_dtr = (unsigned int)-1;
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index d2cb5c6..86800b3 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -386,7 +386,7 @@ int mmc_spi_set_crc(struct mmc_host *host, int use_crc)
 	return err;
 }
 
-int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
+int mmc_do_switch(struct mmc_card *card, u8 access, u8 set, u8 index, u8 value)
 {
 	int err;
 	struct mmc_command cmd;
@@ -398,7 +398,7 @@ int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
 	memset(&cmd, 0, sizeof(struct mmc_command));
 
 	cmd.opcode = MMC_SWITCH;
-	cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
+	cmd.arg = (access << 24) |
 		  (index << 16) |
 		  (value << 8) |
 		  set;
@@ -433,6 +433,12 @@ int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
 	return 0;
 }
 
+int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
+{
+	return mmc_do_switch(card, MMC_SWITCH_MODE_WRITE_BYTE, set, index,
+			     value);
+}
+
 int mmc_send_status(struct mmc_card *card, u32 *status)
 {
 	int err;
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 653eb8e..df07950 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -20,6 +20,7 @@ int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
 int mmc_set_relative_addr(struct mmc_card *card);
 int mmc_send_csd(struct mmc_card *card, u32 *csd);
 int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);
+int mmc_do_switch(struct mmc_card *card, u8 access, u8 set, u8 index, u8 value);
 int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value);
 int mmc_send_status(struct mmc_card *card, u32 *status);
 int mmc_send_cid(struct mmc_host *host, u32 *cid);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index d02d2c6..8ac4351 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -41,6 +41,7 @@ struct mmc_csd {
 
 struct mmc_ext_csd {
 	u8			rev;
+	unsigned int		partition_access:3;
 	unsigned int		sa_timeout;		/* Units: 100ns */
 	unsigned int		hs_max_dtr;
 	unsigned int		sectors;
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index c02c8db..14a161c 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -251,6 +251,7 @@ struct _mmc_csd {
  * EXT_CSD fields
  */
 
+#define EXT_CSD_BOOT_CONFIG	179	/* R/W */
 #define EXT_CSD_BUS_WIDTH	183	/* R/W */
 #define EXT_CSD_HS_TIMING	185	/* R/W */
 #define EXT_CSD_CARD_TYPE	196	/* RO */
@@ -273,6 +274,8 @@ struct _mmc_csd {
 #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
 #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
 
+#define MMC_BOOT_PART_ACC_MASK	0x3
+
 /*
  * MMC_SWITCH access modes
  */
> 
> The reason the patch is needed is because when our  boot loader gets loaded from the private boot area, it sets the partition to the private boot area and then hands control to a secondary loader.  Normally our boot loader would disable access to the partition once boot code was loaded in before passing control to linux but if the process is stopped or the kernel is loaded from another device the partition number is not reset.  
> 
> We can fix this but it seems good practice for linux to reset the flash to a normal state.
> 
> The other concerns I believe were addressed earlier.  The trace showing what was happening with and without the patch.
> 
> regards,
> Philip
> 
> signed-off-by: Philip Rakity <prakity@marvell.com>
> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
> --- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
> @@ -430,6 +432,13 @@
>  	}
>  
>  	/*
> +	* ensure eMMC private booting PARTITION is not enabled
> +	*/
> +	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +		EXT_CSD_BOOT_CONFIG, 0x0);
> +
> +	/*
>  	 * Compute bus speed.
>  	 */
>  	max_dtr = (unsigned int)-1;
> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
> --- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
> @@ -251,6 +252,7 @@
>   * EXT_CSD fields
>   */
>  
> +#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
>  #define EXT_CSD_BUS_WIDTH	183	/* R/W */
>  #define EXT_CSD_HS_TIMING	185	/* R/W */
>  #define EXT_CSD_CARD_TYPE	196	/* RO */
> 
> 
> 
> On Mar 15, 2010, at 1:08 AM, Adrian Hunter wrote:
> 
>> Philip Rakity wrote:
>>> Some eMMC chips have a boot partition that is meant to be used to load in low level boot code.
>>> This partition is available when the chip is powered up.  Normally the boot loader would disable
>>> access to the partition once boot code was loaded in before passing control to linux.  
>>>
>>> if booting occurs from another device (not the eMMC chip) the partition will not be disabled by
>>> the boot loader and control will be passed to linux.  This will cause linux to not recognize user
>>> partitions on the chip unless access to the boot partition is deactivated.
>>>
>>> See JEDEC Standard 84-A44 (eMMC 4.4 spec) -- Page 139
>> Page 139 doesn't say anything about why you need that switch command.
>> Please provide a more useful reference or delete this.
>>
>> Boot mode is terminated by CMD1, so that switch command should not
>> be needed.  Please explain why it is needed in more detail.
>>
>> That switch command should not be used for devices that do not
>> support it e.g. eMMC 4.3 and before.
>>
>>
>>> signed off by:  Philip Rakity <prakity@marvell.com>
>>> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
>>> --- linux-2.6.32.8/drivers/mmc/core/mmc.c	2010-02-09 04:57:19.000000000 -0800
>>> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c	2010-03-12 20:56:16.000000000 -0800
>>> @@ -430,6 +432,13 @@
>>> 	}
>>>
>>> 	/*
>>> +	* ensure eMMC private booting PARTITION is not enabled
>>> +	* see JEDEC Standard No. 84-A44 - Page 139 
>>> +	*/
>>> +	mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>> +		EXT_CSD_BOOT_CONFIG, 0x0);
>>> +
>>> +	/*
>>> 	 * Compute bus speed.
>>> 	 */
>>> 	max_dtr = (unsigned int)-1;
>>> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
>>> --- linux-2.6.32.8/include/linux/mmc/mmc.h	2010-02-09 04:57:19.000000000 -0800
>>> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h	2010-03-12 20:53:48.000000000 -0800
>>> @@ -251,6 +252,7 @@
>>>  * EXT_CSD fields
>>>  */
>>>
>>> +#define EXT_CSD_BOOT_CONFIG	179 /* R/W */
>>> #define EXT_CSD_BUS_WIDTH	183	/* R/W */
>>> #define EXT_CSD_HS_TIMING	185	/* R/W */
>>> #define EXT_CSD_CARD_TYPE	196	/* RO */
>>>
>>> --
>>> 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
>>>

Re: Patch: eMMC boot partition needs to be deactivated for linux to find user partitions

[Adrian Hunter]

Philip Rakity wrote:
> Adrian,
> 
> I modified your suggested patch.     i am comfortable with you suggestion but am worried about being the first person to use MMC_SWITCH_MODE_CLEAR_BITS so I read the value and clear the bits in the code.

As you wish.

> 
> Couple of other changes
> a) Mask is 3 bits for partition

Yes it is 3 bits sorry.

> 
> Bit[2:0] : PARTITION_ACCESS (before BOOT_PARTITION_ACCESS, R/W/E_P)
> User selects partitions to access 0x0 : No access to boot partition (default) 0x1 : R/W boot partition 1 0x2 : R/W boot partition 2 0x3 : R/W Replay Protected Memory Block (RPMB) 0x4 : Access to General Purpose partition 1 0x5 : Access to General Purpose partition 2 0x6 : Access to General Purpose partition 3 0x7 : Access to General Purpose partition 4
> 
> b) do not test for failure on switch failure.   I  believe it is better to just keep going on the grounds in case we happen to have the right mapping.  No strong opinion.

I thought it better to prevent users potentially getting access to the boot partition.

> 
> I will test these changes over the next few days or go back to your original patch (with modified bit mask)
> 
> What do you suggest?
> 
> regards,
> 
> Philip
> 
> On Mar 22, 2010, at 1:25 AM, Adrian Hunter wrote:
> 
> Philip Rakity wrote:
> Adrian,
> 
> Thank you for the feedback.  I have amended the patch.  I tested the patch against mmc4.3 or earlier cards and it works.
> Your comment about the switch boot config not being supported by earlier cards is handled by NOT checking for any errors.
> 
> I am not sure I see the value in doing a switch command
> when it is not needed or expected.
> 
> In addition, messing with the reserved bits and bits that do
> not need to be changed seems like a bad idea.
> 
> I would go for something more complicated:
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 0eac6c8..cb8b2d0 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -247,6 +247,9 @@ static int mmc_read_ext_csd(struct mmc_card *card)
> if (sa_shift > 0 && sa_shift <= 0x17)
> card->ext_csd.sa_timeout =
> 1 << ext_csd[EXT_CSD_S_A_TIMEOUT];
> +
> + card->ext_csd.partition_access = ext_csd[EXT_CSD_BOOT_CONFIG] &
> + MMC_BOOT_PART_ACC_MASK;
> }
> 
> out:
> @@ -430,6 +433,18 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> }
> 
> /*
> + * Ensure eMMC private booting PARTITION is not enabled.
> + */
> + if (card->ext_csd.partition_access != 0) {
> + err = mmc_do_switch(card, MMC_SWITCH_MODE_CLEAR_BITS, 0,
> +    EXT_CSD_BOOT_CONFIG,
> +    MMC_BOOT_PART_ACC_MASK);
> + if (err)
> + goto free_card;
> + card->ext_csd.partition_access = 0;
> + }
> +
> + /*
> * Compute bus speed.
> */
> max_dtr = (unsigned int)-1;
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index d2cb5c6..86800b3 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -386,7 +386,7 @@ int mmc_spi_set_crc(struct mmc_host *host, int use_crc)
> return err;
> }
> 
> -int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
> +int mmc_do_switch(struct mmc_card *card, u8 access, u8 set, u8 index, u8 value)
> {
> int err;
> struct mmc_command cmd;
> @@ -398,7 +398,7 @@ int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
> memset(&cmd, 0, sizeof(struct mmc_command));
> 
> cmd.opcode = MMC_SWITCH;
> - cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
> + cmd.arg = (access << 24) |
>  (index << 16) |
>  (value << 8) |
>  set;
> @@ -433,6 +433,12 @@ int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
> return 0;
> }
> 
> +int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
> +{
> + return mmc_do_switch(card, MMC_SWITCH_MODE_WRITE_BYTE, set, index,
> +     value);
> +}
> +
> int mmc_send_status(struct mmc_card *card, u32 *status)
> {
> int err;
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 653eb8e..df07950 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -20,6 +20,7 @@ int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
> int mmc_set_relative_addr(struct mmc_card *card);
> int mmc_send_csd(struct mmc_card *card, u32 *csd);
> int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);
> +int mmc_do_switch(struct mmc_card *card, u8 access, u8 set, u8 index, u8 value);
> int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value);
> int mmc_send_status(struct mmc_card *card, u32 *status);
> int mmc_send_cid(struct mmc_host *host, u32 *cid);
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index d02d2c6..8ac4351 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -41,6 +41,7 @@ struct mmc_csd {
> 
> struct mmc_ext_csd {
> u8 rev;
> + unsigned int partition_access:3;
> unsigned int sa_timeout; /* Units: 100ns */
> unsigned int hs_max_dtr;
> unsigned int sectors;
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index c02c8db..14a161c 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -251,6 +251,7 @@ struct _mmc_csd {
>  * EXT_CSD fields
>  */
> 
> +#define EXT_CSD_BOOT_CONFIG 179 /* R/W */
> #define EXT_CSD_BUS_WIDTH 183 /* R/W */
> #define EXT_CSD_HS_TIMING 185 /* R/W */
> #define EXT_CSD_CARD_TYPE 196 /* RO */
> @@ -273,6 +274,8 @@ struct _mmc_csd {
> #define EXT_CSD_BUS_WIDTH_4 1 /* Card is in 4 bit mode */
> #define EXT_CSD_BUS_WIDTH_8 2 /* Card is in 8 bit mode */
> 
> +#define MMC_BOOT_PART_ACC_MASK 0x3
> +
> /*
>  * MMC_SWITCH access modes
>  */
> 
> The reason the patch is needed is because when our  boot loader gets loaded from the private boot area, it sets the partition to the private boot area and then hands control to a secondary loader.  Normally our boot loader would disable access to the partition once boot code was loaded in before passing control to linux but if the process is stopped or the kernel is loaded from another device the partition number is not reset.
> 
> We can fix this but it seems good practice for linux to reset the flash to a normal state.
> 
> The other concerns I believe were addressed earlier.  The trace showing what was happening with and without the patch.
> 
> regards,
> Philip
> 
> signed-off-by: Philip Rakity <prakity@marvell.com<mailto:prakity@marvell.com>>
> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
> --- linux-2.6.32.8/drivers/mmc/core/mmc.c 2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c 2010-03-12 20:56:16.000000000 -0800
> @@ -430,6 +432,13 @@
> }
> 
> /*
> + * ensure eMMC private booting PARTITION is not enabled
> + */
> + mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_BOOT_CONFIG, 0x0);
> +
> + /*
> * Compute bus speed.
> */
> max_dtr = (unsigned int)-1;
> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
> --- linux-2.6.32.8/include/linux/mmc/mmc.h 2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h 2010-03-12 20:53:48.000000000 -0800
> @@ -251,6 +252,7 @@
>  * EXT_CSD fields
>  */
> 
> +#define EXT_CSD_BOOT_CONFIG 179 /* R/W */
> #define EXT_CSD_BUS_WIDTH 183 /* R/W */
> #define EXT_CSD_HS_TIMING 185 /* R/W */
> #define EXT_CSD_CARD_TYPE 196 /* RO */
> 
> 
> 
> On Mar 15, 2010, at 1:08 AM, Adrian Hunter wrote:
> 
> Philip Rakity wrote:
> Some eMMC chips have a boot partition that is meant to be used to load in low level boot code.
> This partition is available when the chip is powered up.  Normally the boot loader would disable
> access to the partition once boot code was loaded in before passing control to linux.
> 
> if booting occurs from another device (not the eMMC chip) the partition will not be disabled by
> the boot loader and control will be passed to linux.  This will cause linux to not recognize user
> partitions on the chip unless access to the boot partition is deactivated.
> 
> See JEDEC Standard 84-A44 (eMMC 4.4 spec) -- Page 139
> Page 139 doesn't say anything about why you need that switch command.
> Please provide a more useful reference or delete this.
> 
> Boot mode is terminated by CMD1, so that switch command should not
> be needed.  Please explain why it is needed in more detail.
> 
> That switch command should not be used for devices that do not
> support it e.g. eMMC 4.3 and before.
> 
> 
> signed off by:  Philip Rakity <prakity@marvell.com<mailto:prakity@marvell.com>>
> diff -ru linux-2.6.32.8/drivers/mmc/core/mmc.c linux-2.6.32.8 copy/drivers/mmc/core/mmc.c
> --- linux-2.6.32.8/drivers/mmc/core/mmc.c 2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/drivers/mmc/core/mmc.c 2010-03-12 20:56:16.000000000 -0800
> @@ -430,6 +432,13 @@
> }
> 
> /*
> + * ensure eMMC private booting PARTITION is not enabled
> + * see JEDEC Standard No. 84-A44 - Page 139
> + */
> + mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_BOOT_CONFIG, 0x0);
> +
> + /*
> * Compute bus speed.
> */
> max_dtr = (unsigned int)-1;
> diff -ru linux-2.6.32.8/include/linux/mmc/mmc.h linux-2.6.32.8 copy/include/linux/mmc/mmc.h
> --- linux-2.6.32.8/include/linux/mmc/mmc.h 2010-02-09 04:57:19.000000000 -0800
> +++ linux-2.6.32.8 copy/include/linux/mmc/mmc.h 2010-03-12 20:53:48.000000000 -0800
> @@ -251,6 +252,7 @@
> * EXT_CSD fields
> */
> 
> +#define EXT_CSD_BOOT_CONFIG 179 /* R/W */
> #define EXT_CSD_BUS_WIDTH 183 /* R/W */
> #define EXT_CSD_HS_TIMING 185 /* R/W */
> #define EXT_CSD_CARD_TYPE 196 /* RO */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org<mailto:majordomo@vger.kernel.org>
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> 

[PATCH] sdhci: add quirk for controllers that don't support write only detect

[Philip Rakity]

Some board designs for embedded systems  do not always bring up the SD Host Write Protect Signal.  
This patch defines a new quirk that when enabled enables write access.

>From 032242c62e0fe22eb6f5a4e0311eaab67ef54cbe Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Sun, 19 Sep 2010 14:34:02 -0700
Subject: [PATCH] sdhci: add quirk for controllers that don't support write only detect
 Signed-off-by: Philip Rakity <prakity@marvell.com>

---
 drivers/mmc/host/sdhci.c |    2 ++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 401527d..e2b1f32 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1213,6 +1213,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		present = 0;
+	else if (host->quirks & SDHCI_NO_SUPPORT_FOR_WRITE_PROTECT)
+		present = SDHCI_WRITE_PROTECT;
 	else
 		present = sdhci_readl(host, SDHCI_PRESENT_STATE);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d316bc7..290b5a8 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -247,6 +247,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12		(1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT			(1<<29)
+/* Controller does not have write protect signal		*/
+#define SDHCI_NO_SUPPORT_FOR_WRITE_PROTECT		(1<<30)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.6.0.4
 

Re: [PATCH] sdhci: add quirk for controllers that don't support write only detect

[Wolfram Sang]

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

On Sun, Sep 19, 2010 at 02:46:40PM -0700, Philip Rakity wrote:
> 
> 
> Some board designs for embedded systems  do not always bring up the SD Host Write Protect Signal.  
> This patch defines a new quirk that when enabled enables write access.

I am preparing a patch series which will also make get_ro an overloadable
function. Then, board specific things could be handled there.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] sdhci: add quirk for controllers that don't support write only detect

[Philip Rakity]

There are a couple of other items that I wanted to fix which I can submit patches for separately or can incorporate into your structure or we can work together.  Advise.

a) There needs to be a distinction between PRESENT STATE broken and a SD card that IS present (as in mounted on the board).  There is no need to poll for the later case.  The current quirk makes no distinction between these two cases.

b) Some eMMC chips require 74 clocks to start working.  The current code assumes turning on the clock (in the core/ directory) does this.  This is not always the case.  I modified the set_ios function to fix this.

snippet below (WILL not patch into 2.6.36 tree since I have other code) -- just so you get the idea --

--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -383,7 +383,7 @@ struct sdhci_ops {
 	void		(*platform_set_8_bit)(struct sdhci_host *host);
 	void		(*platform_clear_8_bit)(struct sdhci_host *host);
 	void		(*platform_specific_sdhci_enable_dummy_cmd) (struct sdhci_host *host, int dummycmd);
-	void		(*platform_specific_init_clocks)(struct sdhci_host *host);
+	void		(*platform_generate_initial_74_clocks)(struct sdhci_host *host, u8 power_mode);
 };


--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1279,8 +1279,11 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	if (ios->power_mode == MMC_POWER_OFF)
 		sdhci_set_power(host, -1);
-	else
+	else {
 		sdhci_set_power(host, ios->vdd);
+		if (host->ops->platform_generate_initial_74_clocks)
+			host->ops->platform_generate_initial_74_clocks(host, ios->power_mode);
+	}

In my adaption layer 
+void generate_init_clocks(struct sdhci_host *host, u8 power_mode)
+{
+	if (slot->power_mode == MMC_POWER_UP 
+	&& power_mode == MMC_POWER_ON) {
		/* slot has old state */
+		/* now do chip/board specific activity */
+	}
+	slot->power_mode = power_mode;
+}


Philip

On Sep 19, 2010, at 11:14 PM, Wolfram Sang wrote:

> On Sun, Sep 19, 2010 at 02:46:40PM -0700, Philip Rakity wrote:
>> 
>> 
>> Some board designs for embedded systems  do not always bring up the SD Host Write Protect Signal.  
>> This patch defines a new quirk that when enabled enables write access.
> 
> I am preparing a patch series which will also make get_ro an overloadable
> function. Then, board specific things could be handled there.
> 
> Regards,
> 
>   Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Philip Rakity]

Wolfram,

I generated the patch for eMMC 74 clocks.  I do not know if you want to incorporate into your submission or let it stand on its own.

regards,

Philip



From: Philip Rakity <prakity@marvell.com>
Subject: [PATCH] sdhci: allow for eMMC 74 clock generation by controller
Signed-off-by: Philip Rakity <prakity@marvell.com>

Patch is below and sample usage in sdhci-xxxx.c adaption code is below.

/*
 * eMMC spec calls for the host to send 74 clocks to the card
 * during initialization, right after voltage stabilization.
 * create the clocks manually right here.
 */
void generate_init_clocks_A0(struct sdhci_host *host, u8 power_mode)
{
	struct sdhci_mmc_slot *slot = sdhci_priv(host);

	DBG ("%s: ENTER %s: slot->power_mode = %d, ios->power_mode = %d\n",
	 	__func__, 
		mmc_hostname(host->mmc),
		slot->power_mode, 
		power_mode);

	if (slot->power_mode == MMC_POWER_UP 
	&& power_mode == MMC_POWER_ON) {
	
		/* controller specific code here */
		/* slot->power_mode holds previous power setting */

	}
	slot->power_mode = power_mode;
}



diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 401527d..6aadd0f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1168,6 +1168,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	else
 		sdhci_set_power(host, ios->vdd);
 
+	if (host->ops->platform_generate_initial_74_clocks)
+		host->ops->platform_generate_initial_74_clocks(host, ios->power_mode);
+
 	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 
 	if (ios->bus_width == MMC_BUS_WIDTH_8)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d316bc7..70c5806 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -323,6 +323,8 @@ struct sdhci_ops {
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
+	void		(*platform_generate_initial_74_clocks)(struct sdhci_host *host,
+				u8 power_mode);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Adrian Hunter]

On 21/09/10 08:43, ext Philip Rakity wrote:
>
> Wolfram,
>
> I generated the patch for eMMC 74 clocks.  I do not know if you want to incorporate into your submission or let it stand on its own.

Wouldn't it be better to fix it in MMC core rather than duplicate it in every driver?

>
> regards,
>
> Philip
>
>
>
> From: Philip Rakity<prakity@marvell.com>
> Subject: [PATCH] sdhci: allow for eMMC 74 clock generation by controller
> Signed-off-by: Philip Rakity<prakity@marvell.com>
>
> Patch is below and sample usage in sdhci-xxxx.c adaption code is below.
>
> /*
>   * eMMC spec calls for the host to send 74 clocks to the card
>   * during initialization, right after voltage stabilization.
>   * create the clocks manually right here.
>   */
> void generate_init_clocks_A0(struct sdhci_host *host, u8 power_mode)
> {
> 	struct sdhci_mmc_slot *slot = sdhci_priv(host);
>
> 	DBG ("%s: ENTER %s: slot->power_mode = %d, ios->power_mode = %d\n",
> 	 	__func__,
> 		mmc_hostname(host->mmc),
> 		slot->power_mode,
> 		power_mode);
>
> 	if (slot->power_mode == MMC_POWER_UP
> 	&&  power_mode == MMC_POWER_ON) {
> 	
> 		/* controller specific code here */
> 		/* slot->power_mode holds previous power setting */
>
> 	}
> 	slot->power_mode = power_mode;
> }
>
>
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 401527d..6aadd0f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1168,6 +1168,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>   	else
>   		sdhci_set_power(host, ios->vdd);
>
> +	if (host->ops->platform_generate_initial_74_clocks)
> +		host->ops->platform_generate_initial_74_clocks(host, ios->power_mode);
> +
>   	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>
>   	if (ios->bus_width == MMC_BUS_WIDTH_8)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d316bc7..70c5806 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -323,6 +323,8 @@ struct sdhci_ops {
>   	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>   	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>   	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
> +	void		(*platform_generate_initial_74_clocks)(struct sdhci_host *host,
> +				u8 power_mode);
>   };
>
>   #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>
> --
> 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
>

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Wolfram Sang]

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

Hi Philip,

> I generated the patch for eMMC 74 clocks.  I do not know if you want
> to incorporate into your submission or let it stand on its own.

I won't pick it up as I am missing the technical details for this issue.
I only jumped in because adding quirk bits should be handled very
carefully at the moment IMHO.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Philip Rakity]

Our hardware needs to be programmed to handle this case.  The host-ops option is only needed from h/w that does not handle this.

If you can suggest a way to do this in the mmc layer I would welcome implementing it.  

I looked and the solution requires a call into the driver from the mmc layer after the all setting POWER_ON call.  This was more intrusive than doing it in the driver but if you think this makes sense I can do a patch for that.  

eg

/*
 * Apply power to the MMC stack.  This is a two-stage process.
 * First, we enable power to the card without the clock running.
 * We then wait a bit for the power to stabilise.  Finally,
 * enable the bus drivers and clock to the card.
 *
 * We must _NOT_ enable the clock prior to power stablising.
 *
 * If a host does all the power sequencing itself, ignore the
 * initial MMC_POWER_UP stage.
 */
static void mmc_power_up(struct mmc_host *host)
{
	int bit = fls(host->ocr_avail) - 1;

	host->ios.vdd = bit;
	if (mmc_host_is_spi(host)) {
		host->ios.chip_select = MMC_CS_HIGH;
		host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
	} else {
		host->ios.chip_select = MMC_CS_DONTCARE;
		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
	}
	host->ios.power_mode = MMC_POWER_UP;
	host->ios.bus_width = MMC_BUS_WIDTH_1;
	host->ios.timing = MMC_TIMING_LEGACY;
	mmc_set_ios(host);

	/*  CALL INTO DRIVER HERE TO SEND 74 CLOCKS */   <==========
Philip


On Sep 20, 2010, at 11:04 PM, Adrian Hunter wrote:

> On 21/09/10 08:43, ext Philip Rakity wrote:
>> 
>> Wolfram,
>> 
>> I generated the patch for eMMC 74 clocks.  I do not know if you want to incorporate into your submission or let it stand on its own.
> 
> Wouldn't it be better to fix it in MMC core rather than duplicate it in every driver?
> 
>> 
>> regards,
>> 
>> Philip
>> 
>> 
>> 
>> From: Philip Rakity<prakity@marvell.com>
>> Subject: [PATCH] sdhci: allow for eMMC 74 clock generation by controller
>> Signed-off-by: Philip Rakity<prakity@marvell.com>
>> 
>> Patch is below and sample usage in sdhci-xxxx.c adaption code is below.
>> 
>> /*
>>  * eMMC spec calls for the host to send 74 clocks to the card
>>  * during initialization, right after voltage stabilization.
>>  * create the clocks manually right here.
>>  */
>> void generate_init_clocks_A0(struct sdhci_host *host, u8 power_mode)
>> {
>> 	struct sdhci_mmc_slot *slot = sdhci_priv(host);
>> 
>> 	DBG ("%s: ENTER %s: slot->power_mode = %d, ios->power_mode = %d\n",
>> 	 	__func__,
>> 		mmc_hostname(host->mmc),
>> 		slot->power_mode,
>> 		power_mode);
>> 
>> 	if (slot->power_mode == MMC_POWER_UP
>> 	&&  power_mode == MMC_POWER_ON) {
>> 	
>> 		/* controller specific code here */
>> 		/* slot->power_mode holds previous power setting */
>> 
>> 	}
>> 	slot->power_mode = power_mode;
>> }
>> 
>> 
>> 
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 401527d..6aadd0f 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1168,6 +1168,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>  	else
>>  		sdhci_set_power(host, ios->vdd);
>> 
>> +	if (host->ops->platform_generate_initial_74_clocks)
>> +		host->ops->platform_generate_initial_74_clocks(host, ios->power_mode);
>> +
>>  	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>> 
>>  	if (ios->bus_width == MMC_BUS_WIDTH_8)
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index d316bc7..70c5806 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -323,6 +323,8 @@ struct sdhci_ops {
>>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>>  	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
>> +	void		(*platform_generate_initial_74_clocks)(struct sdhci_host *host,
>> +				u8 power_mode);
>>  };
>> 
>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> 
>> --
>> 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
>> 
> 

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Wolfram Sang]

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

On Tue, Sep 21, 2010 at 08:06:20AM -0700, Philip Rakity wrote:
> 
> Our hardware needs to be programmed to handle this case.  The host-ops option is only needed from h/w that does not handle this.
> 
> If you can suggest a way to do this in the mmc layer I would welcome implementing it.  
> 
> I looked and the solution requires a call into the driver from the mmc layer after the all setting POWER_ON call.  This was more intrusive than doing it in the driver but if you think this makes sense I can do a patch for that.  

Do you have a draft patch for this intrusive way (can be quick'n'dirty)? It is
usually easier to think about such issues if there is some code...

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Adrian Hunter]

On 21/09/10 18:06, Philip Rakity wrote:
>
> Our hardware needs to be programmed to handle this case.  The host-ops option is only needed from h/w that does not handle this.
>
> If you can suggest a way to do this in the mmc layer I would welcome implementing it.

No, I misunderstood sorry :(.  The 74-clocks requirement is standard and
drivers handle it via set_ios.

>
> I looked and the solution requires a call into the driver from the mmc layer after the all setting POWER_ON call.  This was more intrusive than doing it in the driver but if you think this makes sense I can do a patch for that.
>
> eg
>
> /*
>   * Apply power to the MMC stack.  This is a two-stage process.
>   * First, we enable power to the card without the clock running.
>   * We then wait a bit for the power to stabilise.  Finally,
>   * enable the bus drivers and clock to the card.
>   *
>   * We must _NOT_ enable the clock prior to power stablising.
>   *
>   * If a host does all the power sequencing itself, ignore the
>   * initial MMC_POWER_UP stage.
>   */
> static void mmc_power_up(struct mmc_host *host)
> {
> 	int bit = fls(host->ocr_avail) - 1;
>
> 	host->ios.vdd = bit;
> 	if (mmc_host_is_spi(host)) {
> 		host->ios.chip_select = MMC_CS_HIGH;
> 		host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
> 	} else {
> 		host->ios.chip_select = MMC_CS_DONTCARE;
> 		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
> 	}
> 	host->ios.power_mode = MMC_POWER_UP;
> 	host->ios.bus_width = MMC_BUS_WIDTH_1;
> 	host->ios.timing = MMC_TIMING_LEGACY;
> 	mmc_set_ios(host);
>
> 	/*  CALL INTO DRIVER HERE TO SEND 74 CLOCKS */<==========
> Philip
>
>
> On Sep 20, 2010, at 11:04 PM, Adrian Hunter wrote:
>
>> On 21/09/10 08:43, ext Philip Rakity wrote:
>>>
>>> Wolfram,
>>>
>>> I generated the patch for eMMC 74 clocks.  I do not know if you want to incorporate into your submission or let it stand on its own.
>>
>> Wouldn't it be better to fix it in MMC core rather than duplicate it in every driver?
>>
>>>
>>> regards,
>>>
>>> Philip
>>>
>>>
>>>
>>> From: Philip Rakity<prakity@marvell.com>
>>> Subject: [PATCH] sdhci: allow for eMMC 74 clock generation by controller
>>> Signed-off-by: Philip Rakity<prakity@marvell.com>
>>>
>>> Patch is below and sample usage in sdhci-xxxx.c adaption code is below.
>>>
>>> /*
>>>   * eMMC spec calls for the host to send 74 clocks to the card
>>>   * during initialization, right after voltage stabilization.
>>>   * create the clocks manually right here.
>>>   */
>>> void generate_init_clocks_A0(struct sdhci_host *host, u8 power_mode)
>>> {
>>> 	struct sdhci_mmc_slot *slot = sdhci_priv(host);
>>>
>>> 	DBG ("%s: ENTER %s: slot->power_mode = %d, ios->power_mode = %d\n",
>>> 	 	__func__,
>>> 		mmc_hostname(host->mmc),
>>> 		slot->power_mode,
>>> 		power_mode);
>>>
>>> 	if (slot->power_mode == MMC_POWER_UP
>>> 	&&   power_mode == MMC_POWER_ON) {
>>> 	
>>> 		/* controller specific code here */
>>> 		/* slot->power_mode holds previous power setting */
>>>
>>> 	}
>>> 	slot->power_mode = power_mode;
>>> }
>>>
>>>
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 401527d..6aadd0f 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1168,6 +1168,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>>   	else
>>>   		sdhci_set_power(host, ios->vdd);
>>>
>>> +	if (host->ops->platform_generate_initial_74_clocks)
>>> +		host->ops->platform_generate_initial_74_clocks(host, ios->power_mode);
>>> +
>>>   	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>>>
>>>   	if (ios->bus_width == MMC_BUS_WIDTH_8)
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index d316bc7..70c5806 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -323,6 +323,8 @@ struct sdhci_ops {
>>>   	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>>>   	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>>>   	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
>>> +	void		(*platform_generate_initial_74_clocks)(struct sdhci_host *host,
>>> +				u8 power_mode);
>>>   };
>>>
>>>   #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>>>
>>> --
>>> 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
>>>
>>
>
>

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Wolfram Sang]

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

Hi,

>    Patch attached.  We are in a chicken and egg situation.   The controller
>    needs data to cause the clock to start and the first command sent by the
>    mmc layer is CMD0 -- mmc_go_idle().  This is sent without the  eMMC chip
>    seeing the 74 clocks but since there are NOW clocks the chip see's this.
>     It is not able to parse the command and we are struck.

Okay, and some host->ops->generate_initial_74_clocks() could help you
out, it seems. If it is a controller specific issue, the approach seems
in general sensible to me, unless I am missing some details.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[PATCH] sdhci: print out controller name for register debug

[Philip Rakity]

>From 928a3fef14d44f4179cfe075380ad760720fea9e Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Wed, 22 Sep 2010 15:22:25 -0700
Subject: [PATCH] sdhci: print out controller name for register debug

In a multi-controller environment it is helpful to know which controller
has problems.

Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e2b1f32..6b8ca32 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -47,7 +47,8 @@ static void sdhci_finish_command(struct sdhci_host *);
 
 static void sdhci_dumpregs(struct sdhci_host *host)
 {
-	printk(KERN_DEBUG DRIVER_NAME ": ============== REGISTER DUMP ==============\n");
+	printk(KERN_DEBUG DRIVER_NAME ": =========== REGISTER DUMP (%s)===========\n",
+		mmc_hostname(host->mmc));
 
 	printk(KERN_DEBUG DRIVER_NAME ": Sys addr: 0x%08x | Version:  0x%08x\n",
 		sdhci_readl(host, SDHCI_DMA_ADDRESS),
-- 
1.6.0.4

Re: [PATCH] sdhci: print out controller name for register debug

[Chris Ball]

Hi Philip,

On Wed, Sep 22, 2010 at 03:25:13PM -0700, Philip Rakity wrote:
> Subject: [PATCH] sdhci: print out controller name for register debug
> 
> In a multi-controller environment it is helpful to know which controller
> has problems.
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> ---
>  drivers/mmc/host/sdhci.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e2b1f32..6b8ca32 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -47,7 +47,8 @@ static void sdhci_finish_command(struct sdhci_host *);
>  
>  static void sdhci_dumpregs(struct sdhci_host *host)
>  {
> -	printk(KERN_DEBUG DRIVER_NAME ": ============== REGISTER DUMP ==============\n");
> +	printk(KERN_DEBUG DRIVER_NAME ": =========== REGISTER DUMP (%s)===========\n",
> +		mmc_hostname(host->mmc));
>  
>  	printk(KERN_DEBUG DRIVER_NAME ": Sys addr: 0x%08x | Version:  0x%08x\n",
>  		sdhci_readl(host, SDHCI_DMA_ADDRESS),

Thanks, pushed to mmc-next; I'm surprised no-one noticed this before. :)

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] sdhci: print out controller name for register debug

[Wolfram Sang]

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

On Thu, Sep 23, 2010 at 12:08:06AM +0100, Chris Ball wrote:
> Hi Philip,
> 
> On Wed, Sep 22, 2010 at 03:25:13PM -0700, Philip Rakity wrote:
> > Subject: [PATCH] sdhci: print out controller name for register debug
> > 
> > In a multi-controller environment it is helpful to know which controller
> > has problems.
> > 
> > Signed-off-by: Philip Rakity <prakity@marvell.com>
> > ---
> >  drivers/mmc/host/sdhci.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index e2b1f32..6b8ca32 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -47,7 +47,8 @@ static void sdhci_finish_command(struct sdhci_host *);
> >  
> >  static void sdhci_dumpregs(struct sdhci_host *host)
> >  {
> > -	printk(KERN_DEBUG DRIVER_NAME ": ============== REGISTER DUMP ==============\n");
> > +	printk(KERN_DEBUG DRIVER_NAME ": =========== REGISTER DUMP (%s)===========\n",
> > +		mmc_hostname(host->mmc));
> >  
> >  	printk(KERN_DEBUG DRIVER_NAME ": Sys addr: 0x%08x | Version:  0x%08x\n",
> >  		sdhci_readl(host, SDHCI_DMA_ADDRESS),
> 
> Thanks, pushed to mmc-next; I'm surprised no-one noticed this before. :)

Uh, that was fast. I would have preferred to switch the whole printout
to dev_dbg?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Philip Rakity]

Re did patch to fit into 80 char line length rule

=============

>From 1ba80b60d0ca3ba3114e2aa25f1afdb8a6af3842 Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Thu, 23 Sep 2010 08:15:03 -0700
Subject: [PATCH] sdhci: allow for initial eMMC 74 clock generation by controller

resend of patch to fit into 80 char line lengths:

snippet of code for how adaption layer should handle the call.


/*
 * eMMC spec calls for the host to send 74 clocks to the card
 * during initialization, right after voltage stabilization.
 * create the clocks manually right here.
 */
void generate_init_clocks_A0(struct sdhci_host *host, u8 power_mode)
{
	struct sdhci_mmc_slot *slot = sdhci_priv(host);

	DBG ("%s: ENTER %s: slot->power_mode = %d, ios->power_mode = %d\n",
	 	__func__, 
		mmc_hostname(host->mmc),
		slot->power_mode, 
		power_mode);

	if (slot->power_mode == MMC_POWER_UP 
	&& power_mode == MMC_POWER_ON) {
	
		/* controller specific code here */
		/* slot->power_mode holds previous power setting */
	}
	slot->power_mode = power_mode;
}


Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |    3 +++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6b8ca32..ba8f9a0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1169,6 +1169,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	else
 		sdhci_set_power(host, ios->vdd);
 
+	if (host->ops->platform_send_init_74_clocks)
+		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
+
 	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 
 	if (ios->bus_width == MMC_BUS_WIDTH_8)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 290b5a8..d73685a 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -325,6 +325,8 @@ struct sdhci_ops {
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
+	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
+		u8 power_mode);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.6.0.4


=================

On Sep 20, 2010, at 10:43 PM, Philip Rakity wrote:

> 
> Wolfram,
> 
> I generated the patch for eMMC 74 clocks.  I do not know if you want to incorporate into your submission or let it stand on its own.
> 
> regards,
> 
> Philip
> 
> 
> 
> From: Philip Rakity <prakity@marvell.com>
> Subject: [PATCH] sdhci: allow for eMMC 74 clock generation by controller
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> 
> Patch is below and sample usage in sdhci-xxxx.c adaption code is below.
> 
> /*
> * eMMC spec calls for the host to send 74 clocks to the card
> * during initialization, right after voltage stabilization.
> * create the clocks manually right here.
> */
> void generate_init_clocks_A0(struct sdhci_host *host, u8 power_mode)
> {
> 	struct sdhci_mmc_slot *slot = sdhci_priv(host);
> 
> 	DBG ("%s: ENTER %s: slot->power_mode = %d, ios->power_mode = %d\n",
> 	 	__func__, 
> 		mmc_hostname(host->mmc),
> 		slot->power_mode, 
> 		power_mode);
> 
> 	if (slot->power_mode == MMC_POWER_UP 
> 	&& power_mode == MMC_POWER_ON) {
> 	
> 		/* controller specific code here */
> 		/* slot->power_mode holds previous power setting */
> 
> 	}
> 	slot->power_mode = power_mode;
> }
> 
> 
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 401527d..6aadd0f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1168,6 +1168,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> 	else
> 		sdhci_set_power(host, ios->vdd);
> 
> +	if (host->ops->platform_generate_initial_74_clocks)
> +		host->ops->platform_generate_initial_74_clocks(host, ios->power_mode);
> +
> 	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> 
> 	if (ios->bus_width == MMC_BUS_WIDTH_8)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d316bc7..70c5806 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -323,6 +323,8 @@ struct sdhci_ops {
> 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
> 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
> 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
> +	void		(*platform_generate_initial_74_clocks)(struct sdhci_host *host,
> +				u8 power_mode);
> };
> 
> #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> 

Subject: [PATCH] sdhci: Show SD Command when doing debug printks

[Philip Rakity]

>From 8502f5450c8d97d5a096269c44196684e7e0957d Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Thu, 23 Sep 2010 08:45:38 -0700
Subject: [PATCH] sdhci: Show SD Command when doing debug printks

helps to know if read or write to understand error
Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ba8f9a0..fa57fb1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -80,6 +80,8 @@ static void sdhci_dumpregs(struct sdhci_host *host)
 	printk(KERN_DEBUG DRIVER_NAME ": Caps:     0x%08x | Max curr: 0x%08x\n",
 		sdhci_readl(host, SDHCI_CAPABILITIES),
 		sdhci_readl(host, SDHCI_MAX_CURRENT));
+	printk(KERN_DEBUG DRIVER_NAME ": Command:  0x%08x\n",
+		sdhci_readw(host, SDHCI_COMMAND));
 
 	if (host->flags & SDHCI_USE_ADMA)
 		printk(KERN_DEBUG DRIVER_NAME ": ADMA Err: 0x%08x | ADMA Ptr: 0x%08x\n",
-- 
1.6.0.4

Re: [PATCH] sdhci: print out controller name for register debug

[Chris Ball]

Hi Wolfram,

On Thu, Sep 23, 2010 at 10:39:46AM +0200, Wolfram Sang wrote:
> Uh, that was fast.

Sorry, you're right, was fast.  I'm okay with improving patches after
they enter mmc-next, but I'll leave more time for comments on patches
in future before merging.

> I would have preferred to switch the whole printout to dev_dbg?

That's a much bigger change, though -- you'd be making sdhci_dumpregs()
a no-op unless CONFIG_MMC_DEBUG is defined, right?  That's a change in
semantics, since it's intentionally called from many non-debug error
paths right now.

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] sdhci: print out controller name for register debug

[Wolfram Sang]

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


> That's a much bigger change, though -- you'd be making sdhci_dumpregs()
> a no-op unless CONFIG_MMC_DEBUG is defined, right?  That's a change in
> semantics, since it's intentionally called from many non-debug error
> paths right now.

Hmm, true. dev_info would keep the semantics then, but change the loglevel.
Well, not worth the fuss right now...

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] sdhci: add quirk for controllers that don't support write only detect

[Wolfram Sang]

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

On Thu, Sep 23, 2010 at 04:18:02PM -0700, Philip Rakity wrote:
>    Since your patches are not available yet -- I think it makes more sense if
>    this patch is considered for inclusion and you can add then include its
>    functionality in your code.

Hmm, I don't think so. As I mentioned elsewhere, Richard Zhu has posted such
patches you could base your work on. And if his approach works for you and you
like it, you could ack them, or say Tested-by or so...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] sdhci: add quirk for controllers that don't support write only detect

[Philip Rakity]

looked at richard code for get_ro.  It will work.  

On Sep 23, 2010, at 9:49 PM, Wolfram Sang wrote:

> On Thu, Sep 23, 2010 at 04:18:02PM -0700, Philip Rakity wrote:
>>   Since your patches are not available yet -- I think it makes more sense if
>>   this patch is considered for inclusion and you can add then include its
>>   functionality in your code.
> 
> Hmm, I don't think so. As I mentioned elsewhere, Richard Zhu has posted such
> patches you could base your work on. And if his approach works for you and you
> like it, you could ack them, or say Tested-by or so...
> 
> Regards,
> 
>   Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] sdhci: allow for eMMC 74 clock generation by controller

[Chris Ball]

Hi Philip,

On Thu, Sep 23, 2010 at 08:24:32AM -0700, Philip Rakity wrote:
> From: Philip Rakity <prakity@marvell.com>
> Date: Thu, 23 Sep 2010 08:15:03 -0700
> Subject: [PATCH] sdhci: allow for initial eMMC 74 clock generation by controller
> 
> resend of patch to fit into 80 char line lengths:
> 
> snippet of code for how adaption layer should handle the call.
> 
> 
> /*
>  * eMMC spec calls for the host to send 74 clocks to the card
>  * during initialization, right after voltage stabilization.
>  * create the clocks manually right here.
>  */
> void generate_init_clocks_A0(struct sdhci_host *host, u8 power_mode)
> {
> 	struct sdhci_mmc_slot *slot = sdhci_priv(host);
> 
> 	DBG ("%s: ENTER %s: slot->power_mode = %d, ios->power_mode = %d\n",
> 	 	__func__, 
> 		mmc_hostname(host->mmc),
> 		slot->power_mode, 
> 		power_mode);
> 
> 	if (slot->power_mode == MMC_POWER_UP 
> 	&& power_mode == MMC_POWER_ON) {
> 	
> 		/* controller specific code here */
> 		/* slot->power_mode holds previous power setting */
> 	}
> 	slot->power_mode = power_mode;
> }
> 
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> ---
>  drivers/mmc/host/sdhci.c |    3 +++
>  drivers/mmc/host/sdhci.h |    2 ++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6b8ca32..ba8f9a0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1169,6 +1169,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  	else
>  		sdhci_set_power(host, ios->vdd);
>  
> +	if (host->ops->platform_send_init_74_clocks)
> +		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
> +
>  	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>  
>  	if (ios->bus_width == MMC_BUS_WIDTH_8)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 290b5a8..d73685a 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -325,6 +325,8 @@ struct sdhci_ops {
>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
> +	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
> +		u8 power_mode);
>  };
>  
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> -- 

Thanks, applied to mmc-next.

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child