[PATCH] mmc : general purpose partition support.

[Namjae Jeon]

It allows general purpose parition in MMC Device. If it is enable, it will make mmcblk0gp1,gp2,gp3,gp4 partition like this.

>cat /proc/paritition
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp4
179 160 4096 mmcblk0gp3
179 128 4096 mmcblk0gp2
179 96 1052672 mmcblk0gp1
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>
---
drivers/mmc/card/block.c | 36 ++++++++++++++++++++++++++++++++++++
drivers/mmc/core/mmc.c | 42 ++++++++++++++++++++++++++++++++++++++++++
include/linux/mmc/card.h | 4 ++++
include/linux/mmc/mmc.h | 4 ++++
4 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 9b90726..074ec55 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1402,6 +1402,42 @@ static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
return ret;
}

+ if (card->ext_csd.gp1_size) {
+ ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_GP1,
+ card->ext_csd.gp1_size >> 9,
+ false,
+ "gp1");
+ if (ret)
+ return ret;
+ }
+
+ if (card->ext_csd.gp2_size) {
+ ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_GP2,
+ card->ext_csd.gp2_size >> 9,
+ false,
+ "gp2");
+ if (ret)
+ return ret;
+ }
+
+ if (card->ext_csd.gp3_size) {
+ ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_GP3,
+ card->ext_csd.gp3_size >> 9,
+ false,
+ "gp3");
+ if (ret)
+ return ret;
+ }
+
+ if (card->ext_csd.gp4_size) {
+ ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_GP4,
+ card->ext_csd.gp4_size >> 9,
+ false,
+ "gp4");
+ if (ret)
+ return ret;
+ }
+
return ret;
}

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 10f5a19..cc31511 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -393,6 +393,48 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
card->ext_csd.enhanced_area_offset = -EINVAL;
card->ext_csd.enhanced_area_size = -EINVAL;
}
+
+ /*
+ * General purpose partition Support
+ */
+
+ if (ext_csd[EXT_CSD_PARTITION_SUPPORT] & 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];
+
+ card->ext_csd.enhanced_area_en = 1;
+
+ card->ext_csd.gp1_size =
+ (ext_csd[145] << 16) + (ext_csd[144] << 8) +
+ ext_csd[143];
+ card->ext_csd.gp1_size *=
+ (size_t)(hc_erase_grp_sz * hc_wp_grp_sz);
+ card->ext_csd.gp1_size <<= 19;
+
+ card->ext_csd.gp2_size =
+ (ext_csd[148] << 16) + (ext_csd[147] << 8) +
+ ext_csd[146];
+ card->ext_csd.gp2_size *=
+ (size_t)(hc_erase_grp_sz * hc_wp_grp_sz);
+ card->ext_csd.gp2_size <<= 19;
+
+ card->ext_csd.gp3_size =
+ (ext_csd[151] << 16) + (ext_csd[150] << 8) +
+ ext_csd[149];
+ card->ext_csd.gp3_size *=
+ (size_t)(hc_erase_grp_sz * hc_wp_grp_sz);
+ card->ext_csd.gp3_size <<= 19;
+
+ card->ext_csd.gp4_size =
+ (ext_csd[154] << 16) + (ext_csd[153] << 8) +
+ ext_csd[152];
+ card->ext_csd.gp4_size *=
+ (size_t)(hc_erase_grp_sz * hc_wp_grp_sz);
+ card->ext_csd.gp4_size <<= 19;
+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index b460fc2..96a98a7 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -64,6 +64,10 @@ struct mmc_ext_csd {
unsigned long long enhanced_area_offset; /* Units: Byte */
unsigned int enhanced_area_size; /* Units: KB */
unsigned int boot_size; /* in bytes */
+ unsigned int gp1_size; /* in bytes */
+ unsigned int gp2_size; /* in bytes */
+ unsigned int gp3_size; /* in bytes */
+ unsigned int gp4_size; /* in bytes */
u8 raw_partition_support; /* 160 */
u8 raw_erased_mem_count; /* 181 */
u8 raw_ext_csd_structure; /* 194 */
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 5a794cb..380720a 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -303,6 +303,10 @@ struct _mmc_csd {
#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)
#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)
+#define EXT_CSD_PART_CONFIG_ACC_GP1 (0x4)
+#define EXT_CSD_PART_CONFIG_ACC_GP2 (0x5)
+#define EXT_CSD_PART_CONFIG_ACC_GP3 (0x6)
+#define EXT_CSD_PART_CONFIG_ACC_GP4 (0x7)

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--
1.7.4.4

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

[J Freyensee]

if mmc_blk_alloc_part() fails for 'if (card->ext_csd.pt1_size)', why
would you want to give this code the opportunity to call
mmc_blk_alloc_part() again with the next if() below? If
mmc_blk_alloc_part() fails, is it a big problem, like kzalloc() failing?
If so, I would think you would want to immediately quit this function
and report an error (either use errno value or pass the value of ret).

--
J (James/Jay) Freyensee
Storage Technology Group
Intel Corporation

[J Freyensee]

On 09/22/2011 04:15 PM, NamJae Jeon wrote:
> 2011/9/23 J Freyensee<james_p_...@linux.intel.com>:

> -> if card->ext_csd.gp1_size is no zero, it means general purpose
> partition is enable.
> and it can set up to 4 partition in specification. so we should check
> it by next if().

Please document that in your code. Please add a function header to this
function.

[J Freyensee]

> I will add comments at this code. I don't understand why a function
> header should be added yet.

One or the other.

but a function comment header should be added at some point. Yes, the
mmc subsystem is pretty legacy, but any new work that gets added to the
kernel is usually has higher standards for stuff like not just adding
function header comments but putting them in a certain format...at least
this is what I went through when I had things accepted upstream.

[Andrei Warkentin]

Hi Namjae,

I scanned over your changes to enable GP partitions, and while I
did not yet test your changes, they do appear to be ok.

Please CC me on the next version of your patch so I can give
it a spin and Ack on it.

I do suggest at least thinking about ways to improve on this. Back
when I added boot partition support, I already didn't like the fact
that the block driver had to be aware of the eMMC partitions, with all
the code duplication and such. By adding four more partitions,
you've compounded the problem and now you have six pieces of
code that really look the same.

The core/mmc.c right now scans the size values and stores them
in mmc_card, and the block driver is smart enough to know what
to do with them. Why not make the block code generic at the expense
of keeping an array of structures in mmc_card? The block driver
would then just iterate over these and create the additional devices. The partition
switch routine would then operate on these structures, and could then be pulled out
of the block driver, making it simpler.

Each physical partition could be described by something like -
struct mmc_part {
unsigned int size;
unsigned int cookie; /* for mmc, idx used in mmc_switch, part_type from current mmc_blk_data */
char name[DISK_NAME_LEN];
bool force_ro; /* to make boot parts RO by default */
};

Just an idea. Clearly, if MMC grows by another 4-5 possible partitions, it's not
realistic to be copy-pasting the same block of code, nor would it make sense
if/when these types of devices support an arbitrary amount of physical partitions.

Thanks,
A

[NamJae Jeon]

2011/9/23 Andrei Warkentin <awark...@vmware.com>:

Okay, I understand what you want. I will try to make patch included
your suggestion again.
Thanks for your review.

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions

179 0 847872 mmcblk0
179 192 4096 mmcblk0gp4
179 160 4096 mmcblk0gp3
179 128 4096 mmcblk0gp2
179 96 1052672 mmcblk0gp1
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>
---

drivers/mmc/card/block.c | 30 ++++++++++++++++--------------
drivers/mmc/core/mmc.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
include/linux/mmc/card.h | 19 ++++++++++++++++++-
include/linux/mmc/mmc.h | 2 --
4 files changed, 79 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1ff5486..8ec7cfd 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1377,26 +1377,28 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
return 0;
}

+/* MMC Physical partition consist of two boot partitons and
+ * four general purpose partitions.
+ * if the register of respective partitions is set in ext_csd,
+ * it allocate block device to be accessed.
+ */
+

static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)

{
- int ret = 0;
+ int i, ret = 0;

if (!mmc_card_mmc(card))
return 0;

- if (card->ext_csd.boot_size) {
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT0,
- card->ext_csd.boot_size >> 9,
- true,
- "boot0");
- if (ret)
- return ret;
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT1,
- card->ext_csd.boot_size >> 9,
- true,
- "boot1");
- if (ret)
- return ret;
+ for (i = 0; i < MMC_MAX_NUM_PHY_PARTITION; i++) {
+ if (card->part[i].size) {
+ ret = mmc_blk_alloc_part(card, md, card->part[i].cookie,
+ card->part[i].size >> 9,
+ card->part[i].force_ro,
+ card->part[i].name);

+ if (ret)
+ return ret;
+ }
}

return ret;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

index 5700b1c..56c4b9e 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -340,7 +340,18 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
* There are two boot regions of equal size, defined in
* multiples of 128K.
*/
- card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ int i, boot_part_config;
+ if (ext_csd[EXT_CSD_BOOT_MULT]) {
+ for (i = 0, boot_part_config = 0x1;
+ i < MMC_NUM_BOOT_PARTITION;
+ i++, boot_part_config++) {
+ card->part[i].size =
+ ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ card->part[i].cookie = boot_part_config;
+ sprintf(card->part[i].name, "boot%d", i);
+ card->part[i].force_ro = true;
+ }
+ }
}

card->ext_csd.raw_hc_erase_gap_size =
@@ -392,6 +403,39 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

card->ext_csd.enhanced_area_offset = -EINVAL;
card->ext_csd.enhanced_area_size = -EINVAL;
}
+

+ if (ext_csd[EXT_CSD_PARTITION_SUPPORT] & 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];
+
+ card->ext_csd.enhanced_area_en = 1;
+

+ int i, gp_num, gp_part_config, gp_size_mult;
+ for (i = 2, gp_num = 1, gp_part_config = 0x4,
+ gp_size_mult = 143;
+ i < MMC_MAX_NUM_PHY_PARTITION;
+ i++, gp_num++, gp_part_config++,
+ gp_size_mult += 3) {
+ if (!ext_csd[gp_size_mult] &&
+ !ext_csd[gp_size_mult + 1] &&
+ !ext_csd[gp_size_mult + 2])
+ continue;
+ card->part[i].size =
+ (ext_csd[gp_size_mult + 2] << 16) +
+ (ext_csd[gp_size_mult + 1] << 8) +
+ ext_csd[gp_size_mult];
+ card->part[i].size *=
+ (size_t)(hc_erase_grp_sz *
+ hc_wp_grp_sz);
+ card->part[i].size <<= 19;
+ card->part[i].cookie = gp_part_config;
+ sprintf(card->part[i].name,
+ "gp%d", gp_num);
+ card->part[i].force_ro = false;
+ }

