diff mbox

[U-Boot,v2,25/32] dm: usb: Convert USB storage to use driver-model for block devs

Message ID 1456784765-10788-26-git-send-email-sjg@chromium.org
State Accepted
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass Feb. 29, 2016, 10:25 p.m. UTC 
Update this code to support CONFIG_BLK. Each USB storage device can have
one or more block devices as children, each one representing a LUN
(logical unit) of the USB device.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2: None

 common/usb_storage.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 135 insertions(+), 6 deletions(-)

Comments

Marek Vasut Feb. 29, 2016, 11:04 p.m. UTC | #1 
On 02/29/2016 11:25 PM, Simon Glass wrote:
> Update this code to support CONFIG_BLK. Each USB storage device can have
> one or more block devices as children, each one representing a LUN
> (logical unit) of the USB device.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2: None
> 
>  common/usb_storage.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 135 insertions(+), 6 deletions(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index 0475123..1472824 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -43,6 +43,7 @@
>  #include <asm/byteorder.h>
>  #include <asm/processor.h>
>  #include <dm/device-internal.h>
> +#include <dm/lists.h>
>  
>  #include <part.h>
>  #include <usb.h>
> @@ -67,7 +68,9 @@ static __u32 CBWTag;
>  
>  static int usb_max_devs; /* number of highest available usb device */
>  
> +#ifndef CONFIG_BLK
>  static struct blk_desc usb_dev_desc[USB_MAX_STOR_DEV];
> +#endif

You might want to use __maybe_unused to avoid the ifdef, do you think it
makes sense ?

>  struct us_data;
>  typedef int (*trans_cmnd)(ccb *cb, struct us_data *data);

Other than that:

Reviewed-by: Marek Vasut <marex@denx.de>
Simon Glass March 13, 2016, 1:53 a.m. UTC | #2 
Hi Marek,

On 29 February 2016 at 16:04, Marek Vasut <marex@denx.de> wrote:
> On 02/29/2016 11:25 PM, Simon Glass wrote:
>> Update this code to support CONFIG_BLK. Each USB storage device can have
>> one or more block devices as children, each one representing a LUN
>> (logical unit) of the USB device.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2: None
>>
>>  common/usb_storage.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 135 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index 0475123..1472824 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -43,6 +43,7 @@
>>  #include <asm/byteorder.h>
>>  #include <asm/processor.h>
>>  #include <dm/device-internal.h>
>> +#include <dm/lists.h>
>>
>>  #include <part.h>
>>  #include <usb.h>
>> @@ -67,7 +68,9 @@ static __u32 CBWTag;
>>
>>  static int usb_max_devs; /* number of highest available usb device */
>>
>> +#ifndef CONFIG_BLK
>>  static struct blk_desc usb_dev_desc[USB_MAX_STOR_DEV];
>> +#endif
>
> You might want to use __maybe_unused to avoid the ifdef, do you think it
> makes sense ?

This is something that should not exist when driver model is used. So
I'd rather have it explicit so it is obvious that it can be removed
with the driver-model conversion is done.

>
>>  struct us_data;
>>  typedef int (*trans_cmnd)(ccb *cb, struct us_data *data);
>
> Other than that:
>
> Reviewed-by: Marek Vasut <marex@denx.de>
>
> --
> Best regards,
> Marek Vasut

Regards,
Simon
Simon Glass March 13, 2016, 1:53 a.m. UTC | #3 
On 29 February 2016 at 15:25, Simon Glass <sjg@chromium.org> wrote:
> Update this code to support CONFIG_BLK. Each USB storage device can have
> one or more block devices as children, each one representing a LUN
> (logical unit) of the USB device.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2: None
>
>  common/usb_storage.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 135 insertions(+), 6 deletions(-)

Applied to u-boot-dm/next.
Marek Vasut March 13, 2016, 5:41 p.m. UTC | #4 
On 03/13/2016 02:53 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 29 February 2016 at 16:04, Marek Vasut <marex@denx.de> wrote:
>> On 02/29/2016 11:25 PM, Simon Glass wrote:
>>> Update this code to support CONFIG_BLK. Each USB storage device can have
>>> one or more block devices as children, each one representing a LUN
>>> (logical unit) of the USB device.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v2: None
>>>
>>>  common/usb_storage.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 135 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>>> index 0475123..1472824 100644
>>> --- a/common/usb_storage.c
>>> +++ b/common/usb_storage.c
>>> @@ -43,6 +43,7 @@
>>>  #include <asm/byteorder.h>
>>>  #include <asm/processor.h>
>>>  #include <dm/device-internal.h>
>>> +#include <dm/lists.h>
>>>
>>>  #include <part.h>
>>>  #include <usb.h>
>>> @@ -67,7 +68,9 @@ static __u32 CBWTag;
>>>
>>>  static int usb_max_devs; /* number of highest available usb device */
>>>
>>> +#ifndef CONFIG_BLK
>>>  static struct blk_desc usb_dev_desc[USB_MAX_STOR_DEV];
>>> +#endif
>>
>> You might want to use __maybe_unused to avoid the ifdef, do you think it
>> makes sense ?
> 
> This is something that should not exist when driver model is used. So
> I'd rather have it explicit so it is obvious that it can be removed
> with the driver-model conversion is done.

OK fine.

>>
>>>  struct us_data;
>>>  typedef int (*trans_cmnd)(ccb *cb, struct us_data *data);
>>
>> Other than that:
>>
>> Reviewed-by: Marek Vasut <marex@denx.de>
>>
>> --
>> Best regards,
>> Marek Vasut
> 
> Regards,
> Simon
>
Stephen Warren March 14, 2016, 6:01 p.m. UTC | #5 
On 02/29/2016 03:25 PM, Simon Glass wrote:
> Update this code to support CONFIG_BLK. Each USB storage device can have
> one or more block devices as children, each one representing a LUN
> (logical unit) of the USB device.

Note that I accidentally tested some other patches in the wrong place 
today, and found that this patch prevents my USB SD card reader from 
working when attached to Jetson TK1. I get:

U-Boot 2016.03-rc3-00056-gb3601815cf72 (Mar 14 2016 - 11:54:53 -0600)

TEGRA124
Model: NVIDIA Jetson TK1
Board: NVIDIA Jetson TK1
DRAM:  2 GiB
MMC:   Tegra SD/MMC: 0, Tegra SD/MMC: 1
In:    serial
Out:   serial
Err:   serial
Net:   No ethernet found.
Hit any key to stop autoboot:  0
Tegra124 (Jetson TK1) # usb start
starting USB...
USB0:   USB EHCI 1.10
USB1:   USB EHCI 1.10
scanning bus 0 for devices... 1 USB Device(s) found
scanning bus 1 for devices... Device NOT ready
    Request Sense returned 02 3A 00
### ERROR ### Please RESET the board ###

(The Request Sense error happens even in the passing cases, but in those 
cases the overall "usb start" operation succeeds, and then USB storage 
access does actually work. I guess my reader is a little slow to 
initialize or something).

The patch immediately before this one works fine.

Can you reproduce this? If not, let me know and I'll investigate further.
diff mbox

Patch

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 0475123..1472824 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -43,6 +43,7 @@ 
 #include <asm/byteorder.h>
 #include <asm/processor.h>
 #include <dm/device-internal.h>
+#include <dm/lists.h>
 
 #include <part.h>
 #include <usb.h>
@@ -67,7 +68,9 @@  static __u32 CBWTag;
 
 static int usb_max_devs; /* number of highest available usb device */
 
+#ifndef CONFIG_BLK
 static struct blk_desc usb_dev_desc[USB_MAX_STOR_DEV];
+#endif
 
 struct us_data;
 typedef int (*trans_cmnd)(ccb *cb, struct us_data *data);
@@ -108,7 +111,9 @@  struct us_data {
 #define USB_MAX_XFER_BLK	20
 #endif
 
+#ifndef CONFIG_BLK
 static struct us_data usb_stor[USB_MAX_STOR_DEV];
+#endif
 
 #define USB_STOR_TRANSPORT_GOOD	   0
 #define USB_STOR_TRANSPORT_FAILED -1
@@ -118,16 +123,33 @@  int usb_stor_get_info(struct usb_device *dev, struct us_data *us,
 		      struct blk_desc *dev_desc);
 int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 		      struct us_data *ss);
+#ifdef CONFIG_BLK
+static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
+				   lbaint_t blkcnt, void *buffer);
+static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr,
+				    lbaint_t blkcnt, const void *buffer);
+#else
 static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 				   lbaint_t blkcnt, void *buffer);
 static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 				    lbaint_t blkcnt, const void *buffer);
