ums: support block devices not MMC devices
PATCH 1/8 ums: support block devices not MMC devices
From: Stephen Warren <[email protected]>
The USB Mass Storage function could equally well support a SATA device as support an MMC device. Update struct ums to contain a block device descriptor, not an MMC device descriptor.
--- board/samsung/common/ums.c | 7 ++++--- include/usb_mass_storage.h | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-)
board/samsung/common/ums.c +4 -3
@@ -7,12 +7,13 @@#include <common.h> #include <usb_mass_storage.h> +#include <mmc.h> #include <part.h> static int ums_read_sector (struct ums *ums_dev,ulong start,lbaint_t blkcnt,void *buf) { -block_dev_desc_t *block_dev = &ums_dev->mmc->block_dev; +block_dev_desc_t *block_dev = ums_dev->block_dev;lbaint_t blkstart = start + ums_dev->start_sector;int dev_num = block_dev->dev;
@@ -22,7 +23,7 @@ static int ums_read_sector(struct ums *ums_dev,static int ums_write_sector (struct ums *ums_dev,ulong start,lbaint_t blkcnt,const void *buf) { -block_dev_desc_t *block_dev = &ums_dev->mmc->block_dev; +block_dev_desc_t *block_dev = ums_dev->block_dev;lbaint_t blkstart = start + ums_dev->start_sector;int dev_num = block_dev->dev;
@@ -45,7 +46,7 @@ static struct ums *ums_disk_init(struct mmc *mmc)return NULL ; } - ums_dev.mmc = mmc; + ums_dev.block_dev = &mmc->block_dev;if (ums_end_sector <= mmc_end_sector) { ums_dev.start_sector =UMS_START_SECTOR ;
include/usb_mass_storage.h +2 -2
@@ -9,7 +9,7 @@#define __USB_MASS_STORAGE_H__#define SECTOR_SIZE 0x200 -#include <mmc.h> +#include <part.h> #include <linux/usb/composite.h> #ifndef UMS_START_SECTOR
@@ -31,7 +31,7 @@ struct ums {unsigned int start_sector;unsigned int num_sectors;const char *name; -struct mmc *mmc; +block_dev_desc_t *block_dev; };extern struct ums *ums;
Signature · 1 line
1.8.1.5
PATCH 2/8 ums: remove UMS_{NUM, START}_SECTORS + UMS_START_SECTOR
From: Stephen Warren <[email protected]>
These values aren't set anywhere at present, and hence have no effect. The concept of a single global offset/number of sectors to expose through USB Mass Storage doesn't even make sense in the face of multiple storage devices. Remove these defines to simplify the code.
--- board/samsung/common/ums.c | 14 ++------------ include/usb_mass_storage.h | 8 -------- 2 files changed, 2 insertions(+), 20 deletions(-)
board/samsung/common/ums.c +2 -12
@@ -39,7 +39,6 @@ static struct ums ums_dev = {static struct ums *ums_disk_init (struct mmc *mmc) {uint64_t mmc_end_sector = mmc->capacity /SECTOR_SIZE ; -uint64_t ums_end_sector =UMS_NUM_SECTORS +UMS_START_SECTOR ;if (!mmc_end_sector) {error ("MMC capacity is not valid" );
@@ -47,17 +46,8 @@ static struct ums *ums_disk_init(struct mmc *mmc) } ums_dev.block_dev = &mmc->block_dev; - -if (ums_end_sector <= mmc_end_sector) { - ums_dev.start_sector =UMS_START_SECTOR ; -if (UMS_NUM_SECTORS ) - ums_dev.num_sectors =UMS_NUM_SECTORS ; -else - ums_dev.num_sectors = mmc_end_sector -UMS_START_SECTOR ; - }else { - ums_dev.num_sectors = mmc_end_sector; -puts ("UMS: defined bad disk parameters. Using default.\n" ); - } + ums_dev.start_sector =0 ; + ums_dev.num_sectors = mmc_end_sector;printf ("UMS: disk start sector: %#x, count: %#x\n" , ums_dev.start_sector, ums_dev.num_sectors);
include/usb_mass_storage.h +0 -8
@@ -12,14 +12,6 @@#include <part.h> #include <linux/usb/composite.h> -#ifndef UMS_START_SECTOR -#define UMS_START_SECTOR 0 -#endif - -#ifndef UMS_NUM_SECTORS -#define UMS_NUM_SECTORS 0 -#endif -/* Wait at maximum 60 seconds for cable connection */ #define UMS_CABLE_READY_TIMEOUT 60
Signature · 1 line
1.8.1.5
PATCH 3/8 ums: remove error-checking of MMC device size
In reply to Stephen WarrenFrom: Stephen Warren <[email protected]>
There's no reason to believe that an MMC device will incorrectly report its capacity. Remove error checking of this value from ums_disk_init() to simplify it.
--- board/samsung/common/ums.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
board/samsung/common/ums.c +1 -8
@@ -38,16 +38,9 @@ static struct ums ums_dev = {static struct ums *ums_disk_init (struct mmc *mmc) { -uint64_t mmc_end_sector = mmc->capacity /SECTOR_SIZE ; - -if (!mmc_end_sector) { -error ("MMC capacity is not valid" ); -return NULL ; - } - ums_dev.block_dev = &mmc->block_dev; ums_dev.start_sector =0 ; - ums_dev.num_sectors = mmc_end_sector; + ums_dev.num_sectors = mmc->capacity /SECTOR_SIZE ;printf ("UMS: disk start sector: %#x, count: %#x\n" , ums_dev.start_sector, ums_dev.num_sectors);
Signature · 1 line
1.8.1.5
PATCH 3/8 ums: remove error-checking of MMC device size
Quoted 5 lines · Stephen Warren
From: Stephen Warren <[email protected]>
There's no reason to believe that an MMC device will incorrectly report its capacity. Remove error checking of this value from
ums_disk_init()to simplify it.
Hmm. I guess this check might trigger if you tried to export an SD card slot over UMS, yet there was no SD card present. Still, patch 6/8 in this series resolves this, since at least for MMC devices, get_device() calls mmc_init() internally, and hence returns an error in this case. This is more direct that checking the device size.
PATCH 4/8 ums: remove ums_disk_init()
In reply to Stephen WarrenFrom: Stephen Warren <[email protected]>
Now that ums_disk_init() is so simple, there's no need for it to be a separate function. Instead, just add it to the tail end of ums_init().
--- board/samsung/common/ums.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
board/samsung/common/ums.c +7 -12
@@ -36,8 +36,14 @@ static struct ums ums_dev = { .name = "UMS disk", }; -static struct ums *ums_disk_init (struct mmc *mmc) +struct ums *ums_init (unsigned int dev_num) { +struct mmc *mmc =NULL ; + + mmc =find_mmc_device (dev_num); +if (!mmc) +return NULL ; + ums_dev.block_dev = &mmc->block_dev; ums_dev.start_sector =0 ; ums_dev.num_sectors = mmc->capacity /SECTOR_SIZE ;
@@ -47,14 +53,3 @@ static struct ums *ums_disk_init(struct mmc *mmc)return &ums_dev; } - -struct ums *ums_init (unsigned int dev_num) -{ -struct mmc *mmc =NULL ; - - mmc =find_mmc_device (dev_num); -if (!mmc) -return NULL ; - -return ums_disk_init (mmc); -}
Signature · 1 line
1.8.1.5
PATCH 5/8 ums: move IO support code to common location
In reply to Stephen WarrenFrom: Stephen Warren <[email protected]>
There's nothing Samsung-/board-specfic about the implementation of ums_init(). Move the code into cmd_usb_mass_storage.c, so that it can be shared by any user of that command.
--- board/samsung/common/Makefile | 1 - board/samsung/common/ums.c | 55 ------------------------------------------- common/cmd_usb_mass_storage.c | 46 ++++++++++++++++++++++++++++++++++++ include/usb_mass_storage.h | 1 - 4 files changed, 46 insertions(+), 57 deletions(-) delete mode 100644 board/samsung/common/ums.c
board/samsung/common/Makefile +0 -1
@@ -7,7 +7,6 @@ obj-$(CONFIG_SOFT_I2C_MULTI_BUS) += multi_i2c.o obj-$(CONFIG_THOR_FUNCTION) += thor.o -obj-$(CONFIG_CMD_USB_MASS_STORAGE) += ums.o obj-$(CONFIG_MISC_COMMON) += misc.o ifndef CONFIG_SPL_BUILD
board/samsung/common/ums.c deleted +0 -55
55 lines not on this page · raw message
common/cmd_usb_mass_storage.c +46 -0
55 lines not on this page · raw message
include/usb_mass_storage.h +0 -1
@@ -30,7 +30,6 @@ extern struct ums *ums;int fsg_init (struct ums *);void fsg_cleanup (void ); -struct ums *ums_init (unsigned int );int fsg_main_thread (void *);#ifdef CONFIG_USB_GADGET_MASS_STORAGE
Signature · 1 line
1.8.1.5
PATCH 6/8 ums: use get_device() not find_mmc_device();
In reply to Stephen WarrenFrom: Stephen Warren <[email protected]>
get_device() is a generic routine that will support any type of block device. Use this instead of the type-specific find_mmc_device(), for future flexibility.
--- common/cmd_usb_mass_storage.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
common/cmd_usb_mass_storage.c +14 -11
@@ -9,7 +9,6 @@#include <common.h> #include <command.h> #include <g_dnl.h> -#include <mmc.h> #include <part.h> #include <usb.h> #include <usb_mass_storage.h>
@@ -40,17 +39,22 @@ static struct ums ums_dev = { .name = "UMS disk", }; -struct ums *ums_init (unsigned int dev_num) +struct ums *ums_init (const char *devtype,const char *devnum) { -struct mmc *mmc =NULL ; +block_dev_desc_t *block_dev; +int ret; - mmc =find_mmc_device (dev_num); -if (!mmc) + ret =get_device (devtype, devnum, &block_dev); +if (ret <0 )return NULL ; - ums_dev.block_dev = &mmc->block_dev; +/* f_mass_storage.c assumes SECTOR_SIZE sectors */ +if (block_dev->blksz !=SECTOR_SIZE ) +return NULL ; + + ums_dev.block_dev = block_dev; ums_dev.start_sector =0 ; - ums_dev.num_sectors = mmc->capacity /SECTOR_SIZE ; + ums_dev.num_sectors = block_dev->lba;printf ("UMS: disk start sector: %#x, count: %#x\n" , ums_dev.start_sector, ums_dev.num_sectors);
@@ -65,11 +69,10 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,return CMD_RET_USAGE ;const char *usb_controller = argv[1 ]; -const char *mmc_devstring = argv[2 ]; - -unsigned int dev_num =simple_strtoul (mmc_devstring,NULL ,0 ); +const char *devtype ="mmc" ; +const char *devnum = argv[2 ]; -struct ums *ums =ums_init (dev_num); +struct ums *ums =ums_init (devtype, devnum);if (!ums)return CMD_RET_FAILURE ;
Signature · 1 line
1.8.1.5
PATCH 7/8 ums: move all variable declarations to the start of the block
In reply to Stephen WarrenFrom: Stephen Warren <[email protected]>
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
--- common/cmd_usb_mass_storage.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
common/cmd_usb_mass_storage.c +16 -8
@@ -65,25 +65,33 @@ struct ums *ums_init(const char *devtype, const char *devnum)int do_usb_mass_storage (cmd_tbl_t *cmdtp,int flag,int argc,char *const argv[]) { +const char *usb_controller; +const char *devtype; +const char *devnum; +struct ums *ums; +unsigned int controller_index; +int rc; +int cable_ready_timeout__maybe_unused ; +if (argc <3 )return CMD_RET_USAGE ; -const char *usb_controller = argv[1 ]; -const char *devtype ="mmc" ; -const char *devnum = argv[2 ]; + usb_controller = argv[1 ]; + devtype ="mmc" ; + devnum = argv[2 ]; -struct ums *ums =ums_init (devtype, devnum); + ums =ums_init (devtype, devnum);if (!ums)return CMD_RET_FAILURE ; -unsigned int controller_index = (unsigned int )(simple_strtoul ( - usb_controller,NULL ,0 )); + controller_index = (unsigned int )(simple_strtoul ( + usb_controller,NULL ,0 ));if (board_usb_init (controller_index,USB_INIT_DEVICE )) {error ("Couldn't init USB controller." );return CMD_RET_FAILURE ; } -int rc =fsg_init (ums); + rc =fsg_init (ums);if (rc) {error ("fsg_init failed" );return CMD_RET_FAILURE ;
@@ -93,7 +101,7 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,#ifdef CONFIG_USB_CABLE_CHECK /* Timeout unit: seconds */ -int cable_ready_timeout =UMS_CABLE_READY_TIMEOUT ; + cable_ready_timeout =UMS_CABLE_READY_TIMEOUT ;if (!usb_cable_connected ()) {puts ("Please connect USB cable.\n" );
Signature · 1 line
1.8.1.5
PATCH 7/8 ums: move all variable declarations to the start of the block
Quoted 11 lines · Stephen Warren
From: Stephen Warren <[email protected]>
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
Cc Przemyslaw Marczak <[email protected]>Cc Lukasz Majewski <[email protected]>Signed-off-by Stephen Warren <[email protected]>--- common/cmd_usb_mass_storage.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
This doesn't apply on u-boot-usb/master, sorry.
Best regards,
Marek Vasut
PATCH 7/8 ums: move all variable declarations to the start of the block
Quoted 6 lines · Marek Vasut
Quoted 5 lines · Stephen Warren
From: Stephen Warren <[email protected]>
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
Quoted 8 lines
Quoted 6 lines
Cc Przemyslaw Marczak <[email protected]>Cc Lukasz Majewski <[email protected]>Signed-off-by Stephen Warren <[email protected]>--- common/cmd_usb_mass_storage.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)This doesn't apply on u-boot-usb/master, sorry.
Is this the first patch in the series which doesn't apply? Did you apply any of the earlier patches? I guess I'll go try to rebase the series and find out myself...
This is why we need a "uboot-next" just like linux-next, and stricter controls on which git repos take patches that touch subsystem code, to avoid cross-repo conflicts:-(
PATCH 7/8 ums: move all variable declarations to the start of the block
Hi Stephen,
Quoted 17 lines · Marek Vasut
Quoted 6 lines · Stephen Warren
Quoted 5 lines
From: Stephen Warren <[email protected]>
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
Quoted 8 lines
Quoted 6 lines
Cc Przemyslaw Marczak <[email protected]>Cc Lukasz Majewski <[email protected]>Signed-off-by Stephen Warren <[email protected]>--- common/cmd_usb_mass_storage.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)This doesn't apply on u-boot-usb/master, sorry.
Quoted 7 lines
Is this the first patch in the series which doesn't apply? Did you apply any of the earlier patches? I guess I'll go try to rebase the series and find out myself...
This is why we need a "uboot-next" just like linux-next, and stricter controls on which git repos take patches that touch subsystem code, to avoid cross-repo conflicts:-(
+1
Yesterday Marek and I agreed that some patches regarding DFU and gadgets (patches 1 to 8) done by Mateusz:
http://patchwork.ozlabs.org/patch/343517/
will be go through u-boot-dfu tree, which will be rebased on top of u-boot-usb tree. Afterwards I will send PR to Marek.
Unfortunately I didn't manage to add them to u-boot-denx tree yesterday. I will do it on Monday as well as the review regarding your ums work. I hope, that you can wait until then.
Best regards,
Lukasz Majewski
Quoted 4 lines
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140501/506ce407/attachment.pgp>
PATCH 7/8 ums: move all variable declarations to the start of the block
Quoted 32 lines · Lukasz Majewski
Hi Stephen,
Quoted 17 lines · Marek Vasut
Quoted 6 lines · Stephen Warren
Quoted 5 lines
From: Stephen Warren <[email protected]>
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
Quoted 8 lines
Quoted 6 lines
Cc Przemyslaw Marczak <[email protected]>Cc Lukasz Majewski <[email protected]>Signed-off-by Stephen Warren <[email protected]>--- common/cmd_usb_mass_storage.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)This doesn't apply on u-boot-usb/master, sorry.
Quoted 7 lines
Is this the first patch in the series which doesn't apply? Did you apply any of the earlier patches? I guess I'll go try to rebase the series and find out myself...
This is why we need a "uboot-next" just like linux-next, and stricter controls on which git repos take patches that touch subsystem code, to avoid cross-repo conflicts:-(
+1
Yesterday Marek and I agreed that some patches regarding DFU and gadgets (patches 1 to 8) done by Mateusz:
Quoted 8 lines
http://patchwork.ozlabs.org/patch/343517/
will be go through u-boot-dfu tree, which will be rebased on top of u-boot-usb tree. Afterwards I will send PR to Marek.
Unfortunately I didn't manage to add them to u-boot-denx tree yesterday. I will do it on Monday as well as the review regarding your ums work. I hope, that you can wait until then.
OK, that's fine.
FWIW, I have applied that series locally and rebased my patches on top of it. If you want to see the latest version, feel free to look at:
git://github.com/swarren/u-boot.git tegra_dev
Note that branch gets rebased as I do local development.
PATCH 7/8 ums: move all variable declarations to the start of the block
Hello Stephen,
Quoted 42 lines · Stephen Warren
Quoted 32 lines · Lukasz Majewski
Hi Stephen,
Quoted 17 lines · Marek Vasut
Quoted 6 lines · Stephen Warren
Quoted 5 lines
From: Stephen Warren <[email protected]>
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
Quoted 8 lines
Quoted 6 lines
Cc Przemyslaw Marczak <[email protected]>Cc Lukasz Majewski <[email protected]>Signed-off-by Stephen Warren <[email protected]>--- common/cmd_usb_mass_storage.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)This doesn't apply on u-boot-usb/master, sorry.
Quoted 7 lines
Is this the first patch in the series which doesn't apply? Did you apply any of the earlier patches? I guess I'll go try to rebase the series and find out myself...
This is why we need a "uboot-next" just like linux-next, and stricter controls on which git repos take patches that touch subsystem code, to avoid cross-repo conflicts:-(
+1
Yesterday Marek and I agreed that some patches regarding DFU and gadgets (patches 1 to 8) done by Mateusz:
Quoted 8 lines
http://patchwork.ozlabs.org/patch/343517/
will be go through u-boot-dfu tree, which will be rebased on top of u-boot-usb tree. Afterwards I will send PR to Marek.
Unfortunately I didn't manage to add them to u-boot-denx tree yesterday. I will do it on Monday as well as the review regarding your ums work. I hope, that you can wait until then.
Quoted 9 lines
OK, that's fine.
FWIW, I have applied that series locally and rebased my patches on top of it. If you want to see the latest version, feel free to look at:
git://github.com/swarren/u-boot.gittegra_devNote that branch gets rebased as I do local development.
I tested your tegra_dev branch on trats2 and also on goni(after apply the rest of Mateusz Zalega patches) and it works fine.
So for this patch set:
Thanks,
Signature · 4 lines
Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com
PATCH 7/8 ums: move all variable declarations to the start of the block
On 05/05/2014 03:16 AM, Przemyslaw Marczak wrote:
...
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Stephen Warren
Quoted 2 lines
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
...
Quoted 4 lines
Quoted 4 lines
FWIW, I have applied that series locally and rebased my patches on top of it. If you want to see the latest version, feel free to look at:
git://github.com/swarren/u-boot.gittegra_dev
...
Quoted 5 lines
I tested your
tegra_devbranch on trats2 and also on goni(after apply the rest of Mateusz Zalega patches) and it works fine.So for this patch set:
Good new:-) Thanks for the testing.
PATCH 7/8 ums: move all variable declarations to the start of the block
In reply to Stephen WarrenQuoted 12 lines · Stephen Warren
Quoted 11 lines · Marek Vasut
Quoted 10 lines · Stephen Warren
From: Stephen Warren <[email protected]>
It's easier to assign values to the variables inside an if statement body if the assignment and declaration are separate.
Cc Przemyslaw Marczak <[email protected]>Cc Lukasz Majewski <[email protected]>Signed-off-by Stephen Warren <[email protected]>---
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
common/cmd_usb_mass_storage.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)This doesn't apply on u-boot-usb/master, sorry.
Is this the first patch in the series which doesn't apply? Did you apply any of the earlier patches? I guess I'll go try to rebase the series and find out myself...
7/8 doesn't apply .
Quoted 3 lines
This is why we need a "uboot-next" just like linux-next, and stricter controls on which git repos take patches that touch subsystem code, to avoid cross-repo conflicts:-(
Yep, u-boot-next would be nice and so would be u-boot-stable . Volunteers welcome ;-)
Best regards,
Marek Vasut
PATCH 8/8 ums: allow the user to specify the device type
In reply to Stephen WarrenFrom: Stephen Warren <[email protected]>
Allow an optional devtype parameter to the ums command, which specifies the type of the device to be exported. This could allow exporting a SATA or even another USB device.
--- common/cmd_usb_mass_storage.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
common/cmd_usb_mass_storage.c +10 -4
@@ -77,8 +77,13 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag,return CMD_RET_USAGE ; usb_controller = argv[1 ]; - devtype ="mmc" ; - devnum = argv[2 ]; +if (argc >=4 ) { + devtype = argv[2 ]; + devnum = argv[3 ]; + }else { + devtype ="mmc" ; + devnum = argv[2 ]; + } ums =ums_init (devtype, devnum);if (!ums)
@@ -146,7 +151,8 @@ exit:return CMD_RET_SUCCESS ; } -U_BOOT_CMD (ums,CONFIG_SYS_MAXARGS ,1 , do_usb_mass_storage, +U_BOOT_CMD (ums,4 ,1 , do_usb_mass_storage,"Use the UMS [User Mass Storage]" , -"ums <USB_controller> <mmc_dev> e.g. ums 0 0" +"ums <USB_controller> [<devtype>] <devnum> e.g. ums 0 mmc 0\n" +" devtype defaults to mmc" );
Signature · 1 line
1.8.1.5