+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h

index b460fc2..842f702 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -12,6 +12,7 @@

#include <linux/mmc/core.h>
#include <linux/mod_devicetable.h>
+#include <linux/genhd.h>

struct mmc_cid {
unsigned int manfid;
@@ -63,7 +64,6 @@ struct mmc_ext_csd {
bool enhanced_area_en; /* enable bit */

unsigned long long enhanced_area_offset; /* Units: Byte */
unsigned int enhanced_area_size; /* Units: KB */

- unsigned int boot_size; /* in bytes */

u8 raw_partition_support; /* 160 */
u8 raw_erased_mem_count; /* 181 */
u8 raw_ext_csd_structure; /* 194 */

@@ -157,6 +157,22 @@ struct sdio_func_tuple;

#define SDIO_MAX_FUNCS 7

+/* The number of MMC physical partitions
+ * It consist of boot partitions(2), general purpose partitions(4) in MMC v4.4
+ */
+#define MMC_NUM_BOOT_PARTITION 2
+#define MMC_MAX_NUM_PHY_PARTITION 6
+
+/*
+ * MMC Physical partitions
+ */
+struct mmc_part {
+ unsigned int size; /* partition size (in bytes) */
+ unsigned int cookie; /* it used to part_type */
+ char name[DISK_NAME_LEN];
+ bool force_ro; /* to make boot parts RO by default */
+};
+
/*
* MMC device
*/
@@ -216,6 +232,7 @@ struct mmc_card {
unsigned int sd_bus_speed; /* Bus Speed Mode set for the card */

struct dentry *debugfs_root;
+ struct mmc_part part[MMC_MAX_NUM_PHY_PARTITION]; /* mmc physical partitions */
};

/*
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 5a794cb..33eae3d 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -301,8 +301,6 @@ struct _mmc_csd {
#define EXT_CSD_WR_REL_PARAM_EN (1<<2)

#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--

1.7.4.4

[Linus Walleij]

On Sat, Sep 24, 2011 at 7:07 AM, Namjae Jeon <linki...@gmail.com> wrote:

> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
>> cat /proc/partitions
>          179 0 847872 mmcblk0
>          179 192 4096 mmcblk0gp4
>          179 160 4096 mmcblk0gp3
>          179 128 4096 mmcblk0gp2
>          179 96  1052672 mmcblk0gp1

We notice there are 32 minors between 128 and 96,
how are these allocated?

>          179 64  1024 mmcblk0boot1
>          179 32  1024 mmcblk0boot0

Does this mean that each GP-partition can contain an MBR and thus several
subpartitions so we get things like:

mmcblk0gp1p1, mmcblk0gp1p2...

?

Yours,
Linus Walleij

[NamJae Jeon]

2011/9/28 Linus Walleij <linus....@linaro.org>:

> On Sat, Sep 24, 2011 at 7:07 AM, Namjae Jeon <linki...@gmail.com> wrote:
>
>> It allows gerneral purpose partitions in MMC Device.
>> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
>> After patching, we can see general purpose partitions like this.
>>> cat /proc/partitions
>>          179 0 847872 mmcblk0
>>          179 192 4096 mmcblk0gp4
>>          179 160 4096 mmcblk0gp3
>>          179 128 4096 mmcblk0gp2
>>          179 96  1052672 mmcblk0gp1
>
> We notice there are 32 minors between 128 and 96,
> how are these allocated?

You can insert maximum minors in menuconfig. I set to 32.

>
>>          179 64  1024 mmcblk0boot1
>>          179 32  1024 mmcblk0boot0
>
> Does this mean that each GP-partition can contain an MBR and thus several
> subpartitions so we get things like:
>
> mmcblk0gp1p1, mmcblk0gp1p2...
>
>

Yes, We can divide dos partitions(included MBR) using fdisk like your
words(mmcblk0gp1p1, mmcblk0gp1p2...)

[Andrei Warkentin]

Hi Namjae,

In general I think your approach is fine and solves the problem. See further inline
comments.

----- Original Message -----
> From: "Namjae Jeon" <linki...@gmail.com>
> To: c...@laptop.org, linu...@vger.kernel.org
> Cc: linux-...@vger.kernel.org, awark...@vmware.com, "adrian hunter" <adrian...@intel.com>, "james p
> freyensee" <james_p_...@linux.intel.com>, "Namjae Jeon" <linki...@gmail.com>
> Sent: Saturday, September 24, 2011 1:07:00 AM
> Subject: [PATCH v2] mmc : general purpose partition support.
>
> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part
> structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
> > cat /proc/partitions
> 179 0 847872 mmcblk0
> 179 192 4096 mmcblk0gp4
> 179 160 4096 mmcblk0gp3
> 179 128 4096 mmcblk0gp2
> 179 96 1052672 mmcblk0gp1
> 179 64 1024 mmcblk0boot1
> 179 32 1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>

> + if (ext_csd[EXT_CSD_BOOT_MULT]) {
> + for (i = 0, boot_part_config = 0x1;
> + i < MMC_NUM_BOOT_PARTITION;
> + i++, boot_part_config++) {

> + card->part[i].size = ...
> + card->part[i].cookie = ...

> + sprintf(card->part[i].name, "boot%d", i);

> + card->part[i].force_ro = ...
> + }

> + }
> }
>
>
> + if (ext_csd[EXT_CSD_PARTITION_SUPPORT] & 0x1) {

> + ....

> + int i, gp_num, gp_part_config, gp_size_mult;
> + for (i = 2, gp_num = 1, gp_part_config = 0x4,

> + card->part[i].size = ...
> + card->part[i].cookie = ...

> + sprintf(card->part[i].name,
> + "gp%d", gp_num);

> + card->part[i].force_ro = ..
> + }
> + }
>

I feel that you should factor out a function that operates on the static part[] array and
adds a new entry base name, index (i.e. the %d for gp%d), cookie, size, force.
Otherwise you end up with these hidden mines like fixed indeces for
particular parts (i = 2, etc...) which becomes indecipherable for others.
Plus you're mostly doing the same thing.

Thanks,
A

[NamJae Jeon]

2011/9/29 Andrei Warkentin <awark...@vmware.com>:

Hi.
I think that factoring out a function will be not inefficient by many
agument and size calculation,. etc.
And I agree about readabiliby, readability is dropped by some fixed
value. so I will modify current patch.
plz review one more when I post new patch.
Thanks.

[Linus Walleij]

On Wed, Sep 28, 2011 at 3:50 PM, NamJae Jeon <linki...@gmail.com> wrote:
> 2011/9/28 Linus Walleij <linus....@linaro.org>:
>> On Sat, Sep 24, 2011 at 7:07 AM, Namjae Jeon <linki...@gmail.com> wrote:
>>>
>>>          179 64  1024 mmcblk0boot1
>>>          179 32  1024 mmcblk0boot0
>>
>> Does this mean that each GP-partition can contain an MBR and thus several
>> subpartitions so we get things like:
>>
>> mmcblk0gp1p1, mmcblk0gp1p2...
>>
> Yes, We can divide dos partitions(included MBR) using fdisk like your
> words(mmcblk0gp1p1, mmcblk0gp1p2...)

OK that seems logical.

If this goes in, make a mental note to patch udev default rules
too, because currently they look like this:

rules.d/60-persistent-storage.rules:KERNEL=="mmcblk[0-9]",
SUBSYSTEMS=="mmc", ATTRS{name}=="?*", ATTRS{serial}=="?*",
ENV{ID_NAME}="$attr{name}", ENV{ID_SERIAL}="$attr{serial}",
SYMLINK+="disk/by-id/mmc-$env{ID_NAME}_$env{ID_SERIAL}"
rules.d/60-persistent-storage.rules:KERNEL=="mmcblk[0-9]p[0-9]",
ENV{ID_NAME}=="?*", ENV{ID_SERIAL}=="?*",
SYMLINK+="disk/by-id/mmc-$env{ID_NAME}_$env{ID_SERIAL}-part%n"

(I guess Android also has this kind of rule baked into its "big init"
process that handles also what udev normally takes care of.)

Anyway:
Acked-by: Linus Walleij <linus....@linaro.org>

[NamJae Jeon]

2011/9/29 Linus Walleij <linus....@linaro.org>:

> On Wed, Sep 28, 2011 at 3:50 PM, NamJae Jeon <linki...@gmail.com> wrote:
>> 2011/9/28 Linus Walleij <linus....@linaro.org>:
>>> On Sat, Sep 24, 2011 at 7:07 AM, Namjae Jeon <linki...@gmail.com> wrote:
>>>>
>>>>          179 64  1024 mmcblk0boot1
>>>>          179 32  1024 mmcblk0boot0
>>>
>>> Does this mean that each GP-partition can contain an MBR and thus several
>>> subpartitions so we get things like:
>>>
>>> mmcblk0gp1p1, mmcblk0gp1p2...
>>>
>> Yes, We can divide dos partitions(included MBR) using fdisk like your
>> words(mmcblk0gp1p1, mmcblk0gp1p2...)
>
> OK that seems logical.
>
> If this goes in, make a mental note to patch udev default rules
> too, because currently they look like this:
>
> rules.d/60-persistent-storage.rules:KERNEL=="mmcblk[0-9]",
> SUBSYSTEMS=="mmc", ATTRS{name}=="?*", ATTRS{serial}=="?*",
> ENV{ID_NAME}="$attr{name}", ENV{ID_SERIAL}="$attr{serial}",
> SYMLINK+="disk/by-id/mmc-$env{ID_NAME}_$env{ID_SERIAL}"
> rules.d/60-persistent-storage.rules:KERNEL=="mmcblk[0-9]p[0-9]",
> ENV{ID_NAME}=="?*", ENV{ID_SERIAL}=="?*",
> SYMLINK+="disk/by-id/mmc-$env{ID_NAME}_$env{ID_SERIAL}-part%n"
>
> (I guess Android also has this kind of rule baked into its "big init"
> process that handles also what udev normally takes care of.)
>
> Anyway:
> Acked-by: Linus Walleij <linus....@linaro.org>

Thanks Linus~
I will modify a little code to improve the readability.
When I post patch v3 again, plz ack one more.

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0

179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0

179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>
---

drivers/mmc/card/block.c | 43 ++++++++++++++++++++++++++------------
drivers/mmc/core/mmc.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-
include/linux/mmc/card.h | 20 +++++++++++++++++-
include/linux/mmc/mmc.h | 13 +++++++++--
4 files changed, 107 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1ff5486..e48529c 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1377,26 +1377,41 @@ static int mmc_blk_alloc_part(struct mmc_card *card,

return 0;
}

+/* MMC Physical partition consist of two boot partitons and
+ * four general purpose partitions.
+ * if the register of respective partitions is set in ext_csd,
+ * it allocate block device to be accessed.
+ */
+
static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
{
- int ret = 0;

+ int idx, ret = 0;

if (!mmc_card_mmc(card))
return 0;

- if (card->ext_csd.boot_size) {
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT0,
- card->ext_csd.boot_size >> 9,
- true,
- "boot0");
- if (ret)
- return ret;
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT1,
- card->ext_csd.boot_size >> 9,
- true,
- "boot1");
- if (ret)
- return ret;

+ for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {
+ if (card->boot_part[idx].size) {

+ ret = mmc_blk_alloc_part(card, md,

+ card->boot_part[idx].cookie,
+ card->boot_part[idx].size >> 9,
+ card->boot_part[idx].force_ro,
+ card->boot_part[idx].name);

+ if (ret)
+ return ret;
+ }

+ }
+
+ for (idx = 0; idx < MMC_NUM_GP_PARTITION; idx++) {
+ if (card->gp_part[idx].size) {

+ ret = mmc_blk_alloc_part(card, md,

+ card->gp_part[idx].cookie,
+ card->gp_part[idx].size >> 9,
+ card->gp_part[idx].force_ro,
+ card->gp_part[idx].name);

+ if (ret)
+ return ret;
+ }
}

return ret;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

index 5700b1c..b36b906 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -239,7 +239,7 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)
*/

static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

{
- int err = 0;
+ int err = 0, idx, part_cfg, gp_size_mult;

BUG_ON(!card);

@@ -340,7 +340,17 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

* There are two boot regions of equal size, defined in
* multiples of 128K.
*/
- card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;

+ if (ext_csd[EXT_CSD_BOOT_MULT]) {

+ part_cfg = EXT_CSD_PART_CONFIG_ACC_BOOT0;
+ for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {
+ card->boot_part[idx].size =
+ ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ card->boot_part[idx].cookie = part_cfg++;
+ sprintf(card->boot_part[idx].name, "boot%d",
+ idx);
+ card->boot_part[idx].force_ro = true;

+ }
+ }
}

card->ext_csd.raw_hc_erase_gap_size =

@@ -392,6 +402,43 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

card->ext_csd.enhanced_area_offset = -EINVAL;
card->ext_csd.enhanced_area_size = -EINVAL;
}
+