+#endif
 void uhci_show_temp_int_td(void);
 
 #ifdef CONFIG_PARTITIONS
 struct blk_desc *usb_stor_get_dev(int index)
 {
+#ifdef CONFIG_BLK
+	struct udevice *dev;
+	int ret;
+
+	ret = blk_get_device(IF_TYPE_USB, index, &dev);
+	if (ret)
+		return NULL;
+	return dev_get_uclass_platdata(dev);
+#else
 	return (index < usb_max_devs) ? &usb_dev_desc[index] : NULL;
+#endif
 }
 #endif
 
@@ -143,6 +165,19 @@  static void usb_show_progress(void)
 int usb_stor_info(void)
 {
 	int count = 0;
+#ifdef CONFIG_BLK
+	struct udevice *dev;
+
+	for (blk_first_device(IF_TYPE_USB, &dev);
+	     dev;
+	     blk_next_device(&dev)) {
+		struct blk_desc *desc = dev_get_uclass_platdata(dev);
+
+		printf("  Device %d: ", desc->devnum);
+		dev_print(desc);
+		count++;
+	}
+#else
 	int i;
 
 	if (usb_max_devs > 0) {
@@ -152,7 +187,7 @@  int usb_stor_info(void)
 		}
 		return 0;
 	}
-
+#endif
 	if (!count) {
 		printf("No storage devices, perhaps not 'usb start'ed..?\n");
 		return 1;
@@ -179,11 +214,63 @@  static unsigned int usb_get_max_lun(struct us_data *us)
 static int usb_stor_probe_device(struct usb_device *udev)
 {
 	int lun, max_lun;
+
+#ifdef CONFIG_BLK
+	struct us_data *data;
+	char dev_name[30], *str;
+	int ret;
+#else
 	int start;
 
 	if (udev == NULL)
 		return -ENOENT; /* no more devices available */
+#endif
+
+	debug("\n\nProbing for storage\n");
+#ifdef CONFIG_BLK
+	/*
+	 * We store the us_data in the mass storage device's platdata. It
+	 * is shared by all LUNs (block devices) attached to this mass storage
+	 * device.
+	 */
+	data = dev_get_platdata(udev->dev);
+	if (!usb_storage_probe(udev, 0, data))
+		return 0;
+	max_lun = usb_get_max_lun(data);
+	for (lun = 0; lun <= max_lun; lun++) {
+		struct blk_desc *blkdev;
+		struct udevice *dev;
+
+		snprintf(dev_name, sizeof(dev_name), "%s.lun%d",
+			 udev->dev->name, lun);
+		str = strdup(dev_name);
+		if (!str)
+			return -ENOMEM;
+		ret = blk_create_device(udev->dev, "usb_storage_blk", str,
+				IF_TYPE_USB, usb_max_devs, 512, 0, &dev);
+		if (ret) {
+			debug("Cannot bind driver\n");
+			return ret;
+		}
+
+		blkdev = dev_get_uclass_platdata(dev);
+		blkdev->target = 0xff;
+		blkdev->lun = lun;
 
+		ret = usb_stor_get_info(udev, data, blkdev);
+		if (ret == 1)
+			ret = blk_prepare_device(dev);
+		if (!ret) {
+			usb_max_devs++;
+			debug("%s: Found device %p\n", __func__, udev);
+		} else {
+			debug("usb_stor_get_info: Invalid device\n");
+			ret = device_unbind(dev);
+			if (ret)
+				return ret;
+		}
+	}
+#else
 	/* We don't have space to even probe if we hit the maximum */
 	if (usb_max_devs == USB_MAX_STOR_DEV) {
 		printf("max USB Storage Device reached: %d stopping\n",
@@ -191,7 +278,6 @@  static int usb_stor_probe_device(struct usb_device *udev)
 		return -ENOSPC;
 	}
 
-	debug("\n\nProbing for storage\n");
 	if (!usb_storage_probe(udev, 0, &usb_stor[usb_max_devs]))
 		return 0;
 
@@ -220,10 +306,14 @@  static int usb_stor_probe_device(struct usb_device *udev)
 
 		if (usb_stor_get_info(udev, &usb_stor[start],
 				      &usb_dev_desc[usb_max_devs]) == 1) {
+			debug("partype: %d\n", blkdev->part_type);
+			part_init(blkdev);
+			debug("partype: %d\n", blkdev->part_type);
 			usb_max_devs++;
 			debug("%s: Found device %p\n", __func__, udev);
 		}
 	}
+#endif
 
 	return 0;
 }
@@ -1034,8 +1124,13 @@  static void usb_bin_fixup(struct usb_device_descriptor descriptor,
 }
 #endif /* CONFIG_USB_BIN_FIXUP */
 
+#ifdef CONFIG_BLK
+static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
+				   lbaint_t blkcnt, void *buffer)
+#else
 static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 				   lbaint_t blkcnt, void *buffer)
