Re: [PATCH v7] mmc : general purpose partition support.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2011/10/3 Sebastian Rasmussen <sebras@xxxxxxxxx>:
> 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.
>
>> 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 <linkinjeon@xxxxxxxxx>
>> ---
>>  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(-)
>>
>> 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,
>>        return 0;
>>  }
>>
>> +/* MMC Physical partition consist of two boot partitons and
>> + * four general purpose partitions.
>
> up to four general purpose partitions.
Hi~
I will add.
>
>> + * 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.
I wiil modify also.
>
>> + */
>> +
>>  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].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..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.
>
> What does part_type refer to?
it refer to cookie, and it will be used for part_type.
I will modify with below your suggestion.
>
>> +                */
>> +
>> +               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?
yes, it can dupilicatley set in case of user ehanced area is set.
I will add.
>
>> +
>> +                       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?
yes, it is compiled without problem. but I will modify part_size << 19.
>
>> +                                       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.
yes, I agree. I followed current other code. As you know, user
enhanced area set code is used to constant value directly.
I think that your opinion is correct.
and.. I don't know why idx * 3 is used.
>
> #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);
>
> }
>
>> +               }
>>                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
>> --- 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
>
> 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.
As you know, any other fields was also over 80 characters in struct
card. So I thought that it is permitted in this structure.
And the meaning of GP should be "General Purpose"
>
>> +
>> +/*
>> + * 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?
It called to partition config in ext_csd filed. so I used part_config
name when using this value first.
And I think that cookie meaning is shortly stored and used.
so I used it. so I think that these have each meaning.
I can change part_config in mmc_part instead of cookie.
how do you think ?

Thanks for your review.
>
>> +       char    name[DISK_NAME_LEN];
>> +       bool    force_ro;       /* to make boot parts RO by default */
>> +};
>> +
>>  /*
>>  * MMC device
>>  */
>> @@ -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
>>
>>
>
>  / Sebastian
>
��.n��������+%������w��{.n�����{��i��)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥



[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux