diff mbox

[U-Boot] ums: Add "dev num" parameter. Check mmc device before do ums init.

Message ID 1378213073-25692-2-git-send-email-p.marczak@samsung.com
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Commit Message

Przemyslaw Marczak Sept. 3, 2013, 12:57 p.m. UTC 
This change allows using every mmc device instance with ums, like eMMC
or SD cards. Now MMC device is checked before ums is inited.

Example of use: ums <device_number> for mmc devices.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
CC: Marek Vasut <marek.vasut@gmail.com>
---
 board/samsung/trats/trats.c   |   12 +++---------
 common/cmd_usb_mass_storage.c |   30 ++++++++++++++----------------
 include/usb_mass_storage.h    |    4 ++--
 3 files changed, 19 insertions(+), 27 deletions(-)

Comments

Marek Vasut Sept. 3, 2013, 10:26 p.m. UTC | #1 
Dear Przemyslaw Marczak,

> This change allows using every mmc device instance with ums, like eMMC
> or SD cards. Now MMC device is checked before ums is inited.
> 
> Example of use: ums <device_number> for mmc devices.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> CC: Marek Vasut <marek.vasut@gmail.com>
> ---
>  board/samsung/trats/trats.c   |   12 +++---------
>  common/cmd_usb_mass_storage.c |   30 ++++++++++++++----------------
>  include/usb_mass_storage.h    |    4 ++--
>  3 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
> index 7f61d17..b7f7b05 100644
> --- a/board/samsung/trats/trats.c
> +++ b/board/samsung/trats/trats.c
> @@ -816,17 +816,11 @@ static struct ums_board_info ums_board = {
>  	},
>  };
> 
> -struct ums_board_info *board_ums_init(unsigned int dev_num, unsigned int
> offset, -				      unsigned int part_size)
> +struct ums_board_info *board_ums_init(struct mmc *mmc, unsigned int
> offset, +					unsigned int part_size)
>  {
> -	struct mmc *mmc;
> -
> -	mmc = find_mmc_device(dev_num);
> -	if (!mmc)
> -		return NULL;
> -
>  	ums_board.ums_dev.mmc = mmc;
> -	ums_board.ums_dev.dev_num = dev_num;
> +	ums_board.ums_dev.dev_num = mmc->block_dev.dev;

You already pass "mmc", why pass mmc->block_dev.dev too? Is it not a little 
redundant?

>  	ums_board.ums_dev.offset = offset;
>  	ums_board.ums_dev.part_size = part_size;
> 
> diff --git a/common/cmd_usb_mass_storage.c b/common/cmd_usb_mass_storage.c
> index 33a4715..62c1308 100644
> --- a/common/cmd_usb_mass_storage.c
> +++ b/common/cmd_usb_mass_storage.c
> @@ -14,6 +14,7 @@
>  int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
>  			       int argc, char * const argv[])
>  {
> +	struct mmc *mmc = NULL;
>  	char *ep;
>  	unsigned int dev_num = 0, offset = 0, part_size = 0;
>  	int rc;
> @@ -21,29 +22,29 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
>  	struct ums_board_info *ums_info;
>  	static char *s = "ums";
> 
> -	if (argc < 2) {
> -		printf("usage: ums <dev> - e.g. ums 0\n");
> -		return 0;
> -	}
> +	if (argc < 2)
> +		return CMD_RET_USAGE;

UMS works on MMC devices only right now?

>  	dev_num = (int)simple_strtoul(argv[1], &ep, 16);
> 
> -	if (dev_num) {
> -		puts("\nSet eMMC device to 0! - e.g. ums 0\n");
> -		goto fail;
> +	mmc = find_mmc_device(dev_num);
> +	if (!mmc) {
> +		printf("UMS error: invalid mmc device num: %d.\n", dev_num);
> +		return CMD_RET_FAILURE;
>  	}
> 
>  	board_usb_init();
> -	ums_info = board_ums_init(dev_num, offset, part_size);
> 
> +	ums_info = board_ums_init(mmc, offset, part_size);
>  	if (!ums_info) {
> -		printf("MMC: %d -> NOT available\n", dev_num);
> -		goto fail;
> +		printf("UMS is not supported on this board.\n");

puts()

> +		return CMD_RET_FAILURE;
>  	}
> +
>  	rc = fsg_init(ums_info);
>  	if (rc) {
>  		printf("cmd ums: fsg_init failed\n");
> -		goto fail;
> +		return CMD_RET_FAILURE;
>  	}
> 
>  	g_dnl_register(s);
> @@ -61,13 +62,10 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
>  	}
>  exit:
>  	g_dnl_unregister();
> -	return 0;
> -
> -fail:
> -	return -1;
> +	return CMD_RET_SUCCESS;
>  }
> 
>  U_BOOT_CMD(ums, CONFIG_SYS_MAXARGS, 1, do_usb_mass_storage,
>  	"Use the UMS [User Mass Storage]",
> -	"ums - User Mass Storage Gadget"
> +	"ums <dev> e.g. ums 0"
>  );
> diff --git a/include/usb_mass_storage.h b/include/usb_mass_storage.h
> index 35cdcc3..0f94f31 100644
> --- a/include/usb_mass_storage.h
> +++ b/include/usb_mass_storage.h
> @@ -34,8 +34,8 @@ extern void board_usb_init(void);
> 
>  extern int fsg_init(struct ums_board_info *);
>  extern void fsg_cleanup(void);
> -extern struct ums_board_info *board_ums_init(unsigned int,
> -					     unsigned int, unsigned int);
> +extern struct ums_board_info *board_ums_init(struct mmc *, unsigned int,
> +						unsigned int);
>  extern int usb_gadget_handle_interrupts(void);
>  extern int fsg_main_thread(void *);