+ /*
+ * General purpose partition feature support --
+ * If ext_csd have the size of general purpose partitions,
+ * set size, part_type, partition name in mmc_part.
+ */

+
+ if (ext_csd[EXT_CSD_PARTITION_SUPPORT] & 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];
+
+ card->ext_csd.enhanced_area_en = 1;
+

+ part_cfg = EXT_CSD_PART_CONFIG_ACC_GP0;
+ for (idx = 0, gp_size_mult = 143;
+ idx < MMC_NUM_GP_PARTITION; idx++,

+ gp_size_mult += 3) {
+ if (!ext_csd[gp_size_mult] &&
+ !ext_csd[gp_size_mult + 1] &&
+ !ext_csd[gp_size_mult + 2])
+ continue;

+ card->gp_part[idx].size =

+ (ext_csd[gp_size_mult + 2] << 16) +
+ (ext_csd[gp_size_mult + 1] << 8) +
+ ext_csd[gp_size_mult];

+ card->gp_part[idx].size *=

+ (size_t)(hc_erase_grp_sz *
+ hc_wp_grp_sz);

+ card->gp_part[idx].size <<= 19;
+ card->gp_part[idx].cookie = part_cfg++;
+ sprintf(card->gp_part[idx].name,
+ "gp%d", idx);
+ card->gp_part[idx].force_ro = false;

+ }
+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h

index b460fc2..aa67bf8 100644

--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -12,6 +12,7 @@

#include <linux/mmc/core.h>
#include <linux/mod_devicetable.h>
+#include <linux/genhd.h>

