Hi Amarendra,
On Fri, Jan 11, 2013 at 5:50 AM, Amarendra Reddy
<amar.lavan...@gmail.com> wrote:
> Hi Simon / Jaehoon,
>
> Please find my responses below.
>
> Thanks & Regards
> Amarendra reddy
>
> On 11 January 2013 11:11, Simon Glass <s...@chromium.org> wrote:
>>
>> HI Jaehoon,
>>
>> On Thu, Jan 10, 2013 at 7:54 PM, Jaehoon Chung <jh80.ch...@samsung.com>
>> wrote:
>> > On 01/11/2013 01:46 AM, Simon Glass wrote:
>> >> Hi Amar,
>> >>
>> >> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra...@samsung.com> wrote:
>> >>> This patch adds commands to open, close and resize boot partitions on
>> >>> EMMC.
>> >>>
>> >>> Changes from V1:
>> >>> 1)Combined the common piece of code between 'open' and 'close'
>> >>> operations.
>> >>>
>> >>> Changes from V2:
>> >>> 1)Updation of commit message and resubmition of proper patch
>> >>> set.
>> >>>
>> >>> Changes from V3:
>> >>> No change.
>> >>>
>> >>> Signed-off-by: Amar <amarendra...@samsung.com>
>> >>> ---
>> >>> common/cmd_mmc.c | 84
>> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> >>> 1 file changed, 83 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
>> >>> index 7dacd51..1dabb5b 100644
>> >>> --- a/common/cmd_mmc.c
>> >>> +++ b/common/cmd_mmc.c
>> >>> @@ -248,6 +248,84 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag,
>> >>> int argc, char * const argv[])
>> >>> curr_device, mmc->part_num);
>> >>>
>> >>> return 0;
>> >>> + } else if ((strcmp(argv[1], "open") == 0) ||
>> >>> + (strcmp(argv[1], "close") == 0)) {
>> >>
>> >> How about putting this block in its own function?
>
>
> Ok. Shall put the entire block in a new function.
>>
>> >>
>> >>> + int dev;
>> >>> + struct mmc *mmc;
>> >>> +
>> >>> + if (argc == 2)
>> >>> + dev = curr_device;
>> >>> + else if (argc == 3)
>> >>> + dev = simple_strtoul(argv[2], NULL, 10);
>> >>> + else
>> >>> + return CMD_RET_USAGE;
>> >>> +
>> >>> + mmc = find_mmc_device(dev);
>> >>> + if (!mmc) {
>> >>> + printf("no mmc device at slot %x\n", dev);
>> >>> + return 1;
>> >>> + }
>> >>> +
>> >>> + if (IS_SD(mmc)) {
>> >>> + printf("SD device cannot be opened/closed\n");
>> >>> + return 1;
>> >>> + }
>> >>> +
>> >>> + if (strcmp(argv[1], "open") == 0) {
>> >>> + if (!(mmc_boot_open(mmc))) {
>> >>> + printf("EMMC OPEN Success.\n");
>> >>> + printf("\t\t\t!!!Notice!!!\n");
>> >>> + printf("!You must close EMMC"
>> >>> + " boot Partition after all"
>> >>> + " images are written\n");
>> >>
>> >> Do you need to split these strings so much? Perhaps when it is in a
>> >> function the indenting will be less?
>
> Ok.
>>
>> >>
>> >>> + printf("!EMMC boot partition"
>> >>> + " has continuity at"
>> >>> + " image writing time.\n");
>> >>> + printf("!So, Do not close boot"
>> >>> + " partition, Before, all"
>> >>> + " images are written.\n");
>> >>> + return 0;
>> >>> + } else {
>> >>> + printf("EMMC OPEN Failed.\n");
>> >>> + return 1;
>> >>
>> >> You could put this above the other block and reduce indenting:
>> >>
>> >> if (mmc_boot_open(mmc)) {
>> >> printf("EMMC OPEN Failed.\n");
>> >> return 1;
>> >> }
>> >> ...code continues
>> >>
>
> Ok.
>>
>> >>> + }
>> >>> + }
>> >>> +
>> >>> + if (strcmp(argv[1], "close") == 0) {
>> >>> + if (!(mmc_boot_close(mmc))) {
>> >>> + printf("EMMC CLOSE Success.\n");
>> >>
>> >> Shouldn't print a message on success
>> >>
>
> Ok. shall remove the print message in success case.
>
>>>> + return 0;
>>>> + } else {
>>>> + printf("EMMC CLOSE Failed.\n");
>>>> + return 1;
>>>> + }
>>>> + }
>>>> + } else if (strcmp(argv[1], "bootpart") == 0) {
>>>> + int dev;
>>>> + dev = simple_strtoul(argv[2], NULL, 10);
>>>> +
>>>> + u32 bootsize = simple_strtoul(argv[3], NULL, 10);
>>>> + u32 rpmbsize = simple_strtoul(argv[4], NULL, 10);
>>>> + struct mmc *mmc = find_mmc_device(dev);
>>>> + if (!mmc) {
>>>> + printf("no mmc device at slot %x\n", dev);
>>>> + return 1;
>>>> + }
>>>> +
>>>> + if (IS_SD(mmc)) {
>>>> + printf("It is not a EMMC device\n");
>>>> + return 1;
>>>> + }
>>>> +
>>>> + if (0 == mmc_boot_partition_size_change(mmc,
>>>> + bootsize, rpmbsize)) {
>>>> + printf("EMMC boot partition Size %d MB\n",
>>>> bootsize);
>>>> + printf("EMMC RPMB partition Size %d MB\n",
>>>> rpmbsize);
>>>> + return 0;
>>>> + } else {
>>>> + printf("EMMC boot partition Size change
>>>> Failed.\n");
>>>> + return 1;
>>>> + }
>>>> }
>>>>
>>>> state = MMC_INVALID;
>>>> @@ -317,5 +395,9 @@ U_BOOT_CMD(
>>>> "mmc rescan\n"
>>>> "mmc part - lists available partition on current mmc device\n"
>>>> "mmc dev [dev] [part] - show or set current mmc device
>>>> [partition]\n"
>>>> - "mmc list - lists available devices");
>>>> + "mmc list - lists available devices\n"
>>>> + "mmc open <device num> - opens the specified device\n"
>>>> + "mmc close <device num> - closes the specified device\n"
>>>> + "mmc bootpart <device num> <boot part size MB> <RPMB part size
>>>> MB>\n"
>>>> + " - change sizes of boot and RPMB partions of specified
>>>> device\n");
>>>> #endif
>>>
>>> Also did you see Wolfgang's suggestion that we put the partition stuff
>>> in the 'part' command (at least that's what I think he said). You
>>> could have 'part open', 'part close' and maybe 'part resize'?
>> How about using "mmc bootpart <device_num> <ack> <enable> <access>"
>
>> Maybe - what do these parameters mean?
>>
>
> The functions "mmc_boot_open()" and "mmc_boot_close()" have lot of commom
> code. So Jaehoon suggested to
> combine them into single generic function as below
> 1) So a single generic function "mmc_boot_part_access(struct mmc *mmc, int
> ack, int part_num, int access)" to be used
> instead of two functions open() and close().
> 2) By doing so user can specify which boot partition to be accessed (opened
> / closed).
>
> The parameters ack, part_num, access, represent the values of bits in the
> PARTITION_CONFIG field
> of the Extended CSD register in order to address one of the partitions.
> PARTITION_CONFIG - [179]:
> -------------------------------------------
> Bit 6: BOOT_ACK (R/W/E)
> 0x0 : No boot acknowledge sent (default)
> 0x1 : Boot acknowledge sent during boot operation
> Bit[5:3] : BOOT_PARTITION_ENABLE (R/W/E)
> User selects boot data that will be sent to master
> 0x0 : Device not boot enabled (default)
> 0x1 : Boot partition 1 enabled for boot
> 0x2 : Boot partition 2 enabled for boot
> Bit[2:0] : PARTITION_ACCESS (before BOOT_PARTITION_ACCESS, R/W/E_P)
> User selects partitions to access
> 0x0 : No access to boot partition (default)
> 0x1 : R/W boot partition 1
> 0x2 : R/W boot partition 2
> 0x3 : R/W Replay Protected Memory Block (RPMB)
>
> Please comment on the above.
Yes sounds good.
>>
>> > Also i think that we can reduce the code line.
>>
>> OK good.
>>
[snip]
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