Best regards,
Marek Vasut
Przemyslaw Marczak Sept. 4, 2013, 9:57 a.m. UTC | #2 
Hello Marek,
Thank you for reply.

On 09/04/2013 12:26 AM, Marek Vasut wrote:
> Dear Przemyslaw Marczak,
>
>> This change allows using every mmc device instance with ums, like eMMC
>> or SD cards. Now MMC device is checked before ums is inited.
>>
>> Example of use: ums <device_number> for mmc devices.
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> CC: Marek Vasut <marek.vasut@gmail.com>
>> ---
>>   board/samsung/trats/trats.c   |   12 +++---------
>>   common/cmd_usb_mass_storage.c |   30 ++++++++++++++----------------
>>   include/usb_mass_storage.h    |    4 ++--
>>   3 files changed, 19 insertions(+), 27 deletions(-)
>>
>> diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
>> index 7f61d17..b7f7b05 100644
>> --- a/board/samsung/trats/trats.c
>> +++ b/board/samsung/trats/trats.c
>> @@ -816,17 +816,11 @@ static struct ums_board_info ums_board = {
>>   	},
>>   };
>>
>> -struct ums_board_info *board_ums_init(unsigned int dev_num, unsigned int
>> offset, -				      unsigned int part_size)
>> +struct ums_board_info *board_ums_init(struct mmc *mmc, unsigned int
>> offset, +					unsigned int part_size)
>>   {
>> -	struct mmc *mmc;
>> -
>> -	mmc = find_mmc_device(dev_num);
>> -	if (!mmc)
>> -		return NULL;
>> -
>>   	ums_board.ums_dev.mmc = mmc;
>> -	ums_board.ums_dev.dev_num = dev_num;
>> +	ums_board.ums_dev.dev_num = mmc->block_dev.dev;
>
> You already pass "mmc", why pass mmc->block_dev.dev too? Is it not a little
> redundant?
>

You are right, it is little redundant but pointer to this structure is 
returned so we expect that structure fields were proper filled, right?

>>   	ums_board.ums_dev.offset = offset;
>>   	ums_board.ums_dev.part_size = part_size;
>>
>> diff --git a/common/cmd_usb_mass_storage.c b/common/cmd_usb_mass_storage.c
>> index 33a4715..62c1308 100644
>> --- a/common/cmd_usb_mass_storage.c
>> +++ b/common/cmd_usb_mass_storage.c
>> @@ -14,6 +14,7 @@
>>   int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
>>   			       int argc, char * const argv[])
>>   {
>> +	struct mmc *mmc = NULL;
>>   	char *ep;
>>   	unsigned int dev_num = 0, offset = 0, part_size = 0;
>>   	int rc;
>> @@ -21,29 +22,29 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
>>   	struct ums_board_info *ums_info;
>>   	static char *s = "ums";
>>
>> -	if (argc < 2) {
>> -		printf("usage: ums <dev> - e.g. ums 0\n");
>> -		return 0;
>> -	}
>> +	if (argc < 2)
>> +		return CMD_RET_USAGE;
>
> UMS works on MMC devices only right now?
>

Yes, it is.

>>   	dev_num = (int)simple_strtoul(argv[1], &ep, 16);
>>
>> -	if (dev_num) {
>> -		puts("\nSet eMMC device to 0! - e.g. ums 0\n");
>> -		goto fail;
>> +	mmc = find_mmc_device(dev_num);
>> +	if (!mmc) {
>> +		printf("UMS error: invalid mmc device num: %d.\n", dev_num);
>> +		return CMD_RET_FAILURE;
>>   	}
>>
>>   	board_usb_init();
>> -	ums_info = board_ums_init(dev_num, offset, part_size);
>>
>> +	ums_info = board_ums_init(mmc, offset, part_size);
>>   	if (!ums_info) {
>> -		printf("MMC: %d -> NOT available\n", dev_num);
>> -		goto fail;
>> +		printf("UMS is not supported on this board.\n");
>
> puts()
>

Right, I will change it in patch v2.

>> +		return CMD_RET_FAILURE;
>>   	}
>> +
>>   	rc = fsg_init(ums_info);
>>   	if (rc) {
>>   		printf("cmd ums: fsg_init failed\n");
>> -		goto fail;
>> +		return CMD_RET_FAILURE;
>>   	}
>>
>>   	g_dnl_register(s);
>> @@ -61,13 +62,10 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
>>   	}
>>   exit:
>>   	g_dnl_unregister();
>> -	return 0;
>> -
>> -fail:
>> -	return -1;
>> +	return CMD_RET_SUCCESS;
>>   }
>>
>>   U_BOOT_CMD(ums, CONFIG_SYS_MAXARGS, 1, do_usb_mass_storage,
>>   	"Use the UMS [User Mass Storage]",
>> -	"ums - User Mass Storage Gadget"
>> +	"ums <dev> e.g. ums 0"
>>   );
>> diff --git a/include/usb_mass_storage.h b/include/usb_mass_storage.h
>> index 35cdcc3..0f94f31 100644
>> --- a/include/usb_mass_storage.h
>> +++ b/include/usb_mass_storage.h
>> @@ -34,8 +34,8 @@ extern void board_usb_init(void);
>>
>>   extern int fsg_init(struct ums_board_info *);
>>   extern void fsg_cleanup(void);
>> -extern struct ums_board_info *board_ums_init(unsigned int,
>> -					     unsigned int, unsigned int);
>> +extern struct ums_board_info *board_ums_init(struct mmc *, unsigned int,
>> +						unsigned int);
>>   extern int usb_gadget_handle_interrupts(void);
>>   extern int fsg_main_thread(void *);
>
> Best regards,
> Marek Vasut
>

