| Message ID | 20200406122944.105432-1-marek.vasut+renesas@gmail.com |
|---|---|
| State | Accepted |
| Commit | 31232de07ef2bd97ff67625976eecd97eeb1bd3d |
| Headers | show |
| Series | usb: Keep async schedule running only across mass storage xfers | expand |
On Mon, Apr 06, 2020 at 02:29:44PM +0200, Marek Vasut wrote: > Rather than keeping the asynchronous schedule running always, keep it > running only across USB mass storage transfers for now, as it seems > that keeping it running all the time interferes with certain control > transfers during device enumeration. > > Note that running the async schedule all the time should not be an > issue, especially on EHCI HCD, as that one implements most of the > transfers using async schedule. > > Note that we have usb_disable_asynch(), which however is utterly broken. > The usb_disable_asynch() blocks the USB core from doing async transfers > by setting a global flag. The async schedule should however be disabled > per USB controller. Moreover, setting a global flag does not prevent the > controller from using the async schedule, which e.g. the EHCI HCD does. > > This patch implements additional callback to the controller, which > permits it to lock the async schedule and keep it running across > multiple transfers. Once the schedule is unlocked, it must also be > disabled. This thus prevents the async schedule from running outside > of the USB mass storage transfers. > > Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com> > Cc: Lukasz Majewski <lukma at denx.de> > Cc: Tom Rini <trini at konsulko.com> Tested-by: Tom Rini <trini at konsulko.com> [omap3_beagle, previously failing]
On Mon, Apr 06, 2020 at 02:29:44PM +0200, Marek Vasut wrote: > Rather than keeping the asynchronous schedule running always, keep it > running only across USB mass storage transfers for now, as it seems > that keeping it running all the time interferes with certain control > transfers during device enumeration. > > Note that running the async schedule all the time should not be an > issue, especially on EHCI HCD, as that one implements most of the > transfers using async schedule. > > Note that we have usb_disable_asynch(), which however is utterly broken. > The usb_disable_asynch() blocks the USB core from doing async transfers > by setting a global flag. The async schedule should however be disabled > per USB controller. Moreover, setting a global flag does not prevent the > controller from using the async schedule, which e.g. the EHCI HCD does. > > This patch implements additional callback to the controller, which > permits it to lock the async schedule and keep it running across > multiple transfers. Once the schedule is unlocked, it must also be > disabled. This thus prevents the async schedule from running outside > of the USB mass storage transfers. > > Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com> > Cc: Lukasz Majewski <lukma at denx.de> > Cc: Tom Rini <trini at konsulko.com> > Tested-by: Tom Rini <trini at konsulko.com> [omap3_beagle, previously failing] Applied to u-boot/master, thanks!
On Thu, 9 Apr 2020 19:23:26 -0400 Tom Rini <trini at konsulko.com> wrote: > On Mon, Apr 06, 2020 at 02:29:44PM +0200, Marek Vasut wrote: > > > Rather than keeping the asynchronous schedule running always, keep > > it running only across USB mass storage transfers for now, as it > > seems that keeping it running all the time interferes with certain > > control transfers during device enumeration. > > > > Note that running the async schedule all the time should not be an > > issue, especially on EHCI HCD, as that one implements most of the > > transfers using async schedule. > > > > Note that we have usb_disable_asynch(), which however is utterly > > broken. The usb_disable_asynch() blocks the USB core from doing > > async transfers by setting a global flag. The async schedule should > > however be disabled per USB controller. Moreover, setting a global > > flag does not prevent the controller from using the async schedule, > > which e.g. the EHCI HCD does. > > > > This patch implements additional callback to the controller, which > > permits it to lock the async schedule and keep it running across > > multiple transfers. Once the schedule is unlocked, it must also be > > disabled. This thus prevents the async schedule from running outside > > of the USB mass storage transfers. > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com> > > Cc: Lukasz Majewski <lukma at denx.de> > > Cc: Tom Rini <trini at konsulko.com> > > Tested-by: Tom Rini <trini at konsulko.com> [omap3_beagle, previously > > failing] > > Applied to u-boot/master, thanks! > It is a really short time between posting the patch (Monday) and being applied (midnight on Friday). It is the Pre-Easter period with some other world-wide issues... and not all of us have enough time now to review and test patches. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200410/7d8e1d42/attachment.sig>
On Fri, Apr 10, 2020 at 08:21:20AM +0200, Lukasz Majewski wrote: > On Thu, 9 Apr 2020 19:23:26 -0400 > Tom Rini <trini at konsulko.com> wrote: > > > On Mon, Apr 06, 2020 at 02:29:44PM +0200, Marek Vasut wrote: > > > > > Rather than keeping the asynchronous schedule running always, keep > > > it running only across USB mass storage transfers for now, as it > > > seems that keeping it running all the time interferes with certain > > > control transfers during device enumeration. > > > > > > Note that running the async schedule all the time should not be an > > > issue, especially on EHCI HCD, as that one implements most of the > > > transfers using async schedule. > > > > > > Note that we have usb_disable_asynch(), which however is utterly > > > broken. The usb_disable_asynch() blocks the USB core from doing > > > async transfers by setting a global flag. The async schedule should > > > however be disabled per USB controller. Moreover, setting a global > > > flag does not prevent the controller from using the async schedule, > > > which e.g. the EHCI HCD does. > > > > > > This patch implements additional callback to the controller, which > > > permits it to lock the async schedule and keep it running across > > > multiple transfers. Once the schedule is unlocked, it must also be > > > disabled. This thus prevents the async schedule from running outside > > > of the USB mass storage transfers. > > > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com> > > > Cc: Lukasz Majewski <lukma at denx.de> > > > Cc: Tom Rini <trini at konsulko.com> > > > Tested-by: Tom Rini <trini at konsulko.com> [omap3_beagle, previously > > > failing] > > > > Applied to u-boot/master, thanks! > > > > It is a really short time between posting the patch (Monday) and being > applied (midnight on Friday). > > It is the Pre-Easter period with some other world-wide issues... and not > all of us have enough time now to review and test patches. So, I took this patch at this point in the cycle as it fixes a regression that we've had reported for a while and because the custodian asked me to grab it directly. All things considered, it is unlikely to make the situation worse and may fix some of the other regressions we've had in the area. Thanks!
diff --git a/common/usb.c b/common/usb.c index 349e838f1d..534cb1c0ef 100644 --- a/common/usb.c +++ b/common/usb.c @@ -172,6 +172,11 @@ int usb_detect_change(void) return change; } +/* Lock or unlock async schedule on the controller */ +__weak int usb_lock_async(struct usb_device *dev, int lock) +{ +} + /* * disables the asynch behaviour of the control message. This is used for data * transfers that uses the exclusiv access to the control and bulk messages. diff --git a/common/usb_storage.c b/common/usb_storage.c index 097b6729c1..b291ac55d1 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -1157,6 +1157,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, ss = (struct us_data *)udev->privptr; usb_disable_asynch(1); /* asynch transfer not allowed */ + usb_lock_async(udev, 1); srb->lun = block_dev->lun; buf_addr = (uintptr_t)buffer; start = blknr; @@ -1195,6 +1196,7 @@ retry_it: debug("usb_read: end startblk " LBAF ", blccnt %x buffer %lx\n", start, smallblks, buf_addr); + usb_lock_async(udev, 0); usb_disable_asynch(0); /* asynch transfer allowed */ if (blkcnt >= ss->max_xfer_blk) debug("\n"); @@ -1239,6 +1241,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, ss = (struct us_data *)udev->privptr; usb_disable_asynch(1); /* asynch transfer not allowed */ + usb_lock_async(udev, 1); srb->lun = block_dev->lun; buf_addr = (uintptr_t)buffer; @@ -1280,6 +1283,7 @@ retry_it: debug("usb_write: end startblk " LBAF ", blccnt %x buffer %lx\n", start, smallblks, buf_addr); + usb_lock_async(udev, 0); usb_disable_asynch(0); /* asynch transfer allowed */ if (blkcnt >= ss->max_xfer_blk) debug("\n"); diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 1cc02052f5..1edb344d0f 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -298,6 +298,51 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev, QH_ENDPT2_HUBADDR(hubaddr)); } +static int ehci_enable_async(struct ehci_ctrl *ctrl) +{ + u32 cmd; + int ret; + + /* Enable async. schedule. */ + cmd = ehci_readl(&ctrl->hcor->or_usbcmd); + if (cmd & CMD_ASE) + return 0; + + cmd |= CMD_ASE; + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); + + ret = handshake((uint32_t *)&ctrl->hcor->or_usbsts, STS_ASS, STS_ASS, + 100 * 1000); + if (ret < 0) + printf("EHCI fail timeout STS_ASS set\n"); + + return ret; +} + +static int ehci_disable_async(struct ehci_ctrl *ctrl) +{ + u32 cmd; + int ret; + + if (ctrl->async_locked) + return 0; + + /* Disable async schedule. */ + cmd = ehci_readl(&ctrl->hcor->or_usbcmd); + if (!(cmd & CMD_ASE)) + return 0; + + cmd &= ~CMD_ASE; + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); + + ret = handshake((uint32_t *)&ctrl->hcor->or_usbsts, STS_ASS, 0, + 100 * 1000); + if (ret < 0) + printf("EHCI fail timeout STS_ASS reset\n"); + + return ret; +} + static int ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int length, struct devrequest *req) @@ -311,7 +356,6 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, uint32_t *tdp; uint32_t endpt, maxpacket, token, usbsts, qhtoken; uint32_t c, toggle; - uint32_t cmd; int timeout; int ret = 0; struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); @@ -556,19 +600,9 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, usbsts = ehci_readl(&ctrl->hcor->or_usbsts); ehci_writel(&ctrl->hcor->or_usbsts, (usbsts & 0x3f)); - /* Enable async. schedule. */ - cmd = ehci_readl(&ctrl->hcor->or_usbcmd); - if (!(cmd & CMD_ASE)) { - cmd |= CMD_ASE; - ehci_writel(&ctrl->hcor->or_usbcmd, cmd); - - ret = handshake((uint32_t *)&ctrl->hcor->or_usbsts, STS_ASS, STS_ASS, - 100 * 1000); - if (ret < 0) { - printf("EHCI fail timeout STS_ASS set\n"); - goto fail; - } - } + ret = ehci_enable_async(ctrl); + if (ret) + goto fail; /* Wait for TDs to be processed. */ ts = get_timer(0); @@ -611,6 +645,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, if (QT_TOKEN_GET_STATUS(token) & QT_TOKEN_STATUS_ACTIVE) printf("EHCI timed out on TD - token=%#x\n", token); + ret = ehci_disable_async(ctrl); + if (ret) + goto fail; + if (!(QT_TOKEN_GET_STATUS(qhtoken) & QT_TOKEN_STATUS_ACTIVE)) { debug("TOKEN=%#x\n", qhtoken); switch (QT_TOKEN_GET_STATUS(qhtoken) & @@ -1512,6 +1550,16 @@ static int _ehci_submit_int_msg(struct usb_device *dev, unsigned long pipe, return result; } +static int _ehci_lock_async(struct ehci_ctrl *ctrl, int lock) +{ + ctrl->async_locked = lock; + + if (lock) + return 0; + + return ehci_disable_async(ctrl); +} + #if !CONFIG_IS_ENABLED(DM_USB) int submit_bulk_msg(struct usb_device *dev, unsigned long pipe, void *buffer, int length) @@ -1549,6 +1597,13 @@ int destroy_int_queue(struct usb_device *dev, struct int_queue *queue) { return _ehci_destroy_int_queue(dev, queue); } + +int usb_lock_async(struct usb_device *dev, int lock) +{ + struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); + + return _ehci_lock_async(ctrl, lock); +} #endif #if CONFIG_IS_ENABLED(DM_USB) @@ -1612,6 +1667,13 @@ static int ehci_get_max_xfer_size(struct udevice *dev, size_t *size) return 0; } +static int ehci_lock_async(struct udevice *dev, int lock) +{ + struct ehci_ctrl *ctrl = dev_get_priv(dev); + + return _ehci_lock_async(ctrl, lock); +} + int ehci_register(struct udevice *dev, struct ehci_hccr *hccr, struct ehci_hcor *hcor, const struct ehci_ops *ops, uint tweaks, enum usb_init_type init) @@ -1678,6 +1740,7 @@ struct dm_usb_ops ehci_usb_ops = { .poll_int_queue = ehci_poll_int_queue, .destroy_int_queue = ehci_destroy_int_queue, .get_max_xfer_size = ehci_get_max_xfer_size, + .lock_async = ehci_lock_async, }; #endif diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index 6c359af90c..66c1d61dbf 100644 --- a/drivers/usb/host/ehci.h +++ b/drivers/usb/host/ehci.h @@ -255,6 +255,7 @@ struct ehci_ctrl { int periodic_schedules; int ntds; bool has_fsl_erratum_a005275; /* Freescale HS silicon quirk */ + bool async_locked; struct ehci_ops ops; void *priv; /* client's private data */ }; diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 8521651588..5e423012df 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -22,6 +22,17 @@ struct usb_uclass_priv { int companion_device_count; }; +int usb_lock_async(struct usb_device *udev, int lock) +{ + struct udevice *bus = udev->controller_dev; + struct dm_usb_ops *ops = usb_get_ops(bus); + + if (!ops->lock_async) + return -ENOSYS; + + return ops->lock_async(bus, lock); +} + int usb_disable_asynch(int disable) { int old_value = asynch_allowed; diff --git a/include/usb.h b/include/usb.h index efb67ea33f..22f6088fe6 100644 --- a/include/usb.h +++ b/include/usb.h @@ -269,6 +269,7 @@ int usb_bulk_msg(struct usb_device *dev, unsigned int pipe, void *data, int len, int *actual_length, int timeout); int usb_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, int transfer_len, int interval, bool nonblock); +int usb_lock_async(struct usb_device *dev, int lock); int usb_disable_asynch(int disable); int usb_maxpacket(struct usb_device *dev, unsigned long pipe); int usb_get_configuration_no(struct usb_device *dev, int cfgno, @@ -791,6 +792,16 @@ struct dm_usb_ops { * in a USB transfer. USB class driver needs to be aware of this. */ int (*get_max_xfer_size)(struct udevice *bus, size_t *size); + + /** + * lock_async() - Keep async schedule after a transfer + * + * It may be desired to keep the asynchronous schedule running even + * after a transfer finishes, usually when doing multiple transfers + * back-to-back. This callback allows signalling the USB controller + * driver to do just that. + */ + int (*lock_async)(struct udevice *udev, int lock); }; #define usb_get_ops(dev) ((struct dm_usb_ops *)(dev)->driver->ops)
Rather than keeping the asynchronous schedule running always, keep it running only across USB mass storage transfers for now, as it seems that keeping it running all the time interferes with certain control transfers during device enumeration. Note that running the async schedule all the time should not be an issue, especially on EHCI HCD, as that one implements most of the transfers using async schedule. Note that we have usb_disable_asynch(), which however is utterly broken. The usb_disable_asynch() blocks the USB core from doing async transfers by setting a global flag. The async schedule should however be disabled per USB controller. Moreover, setting a global flag does not prevent the controller from using the async schedule, which e.g. the EHCI HCD does. This patch implements additional callback to the controller, which permits it to lock the async schedule and keep it running across multiple transfers. Once the schedule is unlocked, it must also be disabled. This thus prevents the async schedule from running outside of the USB mass storage transfers. Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com> Cc: Lukasz Majewski <lukma at denx.de> Cc: Tom Rini <trini at konsulko.com> --- common/usb.c | 5 ++ common/usb_storage.c | 4 ++ drivers/usb/host/ehci-hcd.c | 91 +++++++++++++++++++++++++++++------ drivers/usb/host/ehci.h | 1 + drivers/usb/host/usb-uclass.c | 11 +++++ include/usb.h | 11 +++++ 6 files changed, 109 insertions(+), 14 deletions(-)