[PATCH] fastboot: release usb_gadget on reboot commands
When host issues "fastboot reboot fastboot", it's expected that the board drops the USB connection before resetting.
On some boards, such as Khadas VIM3L and SEI610, this is not the case.
We observe the following error: $ fastboot reboot fastboot Rebooting into fastboot OKAY [ 0.004s] fastboot: error: Failed to boot into userspace fastboot; one or more components might be unbootable.
This does not happen when we use the RST button on the board, nor in linux. We might hit a undefined hardware behavior, where D+ and D- are in an unknown state.
Change the logic in cmd/fastboot.c to do a "reset request" instead of actually calling reset. This allows us to get out of the infinite loop in fastboot.c, de-initialize the usb stack and do a clean reset.
Signed-off-by: Mattijs Korpershoek mkorpershoek@baylibre.com --- cmd/fastboot.c | 3 +++ drivers/usb/gadget/f_fastboot.c | 3 ++- drivers/usb/gadget/g_dnl.c | 17 +++++++++++++++++ include/g_dnl.h | 5 +++++ 4 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/cmd/fastboot.c b/cmd/fastboot.c index 033a2c95e8f0..97ce311e6d9b 100644 --- a/cmd/fastboot.c +++ b/cmd/fastboot.c @@ -87,6 +87,9 @@ exit: g_dnl_clear_detach(); usb_gadget_release(controller_index);
+ if (g_dnl_should_reset()) + do_reset(NULL, 0, 0, NULL); + return ret; #else pr_err("Fastboot USB not enabled\n"); diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 8ba55aab9f8f..a00d1ca571d1 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) { - do_reset(NULL, 0, 0, NULL); + g_dnl_trigger_detach(); + g_dnl_trigger_reset_request(); }
static unsigned int rx_bytes_expected(struct usb_ep *ep) diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c index afb7b74f3057..d083a214290d 100644 --- a/drivers/usb/gadget/g_dnl.c +++ b/drivers/usb/gadget/g_dnl.c @@ -200,6 +200,23 @@ void g_dnl_clear_detach(void) g_dnl_detach_request = false; }
+static bool g_dnl_reset_request; + +bool g_dnl_should_reset(void) +{ + return g_dnl_reset_request; +} + +void g_dnl_trigger_reset_request(void) +{ + g_dnl_reset_request = true; +} + +void g_dnl_clear_reset_request(void) +{ + g_dnl_reset_request = false; +} + static int g_dnl_get_bcd_device_number(struct usb_composite_dev *cdev) { struct usb_gadget *gadget = cdev->gadget; diff --git a/include/g_dnl.h b/include/g_dnl.h index 836ee602c8da..95c772385fa6 100644 --- a/include/g_dnl.h +++ b/include/g_dnl.h @@ -43,6 +43,11 @@ void g_dnl_set_product(const char *s); bool g_dnl_detach(void); void g_dnl_trigger_detach(void); void g_dnl_clear_detach(void); + +bool g_dnl_should_reset(void); +void g_dnl_trigger_reset_request(void); +void g_dnl_clear_reset_request(void); + int run_usb_dnl_gadget(int usbctrl_index, char *usb_dnl_gadget);
#endif /* __G_DOWNLOAD_H_ */
On 7/21/22 15:59, Mattijs Korpershoek wrote:
[...]
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 8ba55aab9f8f..a00d1ca571d1 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
- do_reset(NULL, 0, 0, NULL);
- g_dnl_trigger_detach();
- g_dnl_trigger_reset_request();
Wouldn't it be enough to call usb_gadget_release() before do_reset() here ? Or actually fix up the hardware state in your platform reset implementation, which would cover all such odd states for every other USB UDC mode of operation, not just fastboot ?
On ven., juil. 22, 2022 at 23:48, Marek Vasut marex@denx.de wrote:
On 7/21/22 15:59, Mattijs Korpershoek wrote:
[...]
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 8ba55aab9f8f..a00d1ca571d1 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
- do_reset(NULL, 0, 0, NULL);
- g_dnl_trigger_detach();
- g_dnl_trigger_reset_request();
Hi Marek,
Thank you for your review and your suggestions.
Wouldn't it be enough to call usb_gadget_release() before do_reset() here ? Or actually fix up the hardware state in your platform reset
Calling usb_gadget_release() just before do_reset() is sufficient as well. However usb_gadget_release(int index) takes a controller index as argument. This controller index is retrieved from the "=> fastboot" cmd and I could not come up with something elegant to retrieve it from f_fastboot.c. I can have another look if you think that's a better solution.
implementation, which would cover all such odd states for every other USB UDC mode of operation, not just fastboot ?
Implementing a platform_reset to reset the usb controller also works. I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix (this one) because it's generic and might fix issues for other boards.
So, should I abandon this version and go with the platform specific fix instead ?
[1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc203...
On 7/25/22 15:19, Mattijs Korpershoek wrote:
On ven., juil. 22, 2022 at 23:48, Marek Vasut marex@denx.de wrote:
On 7/21/22 15:59, Mattijs Korpershoek wrote:
[...]
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 8ba55aab9f8f..a00d1ca571d1 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
- do_reset(NULL, 0, 0, NULL);
- g_dnl_trigger_detach();
- g_dnl_trigger_reset_request();
Hi Marek,
Thank you for your review and your suggestions.
Wouldn't it be enough to call usb_gadget_release() before do_reset() here ? Or actually fix up the hardware state in your platform reset
Calling usb_gadget_release() just before do_reset() is sufficient as well. However usb_gadget_release(int index) takes a controller index as argument. This controller index is retrieved from the "=> fastboot" cmd and I could not come up with something elegant to retrieve it from f_fastboot.c. I can have another look if you think that's a better solution.
I think it would be nice to have this kind of generic fix in addition to one specific to your platform , as discussed below.
implementation, which would cover all such odd states for every other USB UDC mode of operation, not just fastboot ?
Implementing a platform_reset to reset the usb controller also works. I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix (this one) because it's generic and might fix issues for other boards.
So, should I abandon this version and go with the platform specific fix instead ?
[1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc203...
If your controller is responsible for causing the platform to fail to reboot, then I think it makes sense to do a fix either in the reset implementation for that platform, or the controller driver.
On Mon, Jul 25, 2022 at 17:30, Marek Vasut marex@denx.de wrote:
On 7/25/22 15:19, Mattijs Korpershoek wrote:
On ven., juil. 22, 2022 at 23:48, Marek Vasut marex@denx.de wrote:
On 7/21/22 15:59, Mattijs Korpershoek wrote:
[...]
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 8ba55aab9f8f..a00d1ca571d1 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
- do_reset(NULL, 0, 0, NULL);
- g_dnl_trigger_detach();
- g_dnl_trigger_reset_request();
Hi Marek,
Thank you for your review and your suggestions.
Wouldn't it be enough to call usb_gadget_release() before do_reset() here ? Or actually fix up the hardware state in your platform reset
Calling usb_gadget_release() just before do_reset() is sufficient as well. However usb_gadget_release(int index) takes a controller index as argument. This controller index is retrieved from the "=> fastboot" cmd and I could not come up with something elegant to retrieve it from f_fastboot.c. I can have another look if you think that's a better solution.
I think it would be nice to have this kind of generic fix in addition to one specific to your platform , as discussed below.
ACK
implementation, which would cover all such odd states for every other USB UDC mode of operation, not just fastboot ?
Implementing a platform_reset to reset the usb controller also works. I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix (this one) because it's generic and might fix issues for other boards.
So, should I abandon this version and go with the platform specific fix instead ?
[1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc203...
If your controller is responsible for causing the platform to fail to reboot, then I think it makes sense to do a fix either in the reset implementation for that platform, or the controller driver.
To be clear, the usb controller is *not* causing the whole platform to reboot. On do_reset(), the platform resets fine. However, the usb controller is *not* put in reset. Because of that, the host does not detect a USB disconnection. Which is what I'm trying to solve here.
If we do the generic fix, there is not really a point into implementing a platform specific reset. Note that on linux, when we run "reboot" the driver's remove() function takes care of usb reset.
On 7/26/22 10:25, Mattijs Korpershoek wrote: [...]
implementation, which would cover all such odd states for every other USB UDC mode of operation, not just fastboot ?
Implementing a platform_reset to reset the usb controller also works. I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix (this one) because it's generic and might fix issues for other boards.
So, should I abandon this version and go with the platform specific fix instead ?
[1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc203...
If your controller is responsible for causing the platform to fail to reboot, then I think it makes sense to do a fix either in the reset implementation for that platform, or the controller driver.
To be clear, the usb controller is *not* causing the whole platform to reboot. On do_reset(), the platform resets fine. However, the usb controller is *not* put in reset. Because of that, the host does not detect a USB disconnection. Which is what I'm trying to solve here.
Yes, this is how I understand the problem too.
Assume someone would run e.g. ums code, which would bring up the controller too, I suspect it would also cause the same reset issues. Which is why I wonder whether we shouldn't add this kind of fix into the reset handler for the platform itself.
If we do the generic fix, there is not really a point into implementing a platform specific reset. Note that on linux, when we run "reboot" the driver's remove() function takes care of usb reset.
Does that also work with sysrq-B or reboot -nf (one of the fast reboots) ?
Hi Marek,
On Tue, Jul 26, 2022 at 15:36, Marek Vasut marex@denx.de wrote:
On 7/26/22 10:25, Mattijs Korpershoek wrote: [...]
implementation, which would cover all such odd states for every other USB UDC mode of operation, not just fastboot ?
Implementing a platform_reset to reset the usb controller also works. I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix (this one) because it's generic and might fix issues for other boards.
So, should I abandon this version and go with the platform specific fix instead ?
[1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc203...
If your controller is responsible for causing the platform to fail to reboot, then I think it makes sense to do a fix either in the reset implementation for that platform, or the controller driver.
To be clear, the usb controller is *not* causing the whole platform to reboot. On do_reset(), the platform resets fine. However, the usb controller is *not* put in reset. Because of that, the host does not detect a USB disconnection. Which is what I'm trying to solve here.
Yes, this is how I understand the problem too.
Assume someone would run e.g. ums code, which would bring up the controller too, I suspect it would also cause the same reset issues. Which is why I wonder whether we shouldn't add this kind of fix into the reset handler for the platform itself.
Oh. Got it. I was solely focusing on my use case (fastboot) and not considering ums or other things. Thank you for the explanation. It probably makes sense to add it in the platform reset in that case.
If we do the generic fix, there is not really a point into implementing a platform specific reset. Note that on linux, when we run "reboot" the driver's remove() function takes care of usb reset.
Does that also work with sysrq-B or reboot -nf (one of the fast reboots) ?
# echo b > /proc/sysrq-trigger [ 3713.306318] sysrq: Resetting [ 3713.306462] SMP: stopping secondary CPUs
Also triggers the same bug as the one I'm attempting to solve here. We observe a very long delay before the host reports: [31124.104342] usb 1-1: USB disconnect, device number 20
Thank you for bringing that up!
participants (2)
-
Marek Vasut -
Mattijs Korpershoek