Thank you,
Marek Vasut Sept. 4, 2013, 12:34 p.m. UTC | #3 
Dear Przemyslaw Marczak,

> Hello Marek,
> Thank you for reply.
> 
> On 09/04/2013 12:26 AM, Marek Vasut wrote:
> > Dear Przemyslaw Marczak,
> > 
> >> This change allows using every mmc device instance with ums, like eMMC
> >> or SD cards. Now MMC device is checked before ums is inited.
> >> 
> >> Example of use: ums <device_number> for mmc devices.
> >> 
> >> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> CC: Marek Vasut <marek.vasut@gmail.com>
> >> ---
> >> 
> >>   board/samsung/trats/trats.c   |   12 +++---------
> >>   common/cmd_usb_mass_storage.c |   30 ++++++++++++++----------------
> >>   include/usb_mass_storage.h    |    4 ++--
> >>   3 files changed, 19 insertions(+), 27 deletions(-)
> >> 
> >> diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
> >> index 7f61d17..b7f7b05 100644
> >> --- a/board/samsung/trats/trats.c
> >> +++ b/board/samsung/trats/trats.c
> >> @@ -816,17 +816,11 @@ static struct ums_board_info ums_board = {
> >> 
> >>   	},
> >>   
> >>   };
> >> 
> >> -struct ums_board_info *board_ums_init(unsigned int dev_num, unsigned
> >> int offset, -				      unsigned int part_size)
> >> +struct ums_board_info *board_ums_init(struct mmc *mmc, unsigned int
> >> offset, +					unsigned int part_size)
> >> 
> >>   {
> >> 
> >> -	struct mmc *mmc;
> >> -
> >> -	mmc = find_mmc_device(dev_num);
> >> -	if (!mmc)
> >> -		return NULL;
> >> -
> >> 
> >>   	ums_board.ums_dev.mmc = mmc;
> >> 
> >> -	ums_board.ums_dev.dev_num = dev_num;
> >> +	ums_board.ums_dev.dev_num = mmc->block_dev.dev;
> > 
> > You already pass "mmc", why pass mmc->block_dev.dev too? Is it not a
> > little redundant?
> 
> You are right, it is little redundant but pointer to this structure is
> returned so we expect that structure fields were proper filled, right?

Why not just remove the dev_num field ? The UMS core can retrieve that 
information itself.

[...]

Best regards,
Marek Vasut
Przemyslaw Marczak Sept. 4, 2013, 2:01 p.m. UTC | #4 
On 09/04/2013 02:34 PM, Marek Vasut wrote:
> Dear Przemyslaw Marczak,
>
>> Hello Marek,
>> Thank you for reply.
>>
>> On 09/04/2013 12:26 AM, Marek Vasut wrote:
>>> Dear Przemyslaw Marczak,
>>>
>>>> This change allows using every mmc device instance with ums, like eMMC
>>>> or SD cards. Now MMC device is checked before ums is inited.
>>>>
>>>> Example of use: ums <device_number> for mmc devices.
>>>>
>>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> CC: Marek Vasut <marek.vasut@gmail.com>
>>>> ---
>>>>
>>>>    board/samsung/trats/trats.c   |   12 +++---------
>>>>    common/cmd_usb_mass_storage.c |   30 ++++++++++++++----------------
>>>>    include/usb_mass_storage.h    |    4 ++--
>>>>    3 files changed, 19 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
>>>> index 7f61d17..b7f7b05 100644
>>>> --- a/board/samsung/trats/trats.c
>>>> +++ b/board/samsung/trats/trats.c
>>>> @@ -816,17 +816,11 @@ static struct ums_board_info ums_board = {
>>>>
>>>>    	},
>>>>
>>>>    };
>>>>
>>>> -struct ums_board_info *board_ums_init(unsigned int dev_num, unsigned
>>>> int offset, -				      unsigned int part_size)
>>>> +struct ums_board_info *board_ums_init(struct mmc *mmc, unsigned int
>>>> offset, +					unsigned int part_size)
>>>>
>>>>    {
>>>>
>>>> -	struct mmc *mmc;
>>>> -
>>>> -	mmc = find_mmc_device(dev_num);
>>>> -	if (!mmc)
>>>> -		return NULL;
>>>> -
>>>>
>>>>    	ums_board.ums_dev.mmc = mmc;
>>>>
>>>> -	ums_board.ums_dev.dev_num = dev_num;
>>>> +	ums_board.ums_dev.dev_num = mmc->block_dev.dev;
>>>
>>> You already pass "mmc", why pass mmc->block_dev.dev too? Is it not a
>>> little redundant?
>>
>> You are right, it is little redundant but pointer to this structure is
>> returned so we expect that structure fields were proper filled, right?
>
> Why not just remove the dev_num field ? The UMS core can retrieve that
> information itself.
>
> [...]
>
> Best regards,
> Marek Vasut
>

Hello,
I'm making little UMS code refactor now. I will consider your suggestion.

Regards,
diff mbox

Patch

diff --git a/board/samsung/trats/trats.c b/board/samsung/trats/trats.c
index 7f61d17..b7f7b05 100644
--- a/board/samsung/trats/trats.c
+++ b/board/samsung/trats/trats.c
@@ -816,17 +816,11 @@  static struct ums_board_info ums_board = {
 	},
 };
 
