[PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[Chuanxiao Dong]

Enhanced area feature is a new feature defined in eMMC4.4 standard. This
kind of area can help to improve the performance.

MMC driver will read out the enhanced area offset and size and add them
to be device attributes. The feature enabling should be done in manufactory.
To use this feature, bit ERASE_GRP_DEF should also be set.

Documentation/ABI/testing/sysfs-devices-mmc described the two new attributes

Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
---
 Documentation/ABI/testing/sysfs-devices-mmc |   19 ++++++
 drivers/mmc/core/mmc.c                      |   80 +++++++++++++++++++++++++++
 include/linux/mmc/card.h                    |    3 +
 include/linux/mmc/mmc.h                     |    3 +
 4 files changed, 105 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-mmc

diff --git a/Documentation/ABI/testing/sysfs-devices-mmc b/Documentation/ABI/testing/sysfs-devices-mmc
new file mode 100644
index 0000000..6e759ae
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-mmc
@@ -0,0 +1,19 @@
+What:		/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_offset
+Date:		January 2011
+Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
+Description:
+		Enhanced area is a new feature defined in eMMC4.4 standard.eMMC4.4 or
+		later card can support such feature. This kind of area can help to
+		improve the card performance. If the feature is enabled, this attribute
+		will indicate the start address of enhanced data area. If not, this
+		attribute will be -EINVAL. Unit Byte. Format decimal.
+
+What:		/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_size
+Date:		January 2011
+Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
+Description:
+		Enhanced area is a new feature defined in eMMC4.4 standard. eMMC4.4 or
+		later card can support such feature. This kind of area can help to
+		improve the card performance. If the feature is enabled, this attribute
+		will indicate the size of enhanced data area. If not, this attribute
+		will be -EINVAL. Unit KByte. Format decimal.
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 16006ef..5ab44bb 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -302,6 +302,48 @@ static int mmc_read_ext_csd(struct mmc_card *card)
 	}
 
 	if (card->ext_csd.rev >= 4) {
+		/*
+		 * Enhanced area feature support
+		 * check whether eMMC card has the Enhanced area enabled.
+		 * if so, export enhanced area offset and size to
+		 * user by adding sysfs interface
+		 */
+		if ((ext_csd[EXT_CSD_PARTITION_SUPPORT] & 0x2) &&
+				(ext_csd[EXT_CSD_PARTITION_ATTRIBUTE] & 0x1)) {
+			u8 hc_erase_grp_sz =
+				ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
+			u8 hc_wp_grp_sz =
+				ext_csd[EXT_CSD_HC_WP_GRP_SIZE];
+			/*
+			 * set a flag to identify whether the enhanced
+			 * user data is enabled
+			 */
+			card->ext_csd.enhanced_area_en = 1;
+			/*
+			 * calculate the enhanced data area offset, unit B
+			 */
+			card->ext_csd.enhanced_area_offset =
+				(ext_csd[139] << 24) + (ext_csd[138] << 16) +
+				(ext_csd[137] << 8) + ext_csd[136];
+			if (mmc_card_blockaddr(card))
+				card->ext_csd.enhanced_area_offset <<= 9;
+			/*
+			 * calculate the enhanced data area size, unit KB
+			 */
+			card->ext_csd.enhanced_area_size =
+				(ext_csd[142] << 16) + (ext_csd[141] << 8) +
+				ext_csd[140];
+			card->ext_csd.enhanced_area_size *=
+				(size_t)(hc_erase_grp_sz * hc_wp_grp_sz);
+			card->ext_csd.enhanced_area_size <<= 9;
+		} else {
+			/*
+			 * If not enabled enhanced area, disable these
+			 * device attributes
+			 */
+			card->ext_csd.enhanced_area_offset = -EINVAL;
+			card->ext_csd.enhanced_area_size = -EINVAL;
+		}
 		card->ext_csd.sec_trim_mult =
 			ext_csd[EXT_CSD_SEC_TRIM_MULT];
 		card->ext_csd.sec_erase_mult =
@@ -336,6 +378,9 @@ MMC_DEV_ATTR(manfid, "0x%06x\n", card->cid.manfid);
 MMC_DEV_ATTR(name, "%s\n", card->cid.prod_name);
 MMC_DEV_ATTR(oemid, "0x%04x\n", card->cid.oemid);
 MMC_DEV_ATTR(serial, "0x%08x\n", card->cid.serial);
+MMC_DEV_ATTR(enhanced_area_offset, "%lld\n",
+		card->ext_csd.enhanced_area_offset);
+MMC_DEV_ATTR(enhanced_area_size, "%d\n", card->ext_csd.enhanced_area_size);
 
 static struct attribute *mmc_std_attrs[] = {
 	&dev_attr_cid.attr,
@@ -349,6 +394,8 @@ static struct attribute *mmc_std_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_oemid.attr,
 	&dev_attr_serial.attr,
+	&dev_attr_enhanced_area_offset.attr,
+	&dev_attr_enhanced_area_size.attr,
 	NULL,
 };
 
@@ -484,6 +531,39 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	}
 
 	/*
+	 * If enhanced_area_en is TRUE, host need to enable
+	 * ERASE_GRP_DEF bit.
+	 * ERASE_GRP_DEF bit will be lost every time
+	 * after a reset or power off.
+	 */
+	if (card->ext_csd.enhanced_area_en) {
+		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				EXT_CSD_ERASE_GROUP_DEF, 1);
+
+		if (err && err != -EBADMSG)
+			goto free_card;
+
+		if (err) {
+			err = 0;
+			/*
+			 * Just disable enhanced area off & sz
+			 * will try to enable ERASE_GROUP_DEF
+			 * during next time reinit
+			 */
+			card->ext_csd.enhanced_area_offset = -EINVAL;
+			card->ext_csd.enhanced_area_size = -EINVAL;
+		} else {
+			card->ext_csd.erase_group_def = 1;
+			/*
+			 * enable ERASE_GRP_DEF successfully.
+			 * This will affect the erase size, so
+			 * here need to reset erase size
+			 */
+			mmc_set_erase_size(card);
+		}
+	}
+
+	/*
 	 * Activate high speed (if supported)
 	 */
 	if ((card->ext_csd.hs_max_dtr != 0) &&
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 8ce0827..736697f 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -54,6 +54,9 @@ struct mmc_ext_csd {
 	unsigned int		sec_trim_mult;	/* Secure trim multiplier  */
 	unsigned int		sec_erase_mult;	/* Secure erase multiplier */
 	unsigned int		trim_timeout;		/* In milliseconds */
+	bool			enhanced_area_en;	/* enhanced area en */
+	loff_t		enhanced_area_offset;	/* enhanced area addr */
+	size_t		enhanced_area_size;		/* enhanced area size */
 };
 
 struct sd_scr {
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 612301f..264ba54 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -253,6 +253,8 @@ struct _mmc_csd {
  * EXT_CSD fields
  */
 
+#define EXT_CSD_PARTITION_ATTRIBUTE	156	/* R/W */
+#define EXT_CSD_PARTITION_SUPPORT	160	/* RO */
 #define EXT_CSD_ERASE_GROUP_DEF		175	/* R/W */
 #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
 #define EXT_CSD_BUS_WIDTH		183	/* R/W */
@@ -262,6 +264,7 @@ struct _mmc_csd {
 #define EXT_CSD_CARD_TYPE		196	/* RO */
 #define EXT_CSD_SEC_CNT			212	/* RO, 4 bytes */
 #define EXT_CSD_S_A_TIMEOUT		217	/* RO */
+#define EXT_CSD_HC_WP_GRP_SIZE		221	/* RO */
 #define EXT_CSD_ERASE_TIMEOUT_MULT	223	/* RO */
 #define EXT_CSD_HC_ERASE_GRP_SIZE	224	/* RO */
 #define EXT_CSD_SEC_TRIM_MULT		229	/* RO */
-- 
1.6.6.1

Re: [PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[Chris Ball]

Hi Chuanxiao,

On Thu, Jan 20, 2011 at 03:55:56PM +0800, Chuanxiao Dong wrote:
> Enhanced area feature is a new feature defined in eMMC4.4 standard. This
> kind of area can help to improve the performance.
> 
> MMC driver will read out the enhanced area offset and size and add them
> to be device attributes. The feature enabling should be done in manufactory.
> To use this feature, bit ERASE_GRP_DEF should also be set.
> 
> Documentation/ABI/testing/sysfs-devices-mmc described the two new attributes
> 
> Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-devices-mmc |   19 ++++++
>  drivers/mmc/core/mmc.c                      |   80 +++++++++++++++++++++++++++
>  include/linux/mmc/card.h                    |    3 +
>  include/linux/mmc/mmc.h                     |    3 +
>  4 files changed, 105 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-mmc
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-mmc b/Documentation/ABI/testing/sysfs-devices-mmc
> new file mode 100644
> index 0000000..6e759ae
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-mmc
> @@ -0,0 +1,19 @@
> +What:		/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_offset
> +Date:		January 2011
> +Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
> +Description:
> +		Enhanced area is a new feature defined in eMMC4.4 standard.eMMC4.4 or
> +		later card can support such feature. This kind of area can help to
> +		improve the card performance. If the feature is enabled, this attribute
> +		will indicate the start address of enhanced data area. If not, this
> +		attribute will be -EINVAL. Unit Byte. Format decimal.
> +
> +What:		/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_size
> +Date:		January 2011
> +Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
> +Description:
> +		Enhanced area is a new feature defined in eMMC4.4 standard. eMMC4.4 or
> +		later card can support such feature. This kind of area can help to
> +		improve the card performance. If the feature is enabled, this attribute
> +		will indicate the size of enhanced data area. If not, this attribute
> +		will be -EINVAL. Unit KByte. Format decimal.

This is still wrapped at > 80 columns.

> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 16006ef..5ab44bb 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -302,6 +302,48 @@ static int mmc_read_ext_csd(struct mmc_card *card)
>  	}
>  
>  	if (card->ext_csd.rev >= 4) {
> +		/*
> +		 * Enhanced area feature support
> +		 * check whether eMMC card has the Enhanced area enabled.
> +		 * if so, export enhanced area offset and size to
> +		 * user by adding sysfs interface
> +		 */
> +		if ((ext_csd[EXT_CSD_PARTITION_SUPPORT] & 0x2) &&
> +				(ext_csd[EXT_CSD_PARTITION_ATTRIBUTE] & 0x1)) {
> +			u8 hc_erase_grp_sz =
> +				ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
> +			u8 hc_wp_grp_sz =
> +				ext_csd[EXT_CSD_HC_WP_GRP_SIZE];
> +			/*
> +			 * set a flag to identify whether the enhanced
> +			 * user data is enabled
> +			 */
> +			card->ext_csd.enhanced_area_en = 1;
> +			/*
> +			 * calculate the enhanced data area offset, unit B
> +			 */
> +			card->ext_csd.enhanced_area_offset =
> +				(ext_csd[139] << 24) + (ext_csd[138] << 16) +
> +				(ext_csd[137] << 8) + ext_csd[136];
> +			if (mmc_card_blockaddr(card))
> +				card->ext_csd.enhanced_area_offset <<= 9;
> +			/*
> +			 * calculate the enhanced data area size, unit KB
> +			 */
> +			card->ext_csd.enhanced_area_size =
> +				(ext_csd[142] << 16) + (ext_csd[141] << 8) +
> +				ext_csd[140];
> +			card->ext_csd.enhanced_area_size *=
> +				(size_t)(hc_erase_grp_sz * hc_wp_grp_sz);
> +			card->ext_csd.enhanced_area_size <<= 9;
> +		} else {
> +			/*
> +			 * If not enabled enhanced area, disable these
> +			 * device attributes
> +			 */
> +			card->ext_csd.enhanced_area_offset = -EINVAL;
> +			card->ext_csd.enhanced_area_size = -EINVAL;
> +		}
>  		card->ext_csd.sec_trim_mult =
>  			ext_csd[EXT_CSD_SEC_TRIM_MULT];
>  		card->ext_csd.sec_erase_mult =
> @@ -336,6 +378,9 @@ MMC_DEV_ATTR(manfid, "0x%06x\n", card->cid.manfid);
>  MMC_DEV_ATTR(name, "%s\n", card->cid.prod_name);
>  MMC_DEV_ATTR(oemid, "0x%04x\n", card->cid.oemid);
>  MMC_DEV_ATTR(serial, "0x%08x\n", card->cid.serial);
> +MMC_DEV_ATTR(enhanced_area_offset, "%lld\n",
> +		card->ext_csd.enhanced_area_offset);
> +MMC_DEV_ATTR(enhanced_area_size, "%d\n", card->ext_csd.enhanced_area_size);
>  
>  static struct attribute *mmc_std_attrs[] = {
>  	&dev_attr_cid.attr,
> @@ -349,6 +394,8 @@ static struct attribute *mmc_std_attrs[] = {
>  	&dev_attr_name.attr,
>  	&dev_attr_oemid.attr,
>  	&dev_attr_serial.attr,
> +	&dev_attr_enhanced_area_offset.attr,
> +	&dev_attr_enhanced_area_size.attr,
>  	NULL,
>  };
>  
> @@ -484,6 +531,39 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>  	}
>  
>  	/*
> +	 * If enhanced_area_en is TRUE, host need to enable
> +	 * ERASE_GRP_DEF bit.
> +	 * ERASE_GRP_DEF bit will be lost every time
> +	 * after a reset or power off.
> +	 */
> +	if (card->ext_csd.enhanced_area_en) {
> +		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +				EXT_CSD_ERASE_GROUP_DEF, 1);
> +
> +		if (err && err != -EBADMSG)
> +			goto free_card;
> +
> +		if (err) {
> +			err = 0;
> +			/*
> +			 * Just disable enhanced area off & sz
> +			 * will try to enable ERASE_GROUP_DEF
> +			 * during next time reinit
> +			 */
> +			card->ext_csd.enhanced_area_offset = -EINVAL;
> +			card->ext_csd.enhanced_area_size = -EINVAL;
> +		} else {
> +			card->ext_csd.erase_group_def = 1;
> +			/*
> +			 * enable ERASE_GRP_DEF successfully.
> +			 * This will affect the erase size, so
> +			 * here need to reset erase size
> +			 */
> +			mmc_set_erase_size(card);
> +		}
> +	}
> +
> +	/*
>  	 * Activate high speed (if supported)
>  	 */
>  	if ((card->ext_csd.hs_max_dtr != 0) &&
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 8ce0827..736697f 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -54,6 +54,9 @@ struct mmc_ext_csd {
>  	unsigned int		sec_trim_mult;	/* Secure trim multiplier  */
>  	unsigned int		sec_erase_mult;	/* Secure erase multiplier */
>  	unsigned int		trim_timeout;		/* In milliseconds */
> +	bool			enhanced_area_en;	/* enhanced area en */
> +	loff_t		enhanced_area_offset;	/* enhanced area addr */
> +	size_t		enhanced_area_size;		/* enhanced area size */

Now enhanced_area_en is aligned correctly, but the other two aren't.

This adds a warning:

drivers/mmc/core/mmc.c: In function ‘mmc_enhanced_area_size_show’:
drivers/mmc/core/mmc.c:383:261: warning: format ‘%d’ expects type ‘int’,
but argument 3 has type ‘size_t’

If you really want size_t, you should use %zu for it -- see
Documentation/printk-formats.txt.  (Declaring it as unsigned int would
work fine too.)

Oh, and let's mention the units of _offset and _size in the comment in
card.h.

>  };
>  
>  struct sd_scr {
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 612301f..264ba54 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -253,6 +253,8 @@ struct _mmc_csd {
>   * EXT_CSD fields
>   */
>  
> +#define EXT_CSD_PARTITION_ATTRIBUTE	156	/* R/W */
> +#define EXT_CSD_PARTITION_SUPPORT	160	/* RO */
>  #define EXT_CSD_ERASE_GROUP_DEF		175	/* R/W */
>  #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
>  #define EXT_CSD_BUS_WIDTH		183	/* R/W */
> @@ -262,6 +264,7 @@ struct _mmc_csd {
>  #define EXT_CSD_CARD_TYPE		196	/* RO */
>  #define EXT_CSD_SEC_CNT			212	/* RO, 4 bytes */
>  #define EXT_CSD_S_A_TIMEOUT		217	/* RO */
> +#define EXT_CSD_HC_WP_GRP_SIZE		221	/* RO */
>  #define EXT_CSD_ERASE_TIMEOUT_MULT	223	/* RO */
>  #define EXT_CSD_HC_ERASE_GRP_SIZE	224	/* RO */
>  #define EXT_CSD_SEC_TRIM_MULT		229	/* RO */

Thanks,

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

RE: [PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[Dong, Chuanxiao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4098 bytes --]

Hi Chris,
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-devices-mmc
> > @@ -0,0 +1,19 @@
> > +What:
> 	/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_offset
> > +Date:		January 2011
> > +Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
> > +Description:
> > +		Enhanced area is a new feature defined in eMMC4.4 standard.eMMC4.4
> or
> > +		later card can support such feature. This kind of area can help to
> > +		improve the card performance. If the feature is enabled, this attribute
> > +		will indicate the start address of enhanced data area. If not, this
> > +		attribute will be -EINVAL. Unit Byte. Format decimal.
> > +
> > +What:		/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_size
> > +Date:		January 2011
> > +Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
> > +Description:
> > +		Enhanced area is a new feature defined in eMMC4.4 standard. eMMC4.4
> or
> > +		later card can support such feature. This kind of area can help to
> > +		improve the card performance. If the feature is enabled, this attribute
> > +		will indicate the size of enhanced data area. If not, this attribute
> > +		will be -EINVAL. Unit KByte. Format decimal.
> 
> This is still wrapped at > 80 columns.
Sorry for that. I have set the textwidth=80 for vim, not understand why this is still wrapped at >80 columns... I know checkpatch.pl can help us to find the style problem, and is there some other script which can help find the style problem of Document or how can I avoid such errors?
Chris, thank you for your kind patience to help me find these stupid errors again...

> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index 8ce0827..736697f 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -54,6 +54,9 @@ struct mmc_ext_csd {
> >  	unsigned int		sec_trim_mult;	/* Secure trim multiplier  */
> >  	unsigned int		sec_erase_mult;	/* Secure erase multiplier */
> >  	unsigned int		trim_timeout;		/* In milliseconds */
> > +	bool			enhanced_area_en;	/* enhanced area en */
> > +	loff_t		enhanced_area_offset;	/* enhanced area addr */
> > +	size_t		enhanced_area_size;		/* enhanced area size */
> 
> Now enhanced_area_en is aligned correctly, but the other two aren't.
> 
> This adds a warning:
> 
> drivers/mmc/core/mmc.c: In function ‘mmc_enhanced_area_size_show’:
> drivers/mmc/core/mmc.c:383:261: warning: format ‘%d’ expects type ‘int’,
> but argument 3 has type ‘size_t’
> 
> If you really want size_t, you should use %zu for it -- see
> Documentation/printk-formats.txt.  (Declaring it as unsigned int would
> work fine too.)
> 
> Oh, and let's mention the units of _offset and _size in the comment in
> card.h.
Chris, you mean the type of _offset and _size? I think the 64bits number for _offset is enough, also the 32bits number for _size since the units of _size is Kbytes. Any suggestions on that?

> 
> >  };
> >
> >  struct sd_scr {
> > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> > index 612301f..264ba54 100644
> > --- a/include/linux/mmc/mmc.h
> > +++ b/include/linux/mmc/mmc.h
> > @@ -253,6 +253,8 @@ struct _mmc_csd {
> >   * EXT_CSD fields
> >   */
> >
> > +#define EXT_CSD_PARTITION_ATTRIBUTE	156	/* R/W */
> > +#define EXT_CSD_PARTITION_SUPPORT	160	/* RO */
> >  #define EXT_CSD_ERASE_GROUP_DEF		175	/* R/W */
> >  #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
> >  #define EXT_CSD_BUS_WIDTH		183	/* R/W */
> > @@ -262,6 +264,7 @@ struct _mmc_csd {
> >  #define EXT_CSD_CARD_TYPE		196	/* RO */
> >  #define EXT_CSD_SEC_CNT			212	/* RO, 4 bytes */
> >  #define EXT_CSD_S_A_TIMEOUT		217	/* RO */
> > +#define EXT_CSD_HC_WP_GRP_SIZE		221	/* RO */
> >  #define EXT_CSD_ERASE_TIMEOUT_MULT	223	/* RO */
> >  #define EXT_CSD_HC_ERASE_GRP_SIZE	224	/* RO */
> >  #define EXT_CSD_SEC_TRIM_MULT		229	/* RO */
> 
> Thanks,
> 
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[Chris Ball]

Hi Chuanxiao,

On Fri, Jan 21, 2011 at 11:06:08AM +0800, Dong, Chuanxiao wrote:
> Hi Chris,
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-devices-mmc
> > > @@ -0,0 +1,19 @@
> > > +What:
> > 	/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_offset
> > > +Date:		January 2011
> > > +Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
> > > +Description:
> > > +		Enhanced area is a new feature defined in eMMC4.4 standard.eMMC4.4
> > or
> > > +		later card can support such feature. This kind of area can help to
> > > +		improve the card performance. If the feature is enabled, this attribute
> > > +		will indicate the start address of enhanced data area. If not, this
> > > +		attribute will be -EINVAL. Unit Byte. Format decimal.
> > > +
> > > +What:		/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_size
> > > +Date:		January 2011
> > > +Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
> > > +Description:
> > > +		Enhanced area is a new feature defined in eMMC4.4 standard. eMMC4.4
> > or
> > > +		later card can support such feature. This kind of area can help to
> > > +		improve the card performance. If the feature is enabled, this attribute
> > > +		will indicate the size of enhanced data area. If not, this attribute
> > > +		will be -EINVAL. Unit KByte. Format decimal.
> > 
> > This is still wrapped at > 80 columns.
> Sorry for that. I have set the textwidth=80 for vim, not understand why this is still wrapped at >80 columns... I know checkpatch.pl can help us to find the style problem, and is there some other script which can help find the style problem of Document or how can I avoid such errors?

checkpatch.pl will warn about >80 chars, so that should be fine on its own.

> > Oh, and let's mention the units of _offset and _size in the comment in
> > card.h.
>
> Chris, you mean the type of _offset and _size? I think the 64bits number for _offset is enough, also the 32bits number for _size since the units of _size is Kbytes. Any suggestions on that?

Ah, I wasn't clear -- I meant the bytes/kilobytes units, not the data type
sizes.  e.g.:

+   size_t          enhanced_area_size;             /* Units: KB */ 

Thanks,

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

RE: [PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[Dong, Chuanxiao]

Hi Chris,
> 	/sys/devices/.../mmc_host/mmcX/mmcX:XXXX/enhanced_area_size
> > > > +Date:		January 2011
> > > > +Contact:	Chuanxiao Dong <chuanxiao.dong@intel.com>
> > > > +Description:
> > > > +		Enhanced area is a new feature defined in eMMC4.4 standard.
> eMMC4.4
> > > or
> > > > +		later card can support such feature. This kind of area can help to
> > > > +		improve the card performance. If the feature is enabled, this
> attribute
> > > > +		will indicate the size of enhanced data area. If not, this attribute
> > > > +		will be -EINVAL. Unit KByte. Format decimal.
> > >
> > > This is still wrapped at > 80 columns.
> > Sorry for that. I have set the textwidth=80 for vim, not understand why this is still
> wrapped at >80 columns... I know checkpatch.pl can help us to find the style
> problem, and is there some other script which can help find the style problem of
> Document or how can I avoid such errors?
> 
> checkpatch.pl will warn about >80 chars, so that should be fine on its own.
I have used the checkpatch.pl to check this patch, and no error or warning found....

> 
> > > Oh, and let's mention the units of _offset and _size in the comment in
> > > card.h.
> >
> > Chris, you mean the type of _offset and _size? I think the 64bits number for
> _offset is enough, also the 32bits number for _size since the units of _size is Kbytes.
> Any suggestions on that?
> 
> Ah, I wasn't clear -- I meant the bytes/kilobytes units, not the data type
> sizes.  e.g.:
> 
> +   size_t          enhanced_area_size;             /* Units: KB */
I think the units of _offset and _size should be easy for user to understand, so just choose the Byte for _offset and Kbyte for _size...
And as spec said, the _size is (ENH_SIZE_MULT_2 << 16 + ENH_SIZE_MULT_1 << 8 + ENH_SIZE_MULT_0) x HC_WP_GRP_SIZE x HC_ERASE_GPR_SIZE x 512kBytes. It must be Kbytes aligned. That is another reason to choose it.
What do you think?

Thanks
Chuanxiao

Re: [PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[Chris Ball]

Hi Chuanxiao,

On Fri, Jan 21, 2011 at 05:12:00PM +0800, Dong, Chuanxiao wrote:
> I have used the checkpatch.pl to check this patch, and no error or warning found....

Oh, you're right!  I wonder if that's because it's in a .txt file rather
than a source file.  Not sure how to get it to warn about this, then.

> > Ah, I wasn't clear -- I meant the bytes/kilobytes units, not the data type
> > sizes.  e.g.:
> > 
> > +   size_t          enhanced_area_size;             /* Units: KB */
> I think the units of _offset and _size should be easy for user to understand, so just choose the Byte for _offset and Kbyte for _size...
> And as spec said, the _size is (ENH_SIZE_MULT_2 << 16 + ENH_SIZE_MULT_1 << 8 + ENH_SIZE_MULT_0) x HC_WP_GRP_SIZE x HC_ERASE_GPR_SIZE x 512kBytes. It must be Kbytes aligned. That is another reason to choose it.

I agree that we we should use these units, I'm just arguing that we
might as well mention them in the comment area.  The other card.h
definitions mention their unit sizes too.  At the moment your comments
for card.h simply repeat the variable name, which isn't useful -- why
not make them repeat some useful and not-necessarily-obvious information
instead?

Thanks,

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

RE: [PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature

[Dong, Chuanxiao]

Hi Chris,

> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Friday, January 21, 2011 11:28 PM
> To: Dong, Chuanxiao
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org;
> akpm@linux-foundation.org
> Subject: Re: [PATCH v3 1/1]mmc: implemented eMMC4.4 enhanced area feature
> 
> Hi Chuanxiao,
> 
> On Fri, Jan 21, 2011 at 05:12:00PM +0800, Dong, Chuanxiao wrote:
> > I have used the checkpatch.pl to check this patch, and no error or warning
> found....
> 
> Oh, you're right!  I wonder if that's because it's in a .txt file rather
> than a source file.  Not sure how to get it to warn about this, then.
> 
> > > Ah, I wasn't clear -- I meant the bytes/kilobytes units, not the data type
> > > sizes.  e.g.:
> > >
> > > +   size_t          enhanced_area_size;             /* Units: KB */
> > I think the units of _offset and _size should be easy for user to understand, so just
> choose the Byte for _offset and Kbyte for _size...
> > And as spec said, the _size is (ENH_SIZE_MULT_2 << 16 + ENH_SIZE_MULT_1 <<
> 8 + ENH_SIZE_MULT_0) x HC_WP_GRP_SIZE x HC_ERASE_GPR_SIZE x 512kBytes. It
> must be Kbytes aligned. That is another reason to choose it.
> 
> I agree that we we should use these units, I'm just arguing that we
> might as well mention them in the comment area.  The other card.h
> definitions mention their unit sizes too.  At the moment your comments
> for card.h simply repeat the variable name, which isn't useful -- why
> not make them repeat some useful and not-necessarily-obvious information
> instead?
Sure, Chris. I will take care of such things in my future submissions. Of cause including the next v4 enhanced area feature patch. :) I also remember to fix the compile warnings. Thanks a lot, Chris.

Thanks
Chuanxiao