+#endif
 {
 	lbaint_t start, blks;
 	uintptr_t buf_addr;
@@ -1044,16 +1139,25 @@  static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 	struct us_data *ss;
 	int retry;
 	ccb *srb = &usb_ccb;
+#ifdef CONFIG_BLK
+	struct blk_desc *block_dev;
+#endif
 
 	if (blkcnt == 0)
 		return 0;
 	/* Setup  device */
+#ifdef CONFIG_BLK
+	block_dev = dev_get_uclass_platdata(dev);
+	udev = dev_get_parent_priv(dev_get_parent(dev));
+	debug("\nusb_read: udev %d\n", block_dev->devnum);
+#else
 	debug("\nusb_read: udev %d\n", block_dev->devnum);
 	udev = usb_dev_desc[block_dev->devnum].priv;
 	if (!udev) {
 		debug("%s: No device\n", __func__);
 		return 0;
 	}
+#endif
 	ss = (struct us_data *)udev->privptr;
 
 	usb_disable_asynch(1); /* asynch transfer not allowed */
@@ -1102,8 +1206,13 @@  retry_it:
 	return blkcnt;
 }
 
+#ifdef CONFIG_BLK
+static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr,
+				    lbaint_t blkcnt, const void *buffer)
+#else
 static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 				    lbaint_t blkcnt, const void *buffer)