struct mmc_cid {
unsigned int manfid;
@@ -63,7 +64,6 @@ struct mmc_ext_csd {
bool enhanced_area_en; /* enable bit */
unsigned long long enhanced_area_offset; /* Units: Byte */
unsigned int enhanced_area_size; /* Units: KB */
- unsigned int boot_size; /* in bytes */
u8 raw_partition_support; /* 160 */
u8 raw_erased_mem_count; /* 181 */
u8 raw_ext_csd_structure; /* 194 */
@@ -157,6 +157,22 @@ struct sdio_func_tuple;

#define SDIO_MAX_FUNCS 7

+/* The number of MMC physical partitions
+ * It consist of boot partitions(2), general purpose partitions(4) in MMC v4.4
+ */
+#define MMC_NUM_BOOT_PARTITION 2

+#define MMC_NUM_GP_PARTITION 4

+
+/*
+ * MMC Physical partitions
+ */
+struct mmc_part {
+ unsigned int size; /* partition size (in bytes) */
+ unsigned int cookie; /* it used to part_type */
+ char name[DISK_NAME_LEN];
+ bool force_ro; /* to make boot parts RO by default */
+};
+
/*
* MMC device
*/

@@ -216,6 +232,8 @@ struct mmc_card {

unsigned int sd_bus_speed; /* Bus Speed Mode set for the card */

struct dentry *debugfs_root;

+ struct mmc_part boot_part[MMC_NUM_BOOT_PARTITION]; /* mmc boot partitions */
+ struct mmc_part gp_part[MMC_NUM_GP_PARTITION]; /* mmc general purpose partitions */

};

/*
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h

index 5a794cb..41fe51a 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -300,9 +300,16 @@ struct _mmc_csd {

#define EXT_CSD_WR_REL_PARAM_EN (1<<2)

-#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)

-#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)

+enum {
+ EXT_CSD_PART_CONFIG_ACC_BOOT0 = 0x1,
+ EXT_CSD_PART_CONFIG_ACC_BOOT1,
+ EXT_CSD_PART_CONFIG_ACC_RPMB, /* not used yet. */
+ EXT_CSD_PART_CONFIG_ACC_GP0,
+ EXT_CSD_PART_CONFIG_ACC_GP1,
+ EXT_CSD_PART_CONFIG_ACC_GP2,
+ EXT_CSD_PART_CONFIG_ACC_GP3,
+ EXT_CSD_PART_CONFIG_ACC_MASK = 0x7
+};

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--
1.7.4.4

[Andrei Warkentin]

Namjae,

----- Original Message -----
> From: "Namjae Jeon" <linki...@gmail.com>
> To: c...@laptop.org, linu...@vger.kernel.org

> Cc: linux-...@vger.kernel.org, awark...@vmware.com, "adrian hunter" <adrian...@intel.com>, "linus walleij"
> <linus....@linaro.org>, "james p freyensee" <james_p_...@linux.intel.com>, seb...@gmail.com, "Ulf Hansson"
> <Ulf.H...@stericsson.com>, "stefan xk nilsson" <stefan.x...@stericsson.com>, "per forlin"
> <per.f...@stericsson.com>, "johan rudholm" <johan....@stericsson.com>, "Namjae Jeon" <linki...@gmail.com>
> Sent: Thursday, September 29, 2011 11:17:11 AM
> Subject: [PATCH v3] mmc : general purpose partition support.
>
> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part
> structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
> > cat /proc/partitions
> 179 0 847872 mmcblk0
> 179 192 4096 mmcblk0gp3
> 179 160 4096 mmcblk0gp2
> 179 128 4096 mmcblk0gp1
> 179 96 1052672 mmcblk0gp0
> 179 64 1024 mmcblk0boot1
> 179 32 1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>

> + for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {
> + if (card->boot_part[idx].size) {
> + ret = mmc_blk_alloc_part(card, md,
> + card->boot_part[idx].cookie,
> + card->boot_part[idx].size >> 9,
> + card->boot_part[idx].force_ro,
> + card->boot_part[idx].name);
> + if (ret)
> + return ret;
> + }
> + }
> +
> + for (idx = 0; idx < MMC_NUM_GP_PARTITION; idx++) {
> + if (card->gp_part[idx].size) {
> + ret = mmc_blk_alloc_part(card, md,
> + card->gp_part[idx].cookie,
> + card->gp_part[idx].size >> 9,
> + card->gp_part[idx].force_ro,
> + card->gp_part[idx].name);
> + if (ret)
> + return ret;

You were on a better track before. Why does the block drivre need to know about the partition types?
You should parse one array.

> - card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
> + if (ext_csd[EXT_CSD_BOOT_MULT]) {
> + part_cfg = EXT_CSD_PART_CONFIG_ACC_BOOT0;
> + for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {
> + card->boot_part[idx].size =
> + ext_csd[EXT_CSD_BOOT_MULT] << 17;
> + card->boot_part[idx].cookie = part_cfg++;
> + sprintf(card->boot_part[idx].name, "boot%d",
> + idx);
> + card->boot_part[idx].force_ro = true;
> + }
> + }
> }
>

My earlier comment still stands - make your code look something like -

if (ext_csd[EXT_CSD_BOOT_MULT]) {
part_cfg = EXT_CSD_PART_CONFIG_ACC_BOOT0;

for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {

mmc_part_add(card, size, part_cfg++, "boot%d", part_cfg++,
"boot%d", idx, true)

}

> + struct mmc_part boot_part[MMC_NUM_BOOT_PARTITION]; /* mmc boot
> partitions */
> + struct mmc_part gp_part[MMC_NUM_GP_PARTITION]; /* mmc general
> purpose partitions */

struct mmc_part parts[MMC_NUM_BOOT_PARTITION + MMC_NUM_GP_PARTITION];
unsigned nr_parts;

A

[NamJae Jeon]

2011/9/30 Andrei Warkentin <awark...@vmware.com>:

Your review will always inspire me.
I will post again.
plz review after..
Thanks.
> A
>

[Adrian Hunter]

Perhaps rebase against Chris' mmc-next branch

[NamJae Jeon]

2011/9/30 Adrian Hunter <adrian...@intel.com>:

> Perhaps rebase against Chris' mmc-next branch

I made this patch after rebasing Chris'mmc-next yesterday.

[NamJae Jeon]

2011/9/30 NamJae Jeon <linki...@gmail.com>:

> 2011/9/30 Adrian Hunter <adrian...@intel.com>:
>> Perhaps rebase against Chris' mmc-next branch
> I made this patch after rebasing Chris'mmc-next yesterday.

Hi. Adrian.
There is the overlapped code between your patch and my patch.
What should we do ?

[Adrian Hunter]

On 30/09/11 11:11, NamJae Jeon wrote:
> 2011/9/30 NamJae Jeon<linki...@gmail.com>:
>> 2011/9/30 Adrian Hunter<adrian...@intel.com>:
>>> Perhaps rebase against Chris' mmc-next branch
>> I made this patch after rebasing Chris'mmc-next yesterday.
> Hi. Adrian.
> There is the overlapped code between your patch and my patch.
> What should we do ?

Rebase and ensure that boot partitions are only added if
mmc_boot_partition_access(card->host) is true

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>

---
drivers/mmc/card/block.c | 31 +++++++++++++++------------
drivers/mmc/core/mmc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++-
include/linux/mmc/card.h | 32 +++++++++++++++++++++++++++-
include/linux/mmc/mmc.h | 3 --
4 files changed, 98 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1ff5486..56f7185 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1377,26 +1377,29 @@ static int mmc_blk_alloc_part(struct mmc_card *card,

+ for (idx = 0; idx < card->nr_parts; idx++) {
+ if (card->part[idx].size) {

+ ret = mmc_blk_alloc_part(card, md,

+ card->part[idx].cookie,
+ card->part[idx].size >> 9,
+ card->part[idx].force_ro,
+ card->part[idx].name);

+ if (ret)
+ return ret;
+ }
}

return ret;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

index 5700b1c..5db4cee 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -41,6 +41,11 @@ static const unsigned int tacc_mant[] = {
35, 40, 45, 50, 55, 60, 70, 80,
};

+/* partition configuration array */
+static const unsigned int part_cfg[] = {
+ 0x1, 0x2, 0x4, 0x5, 0x6, 7,
+};
+
#define UNSTUFF_BITS(resp,start,size) \
({ \
const int __size = size; \
@@ -239,7 +244,8 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)

*/
static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
{
- int err = 0;

+ int err = 0, idx, part_num;
+ unsigned int part_size, gp_size_mult;

BUG_ON(!card);

@@ -340,7 +346,16 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

* There are two boot regions of equal size, defined in
* multiples of 128K.
*/

- card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ if (ext_csd[EXT_CSD_BOOT_MULT]) {

+ card->nr_parts = MMC_NUM_BOOT_PARTITION;
+ for (idx = 0, part_num = 0;
+ idx < MMC_NUM_BOOT_PARTITION; idx++,
+ part_num++) {
+ part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ mmc_part_add(card, part_size, part_cfg[idx],
+ "boot%d", idx, part_num, true);

+ }
+ }
}

card->ext_csd.raw_hc_erase_gap_size =

@@ -392,6 +407,39 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

card->ext_csd.enhanced_area_offset = -EINVAL;
card->ext_csd.enhanced_area_size = -EINVAL;
}
+
+ /*
+ * General purpose partition feature support --
+ * If ext_csd have the size of general purpose partitions,
+ * set size, part_type, partition name in mmc_part.
+ */
+
+ if (ext_csd[EXT_CSD_PARTITION_SUPPORT] & 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];
+
+ card->ext_csd.enhanced_area_en = 1;
+

+ card->nr_parts += MMC_NUM_GP_PARTITION;
+ for (part_num = 0, gp_size_mult = 143;
+ idx < card->nr_parts;
+ idx++, part_num++, gp_size_mult += 3) {

+ if (!ext_csd[gp_size_mult] &&
+ !ext_csd[gp_size_mult + 1] &&
+ !ext_csd[gp_size_mult + 2])
+ continue;

+ part_size = (ext_csd[gp_size_mult + 2] << 16) +

+ (ext_csd[gp_size_mult + 1] << 8) +
+ ext_csd[gp_size_mult];

+ part_size *= (size_t)(hc_erase_grp_sz *
+ hc_wp_grp_sz);
+ part_size <<= 19;
+ mmc_part_add(card, part_size, part_cfg[idx],
+ "gp%d", idx, part_num, false);

+ }
+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h

index b460fc2..941f2d0 100644

@@ -216,9 +232,23 @@ struct mmc_card {

unsigned int sd_bus_speed; /* Bus Speed Mode set for the card */

struct dentry *debugfs_root;

+ struct mmc_part part[MMC_NUM_BOOT_PARTITION + MMC_NUM_GP_PARTITION]; /* mmc physical partitions */
+ unsigned int nr_parts;
};

/*
+ * This function fill contents in mmc_part.
+ */
+static inline void mmc_part_add(struct mmc_card *card, unsigned int size,
+ unsigned int part_cfg, char *name, int idx, int part_num, bool ro)
+{
+ card->part[idx].size = size;
+ card->part[idx].cookie = part_cfg;
+ sprintf(card->part[idx].name, name, part_num);
+ card->part[idx].force_ro = ro;
+}
+
+/*
* The world is not perfect and supplies us with broken mmc/sdio devices.
* For at least some of these bugs we need a work-around.
*/
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 5a794cb..08f1746 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -299,10 +299,7 @@ struct _mmc_csd {
*/

#define EXT_CSD_WR_REL_PARAM_EN (1<<2)
-

#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--
1.7.4.4

[Namjae Jeon]

+ 0x1, 0x2, 0x4, 0x5, 0x6, 0x7,

[Andrei E. Warkentin]

Hi Namjae,

Why don't you update card->nr_parts inside mmc_part_add? You're making
this too complicated. There
is also no need for the part_cfg array.

2011/9/30 Namjae Jeon <linki...@gmail.com>:

> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
>> cat /proc/partitions
>          179 0 847872 mmcblk0
>          179 192 4096 mmcblk0gp3
>          179 160 4096 mmcblk0gp2
>          179 128 4096 mmcblk0gp1
>          179 96  1052672 mmcblk0gp0
>          179 64  1024 mmcblk0boot1
>          179 32  1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>
> ---

>

> +/* partition configuration array */
> +static const unsigned int part_cfg[] = {
> +       0x1,    0x2,    0x4,    0x5,    0x6,    7,
> +};
> +

No need. Plus this is obscure.

> -               card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
> +               if (ext_csd[EXT_CSD_BOOT_MULT]) {
> +                       card->nr_parts = MMC_NUM_BOOT_PARTITION;
> +                       for (idx = 0, part_num = 0;
> +                               idx < MMC_NUM_BOOT_PARTITION; idx++,
> +                               part_num++) {
> +                               part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
> +                               mmc_part_add(card, part_size, part_cfg[idx],
> +                                       "boot%d", idx, part_num, true);
> +                       }
> +               }
>        }

There is no need for part_num (you have idx for that). And there is no
need for passing the actual array
index either - that's why you have mmc_part_add: it will do
card->parts[card->nr_parts++] = new_part.

if (ext_csd[EXT_CSD_BOOT_MULT]) {

for (idx = 0, part_num = 0;

idx < MMC_NUM_BOOT_PARTITION; idx++) {

part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;

mmc_part_add(card, part_size,
EXT_CSD_PART_CONFIG_ACC_BOOT0 + idx,
"boot%d", idx, true);

Same story: get rid of part_num, don't touch card->nr_parts, and get
rid of part_cfg array access.

card->ext_csd.enhanced_area_en = 1;
for (idx = 0, gp_size_mult = 143;
idx < MMC_NUM_GP_PARTITION;
idx++, gp_size_mult += 3) {
if (!ext_csd[gp_size_mult] &&

!ext_csd[gp_size_mult + 1] &&

!ext_csd[gp_size_mult + 2])

continue;

part_size = (ext_csd[gp_size_mult + 2] << 16) +

(ext_csd[gp_size_mult + 1] << 8) +

ext_csd[gp_size_mult];
part_size *= (size_t)(hc_erase_grp_sz *
hc_wp_grp_sz);
part_size <<= 19;
mmc_part_add(card, part_size, idx +
EXT_CSD_PART_CONFIG_ACC_GP0,
"gp%d", idx, false);

}
}


> /*
> + * This function fill contents in mmc_part.
> + */
> +static inline void mmc_part_add(struct mmc_card *card, unsigned int size,
> + unsigned int part_cfg, char *name, int idx, int part_num, bool ro)
> +{
> + card->part[idx].size = size;
> + card->part[idx].cookie = part_cfg;
> + sprintf(card->part[idx].name, name, part_num);
> + card->part[idx].force_ro = ro;
> +}

static inline void mmc_part_add(struct mmc_card *card, unsigned int size,

unsigned int part_cfg, char *name, int idx, bool ro)
{
card->part[card->nr_parts].size = size;
card->part[card->nr_parts].cookie = part_cfg;
sprintf(card->part[card->nr_parts].name, name, idx);
card->part[card->nr_parts].force_ro = ro;
card->nr_parts++;

}

>  #define EXT_CSD_PART_CONFIG_ACC_MASK   (0x7)
> -#define EXT_CSD_PART_CONFIG_ACC_BOOT0  (0x1)
> -#define EXT_CSD_PART_CONFIG_ACC_BOOT1  (0x2)
>

I'd also keep the MMC_NUM_BOOT_PARTITION and MMC_NUM_GP_PARTITION defs
at the same location.

/* The number of MMC physical partitions

* It consist of boot partitions(2), general purpose partitions(4) in MMC v4.4

*/
#define MMC_NUM_BOOT_PARTITION 2
#define MMC_NUM_GP_PARTITION 4

#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4)

A

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>
---

drivers/mmc/card/block.c | 31 +++++++++++++++++--------------
drivers/mmc/core/mmc.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
include/linux/mmc/card.h | 33 ++++++++++++++++++++++++++++++++-
include/linux/mmc/mmc.h | 2 +-
4 files changed, 93 insertions(+), 18 deletions(-)

index 5700b1c..818778f 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -239,7 +239,8 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)

*/
static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
{
- int err = 0;

+ int err = 0, idx;

+ unsigned int part_size, gp_size_mult;

BUG_ON(!card);

@@ -340,7 +341,15 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

* There are two boot regions of equal size, defined in
* multiples of 128K.
*/

- card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ if (ext_csd[EXT_CSD_BOOT_MULT]) {

+ for (idx = 0; idx < MMC_NUM_BOOT_PARTITION;
+ idx++) {

+ part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ mmc_part_add(card, part_size,

+ EXT_CSD_PART_CONFIG_ACC_BOOT0 + idx,
+ "boot%d", idx, true);
+ }
+ }
}

card->ext_csd.raw_hc_erase_gap_size =
@@ -392,6 +401,38 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

card->ext_csd.enhanced_area_offset = -EINVAL;
card->ext_csd.enhanced_area_size = -EINVAL;
}
+
+ /*
+ * General purpose partition feature support --
+ * If ext_csd have the size of general purpose partitions,
+ * set size, part_type, partition name in mmc_part.
+ */
+

+ if (ext_csd[EXT_CSD_PARTITION_SUPPORT] & 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];
+
+ card->ext_csd.enhanced_area_en = 1;
+

+ for (idx = 0, gp_size_mult = 143;
+ idx < MMC_NUM_GP_PARTITION;
+ idx++, gp_size_mult += 3) {

+ if (!ext_csd[gp_size_mult] &&
+ !ext_csd[gp_size_mult + 1] &&
+ !ext_csd[gp_size_mult + 2])
+ continue;
+ part_size = (ext_csd[gp_size_mult + 2] << 16) +
+ (ext_csd[gp_size_mult + 1] << 8) +
+ ext_csd[gp_size_mult];
+ part_size *= (size_t)(hc_erase_grp_sz *
+ hc_wp_grp_sz);

+ mmc_part_add(card, part_size <<= 19,
+ EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
+ "gp%d", idx, false);

+ }
+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h

index b460fc2..550c2ed 100644

@@ -216,9 +232,24 @@ struct mmc_card {

unsigned int sd_bus_speed; /* Bus Speed Mode set for the card */

struct dentry *debugfs_root;
+ struct mmc_part part[MMC_NUM_BOOT_PARTITION + MMC_NUM_GP_PARTITION]; /* mmc physical partitions */
+ unsigned int nr_parts;
};

/*
+ * This function fill contents in mmc_part.
+ */
+static inline void mmc_part_add(struct mmc_card *card, unsigned int size,

+ unsigned int part_cfg, char *name, int idx, bool ro)
+{
+ card->part[card->nr_parts].size = size;
+ card->part[card->nr_parts].cookie = part_cfg;
+ sprintf(card->part[card->nr_parts].name, name, idx);
+ card->part[card->nr_parts].force_ro = ro;
+ card->nr_parts++;

+}
+
+/*
* The world is not perfect and supplies us with broken mmc/sdio devices.
* For at least some of these bugs we need a work-around.
*/
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h

index 5a794cb..29b7cb6 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -302,7 +302,7 @@ struct _mmc_csd {

#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)

#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)

+#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4)

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--
1.7.4.4

[Namjae Jeon]

[Andrei E. Warkentin]

Hi Namjae,

2011/10/1 Namjae Jeon <linki...@gmail.com>:

> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
>> cat /proc/partitions
>          179 0 847872 mmcblk0
>          179 192 4096 mmcblk0gp3
>          179 160 4096 mmcblk0gp2
>          179 128 4096 mmcblk0gp1
>          179 96  1052672 mmcblk0gp0
>          179 64  1024 mmcblk0boot1
>          179 32  1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>
> ---

Looks good.

Acked-by: Andrei Warkentin <andrey.w...@gmail.com>

Thanks,
A

[Sebastian Rasmussen]

Hi!

> It allows gerneral purpose partitions in MMC Device.

Reading this patch raised a few questions with me. I hope
you can find some time to answer some of them.

up to four general purpose partitions.

> + * if the register of respective partitions is set in ext_csd,
> + * it allocate block device to be accessed.

For each partition enabled in EXT_CSD a block device will
be allocated to provide access to the partition.

What does part_type refer to?

> + */
> +
> + if (ext_csd[EXT_CSD_PARTITION_SUPPORT] & 0x1) {

Maybe #define PARTITIONING_EN (0x1) in mmc.h somewhere?

> + 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];
> +
> + card->ext_csd.enhanced_area_en = 1;

Why is it ok to unconditionally enable this without checking
ext_csd[EXT_CSD_PARTITION_SUPPORT] & 0x02, i.e.
ENH_ATTRIBUTE_EN?

> +
> + for (idx = 0, gp_size_mult = 143;
> + idx < MMC_NUM_GP_PARTITION;
> + idx++, gp_size_mult += 3) {
> + if (!ext_csd[gp_size_mult] &&
> + !ext_csd[gp_size_mult + 1] &&
> + !ext_csd[gp_size_mult + 2])
> + continue;
> + part_size = (ext_csd[gp_size_mult + 2] << 16) +
> + (ext_csd[gp_size_mult + 1] << 8) +
> + ext_csd[gp_size_mult];
> + part_size *= (size_t)(hc_erase_grp_sz *
> + hc_wp_grp_sz);
> + mmc_part_add(card, part_size <<= 19,

Is <<= really a valid operator? Does this even compile?

> + EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
> + "gp%d", idx, false);
> + }

I think the gp_size_mult being set to 143 is a magic number
not very well explained. Also I'm of the opinion that the code
above could be improved upon readability-wise. Below you
find my suggestion. This would loose the gp_size_mult variable
and instead depend on a proper constant that should go in mmc.h.
Mind you I haven't compiled or tested the code below.

#define EXT_CSD_GP_SIZE_MULT_X_Y 143 /* R/W */

for (idx = 0; idx < MMC_NUM_GP_PARTITION; idx++)
{
part_size =
(ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y + idx * 3 + 2] << 16) |
(ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y + idx * 3 + 1] << 8) |
(ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y + idx * 3] << 0);

part_size *= (size_t)(hc_erase_grp_sz * hc_wp_grp_sz);

if (part_size)
mmc_part_add(card, part_size << 19,
EXT_CSD_PART_CONFIG_ACC_GP0 + idx,

"gp%d", idx, false);

}

> + }

