From 7a875a8e5c7af1d7c1c7232fbef898d0f18ec7da Mon Sep 17 00:00:00 2001 From: Xavier Drudis Ferran Date: Wed, 21 Jun 2023 09:13:07 +0200 Subject: cmd: usb: Prevent reset in usb tree/info command Commands causing reset in some configs: When bootflow scan is run, this will cause a UCLASS_BOOTDEV device to be added as sibling of those UCLASS_BLK devices found in the search chain defined in environment variable "boot_targets", until boot succeeds from some device. This can happen automatically as part of the default boot process on some boards (example: Rock Pi 4) depending on the board configuration (DISTRO_DEFAULTS, BOOTSTD, BOOTCOMMAND, etc.) because they have bootcmd=bootflow scan. If boot doesn't succeed from any device, and usb is in boot_targets, and an usb storage device is plugged to some usb port at boot time, its UCLASS_MASS_STORAGE device will have a UCLASS_BOOTDEV device as child, besides a UCLASS_BLK child. If once the boot fails the user enters at the U-Boot shell prompt: usb info or usb tree The code in cmd/usb.c will eventually recurse into the UCLASS_BOOTDEV device and pass a null usb_device pointer to usb_show_tree_graph() or usb_show_info() (because it has no parent_priv_). This causes a reset. The expected behaviour would be to ignore the UCLASS_BOOTDEV device, continue listing the usb information and return to the prompt. Minimal test: Another way to trigger this reset as a minimal test or on boards with a different bootcmd would be: - make sure "usb" is in environment variable boot_targets (might need setenv boot_targets usb; and/or saveenv and reset), then, with a usb storage device plugged to a usb port, run: => usb reset ; bootflow scan ; usb info Solution: Fix it (twice) by checking for null parent_priv_ and adding UCLASS_BOOTDEV to the list of ignored class ids before the recursive call. This prevents the current particular problem with UCLASS_BOOTDEV, even in case it ever gets some parent_priv_ struct which is not an usb_device, despite being the child of a usb_device->dev. And it also prevents possible future problems if other children are added to usb devices that don't have parent_priv_ because they are not part of the usb tree, just abstractions of functionality (like UCLASS_BLK and UCLASS_BOOTDEV are now). Signed-off-by: Xavier Drudis Ferran Reviewed-by: Simon Glass Reviewed-by: Marek Vasut Tested-by: Marek Vasut --- cmd/usb.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/usb.c b/cmd/usb.c index 61937283840..23253f22231 100644 --- a/cmd/usb.c +++ b/cmd/usb.c @@ -421,7 +421,9 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) * Ignore emulators and block child devices, we only want * real devices */ - if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) && + if (udev && + (device_get_uclass_id(child) != UCLASS_BOOTDEV) && + (device_get_uclass_id(child) != UCLASS_USB_EMUL) && (device_get_uclass_id(child) != UCLASS_BLK)) { usb_show_tree_graph(udev, pre); pre[index] = 0; @@ -604,10 +606,12 @@ static void usb_show_info(struct usb_device *udev) child; device_find_next_child(&child)) { if (device_active(child) && + (device_get_uclass_id(child) != UCLASS_BOOTDEV) && (device_get_uclass_id(child) != UCLASS_USB_EMUL) && (device_get_uclass_id(child) != UCLASS_BLK)) { udev = dev_get_parent_priv(child); - usb_show_info(udev); + if (udev) + usb_show_info(udev); } } } -- cgit v1.2.3 From 9c9454ac2e4ffd9a8b30744329029f1676d2e7be Mon Sep 17 00:00:00 2001 From: Teik Heng Chong Date: Wed, 21 Jun 2023 11:13:58 +0800 Subject: usb: dwc2: Fix the write to W1C fields in HPRT register Fix the write to the HPRT register which treat W1C fields as if they were mere RW. This leads to unintended clearing of such fields This bug was found during the testing on Simics model. Referring to specification DesignWare Cores USB 2.0 Hi-Speed On-The-Go (OTG) Databook (3.30a)"5.3.4.8 Host Port Control and Status Register (HPRT)", the HPRT.PrtPwr is cleared by this mistake. In the Linux driver (contrary to U-Boot), HPRT is always read using dwc2_read_hprt0 helper function which clears W1C bits. So after write back those bits are zeroes. Signed-off-by: Teik Heng Chong --- drivers/usb/host/dwc2.c | 34 ++++++++-------------------------- drivers/usb/host/dwc2.h | 4 ++++ 2 files changed, 12 insertions(+), 26 deletions(-) diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 23060fc369c..9818f9be94e 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -315,9 +315,7 @@ static void dwc_otg_core_host_init(struct udevice *dev, /* Turn on the vbus power. */ if (readl(®s->gintsts) & DWC2_GINTSTS_CURMODE_HOST) { - hprt0 = readl(®s->hprt0); - hprt0 &= ~(DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET); - hprt0 &= ~(DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG); + hprt0 = readl(®s->hprt0) & ~DWC2_HPRT0_W1C_MASK; if (!(hprt0 & DWC2_HPRT0_PRTPWR)) { hprt0 |= DWC2_HPRT0_PRTPWR; writel(hprt0, ®s->hprt0); @@ -748,7 +746,7 @@ static int dwc_otg_submit_rh_msg_out(struct dwc2_priv *priv, case (USB_REQ_CLEAR_FEATURE << 8) | USB_RECIP_OTHER | USB_TYPE_CLASS: switch (wValue) { case USB_PORT_FEAT_C_CONNECTION: - setbits_le32(®s->hprt0, DWC2_HPRT0_PRTCONNDET); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTCONNDET); break; } break; @@ -759,21 +757,13 @@ static int dwc_otg_submit_rh_msg_out(struct dwc2_priv *priv, break; case USB_PORT_FEAT_RESET: - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | - DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); mdelay(50); - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTRST); + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | DWC2_HPRT0_PRTRST); break; case USB_PORT_FEAT_POWER: - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | - DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); break; case USB_PORT_FEAT_ENABLE: @@ -1213,14 +1203,9 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) dwc_otg_core_host_init(dev, regs); } - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); mdelay(50); - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET | - DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG | - DWC2_HPRT0_PRTRST); + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | DWC2_HPRT0_PRTRST); for (i = 0; i < MAX_DEVICE; i++) { for (j = 0; j < MAX_ENDPOINT; j++) { @@ -1246,10 +1231,7 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) static void dwc2_uninit_common(struct dwc2_core_regs *regs) { /* Put everything in reset. */ - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); } #if !CONFIG_IS_ENABLED(DM_USB) diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h index a6f562fe60e..6f022e33a19 100644 --- a/drivers/usb/host/dwc2.h +++ b/drivers/usb/host/dwc2.h @@ -543,6 +543,10 @@ struct dwc2_core_regs { #define DWC2_HPRT0_PRTSPD_LOW (2 << 17) #define DWC2_HPRT0_PRTSPD_MASK (0x3 << 17) #define DWC2_HPRT0_PRTSPD_OFFSET 17 +#define DWC2_HPRT0_W1C_MASK (DWC2_HPRT0_PRTCONNDET | \ + DWC2_HPRT0_PRTENA | \ + DWC2_HPRT0_PRTENCHNG | \ + DWC2_HPRT0_PRTOVRCURRCHNG) #define DWC2_HAINT_CH0 (1 << 0) #define DWC2_HAINT_CH0_OFFSET 0 #define DWC2_HAINT_CH1 (1 << 1) -- cgit v1.2.3