+#endif
 {
 	lbaint_t start, blks;
 	uintptr_t buf_addr;
@@ -1112,17 +1221,26 @@  static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 	struct us_data *ss;
 	int retry;
 	ccb *srb = &usb_ccb;
+#ifdef CONFIG_BLK
+	struct blk_desc *block_dev;
+#endif
 
 	if (blkcnt == 0)
 		return 0;
 
 	/* Setup  device */
+#ifdef CONFIG_BLK
+	block_dev = dev_get_uclass_platdata(dev);
+	udev = dev_get_parent_priv(dev_get_parent(dev));
+	debug("\nusb_read: udev %d\n", block_dev->devnum);
+#else
 	debug("\nusb_read: udev %d\n", block_dev->devnum);
 	udev = usb_dev_desc[block_dev->devnum].priv;
 	if (!udev) {
 		debug("%s: No device\n", __func__);
 		return 0;
 	}
+#endif
 	ss = (struct us_data *)udev->privptr;
 
 	usb_disable_asynch(1); /* asynch transfer not allowed */
@@ -1377,11 +1495,7 @@  int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	dev_desc->log2blksz = LOG2(dev_desc->blksz);
 	dev_desc->type = perq;
 	debug(" address %d\n", dev_desc->target);
-	debug("partype: %d\n", dev_desc->part_type);
-
-	part_init(dev_desc);
 
-	debug("partype: %d\n", dev_desc->part_type);
 	return 1;
 }
 
@@ -1409,6 +1523,9 @@  U_BOOT_DRIVER(usb_mass_storage) = {
 	.id	= UCLASS_MASS_STORAGE,
 	.of_match = usb_mass_storage_ids,
 	.probe = usb_mass_storage_probe,
+#ifdef CONFIG_BLK
+	.platdata_auto_alloc_size	= sizeof(struct us_data),
+#endif
 };
 
 UCLASS_DRIVER(usb_mass_storage) = {
@@ -1425,5 +1542,17 @@  static const struct usb_device_id mass_storage_id_table[] = {
 };
 
 U_BOOT_USB_DEVICE(usb_mass_storage, mass_storage_id_table);
+#endif
 
+#ifdef CONFIG_BLK
+static const struct blk_ops usb_storage_ops = {
+	.read	= usb_stor_read,
+	.write	= usb_stor_write,
+};
+
+U_BOOT_DRIVER(usb_storage_blk) = {
+	.name		= "usb_storage_blk",
+	.id		= UCLASS_BLK,
+	.ops		= &usb_storage_ops,
+};
 #endif