On 17/10/21(Sun) 09:06, Christopher Zimmermann wrote:
> Hi,
>
> on my RK3399, a usb device connected to the USB 3 port is not detected
> during boot because it is in SS_INACTIVE (0x00c0) state:
>
> uhub3 at usb3 configuration 1 interface 0 "Generic xHCI root hub" rev
> 3.00/1.00 addr 1
> uhub3: uhub_attach
> uhub3: 2 ports with 2 removable, self powered
> uhub3: intr status=0
> usb_needs_explore: usb3: not exploring before first explore
> uhub3: uhub_explore
> uhub3: port 1 status=0x02a0 change=0x0000
> uhub3: port 2 status=0x02c0 change=0x0040
> usb_explore: usb3: first explore done
> xhci1: port=2 change=0x04
> uhub3: intr status=0
> uhub3: uhub_explore
> uhub3: port 2 status=0x02c0 change=0x0040
> xhci1: port=2 change=0x04
> uhub3: intr status=0
> uhub3: uhub_explore
> uhub3: port 2 status=0x02c0 change=0x0040
>
> [...]
>
> [turn the usb device off and on again]
>
> uhub3: intr status=0
> uhub3: uhub_explore
> uhub3: port 2 status=0x0203 change=0x0001
> usbd_reset_port: port 2 reset done
> xhci1: port=2 change=0x04
> uhub3: intr status=0
> uhub3: port 2 status=0x0203 change=0x0000
> umass0 at uhub3 port 2 configuration 1 interface 0 "ATEC Dual Disk Drive" rev
> 3.00/1.08 addr 2
>
> This might be because u-boot-aarch64-2021.10 from packages left it in that
> state.
> I added this code to reset a device locked in such a state:
It's not clear to me if this a warm reset or not? If the port is in
SS.Inactive it needs a warm reset, no?
If so could you add a comment on top of the block. I'd also suggest
moving the block after the "warm reset change" (BH_PORT_RESET), this
might matter and at least match Linux's logic.
I wish you could add the logic to properly check if a warm reset is
required by checking the proper bits against the port number, but we
can rely on UPS_C_PORT_LINK_STATE for now and do that in a second step.
Comments below:
> Index: uhub.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uhub.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 uhub.c
> --- uhub.c 31 Jul 2020 10:49:33 -0000 1.95
> +++ uhub.c 17 Oct 2021 06:44:14 -0000
> @@ -414,6 +414,24 @@ uhub_explore(struct usbd_device *dev)
> change |= UPS_C_CONNECT_STATUS;
> }
>
> + if (change & UPS_C_PORT_LINK_STATE &&
> + UPS_PORT_LS_GET(status) == UPS_PORT_LS_SS_INACTIVE &&
This should check for the speed of the HUB:
sc->sc_hub->speed == USB_SPEED_SUPER
Should we also check if the link state is UPS_PORT_LS_COMP_MOD?
> + ! (status & UPS_CURRENT_CONNECT_STATUS)) {
^
Please drop the space here
> + DPRINTF("%s: port %d is in in SS_INACTIVE.Quiet
> state. "
> + "Reset port.\n",
> + sc->sc_dev.dv_xname, port);
> + usbd_clear_port_feature(sc->sc_hub, port,
> + UHF_C_PORT_RESET);
> +
> + if (usbd_reset_port(sc->sc_hub, port)) {
> + printf("%s: port %d reset failed\n",
> + DEVNAME(sc), port);
> + return (-1);
> + }
> +
> + change |= UPS_C_CONNECT_STATUS;
> + }
> +
> if (change & UPS_C_BH_PORT_RESET &&
> sc->sc_hub->speed == USB_SPEED_SUPER) {
> usbd_clear_port_feature(sc->sc_hub, port,
>
>
> Now the device attaches during boot. A redundant second reset of the device
> is performed during uhub_port_connect():
>
> uhub3 at usb3 configuration 1 interface 0 "Generic xHCI root hub" rev
> 3.00/1.00 addr 1
> uhub3: uhub_attach
> uhub3: 2 ports with 2 removable, self powered
> xhci1: port=2 change=0x04
> uhub3: intr status=0
> usb_needs_explore: usb3: not exploring before first explore
> uhub3: uhub_explore
> uhub3: port 1 status=0x02a0 change=0x0000
> uhub3: port 2 status=0x02c0 change=0x0040
> uhub3: port 2 is in in SS_INACTIVE.Quiet state. Reset port.
> usbd_reset_port: port 2 reset done
> usb_explore: usb3: first explore done
> xhci1: port=2 change=0x04
> uhub3: intr status=0
> uhub3: uhub_explore
> uhub3: port 2 status=0x0203 change=0x0031
> uhub3: uhub_port_connect
> usbd_reset_port: port 2 reset done
> xhci1: port=2 change=0x04
> uhub3: intr status=0
> uhub3: port 2 status=0x0203 change=0x0000
> umass0 at uhub3 port 2 configuration 1 interface 0 "ATEC Dual Disk Drive" rev
> 3.00/1.08 addr 2
>
>
> OK to commit this diff? Or should this be done some other way?
>
>
> Christopher
>
