[U-Boot] [PATCH v2] usb: fix usb_stor_read/write on DM
Prior to DM, we could not enable different types of USB controllers at the same time. DM was supposed to loosen the limitation. It is true that we can compile drivers, but they do not work.
For example, if EHCI is enabled, xHCI fails as follows:
=> usb read 82000000 0 2000
USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! BUG! ### ERROR ### Please RESET the board ###
The cause of the error seems the following code:
#ifdef CONFIG_USB_EHCI_HCD /* * The U-Boot EHCI driver can handle any transfer length as long as there is * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are * limited to 65535 blocks. */ #define USB_MAX_XFER_BLK 65535 #else #define USB_MAX_XFER_BLK 20 #endif
To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: - Run-time detection of host if
common/usb_storage.c | 30 ++++++++++++++++++++---------- drivers/usb/host/ehci-hcd.c | 1 + drivers/usb/host/ohci-hcd.c | 1 + drivers/usb/host/xhci.c | 1 + include/usb.h | 9 +++++++++ 5 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index df0b05730879..d17b12639cad 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -1109,7 +1109,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, lbaint_t blkcnt, void *buffer) #endif { - lbaint_t start, blks; + lbaint_t start, blks, max_xfer_blk; uintptr_t buf_addr; unsigned short smallblks; struct usb_device *udev; @@ -1118,6 +1118,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, struct scsi_cmd *srb = &usb_ccb; #ifdef CONFIG_BLK struct blk_desc *block_dev; + struct usb_bus_priv *bus_priv; #endif
if (blkcnt == 0) @@ -1127,6 +1128,9 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, 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); + + bus_priv = dev_get_uclass_priv(dev_get_parent(dev_get_parent(dev_get_parent(dev)))); + max_xfer_blk = bus_priv->host_if == USB_HOST_EHCI ? 65535 : 20; #else debug("\nusb_read: udev %d\n", block_dev->devnum); udev = usb_dev_desc[block_dev->devnum].priv; @@ -1134,6 +1138,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, debug("%s: No device\n", __func__); return 0; } + max_xfer_blk = USB_MAX_XFER_BLK; #endif ss = (struct us_data *)udev->privptr;
@@ -1150,12 +1155,12 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, /* XXX need some comment here */ retry = 2; srb->pdata = (unsigned char *)buf_addr; - if (blks > USB_MAX_XFER_BLK) - smallblks = USB_MAX_XFER_BLK; + if (blks > max_xfer_blk) + smallblks = max_xfer_blk; else smallblks = (unsigned short) blks; retry_it: - if (smallblks == USB_MAX_XFER_BLK) + if (smallblks == max_xfer_blk) usb_show_progress(); srb->datalen = block_dev->blksz * smallblks; srb->pdata = (unsigned char *)buf_addr; @@ -1178,7 +1183,7 @@ retry_it: start, smallblks, buf_addr);
usb_disable_asynch(0); /* asynch transfer allowed */ - if (blkcnt >= USB_MAX_XFER_BLK) + if (blkcnt >= max_xfer_blk) debug("\n"); return blkcnt; } @@ -1191,7 +1196,7 @@ 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; + lbaint_t start, blks, max_xfer_blk; uintptr_t buf_addr; unsigned short smallblks; struct usb_device *udev; @@ -1200,6 +1205,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, struct scsi_cmd *srb = &usb_ccb; #ifdef CONFIG_BLK struct blk_desc *block_dev; + struct usb_bus_priv *bus_priv; #endif
if (blkcnt == 0) @@ -1210,6 +1216,9 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, 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); + + bus_priv = dev_get_uclass_priv(dev_get_parent(dev_get_parent(dev_get_parent(dev)))); + max_xfer_blk = bus_priv->host_if == USB_HOST_EHCI ? 65535 : 20; #else debug("\nusb_read: udev %d\n", block_dev->devnum); udev = usb_dev_desc[block_dev->devnum].priv; @@ -1217,6 +1226,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, debug("%s: No device\n", __func__); return 0; } + max_xfer_blk = USB_MAX_XFER_BLK; #endif ss = (struct us_data *)udev->privptr;
@@ -1236,12 +1246,12 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, */ retry = 2; srb->pdata = (unsigned char *)buf_addr; - if (blks > USB_MAX_XFER_BLK) - smallblks = USB_MAX_XFER_BLK; + if (blks > max_xfer_blk) + smallblks = max_xfer_blk; else smallblks = (unsigned short) blks; retry_it: - if (smallblks == USB_MAX_XFER_BLK) + if (smallblks == max_xfer_blk) usb_show_progress(); srb->datalen = block_dev->blksz * smallblks; srb->pdata = (unsigned char *)buf_addr; @@ -1263,7 +1273,7 @@ retry_it: PRIxPTR "\n", start, smallblks, buf_addr);
usb_disable_asynch(0); /* asynch transfer allowed */ - if (blkcnt >= USB_MAX_XFER_BLK) + if (blkcnt >= max_xfer_blk) debug("\n"); return blkcnt;
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index f08709d0212d..b55961954a33 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1604,6 +1604,7 @@ int ehci_register(struct udevice *dev, struct ehci_hccr *hccr, dev->name, ctrl, hccr, hcor, init);
priv->desc_before_addr = true; + priv->host_if = USB_HOST_EHCI;
ehci_setup_ops(ctrl, ops); ctrl->hccr = hccr; diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index fdfc870efa3e..d877f2bb0bbf 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -2193,6 +2193,7 @@ int ohci_register(struct udevice *dev, struct ohci_regs *regs) u32 reg;
priv->desc_before_addr = true; + priv->host_if = USB_HOST_OHCI;
ohci->regs = regs; ohci->hcca = memalign(256, sizeof(struct ohci_hcca)); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index bd8f4c43b4b2..2df78f29cd78 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1207,6 +1207,7 @@ int xhci_register(struct udevice *dev, struct xhci_hccr *hccr, * of that is done for XHCI unlike EHCI. */ priv->desc_before_addr = false; + priv->host_if = USB_HOST_XHCI;
ret = xhci_reset(hcor); if (ret) diff --git a/include/usb.h b/include/usb.h index 62f051fe535c..675761edfc08 100644 --- a/include/usb.h +++ b/include/usb.h @@ -604,6 +604,14 @@ struct usb_dev_platdata { int configno; };
+enum usb_host_if { + USB_HOST_UNKNOWN, + USB_HOST_OHCI, + USB_HOST_UHCI, + USB_HOST_EHCI, + USB_HOST_XHCI, +}; + /** * struct usb_bus_priv - information about the USB controller * @@ -623,6 +631,7 @@ struct usb_bus_priv { int next_addr; bool desc_before_addr; bool companion; + enum usb_host_if host_if; };
/**
On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
Prior to DM, we could not enable different types of USB controllers at the same time. DM was supposed to loosen the limitation. It is true that we can compile drivers, but they do not work.
For example, if EHCI is enabled, xHCI fails as follows:
=> usb read 82000000 0 2000
USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! BUG! ### ERROR ### Please RESET the board ###
The cause of the error seems the following code:
#ifdef CONFIG_USB_EHCI_HCD /*
- The U-Boot EHCI driver can handle any transfer length as long as there is
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- limited to 65535 blocks.
*/ #define USB_MAX_XFER_BLK 65535 #else #define USB_MAX_XFER_BLK 20 #endif
To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
What happens if CONFIG_BLK is not set ?
Why is it 20 for XHCI anyway ?
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
Changes in v2:
- Run-time detection of host if
common/usb_storage.c | 30 ++++++++++++++++++++---------- drivers/usb/host/ehci-hcd.c | 1 + drivers/usb/host/ohci-hcd.c | 1 + drivers/usb/host/xhci.c | 1 + include/usb.h | 9 +++++++++ 5 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index df0b05730879..d17b12639cad 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -1109,7 +1109,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, lbaint_t blkcnt, void *buffer) #endif {
- lbaint_t start, blks;
- lbaint_t start, blks, max_xfer_blk; uintptr_t buf_addr; unsigned short smallblks; struct usb_device *udev;
@@ -1118,6 +1118,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, struct scsi_cmd *srb = &usb_ccb; #ifdef CONFIG_BLK struct blk_desc *block_dev;
- struct usb_bus_priv *bus_priv;
#endif
if (blkcnt == 0) @@ -1127,6 +1128,9 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, 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);
- bus_priv = dev_get_uclass_priv(dev_get_parent(dev_get_parent(dev_get_parent(dev))));
- max_xfer_blk = bus_priv->host_if == USB_HOST_EHCI ? 65535 : 20;
#else debug("\nusb_read: udev %d\n", block_dev->devnum); udev = usb_dev_desc[block_dev->devnum].priv; @@ -1134,6 +1138,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, debug("%s: No device\n", __func__); return 0; }
- max_xfer_blk = USB_MAX_XFER_BLK;
#endif ss = (struct us_data *)udev->privptr;
@@ -1150,12 +1155,12 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, /* XXX need some comment here */ retry = 2; srb->pdata = (unsigned char *)buf_addr;
if (blks > USB_MAX_XFER_BLK)smallblks = USB_MAX_XFER_BLK;
if (blks > max_xfer_blk)smallblks = max_xfer_blk;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
if (smallblks == max_xfer_blk) usb_show_progress();@@ -1178,7 +1183,7 @@ retry_it: start, smallblks, buf_addr);
usb_disable_asynch(0); /* asynch transfer allowed */
- if (blkcnt >= USB_MAX_XFER_BLK)
- if (blkcnt >= max_xfer_blk) debug("\n"); return blkcnt;
} @@ -1191,7 +1196,7 @@ 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;
- lbaint_t start, blks, max_xfer_blk; uintptr_t buf_addr; unsigned short smallblks; struct usb_device *udev;
@@ -1200,6 +1205,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, struct scsi_cmd *srb = &usb_ccb; #ifdef CONFIG_BLK struct blk_desc *block_dev;
- struct usb_bus_priv *bus_priv;
#endif
if (blkcnt == 0) @@ -1210,6 +1216,9 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, 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);
- bus_priv = dev_get_uclass_priv(dev_get_parent(dev_get_parent(dev_get_parent(dev))));
- max_xfer_blk = bus_priv->host_if == USB_HOST_EHCI ? 65535 : 20;
#else debug("\nusb_read: udev %d\n", block_dev->devnum); udev = usb_dev_desc[block_dev->devnum].priv; @@ -1217,6 +1226,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, debug("%s: No device\n", __func__); return 0; }
- max_xfer_blk = USB_MAX_XFER_BLK;
#endif ss = (struct us_data *)udev->privptr;
@@ -1236,12 +1246,12 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, */ retry = 2; srb->pdata = (unsigned char *)buf_addr;
if (blks > USB_MAX_XFER_BLK)smallblks = USB_MAX_XFER_BLK;
if (blks > max_xfer_blk)smallblks = max_xfer_blk;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
if (smallblks == max_xfer_blk) usb_show_progress();@@ -1263,7 +1273,7 @@ retry_it: PRIxPTR "\n", start, smallblks, buf_addr);
usb_disable_asynch(0); /* asynch transfer allowed */
- if (blkcnt >= USB_MAX_XFER_BLK)
- if (blkcnt >= max_xfer_blk) debug("\n"); return blkcnt;
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index f08709d0212d..b55961954a33 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1604,6 +1604,7 @@ int ehci_register(struct udevice *dev, struct ehci_hccr *hccr, dev->name, ctrl, hccr, hcor, init);
priv->desc_before_addr = true;
priv->host_if = USB_HOST_EHCI;
ehci_setup_ops(ctrl, ops); ctrl->hccr = hccr;
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index fdfc870efa3e..d877f2bb0bbf 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -2193,6 +2193,7 @@ int ohci_register(struct udevice *dev, struct ohci_regs *regs) u32 reg;
priv->desc_before_addr = true;
priv->host_if = USB_HOST_OHCI;
ohci->regs = regs; ohci->hcca = memalign(256, sizeof(struct ohci_hcca));
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index bd8f4c43b4b2..2df78f29cd78 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1207,6 +1207,7 @@ int xhci_register(struct udevice *dev, struct xhci_hccr *hccr, * of that is done for XHCI unlike EHCI. */ priv->desc_before_addr = false;
priv->host_if = USB_HOST_XHCI;
ret = xhci_reset(hcor); if (ret)
diff --git a/include/usb.h b/include/usb.h index 62f051fe535c..675761edfc08 100644 --- a/include/usb.h +++ b/include/usb.h @@ -604,6 +604,14 @@ struct usb_dev_platdata { int configno; };
+enum usb_host_if {
- USB_HOST_UNKNOWN,
- USB_HOST_OHCI,
- USB_HOST_UHCI,
- USB_HOST_EHCI,
- USB_HOST_XHCI,
+};
/**
- struct usb_bus_priv - information about the USB controller
@@ -623,6 +631,7 @@ struct usb_bus_priv { int next_addr; bool desc_before_addr; bool companion;
- enum usb_host_if host_if;
};
/**
2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de:
On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
Prior to DM, we could not enable different types of USB controllers at the same time. DM was supposed to loosen the limitation. It is true that we can compile drivers, but they do not work.
For example, if EHCI is enabled, xHCI fails as follows:
=> usb read 82000000 0 2000
USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! BUG! ### ERROR ### Please RESET the board ###
The cause of the error seems the following code:
#ifdef CONFIG_USB_EHCI_HCD /*
- The U-Boot EHCI driver can handle any transfer length as long as there is
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- limited to 65535 blocks.
*/ #define USB_MAX_XFER_BLK 65535 #else #define USB_MAX_XFER_BLK 20 #endif
To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
What happens if CONFIG_BLK is not set ?
USB_MAX_XFER_BLK is chosen.
Why is it 20 for XHCI anyway ?
You are the maintainer. (I hope) you have better knowledge with this.
Looks like the following commit was picked up by you.
commit cffcc503580907d733e694d505e12ff6ec6c679a Author: Benoît Thébaudeau benoit.thebaudeau@advansee.com AuthorDate: Fri Aug 10 18:26:50 2012 +0200 Commit: Marek Vasut marex@denx.de CommitDate: Sat Sep 1 16:21:52 2012 +0200
usb_storage: Restore non-EHCI support
The commit 5dd95cf made the MSC driver EHCI-specific. This patch restores a basic support of non-EHCI HCDs, like before that commit.
The fallback transfer size is certainly not optimal, but at least it should work like before.
Signed-off-by: Benoît Thébaudeau benoit.thebaudeau@advansee.com Cc: Marek Vasut marex@denx.de Cc: Ilya Yanok ilya.yanok@cogentembedded.com Cc: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net
diff --git a/common/usb_storage.c b/common/usb_storage.c index bdc306f587fd..0cd6399a3c42 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -155,11 +155,15 @@ struct us_data { trans_cmnd transport; /* transport routine */ };
+#ifdef CONFIG_USB_EHCI /* * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers * of 4096 bytes in a transfer without running itself out of qt_buffers */ #define USB_MAX_XFER_BLK(start, blksz) (((4096 * 5) - (start % 4096)) / blksz) +#else +#define USB_MAX_XFER_BLK(start, blksz) 20 +#endif
static struct us_data usb_stor[USB_MAX_STOR_DEV];
On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de:
On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
Prior to DM, we could not enable different types of USB controllers at the same time. DM was supposed to loosen the limitation. It is true that we can compile drivers, but they do not work.
For example, if EHCI is enabled, xHCI fails as follows:
=> usb read 82000000 0 2000
USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! BUG! ### ERROR ### Please RESET the board ###
The cause of the error seems the following code:
#ifdef CONFIG_USB_EHCI_HCD /*
- The U-Boot EHCI driver can handle any transfer length as long as there is
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- limited to 65535 blocks.
*/ #define USB_MAX_XFER_BLK 65535 #else #define USB_MAX_XFER_BLK 20 #endif
To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
What happens if CONFIG_BLK is not set ?
USB_MAX_XFER_BLK is chosen.
And can we fix that even for non-CONFIG_BLK ?
Why is it 20 for XHCI anyway ?
You are the maintainer. (I hope) you have better knowledge with this.
Heh, way to deflect the question. I seem to remember some discussion about the DMA (?) limitation on XHCI, but I'd have to dig through the ML archives myself.
Looks like the following commit was picked up by you.
5 years ago, way before DM was what it is today .
commit cffcc503580907d733e694d505e12ff6ec6c679a Author: Benoît Thébaudeau benoit.thebaudeau@advansee.com AuthorDate: Fri Aug 10 18:26:50 2012 +0200 Commit: Marek Vasut marex@denx.de CommitDate: Sat Sep 1 16:21:52 2012 +0200
usb_storage: Restore non-EHCI support The commit 5dd95cf made the MSC driver EHCI-specific. This patch restores a basic support of non-EHCI HCDs, like before that commit. The fallback transfer size is certainly not optimal, but at leastit should work like before.
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> Cc: Marek Vasut <marex@denx.de> Cc: Ilya Yanok <ilya.yanok@cogentembedded.com> Cc: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>diff --git a/common/usb_storage.c b/common/usb_storage.c index bdc306f587fd..0cd6399a3c42 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -155,11 +155,15 @@ struct us_data { trans_cmnd transport; /* transport routine */ };
+#ifdef CONFIG_USB_EHCI /*
- The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
- of 4096 bytes in a transfer without running itself out of qt_buffers
*/ #define USB_MAX_XFER_BLK(start, blksz) (((4096 * 5) - (start % 4096)) / blksz) +#else +#define USB_MAX_XFER_BLK(start, blksz) 20 +#endif
static struct us_data usb_stor[USB_MAX_STOR_DEV];
On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de:
On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
Prior to DM, we could not enable different types of USB controllers at the same time. DM was supposed to loosen the limitation. It is true that we can compile drivers, but they do not work.
For example, if EHCI is enabled, xHCI fails as follows:
=> usb read 82000000 0 2000
USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! BUG! ### ERROR ### Please RESET the board ###
The cause of the error seems the following code:
#ifdef CONFIG_USB_EHCI_HCD /*
- The U-Boot EHCI driver can handle any transfer length as long as there is
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- limited to 65535 blocks.
*/ #define USB_MAX_XFER_BLK 65535 #else #define USB_MAX_XFER_BLK 20 #endif
To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
What happens if CONFIG_BLK is not set ?
USB_MAX_XFER_BLK is chosen.
And can we fix that even for non-CONFIG_BLK ?
Why is it 20 for XHCI anyway ?
You are the maintainer. (I hope) you have better knowledge with this.
Heh, way to deflect the question. I seem to remember some discussion about the DMA (?) limitation on XHCI, but I'd have to dig through the ML archives myself.
Looks like the following commit was picked up by you.
5 years ago, way before DM was what it is today .
And even way before the introduction of XHCI into U-Boot, which means that this 20 was targeting OHCI or proprietary HCDs, not XHCI. USB_MAX_READ_BLK was already set to 20 in the initial revision of usb_storage.c. As I said in the commit message, this 20 was certainly not optimal for these non-EHCI HCDs, but it restored the previous (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 KiB code, which was specific to ehci-hcd.c at that time. Without knowing the rationale for the legacy 20 blocks, the safest approach for non-EHCI HCDs was to use this value in order to avoid breaking a platform or something. Looking at ohci-hcd.c, it limits the transfer size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the maximum number of transfers would depend on the MSC block size. dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to have any limit caused by these drivers. The limit with the current XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK could be set to 65535 for all HCDs but OHCI and XHCI, which require specific rules depending on the MSC block size.
commit cffcc503580907d733e694d505e12ff6ec6c679a Author: Benoît Thébaudeau benoit.thebaudeau@advansee.com AuthorDate: Fri Aug 10 18:26:50 2012 +0200 Commit: Marek Vasut marex@denx.de CommitDate: Sat Sep 1 16:21:52 2012 +0200
usb_storage: Restore non-EHCI support The commit 5dd95cf made the MSC driver EHCI-specific. This patch restores a basic support of non-EHCI HCDs, like before that commit. The fallback transfer size is certainly not optimal, but at leastit should work like before.
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> Cc: Marek Vasut <marex@denx.de> Cc: Ilya Yanok <ilya.yanok@cogentembedded.com> Cc: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>diff --git a/common/usb_storage.c b/common/usb_storage.c index bdc306f587fd..0cd6399a3c42 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -155,11 +155,15 @@ struct us_data { trans_cmnd transport; /* transport routine */ };
+#ifdef CONFIG_USB_EHCI /*
- The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
- of 4096 bytes in a transfer without running itself out of qt_buffers
*/ #define USB_MAX_XFER_BLK(start, blksz) (((4096 * 5) - (start % 4096)) / blksz) +#else +#define USB_MAX_XFER_BLK(start, blksz) 20 +#endif
static struct us_data usb_stor[USB_MAX_STOR_DEV];
Best regards, Benoît
On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de:
On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
Prior to DM, we could not enable different types of USB controllers at the same time. DM was supposed to loosen the limitation. It is true that we can compile drivers, but they do not work.
For example, if EHCI is enabled, xHCI fails as follows:
=> usb read 82000000 0 2000
USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! BUG! ### ERROR ### Please RESET the board ###
The cause of the error seems the following code:
#ifdef CONFIG_USB_EHCI_HCD /*
- The U-Boot EHCI driver can handle any transfer length as long as there is
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- limited to 65535 blocks.
*/ #define USB_MAX_XFER_BLK 65535 #else #define USB_MAX_XFER_BLK 20 #endif
To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
What happens if CONFIG_BLK is not set ?
USB_MAX_XFER_BLK is chosen.
And can we fix that even for non-CONFIG_BLK ?
Why is it 20 for XHCI anyway ?
You are the maintainer. (I hope) you have better knowledge with this.
Heh, way to deflect the question. I seem to remember some discussion about the DMA (?) limitation on XHCI, but I'd have to dig through the ML archives myself.
Looks like the following commit was picked up by you.
5 years ago, way before DM was what it is today .
And even way before the introduction of XHCI into U-Boot, which means that this 20 was targeting OHCI or proprietary HCDs, not XHCI. USB_MAX_READ_BLK was already set to 20 in the initial revision of usb_storage.c. As I said in the commit message, this 20 was certainly not optimal for these non-EHCI HCDs, but it restored the previous (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 KiB code, which was specific to ehci-hcd.c at that time. Without knowing the rationale for the legacy 20 blocks, the safest approach for non-EHCI HCDs was to use this value in order to avoid breaking a platform or something. Looking at ohci-hcd.c, it limits the transfer size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the maximum number of transfers would depend on the MSC block size. dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to have any limit caused by these drivers. The limit with the current XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK could be set to 65535 for all HCDs but OHCI and XHCI, which require specific rules depending on the MSC block size.
For whatever reason, something tells me that setting the block size to 64k for XHCI broke things, but I cannot locate the thread. But there's something in the back of my head ...
[...]
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de:
On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
Prior to DM, we could not enable different types of USB controllers at the same time. DM was supposed to loosen the limitation. It is true that we can compile drivers, but they do not work.
For example, if EHCI is enabled, xHCI fails as follows:
=> usb read 82000000 0 2000
USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! BUG! ### ERROR ### Please RESET the board ###
The cause of the error seems the following code:
#ifdef CONFIG_USB_EHCI_HCD /*
- The U-Boot EHCI driver can handle any transfer length as long as there is
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- limited to 65535 blocks.
*/ #define USB_MAX_XFER_BLK 65535 #else #define USB_MAX_XFER_BLK 20 #endif
To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
What happens if CONFIG_BLK is not set ?
USB_MAX_XFER_BLK is chosen.
And can we fix that even for non-CONFIG_BLK ?
Why is it 20 for XHCI anyway ?
You are the maintainer. (I hope) you have better knowledge with this.
Heh, way to deflect the question. I seem to remember some discussion about the DMA (?) limitation on XHCI, but I'd have to dig through the ML archives myself.
Looks like the following commit was picked up by you.
5 years ago, way before DM was what it is today .
And even way before the introduction of XHCI into U-Boot, which means that this 20 was targeting OHCI or proprietary HCDs, not XHCI. USB_MAX_READ_BLK was already set to 20 in the initial revision of usb_storage.c. As I said in the commit message, this 20 was certainly not optimal for these non-EHCI HCDs, but it restored the previous (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 KiB code, which was specific to ehci-hcd.c at that time. Without knowing the rationale for the legacy 20 blocks, the safest approach for non-EHCI HCDs was to use this value in order to avoid breaking a platform or something. Looking at ohci-hcd.c, it limits the transfer size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the maximum number of transfers would depend on the MSC block size. dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to have any limit caused by these drivers. The limit with the current XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK could be set to 65535 for all HCDs but OHCI and XHCI, which require specific rules depending on the MSC block size.
For whatever reason, something tells me that setting the block size to 64k for XHCI broke things, but I cannot locate the thread. But there's something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
Best regards, Benoît
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de:
On 07/14/2017 04:31 AM, Masahiro Yamada wrote: > Prior to DM, we could not enable different types of USB controllers > at the same time. DM was supposed to loosen the limitation. It is > true that we can compile drivers, but they do not work. > > For example, if EHCI is enabled, xHCI fails as follows: > > => usb read 82000000 0 2000 > > USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. > Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) > BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! > BUG! > ### ERROR ### Please RESET the board ### > > The cause of the error seems the following code: > > #ifdef CONFIG_USB_EHCI_HCD > /* > * The U-Boot EHCI driver can handle any transfer length as long as there is > * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are > * limited to 65535 blocks. > */ > #define USB_MAX_XFER_BLK 65535 > #else > #define USB_MAX_XFER_BLK 20 > #endif > > To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
What happens if CONFIG_BLK is not set ?
USB_MAX_XFER_BLK is chosen.
And can we fix that even for non-CONFIG_BLK ?
Why is it 20 for XHCI anyway ?
You are the maintainer. (I hope) you have better knowledge with this.
Heh, way to deflect the question. I seem to remember some discussion about the DMA (?) limitation on XHCI, but I'd have to dig through the ML archives myself.
Looks like the following commit was picked up by you.
5 years ago, way before DM was what it is today .
And even way before the introduction of XHCI into U-Boot, which means that this 20 was targeting OHCI or proprietary HCDs, not XHCI. USB_MAX_READ_BLK was already set to 20 in the initial revision of usb_storage.c. As I said in the commit message, this 20 was certainly not optimal for these non-EHCI HCDs, but it restored the previous (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 KiB code, which was specific to ehci-hcd.c at that time. Without knowing the rationale for the legacy 20 blocks, the safest approach for non-EHCI HCDs was to use this value in order to avoid breaking a platform or something. Looking at ohci-hcd.c, it limits the transfer size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the maximum number of transfers would depend on the MSC block size. dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to have any limit caused by these drivers. The limit with the current XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK could be set to 65535 for all HCDs but OHCI and XHCI, which require specific rules depending on the MSC block size.
For whatever reason, something tells me that setting the block size to 64k for XHCI broke things, but I cannot locate the thread. But there's something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: > On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >> Prior to DM, we could not enable different types of USB controllers >> at the same time. DM was supposed to loosen the limitation. It is >> true that we can compile drivers, but they do not work. >> >> For example, if EHCI is enabled, xHCI fails as follows: >> >> => usb read 82000000 0 2000 >> >> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >> BUG! >> ### ERROR ### Please RESET the board ### >> >> The cause of the error seems the following code: >> >> #ifdef CONFIG_USB_EHCI_HCD >> /* >> * The U-Boot EHCI driver can handle any transfer length as long as there is >> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >> * limited to 65535 blocks. >> */ >> #define USB_MAX_XFER_BLK 65535 >> #else >> #define USB_MAX_XFER_BLK 20 >> #endif >> >> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. > > What happens if CONFIG_BLK is not set ?
USB_MAX_XFER_BLK is chosen.
And can we fix that even for non-CONFIG_BLK ?
> Why is it 20 for XHCI anyway ?
You are the maintainer. (I hope) you have better knowledge with this.
Heh, way to deflect the question. I seem to remember some discussion about the DMA (?) limitation on XHCI, but I'd have to dig through the ML archives myself.
Looks like the following commit was picked up by you.
5 years ago, way before DM was what it is today .
And even way before the introduction of XHCI into U-Boot, which means that this 20 was targeting OHCI or proprietary HCDs, not XHCI. USB_MAX_READ_BLK was already set to 20 in the initial revision of usb_storage.c. As I said in the commit message, this 20 was certainly not optimal for these non-EHCI HCDs, but it restored the previous (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 KiB code, which was specific to ehci-hcd.c at that time. Without knowing the rationale for the legacy 20 blocks, the safest approach for non-EHCI HCDs was to use this value in order to avoid breaking a platform or something. Looking at ohci-hcd.c, it limits the transfer size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the maximum number of transfers would depend on the MSC block size. dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to have any limit caused by these drivers. The limit with the current XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK could be set to 65535 for all HCDs but OHCI and XHCI, which require specific rules depending on the MSC block size.
For whatever reason, something tells me that setting the block size to 64k for XHCI broke things, but I cannot locate the thread. But there's something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 01:03 PM, Masahiro Yamada wrote: > 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>> Prior to DM, we could not enable different types of USB controllers >>> at the same time. DM was supposed to loosen the limitation. It is >>> true that we can compile drivers, but they do not work. >>> >>> For example, if EHCI is enabled, xHCI fails as follows: >>> >>> => usb read 82000000 0 2000 >>> >>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>> BUG! >>> ### ERROR ### Please RESET the board ### >>> >>> The cause of the error seems the following code: >>> >>> #ifdef CONFIG_USB_EHCI_HCD >>> /* >>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>> * limited to 65535 blocks. >>> */ >>> #define USB_MAX_XFER_BLK 65535 >>> #else >>> #define USB_MAX_XFER_BLK 20 >>> #endif >>> >>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >> >> What happens if CONFIG_BLK is not set ? > > USB_MAX_XFER_BLK is chosen.
And can we fix that even for non-CONFIG_BLK ?
>> Why is it 20 for XHCI anyway ? > > You are the maintainer. > (I hope) you have better knowledge with this.
Heh, way to deflect the question. I seem to remember some discussion about the DMA (?) limitation on XHCI, but I'd have to dig through the ML archives myself.
> Looks like the following commit was picked up by you.
5 years ago, way before DM was what it is today .
And even way before the introduction of XHCI into U-Boot, which means that this 20 was targeting OHCI or proprietary HCDs, not XHCI. USB_MAX_READ_BLK was already set to 20 in the initial revision of usb_storage.c. As I said in the commit message, this 20 was certainly not optimal for these non-EHCI HCDs, but it restored the previous (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 KiB code, which was specific to ehci-hcd.c at that time. Without knowing the rationale for the legacy 20 blocks, the safest approach for non-EHCI HCDs was to use this value in order to avoid breaking a platform or something. Looking at ohci-hcd.c, it limits the transfer size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the maximum number of transfers would depend on the MSC block size. dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to have any limit caused by these drivers. The limit with the current XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK could be set to 65535 for all HCDs but OHCI and XHCI, which require specific rules depending on the MSC block size.
For whatever reason, something tells me that setting the block size to 64k for XHCI broke things, but I cannot locate the thread. But there's something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: > On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>>> Prior to DM, we could not enable different types of USB controllers >>>> at the same time. DM was supposed to loosen the limitation. It is >>>> true that we can compile drivers, but they do not work. >>>> >>>> For example, if EHCI is enabled, xHCI fails as follows: >>>> >>>> => usb read 82000000 0 2000 >>>> >>>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>>> BUG! >>>> ### ERROR ### Please RESET the board ### >>>> >>>> The cause of the error seems the following code: >>>> >>>> #ifdef CONFIG_USB_EHCI_HCD >>>> /* >>>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>>> * limited to 65535 blocks. >>>> */ >>>> #define USB_MAX_XFER_BLK 65535 >>>> #else >>>> #define USB_MAX_XFER_BLK 20 >>>> #endif >>>> >>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >>> >>> What happens if CONFIG_BLK is not set ? >> >> USB_MAX_XFER_BLK is chosen. > > And can we fix that even for non-CONFIG_BLK ? > >>> Why is it 20 for XHCI anyway ? >> >> You are the maintainer. >> (I hope) you have better knowledge with this. > > Heh, way to deflect the question. I seem to remember some discussion > about the DMA (?) limitation on XHCI, but I'd have to dig through the ML > archives myself. > >> Looks like the following commit was picked up by you. > > 5 years ago, way before DM was what it is today .
And even way before the introduction of XHCI into U-Boot, which means that this 20 was targeting OHCI or proprietary HCDs, not XHCI. USB_MAX_READ_BLK was already set to 20 in the initial revision of usb_storage.c. As I said in the commit message, this 20 was certainly not optimal for these non-EHCI HCDs, but it restored the previous (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 KiB code, which was specific to ehci-hcd.c at that time. Without knowing the rationale for the legacy 20 blocks, the safest approach for non-EHCI HCDs was to use this value in order to avoid breaking a platform or something. Looking at ohci-hcd.c, it limits the transfer size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the maximum number of transfers would depend on the MSC block size. dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to have any limit caused by these drivers. The limit with the current XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK could be set to 65535 for all HCDs but OHCI and XHCI, which require specific rules depending on the MSC block size.
For whatever reason, something tells me that setting the block size to 64k for XHCI broke things, but I cannot locate the thread. But there's something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote:
On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote: > On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: >> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >>> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>>>> Prior to DM, we could not enable different types of USB controllers >>>>> at the same time. DM was supposed to loosen the limitation. It is >>>>> true that we can compile drivers, but they do not work. >>>>> >>>>> For example, if EHCI is enabled, xHCI fails as follows: >>>>> >>>>> => usb read 82000000 0 2000 >>>>> >>>>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>>>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>>>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>>>> BUG! >>>>> ### ERROR ### Please RESET the board ### >>>>> >>>>> The cause of the error seems the following code: >>>>> >>>>> #ifdef CONFIG_USB_EHCI_HCD >>>>> /* >>>>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>>>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>>>> * limited to 65535 blocks. >>>>> */ >>>>> #define USB_MAX_XFER_BLK 65535 >>>>> #else >>>>> #define USB_MAX_XFER_BLK 20 >>>>> #endif >>>>> >>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >>>> >>>> What happens if CONFIG_BLK is not set ? >>> >>> USB_MAX_XFER_BLK is chosen. >> >> And can we fix that even for non-CONFIG_BLK ? >> >>>> Why is it 20 for XHCI anyway ? >>> >>> You are the maintainer. >>> (I hope) you have better knowledge with this. >> >> Heh, way to deflect the question. I seem to remember some discussion >> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML >> archives myself. >> >>> Looks like the following commit was picked up by you. >> >> 5 years ago, way before DM was what it is today . > > And even way before the introduction of XHCI into U-Boot, which means > that this 20 was targeting OHCI or proprietary HCDs, not XHCI. > USB_MAX_READ_BLK was already set to 20 in the initial revision of > usb_storage.c. As I said in the commit message, this 20 was certainly > not optimal for these non-EHCI HCDs, but it restored the previous > (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 > KiB code, which was specific to ehci-hcd.c at that time. Without > knowing the rationale for the legacy 20 blocks, the safest approach > for non-EHCI HCDs was to use this value in order to avoid breaking a > platform or something. Looking at ohci-hcd.c, it limits the transfer > size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the > maximum number of transfers would depend on the MSC block size. > dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to > have any limit caused by these drivers. The limit with the current > XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK > could be set to 65535 for all HCDs but OHCI and XHCI, which require > specific rules depending on the MSC block size.
For whatever reason, something tells me that setting the block size to 64k for XHCI broke things, but I cannot locate the thread. But there's something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
Hmmmmmm, sigh. When is the migration happening, how far is it ?
+Simon
2017-07-20 18:00 GMT+09:00 Marek Vasut marex@denx.de:
On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote: > On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote: >> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: >>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >>>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>>>>> Prior to DM, we could not enable different types of USB controllers >>>>>> at the same time. DM was supposed to loosen the limitation. It is >>>>>> true that we can compile drivers, but they do not work. >>>>>> >>>>>> For example, if EHCI is enabled, xHCI fails as follows: >>>>>> >>>>>> => usb read 82000000 0 2000 >>>>>> >>>>>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>>>>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>>>>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>>>>> BUG! >>>>>> ### ERROR ### Please RESET the board ### >>>>>> >>>>>> The cause of the error seems the following code: >>>>>> >>>>>> #ifdef CONFIG_USB_EHCI_HCD >>>>>> /* >>>>>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>>>>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>>>>> * limited to 65535 blocks. >>>>>> */ >>>>>> #define USB_MAX_XFER_BLK 65535 >>>>>> #else >>>>>> #define USB_MAX_XFER_BLK 20 >>>>>> #endif >>>>>> >>>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >>>>> >>>>> What happens if CONFIG_BLK is not set ? >>>> >>>> USB_MAX_XFER_BLK is chosen. >>> >>> And can we fix that even for non-CONFIG_BLK ? >>> >>>>> Why is it 20 for XHCI anyway ? >>>> >>>> You are the maintainer. >>>> (I hope) you have better knowledge with this. >>> >>> Heh, way to deflect the question. I seem to remember some discussion >>> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML >>> archives myself. >>> >>>> Looks like the following commit was picked up by you. >>> >>> 5 years ago, way before DM was what it is today . >> >> And even way before the introduction of XHCI into U-Boot, which means >> that this 20 was targeting OHCI or proprietary HCDs, not XHCI. >> USB_MAX_READ_BLK was already set to 20 in the initial revision of >> usb_storage.c. As I said in the commit message, this 20 was certainly >> not optimal for these non-EHCI HCDs, but it restored the previous >> (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 >> KiB code, which was specific to ehci-hcd.c at that time. Without >> knowing the rationale for the legacy 20 blocks, the safest approach >> for non-EHCI HCDs was to use this value in order to avoid breaking a >> platform or something. Looking at ohci-hcd.c, it limits the transfer >> size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the >> maximum number of transfers would depend on the MSC block size. >> dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to >> have any limit caused by these drivers. The limit with the current >> XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK >> could be set to 65535 for all HCDs but OHCI and XHCI, which require >> specific rules depending on the MSC block size. > > For whatever reason, something tells me that setting the block size to > 64k for XHCI broke things, but I cannot locate the thread. But there's > something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
Hmmmmmm, sigh. When is the migration happening, how far is it ?
Maybe, we can ask Simon for his plan.
I think there are currently three cases:
[1] CONFIG_DM_USB && CONFIG_BLK
[2] CONFIG_DM_USB && !CONFIG_BLK
[3] !CONFIG_DM_USB
IIUC, only [1] is future-proof. Meanwhile, the code is really dirty. I hope it will not last very long.
+Simon,
On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut marex@denx.de wrote:
On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote: > On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote: >> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: >>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >>>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>>>>> Prior to DM, we could not enable different types of USB controllers >>>>>> at the same time. DM was supposed to loosen the limitation. It is >>>>>> true that we can compile drivers, but they do not work. >>>>>> >>>>>> For example, if EHCI is enabled, xHCI fails as follows: >>>>>> >>>>>> => usb read 82000000 0 2000 >>>>>> >>>>>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>>>>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>>>>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>>>>> BUG! >>>>>> ### ERROR ### Please RESET the board ### >>>>>> >>>>>> The cause of the error seems the following code: >>>>>> >>>>>> #ifdef CONFIG_USB_EHCI_HCD >>>>>> /* >>>>>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>>>>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>>>>> * limited to 65535 blocks. >>>>>> */ >>>>>> #define USB_MAX_XFER_BLK 65535 >>>>>> #else >>>>>> #define USB_MAX_XFER_BLK 20 >>>>>> #endif >>>>>> >>>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >>>>> >>>>> What happens if CONFIG_BLK is not set ? >>>> >>>> USB_MAX_XFER_BLK is chosen. >>> >>> And can we fix that even for non-CONFIG_BLK ? >>> >>>>> Why is it 20 for XHCI anyway ? >>>> >>>> You are the maintainer. >>>> (I hope) you have better knowledge with this. >>> >>> Heh, way to deflect the question. I seem to remember some discussion >>> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML >>> archives myself. >>> >>>> Looks like the following commit was picked up by you. >>> >>> 5 years ago, way before DM was what it is today . >> >> And even way before the introduction of XHCI into U-Boot, which means >> that this 20 was targeting OHCI or proprietary HCDs, not XHCI. >> USB_MAX_READ_BLK was already set to 20 in the initial revision of >> usb_storage.c. As I said in the commit message, this 20 was certainly >> not optimal for these non-EHCI HCDs, but it restored the previous >> (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 >> KiB code, which was specific to ehci-hcd.c at that time. Without >> knowing the rationale for the legacy 20 blocks, the safest approach >> for non-EHCI HCDs was to use this value in order to avoid breaking a >> platform or something. Looking at ohci-hcd.c, it limits the transfer >> size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the >> maximum number of transfers would depend on the MSC block size. >> dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to >> have any limit caused by these drivers. The limit with the current >> XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK >> could be set to 65535 for all HCDs but OHCI and XHCI, which require >> specific rules depending on the MSC block size. > > For whatever reason, something tells me that setting the block size to > 64k for XHCI broke things, but I cannot locate the thread. But there's > something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
Hmmmmmm, sigh. When is the migration happening, how far is it ?
One idea is to force all board to switch to driver model at a preset timeline. After the deadline, boards do not switch to DM will get dropped by the mainline. I noticed that not all boards are actively maintained...
Regards, Bin
On 07/20/2017 11:38 AM, Bin Meng wrote:
+Simon,
On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut marex@denx.de wrote:
On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote: > On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote: >> On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote: >>> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: >>>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >>>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >>>>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>>>>>> Prior to DM, we could not enable different types of USB controllers >>>>>>> at the same time. DM was supposed to loosen the limitation. It is >>>>>>> true that we can compile drivers, but they do not work. >>>>>>> >>>>>>> For example, if EHCI is enabled, xHCI fails as follows: >>>>>>> >>>>>>> => usb read 82000000 0 2000 >>>>>>> >>>>>>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>>>>>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>>>>>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>>>>>> BUG! >>>>>>> ### ERROR ### Please RESET the board ### >>>>>>> >>>>>>> The cause of the error seems the following code: >>>>>>> >>>>>>> #ifdef CONFIG_USB_EHCI_HCD >>>>>>> /* >>>>>>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>>>>>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>>>>>> * limited to 65535 blocks. >>>>>>> */ >>>>>>> #define USB_MAX_XFER_BLK 65535 >>>>>>> #else >>>>>>> #define USB_MAX_XFER_BLK 20 >>>>>>> #endif >>>>>>> >>>>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >>>>>> >>>>>> What happens if CONFIG_BLK is not set ? >>>>> >>>>> USB_MAX_XFER_BLK is chosen. >>>> >>>> And can we fix that even for non-CONFIG_BLK ? >>>> >>>>>> Why is it 20 for XHCI anyway ? >>>>> >>>>> You are the maintainer. >>>>> (I hope) you have better knowledge with this. >>>> >>>> Heh, way to deflect the question. I seem to remember some discussion >>>> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML >>>> archives myself. >>>> >>>>> Looks like the following commit was picked up by you. >>>> >>>> 5 years ago, way before DM was what it is today . >>> >>> And even way before the introduction of XHCI into U-Boot, which means >>> that this 20 was targeting OHCI or proprietary HCDs, not XHCI. >>> USB_MAX_READ_BLK was already set to 20 in the initial revision of >>> usb_storage.c. As I said in the commit message, this 20 was certainly >>> not optimal for these non-EHCI HCDs, but it restored the previous >>> (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 >>> KiB code, which was specific to ehci-hcd.c at that time. Without >>> knowing the rationale for the legacy 20 blocks, the safest approach >>> for non-EHCI HCDs was to use this value in order to avoid breaking a >>> platform or something. Looking at ohci-hcd.c, it limits the transfer >>> size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the >>> maximum number of transfers would depend on the MSC block size. >>> dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to >>> have any limit caused by these drivers. The limit with the current >>> XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK >>> could be set to 65535 for all HCDs but OHCI and XHCI, which require >>> specific rules depending on the MSC block size. >> >> For whatever reason, something tells me that setting the block size to >> 64k for XHCI broke things, but I cannot locate the thread. But there's >> something in the back of my head ... > > Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set > to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / > block, USB_MAX_XFER_BLK can be set to at most 1 segment * > (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 > bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit > is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. > The buffer alignment may also have to be taken into account to adjust > these values, which would require a USB_MAX_XFER_BLK(host_if, start, > blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 > regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, > isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
Hmmmmmm, sigh. When is the migration happening, how far is it ?
One idea is to force all board to switch to driver model at a preset timeline. After the deadline, boards do not switch to DM will get dropped by the mainline. I noticed that not all boards are actively maintained...
Be my guest, there's a few which I'd like to see removed myself :-)
+Tom for question below
Hi,
On 20 July 2017 at 03:40, Marek Vasut marex@denx.de wrote:
On 07/20/2017 11:38 AM, Bin Meng wrote:
+Simon,
On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut marex@denx.de wrote:
On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de: > On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote: >> On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote: >>> On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote: >>>> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: >>>>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >>>>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >>>>>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>>>>>>> Prior to DM, we could not enable different types of USB controllers >>>>>>>> at the same time. DM was supposed to loosen the limitation. It is >>>>>>>> true that we can compile drivers, but they do not work. >>>>>>>> >>>>>>>> For example, if EHCI is enabled, xHCI fails as follows: >>>>>>>> >>>>>>>> => usb read 82000000 0 2000 >>>>>>>> >>>>>>>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>>>>>>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>>>>>>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>>>>>>> BUG! >>>>>>>> ### ERROR ### Please RESET the board ### >>>>>>>> >>>>>>>> The cause of the error seems the following code: >>>>>>>> >>>>>>>> #ifdef CONFIG_USB_EHCI_HCD >>>>>>>> /* >>>>>>>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>>>>>>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>>>>>>> * limited to 65535 blocks. >>>>>>>> */ >>>>>>>> #define USB_MAX_XFER_BLK 65535 >>>>>>>> #else >>>>>>>> #define USB_MAX_XFER_BLK 20 >>>>>>>> #endif >>>>>>>> >>>>>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >>>>>>> >>>>>>> What happens if CONFIG_BLK is not set ? >>>>>> >>>>>> USB_MAX_XFER_BLK is chosen. >>>>> >>>>> And can we fix that even for non-CONFIG_BLK ? >>>>> >>>>>>> Why is it 20 for XHCI anyway ? >>>>>> >>>>>> You are the maintainer. >>>>>> (I hope) you have better knowledge with this. >>>>> >>>>> Heh, way to deflect the question. I seem to remember some discussion >>>>> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML >>>>> archives myself. >>>>> >>>>>> Looks like the following commit was picked up by you. >>>>> >>>>> 5 years ago, way before DM was what it is today . >>>> >>>> And even way before the introduction of XHCI into U-Boot, which means >>>> that this 20 was targeting OHCI or proprietary HCDs, not XHCI. >>>> USB_MAX_READ_BLK was already set to 20 in the initial revision of >>>> usb_storage.c. As I said in the commit message, this 20 was certainly >>>> not optimal for these non-EHCI HCDs, but it restored the previous >>>> (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 >>>> KiB code, which was specific to ehci-hcd.c at that time. Without >>>> knowing the rationale for the legacy 20 blocks, the safest approach >>>> for non-EHCI HCDs was to use this value in order to avoid breaking a >>>> platform or something. Looking at ohci-hcd.c, it limits the transfer >>>> size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the >>>> maximum number of transfers would depend on the MSC block size. >>>> dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to >>>> have any limit caused by these drivers. The limit with the current >>>> XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK >>>> could be set to 65535 for all HCDs but OHCI and XHCI, which require >>>> specific rules depending on the MSC block size. >>> >>> For whatever reason, something tells me that setting the block size to >>> 64k for XHCI broke things, but I cannot locate the thread. But there's >>> something in the back of my head ... >> >> Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set >> to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / >> block, USB_MAX_XFER_BLK can be set to at most 1 segment * >> (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 >> bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit >> is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. >> The buffer alignment may also have to be taken into account to adjust >> these values, which would require a USB_MAX_XFER_BLK(host_if, start, >> blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 >> regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, >> isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c). > > That's probably what I was looking for, thanks. >
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
Hmmmmmm, sigh. When is the migration happening, how far is it ?
One idea is to force all board to switch to driver model at a preset timeline. After the deadline, boards do not switch to DM will get dropped by the mainline. I noticed that not all boards are actively maintained...
Be my guest, there's a few which I'd like to see removed myself :-)
That makes sense although I'm not sure what the deadline should be. CONFIG_BLK is invasive and it is a pain to carry the #ifdefs.
Maybe end of year, or is that too short?
-- Best regards, Marek Vasut
On Fri, Jul 21, 2017 at 04:48:58AM -0600, Simon Glass wrote:
+Tom for question below
Hi,
On 20 July 2017 at 03:40, Marek Vasut marex@denx.de wrote:
On 07/20/2017 11:38 AM, Bin Meng wrote:
+Simon,
On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut marex@denx.de wrote:
On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote: > 2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de: >> On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote: >>> On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote: >>>> On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote: >>>>> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: >>>>>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >>>>>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@denx.de: >>>>>>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote: >>>>>>>>> Prior to DM, we could not enable different types of USB controllers >>>>>>>>> at the same time. DM was supposed to loosen the limitation. It is >>>>>>>>> true that we can compile drivers, but they do not work. >>>>>>>>> >>>>>>>>> For example, if EHCI is enabled, xHCI fails as follows: >>>>>>>>> >>>>>>>>> => usb read 82000000 0 2000 >>>>>>>>> >>>>>>>>> USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway. >>>>>>>>> Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401) >>>>>>>>> BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()! >>>>>>>>> BUG! >>>>>>>>> ### ERROR ### Please RESET the board ### >>>>>>>>> >>>>>>>>> The cause of the error seems the following code: >>>>>>>>> >>>>>>>>> #ifdef CONFIG_USB_EHCI_HCD >>>>>>>>> /* >>>>>>>>> * The U-Boot EHCI driver can handle any transfer length as long as there is >>>>>>>>> * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are >>>>>>>>> * limited to 65535 blocks. >>>>>>>>> */ >>>>>>>>> #define USB_MAX_XFER_BLK 65535 >>>>>>>>> #else >>>>>>>>> #define USB_MAX_XFER_BLK 20 >>>>>>>>> #endif >>>>>>>>> >>>>>>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK. >>>>>>>> >>>>>>>> What happens if CONFIG_BLK is not set ? >>>>>>> >>>>>>> USB_MAX_XFER_BLK is chosen. >>>>>> >>>>>> And can we fix that even for non-CONFIG_BLK ? >>>>>> >>>>>>>> Why is it 20 for XHCI anyway ? >>>>>>> >>>>>>> You are the maintainer. >>>>>>> (I hope) you have better knowledge with this. >>>>>> >>>>>> Heh, way to deflect the question. I seem to remember some discussion >>>>>> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML >>>>>> archives myself. >>>>>> >>>>>>> Looks like the following commit was picked up by you. >>>>>> >>>>>> 5 years ago, way before DM was what it is today . >>>>> >>>>> And even way before the introduction of XHCI into U-Boot, which means >>>>> that this 20 was targeting OHCI or proprietary HCDs, not XHCI. >>>>> USB_MAX_READ_BLK was already set to 20 in the initial revision of >>>>> usb_storage.c. As I said in the commit message, this 20 was certainly >>>>> not optimal for these non-EHCI HCDs, but it restored the previous >>>>> (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4 >>>>> KiB code, which was specific to ehci-hcd.c at that time. Without >>>>> knowing the rationale for the legacy 20 blocks, the safest approach >>>>> for non-EHCI HCDs was to use this value in order to avoid breaking a >>>>> platform or something. Looking at ohci-hcd.c, it limits the transfer >>>>> size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the >>>>> maximum number of transfers would depend on the MSC block size. >>>>> dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to >>>>> have any limit caused by these drivers. The limit with the current >>>>> XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK >>>>> could be set to 65535 for all HCDs but OHCI and XHCI, which require >>>>> specific rules depending on the MSC block size. >>>> >>>> For whatever reason, something tells me that setting the block size to >>>> 64k for XHCI broke things, but I cannot locate the thread. But there's >>>> something in the back of my head ... >>> >>> Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set >>> to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / >>> block, USB_MAX_XFER_BLK can be set to at most 1 segment * >>> (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 >>> bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit >>> is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. >>> The buffer alignment may also have to be taken into account to adjust >>> these values, which would require a USB_MAX_XFER_BLK(host_if, start, >>> blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 >>> regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, >>> isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c). >> >> That's probably what I was looking for, thanks. >> > > > So, how shall we handle this? > > If somebody can fix this in a correct way, > I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
Hmmmmmm, sigh. When is the migration happening, how far is it ?
One idea is to force all board to switch to driver model at a preset timeline. After the deadline, boards do not switch to DM will get dropped by the mainline. I noticed that not all boards are actively maintained...
Be my guest, there's a few which I'd like to see removed myself :-)
That makes sense although I'm not sure what the deadline should be. CONFIG_BLK is invasive and it is a pain to carry the #ifdefs.
Maybe end of year, or is that too short?
9 months, rounded up to next release?
participants (6)
-
Benoît Thébaudeau -
Bin Meng -
Marek Vasut -
Masahiro Yamada -
Simon Glass -
Tom Rini