-struct ums_board_info *board_ums_init(unsigned int dev_num, unsigned int offset,
-				      unsigned int part_size)
+struct ums_board_info *board_ums_init(struct mmc *mmc, unsigned int offset,
+					unsigned int part_size)
 {
-	struct mmc *mmc;
-
-	mmc = find_mmc_device(dev_num);
-	if (!mmc)
-		return NULL;
-
 	ums_board.ums_dev.mmc = mmc;
-	ums_board.ums_dev.dev_num = dev_num;
+	ums_board.ums_dev.dev_num = mmc->block_dev.dev;
 	ums_board.ums_dev.offset = offset;
 	ums_board.ums_dev.part_size = part_size;
 
diff --git a/common/cmd_usb_mass_storage.c b/common/cmd_usb_mass_storage.c
index 33a4715..62c1308 100644
--- a/common/cmd_usb_mass_storage.c
+++ b/common/cmd_usb_mass_storage.c
@@ -14,6 +14,7 @@ 
 int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
 			       int argc, char * const argv[])
 {
+	struct mmc *mmc = NULL;
 	char *ep;
 	unsigned int dev_num = 0, offset = 0, part_size = 0;
 	int rc;
@@ -21,29 +22,29 @@  int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
 	struct ums_board_info *ums_info;
 	static char *s = "ums";
 
-	if (argc < 2) {
-		printf("usage: ums <dev> - e.g. ums 0\n");
-		return 0;
-	}
+	if (argc < 2)
+		return CMD_RET_USAGE;
 
 	dev_num = (int)simple_strtoul(argv[1], &ep, 16);
 
-	if (dev_num) {
-		puts("\nSet eMMC device to 0! - e.g. ums 0\n");
-		goto fail;
+	mmc = find_mmc_device(dev_num);
+	if (!mmc) {
+		printf("UMS error: invalid mmc device num: %d.\n", dev_num);
+		return CMD_RET_FAILURE;
 	}
 
 	board_usb_init();
-	ums_info = board_ums_init(dev_num, offset, part_size);
 
+	ums_info = board_ums_init(mmc, offset, part_size);
 	if (!ums_info) {
-		printf("MMC: %d -> NOT available\n", dev_num);
-		goto fail;
+		printf("UMS is not supported on this board.\n");
+		return CMD_RET_FAILURE;
 	}
+
 	rc = fsg_init(ums_info);
 	if (rc) {
 		printf("cmd ums: fsg_init failed\n");
-		goto fail;
+		return CMD_RET_FAILURE;
 	}
 
 	g_dnl_register(s);
@@ -61,13 +62,10 @@  int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,
 	}
 exit:
 	g_dnl_unregister();
-	return 0;
-
-fail:
-	return -1;
+	return CMD_RET_SUCCESS;
 }
 
 U_BOOT_CMD(ums, CONFIG_SYS_MAXARGS, 1, do_usb_mass_storage,
 	"Use the UMS [User Mass Storage]",
-	"ums - User Mass Storage Gadget"
+	"ums <dev> e.g. ums 0"
 );
diff --git a/include/usb_mass_storage.h b/include/usb_mass_storage.h
index 35cdcc3..0f94f31 100644
--- a/include/usb_mass_storage.h
+++ b/include/usb_mass_storage.h
@@ -34,8 +34,8 @@  extern void board_usb_init(void);
 
 extern int fsg_init(struct ums_board_info *);
 extern void fsg_cleanup(void);
-extern struct ums_board_info *board_ums_init(unsigned int,
-					     unsigned int, unsigned int);
+extern struct ums_board_info *board_ums_init(struct mmc *, unsigned int,
+						unsigned int);
 extern int usb_gadget_handle_interrupts(void);
 extern int fsg_main_thread(void *);