[v2] usb: fix usb_stor_read/write on DM

Message ID	1499999469-9861-1-git-send-email-yamada.masahiro@socionext.com
State	New
Headers	show Delivered-To: patch@linaro.org Received-SPF: pass (google.com: best guess record for domain of u-boot-bounces@lists.denx.de designates 81.169.180.215 as permitted sender) client-ip=81.169.180.215; From: Masahiro Yamada <yamada.masahiro@socionext.com> To: u-boot@lists.denx.de Date: Fri, 14 Jul 2017 11:31:09 +0900 Message-Id: <1499999469-9861-1-git-send-email-yamada.masahiro@socionext.com> Cc: Marek Vasut <marex@denx.de>, Hans de Goede <hdegoede@redhat.com>, Vagrant Cascadian <vagrant@debian.org>, Andre Przywara <andre.przywara@arm.com>, Michal Simek <michal.simek@xilinx.com> Subject: [U-Boot] [PATCH v2] usb: fix usb_stor_read/write on DM Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" <u-boot-bounces@lists.denx.de>

Masahiro Yamada July 14, 2017, 2:31 a.m. UTC

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(-)

Marek Vasut July 14, 2017, 10:07 a.m. UTC | #1

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;
>  		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;
>  };
>  
>  /**
>

Masahiro Yamada July 14, 2017, 11:03 a.m. UTC | #2

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];

Marek Vasut July 14, 2017, 11:50 a.m. UTC | #3

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 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];
> 
> 
> 
> 
> 
> 
>

Benoît Thébaudeau July 14, 2017, 9:46 p.m. UTC | #4

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 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];

Best regards,
Benoît

Marek Vasut July 14, 2017, 10:06 p.m. UTC | #5

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 ...

[...]

Benoît Thébaudeau July 14, 2017, 11:30 p.m. UTC | #6

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

Marek Vasut July 15, 2017, 12:57 p.m. UTC | #7

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.

Masahiro Yamada July 19, 2017, 3:38 p.m. UTC | #8

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.

Marek Vasut July 19, 2017, 5:33 p.m. UTC | #9

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 ?

Masahiro Yamada July 20, 2017, 7:49 a.m. UTC | #10

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?

Marek Vasut July 20, 2017, 9 a.m. UTC | #11

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 ?

Masahiro Yamada July 20, 2017, 9:37 a.m. UTC | #12

+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.

Bin Meng July 20, 2017, 9:38 a.m. UTC | #13

+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

Marek Vasut July 20, 2017, 9:40 a.m. UTC | #14

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 :-)

Simon Glass July 21, 2017, 10:48 a.m. UTC | #15

+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

Tom Rini July 21, 2017, 3:59 p.m. UTC | #16

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?

-- 
Tom

[v2] usb: fix usb_stor_read/write on DM

Commit Message

Comments

Patch