Just to make the code above a little easier to fit into 80 characters,
maybe these should be known as MMC_BOOT_PARTS and
MMC_GENERAL_PARTS? That also expands the GP acronym
without making it too unwieldy.

> +
> +/*
> + * MMC Physical partitions
> + */
> +struct mmc_part {
> + unsigned int size; /* partition size (in bytes) */
> + unsigned int cookie; /* it used to part_type */

This information seems to be called part_type, cookie and
part_cfg in different parts of your patch. A common name
used everywhere is preferable, maybe settle on part_type?

/ Sebastian

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.

And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>
---

drivers/mmc/card/block.c | 31 ++++++++++++++------------
drivers/mmc/core/mmc.c | 53 ++++++++++++++++++++++++++++++++++++++++++---
include/linux/mmc/card.h | 34 ++++++++++++++++++++++++++++-
include/linux/mmc/mmc.h | 5 +++-
4 files changed, 103 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1ff5486..8452872 100644

--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1377,26 +1377,29 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
return 0;
}

+/* MMC Physical partition consist of two boot partitions and
+ * up to four general purpose partitions.
+ * For each partitions enabled in EXT_CSD
+ * The block device will allocated to provide access to the partition.

+ */
+
static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
{
- int ret = 0;
+ int idx, ret = 0;

if (!mmc_card_mmc(card))
return 0;

- if (card->ext_csd.boot_size) {
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT0,
- card->ext_csd.boot_size >> 9,
- true,
- "boot0");
- if (ret)
- return ret;
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT1,
- card->ext_csd.boot_size >> 9,
- true,
- "boot1");
- if (ret)
- return ret;
+ for (idx = 0; idx < card->nr_parts; idx++) {
+ if (card->part[idx].size) {
+ ret = mmc_blk_alloc_part(card, md,

+ card->part[idx].part_cfg,

+ card->part[idx].size >> 9,
+ card->part[idx].force_ro,
+ card->part[idx].name);
+ if (ret)
+ return ret;
+ }
}

return ret;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

index 5700b1c..f38bb6a 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -239,7 +239,9 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)

*/
static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
{
- int err = 0;
+ int err = 0, idx;

+ unsigned int part_size;
+ u8 hc_erase_grp_sz = -EINVAL, hc_wp_grp_sz = -EINVAL;

BUG_ON(!card);

@@ -340,7 +342,15 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

* There are two boot regions of equal size, defined in
* multiples of 128K.
*/
- card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ if (ext_csd[EXT_CSD_BOOT_MULT]) {
+ for (idx = 0; idx < MMC_NUM_BOOT_PARTITION;
+ idx++) {
+ part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ mmc_part_add(card, part_size,
+ EXT_CSD_PART_CONFIG_ACC_BOOT0 + idx,
+ "boot%d", idx, true);
+ }
+ }
}

card->ext_csd.raw_hc_erase_gap_size =

@@ -361,9 +371,9 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
*/
if ((ext_csd[EXT_CSD_PARTITION_SUPPORT] & 0x2) &&
(ext_csd[EXT_CSD_PARTITION_ATTRIBUTE] & 0x1)) {
- u8 hc_erase_grp_sz =
+ hc_erase_grp_sz =
ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
- u8 hc_wp_grp_sz =
+ hc_wp_grp_sz =
ext_csd[EXT_CSD_HC_WP_GRP_SIZE];

card->ext_csd.enhanced_area_en = 1;
@@ -392,6 +402,41 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

card->ext_csd.enhanced_area_offset = -EINVAL;
card->ext_csd.enhanced_area_size = -EINVAL;
}
+
+ /*
+ * General purpose partition feature support --
+ * If ext_csd have the size of general purpose partitions,

+ * set size, part_cfg, partition name in mmc_part.

+ */
+
+ if (ext_csd[EXT_CSD_PARTITION_SUPPORT] &

+ EXT_CSD_PART_SUPPORT_PART_EN) {
+ if (hc_erase_grp_sz == -EINVAL ||
+ hc_wp_grp_sz == -EINVAL) {
+ hc_erase_grp_sz =
+ ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
+ hc_wp_grp_sz =

+ ext_csd[EXT_CSD_HC_WP_GRP_SIZE];
+
+ card->ext_csd.enhanced_area_en = 1;

+ }
+

+ for (idx = 0; idx < MMC_NUM_GP_PARTITION; idx++) {

+ if (!ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y] &&
+ !ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y + 1] &&
+ !ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y + 2])

+ continue;
+ part_size =

+ (ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y + 2] << 16) +
+ (ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y + 1] << 8) +
+ ext_csd[EXT_CSD_GP_SIZE_MULT_X_Y];

+ part_size *= (size_t)(hc_erase_grp_sz *
+ hc_wp_grp_sz);

+ mmc_part_add(card, part_size << 19,

+ EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
+ "gp%d", idx, false);
+ }

+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h

index b460fc2..4bff3d0 100644

--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -12,6 +12,7 @@

#include <linux/mmc/core.h>
#include <linux/mod_devicetable.h>
+#include <linux/genhd.h>

