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

[NamJae Jeon]

2011/9/24 J Freyensee <james_p_freyensee@xxxxxxxxxxxxxxx>:
> On 09/22/2011 05:29 PM, NamJae Jeon wrote:
>>
>> 2011/9/23 J Freyensee<james_p_freyensee@xxxxxxxxxxxxxxx>
>>>
>>> On 09/22/2011 04:58 PM, NamJae Jeon wrote:
>>>>
>>>> 2011/9/23 J Freyensee<james_p_freyensee@xxxxxxxxxxxxxxx>:
>>>>>
>>>>> On 09/22/2011 04:15 PM, NamJae Jeon wrote:
>>>>>>
>>>>>> 2011/9/23 J Freyensee<james_p_freyensee@xxxxxxxxxxxxxxx>:
>>>>>>>
>>>>>>> On 09/22/2011 08:34 AM, Namjae Jeon wrote:
>>>>>>>>
>>>>>>>> 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<linkinjeon@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>> Â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) {
>
> Looks like you may need to also incorporate the ideas Adrian Hunter did in a
> patch that seems similar to yours, mainly also checking
> mmc_boot_partition_access(card->host) in your if() statement?
>
> I attached his patch that was sent today.
>
> Jay
I checked attached patch, see the patch v2. I think that adrian may
modify his patch base on my patch.
Thanks

>>>>>>>>
>>>>>>>> + Â Â Â Â Â Â Â 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 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).
>>>>>>
>>>>>> -> Â Â Â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.
>>>>
>>>> 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.
>>
>> Okay, I will release patch v2 after adding function header comments again.
>> Thanks for your review.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> + Â Â Â 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)
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> J (James/Jay) Freyensee
>>>>>>> Storage Technology Group
>>>>>>> Intel Corporation
>>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> J (James/Jay) Freyensee
>>>>> Storage Technology Group
>>>>> Intel Corporation
>>>>>
>>>
>>>
>>> --
>>> J (James/Jay) Freyensee
>>> Storage Technology Group
>>> Intel Corporation
>>
>> NïïïïïrïïyïïïbïXïïÇvï^ï)Þ{.nï+ïïïï{ïïg"ïï^nïrïïzï ïïhïïïï&ïï ïGïïïhï
>> (ïéïÝj"ïï ï mïïïïïzïÞïïïfïïïhïïï~ïmml==
>
>
> --
> J (James/Jay) Freyensee
> Storage Technology Group
> Intel Corporation
>
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_