struct mmc_cid {
unsigned int manfid;
@@ -63,7 +64,6 @@ struct mmc_ext_csd {
bool enhanced_area_en; /* enable bit */
unsigned long long enhanced_area_offset; /* Units: Byte */
unsigned int enhanced_area_size; /* Units: KB */
- unsigned int boot_size; /* in bytes */
u8 raw_partition_support; /* 160 */
u8 raw_erased_mem_count; /* 181 */
u8 raw_ext_csd_structure; /* 194 */

@@ -157,6 +157,23 @@ struct sdio_func_tuple;

#define SDIO_MAX_FUNCS 7

+/* The number of MMC physical partitions
+ * It consist of boot partitions(2), general purpose partitions(4) in MMC v4.4
+ */
+#define MMC_NUM_BOOT_PARTITION 2
+#define MMC_NUM_GP_PARTITION 4

+#define MMC_NUM_PHY_PARTITION 6

+
+/*
+ * MMC Physical partitions
+ */
+struct mmc_part {
+ unsigned int size; /* partition size (in bytes) */

+ unsigned int part_cfg; /* it used to part_type */

+ char name[DISK_NAME_LEN];
+ bool force_ro; /* to make boot parts RO by default */
+};
+
/*
* MMC device
*/

@@ -216,9 +233,24 @@ struct mmc_card {

unsigned int sd_bus_speed; /* Bus Speed Mode set for the card */

struct dentry *debugfs_root;

+ struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical partitions */

+ unsigned int nr_parts;
};

/*
+ * This function fill contents in mmc_part.
+ */
+static inline void mmc_part_add(struct mmc_card *card, unsigned int size,
+ unsigned int part_cfg, char *name, int idx, bool ro)
+{
+ card->part[card->nr_parts].size = size;

+ card->part[card->nr_parts].part_cfg = part_cfg;

+ sprintf(card->part[card->nr_parts].name, name, idx);
+ card->part[card->nr_parts].force_ro = ro;
+ card->nr_parts++;
+}
+
+/*
* The world is not perfect and supplies us with broken mmc/sdio devices.
* For at least some of these bugs we need a work-around.
*/
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h

index 5a794cb..284a1b0 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -270,6 +270,7 @@ struct _mmc_csd {
* EXT_CSD fields
*/

+#define EXT_CSD_GP_SIZE_MULT_X_Y 143 /* R/W */
#define EXT_CSD_PARTITION_ATTRIBUTE 156 /* R/W */
#define EXT_CSD_PARTITION_SUPPORT 160 /* RO */
#define EXT_CSD_WR_REL_PARAM 166 /* RO */
@@ -302,7 +303,9 @@ struct _mmc_csd {

#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)
#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)
+#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4)

+
+#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--
1.7.4.4

--

[Chris Ball]

Hi,

On Mon, Oct 03 2011, Namjae Jeon wrote:
> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
>> cat /proc/partitions
> 179 0 847872 mmcblk0
> 179 192 4096 mmcblk0gp3
> 179 160 4096 mmcblk0gp2
> 179 128 4096 mmcblk0gp1
> 179 96 1052672 mmcblk0gp0
> 179 64 1024 mmcblk0boot1
> 179 32 1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>

v8 doesn't apply against mmc-next, please resend. mmc-next has moved to:
git://dev.laptop.org/users/cjb/mmc mmc-next

(Thanks for reviewing, Andrei -- please send another ACK after v9 is
submitted, if you don't mind.)

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

[NamJae Jeon]

2011/10/4 Chris Ball <c...@laptop.org>:

> Hi,
>
> On Mon, Oct 03 2011, Namjae Jeon wrote:
>> It allows gerneral purpose partitions in MMC Device.
>> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
>> After patching, we can see general purpose partitions like this.
>>> cat /proc/partitions
>>           179 0 847872 mmcblk0
>>           179 192 4096 mmcblk0gp3
>>           179 160 4096 mmcblk0gp2
>>           179 128 4096 mmcblk0gp1
>>           179 96  1052672 mmcblk0gp0
>>           179 64  1024 mmcblk0boot1
>>           179 32  1024 mmcblk0boot0
>>
>> Signed-off-by: Namjae Jeon <linki...@gmail.com>
>
> v8 doesn't apply against mmc-next, please resend.  mmc-next has moved to:
>   git://dev.laptop.org/users/cjb/mmc mmc-next
>
> (Thanks for reviewing, Andrei -- please send another ACK after v9 is
> submitted, if you don't mind.)

Hi. Chris.
I will resend patch again after rebase from moved mmc-next.
Thanks.

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>

---
drivers/mmc/card/block.c | 31 +++++++++++++++------------
drivers/mmc/core/mmc.c | 52 ++++++++++++++++++++++++++++++++++++++++++---
include/linux/mmc/card.h | 34 +++++++++++++++++++++++++++++-
include/linux/mmc/mmc.h | 5 +++-
4 files changed, 102 insertions(+), 20 deletions(-)

index 5700b1c..478b251 100644

--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -239,7 +239,9 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)
*/
static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
{
- int err = 0;
+ int err = 0, idx;
+ unsigned int part_size;

+ u8 hc_erase_grp_sz = 0, hc_wp_grp_sz = 0;

@@ -392,6 +402,40 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

card->ext_csd.enhanced_area_offset = -EINVAL;
card->ext_csd.enhanced_area_size = -EINVAL;
}
+
+ /*
+ * General purpose partition feature support --
+ * If ext_csd have the size of general purpose partitions,
+ * set size, part_cfg, partition name in mmc_part.
+ */
+
+ if (ext_csd[EXT_CSD_PARTITION_SUPPORT] &
+ EXT_CSD_PART_SUPPORT_PART_EN) {

+ if (card->ext_csd.enhanced_area_en != 1) {

+ hc_erase_grp_sz =
+ ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
+ hc_wp_grp_sz =
+ ext_csd[EXT_CSD_HC_WP_GRP_SIZE];
+
+ card->ext_csd.enhanced_area_en = 1;
+ }
+
+ for (idx = 0; idx < MMC_NUM_GP_PARTITION; idx++) {

+ if (!ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3] &&
+ !ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3 + 1] &&
+ !ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3 + 2])

+ continue;
+ part_size =

+ (ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3 + 2] << 16) +
+ (ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3 + 1] << 8) +
+ ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3];

+ part_size *= (size_t)(hc_erase_grp_sz *
+ hc_wp_grp_sz);
+ mmc_part_add(card, part_size << 19,
+ EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
+ "gp%d", idx, false);
+ }
+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h

index b460fc2..8e42573 100644

index 5a794cb..ac0422a 100644

--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -270,6 +270,7 @@ struct _mmc_csd {
* EXT_CSD fields
*/

+#define EXT_CSD_GP_SIZE_MULT 143 /* R/W */

#define EXT_CSD_PARTITION_ATTRIBUTE 156 /* R/W */
#define EXT_CSD_PARTITION_SUPPORT 160 /* RO */
#define EXT_CSD_WR_REL_PARAM 166 /* RO */
@@ -302,7 +303,9 @@ struct _mmc_csd {

#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)
#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)
+#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4)
+
+#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--
1.7.4.4

[Namjae Jeon]

[Chris Ball]

Hi,

On Tue, Oct 04 2011, Namjae Jeon wrote:
> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
>> cat /proc/partitions
> 179 0 847872 mmcblk0
> 179 192 4096 mmcblk0gp3
> 179 160 4096 mmcblk0gp2
> 179 128 4096 mmcblk0gp1
> 179 96 1052672 mmcblk0gp0
> 179 64 1024 mmcblk0boot1
> 179 32 1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>

Still doesn't apply here. Are you sure you're on the "mmc-next" branch
of the repository?

Thanks,

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

[NamJae Jeon]

2011/10/5 Chris Ball <c...@laptop.org>:

> Hi,
>
> On Tue, Oct 04 2011, Namjae Jeon wrote:
>> It allows gerneral purpose partitions in MMC Device.
>> And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
>> After patching, we can see general purpose partitions like this.
>>> cat /proc/partitions
>>           179 0 847872 mmcblk0
>>           179 192 4096 mmcblk0gp3
>>           179 160 4096 mmcblk0gp2
>>           179 128 4096 mmcblk0gp1
>>           179 96  1052672 mmcblk0gp0
>>           179 64  1024 mmcblk0boot1
>>           179 32  1024 mmcblk0boot0
>>
>> Signed-off-by: Namjae Jeon <linki...@gmail.com>
>
> Still doesn't apply here.  Are you sure you're on the "mmc-next" branch
> of the repository?

Hi. Chris.

I receive mmc-next you inform like this.

git clone git://dev.laptop.org/users/cjb/mmc

What I did wrong ?

Thanks.

[NamJae Jeon]

2011/10/5 Chris Ball <c...@laptop.org>:
> Hi,
>

> On Tue, Oct 04 2011, NamJae Jeon wrote:
>>> Still doesn't apply here.  Are you sure you're on the "mmc-next" branch
>>> of the repository?
>>

>> I receive mmc-next you inform like this.
>>
>> git clone git://dev.laptop.org/users/cjb/mmc
>>
>> What I did wrong ?
>

> You need to type "git checkout mmc-next" afterwards to switch to the
> mmc-next branch.
Okay I will try it.
Thanks Chris~

[Chris Ball]

Hi,

On Tue, Oct 04 2011, NamJae Jeon wrote:
>> Still doesn't apply here.  Are you sure you're on the "mmc-next" branch
>> of the repository?
>

> I receive mmc-next you inform like this.
>
> git clone git://dev.laptop.org/users/cjb/mmc
>
> What I did wrong ?

You need to type "git checkout mmc-next" afterwards to switch to the
mmc-next branch.

Thanks,

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>

---
drivers/mmc/card/block.c | 34 +++++++++++++++++------------

drivers/mmc/core/mmc.c | 52 ++++++++++++++++++++++++++++++++++++++++++---
include/linux/mmc/card.h | 34 +++++++++++++++++++++++++++++-
include/linux/mmc/mmc.h | 5 +++-

4 files changed, 105 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 782d5f8..fb6f1e9 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1470,26 +1470,32 @@ static int mmc_blk_alloc_part(struct mmc_card *card,

return 0;
}

+/* MMC Physical partition consist of two boot partitions and
+ * up to four general purpose partitions.
+ * For each partitions enabled in EXT_CSD
+ * The block device will allocated to provide access to the partition.
+ */
+
static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
{
- int ret = 0;
+ int idx, ret = 0;

if (!mmc_card_mmc(card))
return 0;

- if (card->ext_csd.boot_size && mmc_boot_partition_access(card->host)) {

- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT0,
- card->ext_csd.boot_size >> 9,
- true,
- "boot0");
- if (ret)
- return ret;
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT1,
- card->ext_csd.boot_size >> 9,
- true,
- "boot1");
- if (ret)
- return ret;
+ for (idx = 0; idx < card->nr_parts; idx++) {

+ if ((idx == 0 || idx == 1) &&
+ !mmc_boot_partition_access(card->host))
+ continue;

+ if (card->part[idx].size) {
+ ret = mmc_blk_alloc_part(card, md,
+ card->part[idx].part_cfg,
+ card->part[idx].size >> 9,
+ card->part[idx].force_ro,
+ card->part[idx].name);
+ if (ret)
+ return ret;
+ }
}

return ret;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

index c632b1f..2f458a0 100644

--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -239,7 +239,9 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)
*/
static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
{
- int err = 0;
+ int err = 0, idx;
+ unsigned int part_size;
+ u8 hc_erase_grp_sz = 0, hc_wp_grp_sz = 0;

BUG_ON(!card);

@@ -340,7 +342,14 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

* There are two boot regions of equal size, defined in
* multiples of 128K.
*/
- card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ if (ext_csd[EXT_CSD_BOOT_MULT]) {

+ for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {

+ part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
+ mmc_part_add(card, part_size,
+ EXT_CSD_PART_CONFIG_ACC_BOOT0 + idx,
+ "boot%d", idx, true);
+ }
+ }
}

card->ext_csd.raw_hc_erase_gap_size =

@@ -362,9 +371,9 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
card->ext_csd.raw_partition_support = ext_csd[EXT_CSD_PARTITION_SUPPORT];

if ((ext_csd[EXT_CSD_PARTITION_SUPPORT] & 0x2) &&
(ext_csd[EXT_CSD_PARTITION_ATTRIBUTE] & 0x1)) {
- u8 hc_erase_grp_sz =
+ hc_erase_grp_sz =
ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE];
- u8 hc_wp_grp_sz =
+ hc_wp_grp_sz =
ext_csd[EXT_CSD_HC_WP_GRP_SIZE];

card->ext_csd.enhanced_area_en = 1;

@@ -393,6 +402,41 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)

+ << 16) +

+ (ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3 + 1]

+ << 8) +

+ ext_csd[EXT_CSD_GP_SIZE_MULT + idx * 3];
+ part_size *= (size_t)(hc_erase_grp_sz *
+ hc_wp_grp_sz);
+ mmc_part_add(card, part_size << 19,
+ EXT_CSD_PART_CONFIG_ACC_GP0 + idx,
+ "gp%d", idx, false);
+ }
+ }
card->ext_csd.sec_trim_mult =
ext_csd[EXT_CSD_SEC_TRIM_MULT];
card->ext_csd.sec_erase_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h

index 5294ddf..0522627 100644

--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -12,6 +12,7 @@

#include <linux/mmc/core.h>
#include <linux/mod_devicetable.h>
+#include <linux/genhd.h>

struct mmc_cid {
unsigned int manfid;

@@ -64,7 +65,6 @@ struct mmc_ext_csd {

bool enhanced_area_en; /* enable bit */
unsigned long long enhanced_area_offset; /* Units: Byte */
unsigned int enhanced_area_size; /* Units: KB */
- unsigned int boot_size; /* in bytes */
u8 raw_partition_support; /* 160 */
u8 raw_erased_mem_count; /* 181 */
u8 raw_ext_csd_structure; /* 194 */

@@ -158,6 +158,23 @@ struct sdio_func_tuple;

#define SDIO_MAX_FUNCS 7

+/* The number of MMC physical partitions
+ * It consist of boot partitions(2), general purpose partitions(4) in MMC v4.4
+ */
+#define MMC_NUM_BOOT_PARTITION 2
+#define MMC_NUM_GP_PARTITION 4
+#define MMC_NUM_PHY_PARTITION 6
+
+/*
+ * MMC Physical partitions
+ */
+struct mmc_part {
+ unsigned int size; /* partition size (in bytes) */
+ unsigned int part_cfg; /* it used to part_type */
+ char name[DISK_NAME_LEN];
+ bool force_ro; /* to make boot parts RO by default */
+};
+
/*
* MMC device
*/

@@ -219,9 +236,24 @@ struct mmc_card {

unsigned int sd_bus_speed; /* Bus Speed Mode set for the card */

struct dentry *debugfs_root;
+ struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical partitions */
+ unsigned int nr_parts;
};

/*
+ * This function fill contents in mmc_part.
+ */
+static inline void mmc_part_add(struct mmc_card *card, unsigned int size,
+ unsigned int part_cfg, char *name, int idx, bool ro)
+{
+ card->part[card->nr_parts].size = size;
+ card->part[card->nr_parts].part_cfg = part_cfg;
+ sprintf(card->part[card->nr_parts].name, name, idx);
+ card->part[card->nr_parts].force_ro = ro;
+ card->nr_parts++;
+}
+
+/*
* The world is not perfect and supplies us with broken mmc/sdio devices.
* For at least some of these bugs we need a work-around.
*/
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h

index 50af227..ea558eb 100644

--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -270,6 +270,7 @@ struct _mmc_csd {
* EXT_CSD fields
*/

+#define EXT_CSD_GP_SIZE_MULT 143 /* R/W */
#define EXT_CSD_PARTITION_ATTRIBUTE 156 /* R/W */
#define EXT_CSD_PARTITION_SUPPORT 160 /* RO */

#define EXT_CSD_RST_N_FUNCTION 162 /* R/W */
@@ -313,7 +314,9 @@ struct _mmc_csd {

#define EXT_CSD_PART_CONFIG_ACC_MASK (0x7)
#define EXT_CSD_PART_CONFIG_ACC_BOOT0 (0x1)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT1 (0x2)
+#define EXT_CSD_PART_CONFIG_ACC_GP0 (0x4)
+
+#define EXT_CSD_PART_SUPPORT_PART_EN (0x1)

#define EXT_CSD_CMD_SET_NORMAL (1<<0)
#define EXT_CSD_CMD_SET_SECURE (1<<1)
--
1.7.4.4

[S, Venkatraman]

Stupid question - This will affect all MMC instances, right ? Doesn't
this constitute a ABI breakage for user space scripts ?

[Chris Ball]

Hi,

On Tue, Oct 04 2011, S, Venkatraman wrote:
> Stupid question - This will affect all MMC instances, right ? Doesn't
> this constitute a ABI breakage for user space scripts ?

As I understand it, these partitions were simply not accessible before
this patch: we're not renaming mmcblkNpY to anything else, we're adding
support for mmcblkNgpY partitions, which are part of the "MMC General
Purpose Partition" spec, and are partitioned separately and not
currently available to userspace.

So, I don't see what would constitute breakage. Does that clear it up?

Thanks,

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

[S, Venkatraman]

On Tue, Oct 4, 2011 at 11:55 PM, Chris Ball <c...@laptop.org> wrote:
> Hi,
>
> On Tue, Oct 04 2011, S, Venkatraman wrote:
>> Stupid question - This will affect all MMC instances, right ? Doesn't
>> this constitute a ABI breakage for user space scripts ?
>
> As I understand it, these partitions were simply not accessible before
> this patch:  we're not renaming mmcblkNpY to anything else, we're adding
> support for mmcblkNgpY partitions, which are part of the "MMC General
> Purpose Partition" spec, and are partitioned separately and not
> currently available to userspace.
>
> So, I don't see what would constitute breakage.  Does that clear it up?
>

It does - thanks !!

[Andrei Warkentin]

Hi,

----- Original Message -----
> From: "Namjae Jeon" <linki...@gmail.com>
> To: c...@laptop.org, linu...@vger.kernel.org
> Cc: linux-...@vger.kernel.org, awark...@vmware.com, "adrian hunter" <adrian...@intel.com>, "linus walleij"
> <linus....@linaro.org>, "james p freyensee" <james_p_...@linux.intel.com>, seb...@gmail.com, "Ulf Hansson"
> <Ulf.H...@stericsson.com>, "stefan xk nilsson" <stefan.x...@stericsson.com>, "per forlin"
> <per.f...@stericsson.com>, "johan rudholm" <johan....@stericsson.com>, "Namjae Jeon" <linki...@gmail.com>

> Sent: Tuesday, October 4, 2011 1:45:17 PM
> Subject: [PATCH v10] mmc : general purpose partition support.
>
> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part
> structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
> > cat /proc/partitions
> 179 0 847872 mmcblk0
> 179 192 4096 mmcblk0gp3
> 179 160 4096 mmcblk0gp2
> 179 128 4096 mmcblk0gp1
> 179 96 1052672 mmcblk0gp0
> 179 64 1024 mmcblk0boot1
> 179 32 1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>
> ---

Acked-by: Andrei Warkentin <andrey.w...@gmail.com>

A

[Adrian Hunter]

> - if (card->ext_csd.boot_size&& mmc_boot_partition_access(card->host)) {

> - ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT0,
> - card->ext_csd.boot_size>> 9,
> - true,
> - "boot0");
> - if (ret)
> - return ret;
> - ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT1,
> - card->ext_csd.boot_size>> 9,
> - true,
> - "boot1");
> - if (ret)
> - return ret;
> + for (idx = 0; idx< card->nr_parts; idx++) {
> + if ((idx == 0 || idx == 1)&&
> + !mmc_boot_partition_access(card->host))
> + continue;

Seems a bit fragile. What if someone decided to put the gp
partitions before the boot partitions. What about:

if (mmc_is_boot_partition(&part[idx]) &&
!mmc_boot_partition_access(card->host)
continue;

And:

int mmc_is_boot_partition(struct mmc_part *part)
{
return part->part_cfg >= EXT_CSD_PART_CONFIG_ACC_BOOT0 &&
part->part_cfg <= EXT_CSD_PART_CONFIG_ACC_BOOT0 +
MMC_NUM_BOOT_PARTITION;

[Adrian Hunter]

On 05/10/11 13:21, NamJae Jeon wrote:
> 2011/10/5 Adrian Hunter<adrian...@intel.com>:

> There are the default two boot partition in mmc spec. I think that
> this case can not be happened.
> And boot partition size of EXT CSD is read-only.
> Can you explain which is case put to gp partitions before the boot partition ?

Relying on the partition order in the partition array is fragile.
You cannot predict how someone may want to change it in the future,
and that person will not expect that certain positions have certain meaning.

>
>>
>> if (mmc_is_boot_partition(&part[idx])&&

[Adrian Hunter]

On 05/10/11 14:22, NamJae Jeon wrote:
> 2011/10/5 NamJae Jeon<linki...@gmail.com>:

> 0,1 of Boot partition is fixed if spec is not changed over. but gp

No. 0,1 are an accidental result of the order in which ext-csd is
parsed.

> partition is variable in 2,3,4,5,6 of array.
> If How someone change code, problem occur ?

If you move code around in mmc_read_ext_csd() the order can change,
hence it is fragile.

[NamJae Jeon]

As I remember, boot size can be default zero in the mmc device 4.3 of
some chip vendor.
and it should set proper size using vendor cmd. but gp partition is
not support in mmc 4.3.
um.. I will add your suggested code to avoid vendor specific part in MMC 4.4.
Thanks Adrian~

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>
---

drivers/mmc/card/block.c | 41 +++++++++++++++++++++++------------

drivers/mmc/core/mmc.c | 52 ++++++++++++++++++++++++++++++++++++++++++---
include/linux/mmc/card.h | 34 +++++++++++++++++++++++++++++-
include/linux/mmc/mmc.h | 5 +++-

4 files changed, 112 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 782d5f8..6091f13 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1470,26 +1470,39 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
return 0;
}

+static int mmc_is_boot_partition(struct mmc_part *part)
+{
+ return (part->part_cfg >= EXT_CSD_PART_CONFIG_ACC_BOOT0) &&
+ (part->part_cfg < (EXT_CSD_PART_CONFIG_ACC_BOOT0 +
+ MMC_NUM_BOOT_PARTITION));
+}
+

+/* MMC Physical partition consist of two boot partitions and
+ * up to four general purpose partitions.
+ * For each partitions enabled in EXT_CSD
+ * The block device will allocated to provide access to the partition.
+ */
+
static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
{
- int ret = 0;
+ int idx, ret = 0;

if (!mmc_card_mmc(card))
return 0;

- if (card->ext_csd.boot_size && mmc_boot_partition_access(card->host)) {

- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT0,
- card->ext_csd.boot_size >> 9,
- true,
- "boot0");
- if (ret)
- return ret;
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT1,
- card->ext_csd.boot_size >> 9,
- true,
- "boot1");
- if (ret)
- return ret;
+ for (idx = 0; idx < card->nr_parts; idx++) {

+ if (mmc_is_boot_partition(&card->part[idx]) &&

+ !mmc_boot_partition_access(card->host))
+ continue;

+ if (card->part[idx].size) {
+ ret = mmc_blk_alloc_part(card, md,
+ card->part[idx].part_cfg,
+ card->part[idx].size >> 9,
+ card->part[idx].force_ro,
+ card->part[idx].name);
+ if (ret)
+ return ret;
+ }
}

return ret;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

index c632b1f..425803d 100644

1.7.4.4

[Andrei Warkentin]

Hi,

----- Original Message -----
> From: "Namjae Jeon" <linki...@gmail.com>
> To: c...@laptop.org, linu...@vger.kernel.org
> Cc: linux-...@vger.kernel.org, awark...@vmware.com, "adrian hunter" <adrian...@intel.com>, "linus walleij"
> <linus....@linaro.org>, "james p freyensee" <james_p_...@linux.intel.com>, seb...@gmail.com, "Ulf Hansson"
> <Ulf.H...@stericsson.com>, "stefan xk nilsson" <stefan.x...@stericsson.com>, "per forlin"
> <per.f...@stericsson.com>, "johan rudholm" <johan....@stericsson.com>, "Namjae Jeon" <linki...@gmail.com>

> + for (idx = 0; idx < card->nr_parts; idx++) {
> + if (mmc_is_boot_partition(&card->part[idx]) &&
> + !mmc_boot_partition_access(card->host))
> + continue;
> + if (card->part[idx].size) {
> + ret = mmc_blk_alloc_part(card, md,
> + card->part[idx].part_cfg,
> + card->part[idx].size >> 9,
> + card->part[idx].force_ro,
> + card->part[idx].name);
> + if (ret)
> + return ret;
> + }
> }

Is there any reason for putting the mmc_boot_partition_access() logic here? If
boot partitions are not allowed by host, then just don't add the mmc_parts to
the parts array, no? Such minutae should belong in core mmc code, not block driver, IMHO.

A

[NamJae Jeon]

2011/10/6 Andrei Warkentin <awark...@vmware.com>:

Hi.

How about changing patch like this ?

if (ext_csd[EXT_CSD_BOOT_MULT] && mmc_boot_partition_access(card->host)) {
for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {

part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;

mmc_part_add(card, part_size,
EXT_CSD_PART_CONFIG_ACC_BOOT0 + idx,
"boot%d", idx, true);
}
}

I want to hear the opinion from andrei and
adrian(mmc_boot_partition_access patch's author)
Thanks.

[Andrei E. Warkentin]

HI NamJae,

2011/10/5 NamJae Jeon <linki...@gmail.com>:

>
> How about changing patch like this ?
>
> if (ext_csd[EXT_CSD_BOOT_MULT] && mmc_boot_partition_access(card->host)) {
>                        for (idx = 0; idx < MMC_NUM_BOOT_PARTITION; idx++) {
>                                part_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;
>                                mmc_part_add(card, part_size,
>                                        EXT_CSD_PART_CONFIG_ACC_BOOT0 + idx,
>                                        "boot%d", idx, true);
>                        }
>                }
>
> I want to hear the opinion from andrei and
> adrian(mmc_boot_partition_access patch's author)
> Thanks.

I like this.

A

[Adrian Hunter]

I like data structures to model reality i.e. the partitions exist but are not
accessible. However I am not fussed - change it or not.

[Namjae Jeon]

It allows gerneral purpose partitions in MMC Device.
And I try to simpliy make mmc_blk_alloc_parts using mmc_part structure suggested by Andrei Warkentin.
After patching, we can see general purpose partitions like this.
> cat /proc/partitions
179 0 847872 mmcblk0
179 192 4096 mmcblk0gp3
179 160 4096 mmcblk0gp2
179 128 4096 mmcblk0gp1
179 96 1052672 mmcblk0gp0
179 64 1024 mmcblk0boot1
179 32 1024 mmcblk0boot0

Signed-off-by: Namjae Jeon <linki...@gmail.com>
---

drivers/mmc/card/block.c | 31 +++++++++++++++------------

drivers/mmc/core/mmc.c | 52 ++++++++++++++++++++++++++++++++++++++++++---
include/linux/mmc/card.h | 34 +++++++++++++++++++++++++++++-
include/linux/mmc/mmc.h | 5 +++-

4 files changed, 102 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 782d5f8..4f4b986 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1470,26 +1470,29 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
return 0;
}

+/* MMC Physical partition consist of two boot partitions and
+ * up to four general purpose partitions.
+ * For each partitions enabled in EXT_CSD
+ * The block device will allocated to provide access to the partition.
+ */
+
static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
{
- int ret = 0;
+ int idx, ret = 0;

if (!mmc_card_mmc(card))
return 0;

- if (card->ext_csd.boot_size && mmc_boot_partition_access(card->host)) {
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT0,
- card->ext_csd.boot_size >> 9,
- true,
- "boot0");
- if (ret)
- return ret;
- ret = mmc_blk_alloc_part(card, md, EXT_CSD_PART_CONFIG_ACC_BOOT1,
- card->ext_csd.boot_size >> 9,
- true,
- "boot1");
- if (ret)
- return ret;

+ for (idx = 0; idx < card->nr_parts; idx++) {

+ if (card->part[idx].size) {
+ ret = mmc_blk_alloc_part(card, md,
+ card->part[idx].part_cfg,
+ card->part[idx].size >> 9,
+ card->part[idx].force_ro,
+ card->part[idx].name);
+ if (ret)
+ return ret;
+ }
}

return ret;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

index c632b1f..5cb3bdc 100644

--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -239,7 +239,9 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)
*/
static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
{
- int err = 0;
+ int err = 0, idx;
+ unsigned int part_size;
+ u8 hc_erase_grp_sz = 0, hc_wp_grp_sz = 0;

BUG_ON(!card);

@@ -340,7 +342,14 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
* There are two boot regions of equal size, defined in
* multiples of 128K.
*/
- card->ext_csd.boot_size = ext_csd[EXT_CSD_BOOT_MULT] << 17;

+ if (ext_csd[EXT_CSD_BOOT_MULT] && mmc_boot_partition_access(card->host)) {

[Andrei Warkentin]

----- Original Message -----
> From: "Namjae Jeon" <linki...@gmail.com>
> To: c...@laptop.org, linu...@vger.kernel.org
> Cc: linux-...@vger.kernel.org, awark...@vmware.com, "adrian hunter" <adrian...@intel.com>, "linus walleij"
> <linus....@linaro.org>, "james p freyensee" <james_p_...@linux.intel.com>, seb...@gmail.com, "Ulf Hansson"
> <Ulf.H...@stericsson.com>, "stefan xk nilsson" <stefan.x...@stericsson.com>, "per forlin"
> <per.f...@stericsson.com>, "johan rudholm" <johan....@stericsson.com>, "Namjae Jeon" <linki...@gmail.com>

> Sent: Thursday, October 6, 2011 10:41:38 AM
> Subject: [PATCH v12] mmc : general purpose partition support.
>
> It allows gerneral purpose partitions in MMC Device.
> And I try to simpliy make mmc_blk_alloc_parts using mmc_part
> structure suggested by Andrei Warkentin.
> After patching, we can see general purpose partitions like this.
> > cat /proc/partitions
> 179 0 847872 mmcblk0
> 179 192 4096 mmcblk0gp3
> 179 160 4096 mmcblk0gp2
> 179 128 4096 mmcblk0gp1
> 179 96 1052672 mmcblk0gp0
> 179 64 1024 mmcblk0boot1
> 179 32 1024 mmcblk0boot0
>
> Signed-off-by: Namjae Jeon <linki...@gmail.com>
> ---

Acked-by: Andrei Warkentin <andrey.w...@gmail.com>

A

[Chris Ball]

Hi,

On Thu, Oct 06 2011, Andrei Warkentin wrote:
>> Subject: [PATCH v12] mmc : general purpose partition support.
>>
>> It allows gerneral purpose partitions in MMC Device.
>> And I try to simpliy make mmc_blk_alloc_parts using mmc_part
>> structure suggested by Andrei Warkentin.
>> After patching, we can see general purpose partitions like this.
>> > cat /proc/partitions
>> 179 0 847872 mmcblk0
>> 179 192 4096 mmcblk0gp3
>> 179 160 4096 mmcblk0gp2
>> 179 128 4096 mmcblk0gp1
>> 179 96 1052672 mmcblk0gp0
>> 179 64 1024 mmcblk0boot1
>> 179 32 1024 mmcblk0boot0
>>
>> Signed-off-by: Namjae Jeon <linki...@gmail.com>
>> ---
>
> Acked-by: Andrei Warkentin <andrey.w...@gmail.com>

Thanks very much! Pushed to mmc-next for 3.2 now.

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