Re: PR 60404: Beaglebone Black: nonfunctional usb wifi

[Yuri Honegger <yuri.honegger%icloud.com@localhost>]

Hi,

> Am 12.07.2026 um 19:05 schrieb Brook Milligan <brook%biology.nmsu.edu@localhost>:
> 
> With the help of yurix@, riastradh@, and rjs@ (thanks!), I have a
> working fix for the ti_motg driver (see below).  With this applied,
> the BBB will detect USB devices, including both mass storage and wifi.
> However, while a mass storage device seems to work fine, wifi devices
> do not.
> 
> I would greatly appreciate feedback before I commit this patch.
> Although all input is welcome, there are several questions I would
> appreciate answers for.
> 
> First, this code gathers all information dynamically from the device
> tree, whereas there are hard-coded constants in, for example,
> ti_otgreg.h; for example, the number of supported USB devices can be
> determined dynamically or statically.  What is the wisdom of one
> approach over the other?

TI_OTG_NPORTS is a bad example because it is unused. In general, ti_otgreg.h contains the register layout, which is the same in all devices with this usb controller.
If you rebase, I’ve removed it.

> 
> Second, this patch was motivated by the problem outlined in PR 60404.
> However, that problem (nonfunctional wifi) turns out to be more
> complex and the solution will involve multiple steps.  This patch
> addresses the first (USB device detection), but the wifi drivers still
> do not work correctly (e.g., panics are triggered, cannot load
> firmware errors, wpa_supplicant triggers errors, etc.).  Most of the
> information currently in PR 60404 turns out to relate only to this
> first USB device detection problem.  When this patch is committed,
> should PR 60404 be closed and a new one opened to focus on the new
> problems, or should it remain open, perhaps with additional
> information?

Indicate that it has been partially fixed and that now the WIFI adapter is detected, but 
still not working. Also add the new crash.

> 
> Thanks a lot for your help.  I look forward to feedback.
> 
> Cheers,
> Brook
> 
> 
> Index: ti_motg.c
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/arm/ti/ti_motg.c,v
> retrieving revision 1.5
> diff -u -r1.5 ti_motg.c
> --- ti_motg.c 2 Feb 2024 22:14:04 -0000 1.5
> +++ ti_motg.c 12 Jul 2026 16:44:11 -0000
> @@ -49,6 +49,8 @@
> 
> #include <dev/fdt/fdtvar.h>
> 
> +#include <libfdt.h>
> +
> #ifdef USB_DEBUG
> #ifndef MOTG_DEBUG
> #define motgdebug 0
> @@ -165,6 +167,47 @@
> 
> aprint_normal_dev(self, "interrupting on %s\n", intrstr);
> 
> + bus_addr_t phy_ctrl_addr;
> + bus_size_t phy_ctrl_size;
> + int phy_phandle = fdtbus_get_phandle(phandle, "phys");

You need to add error handling here in case the property doesn’t exist. Search for other usages of this function if you don’t know how.

> + int cm_phandle = fdtbus_get_phandle(phy_phandle, "ti,ctrl_mod");

Here too

> + fdtbus_get_reg_byname(cm_phandle, "phy_ctrl", &phy_ctrl_addr, &phy_ctrl_size);

Here too

> +
> +#define USB_ALIAS "usb"
> +#define USB_ALIAS_SIZE 16
> +
> + char usb_alias[USB_ALIAS_SIZE] = USB_ALIAS;
> + int usb_ctrl_offset = 0;
> + int n = 0;
> + const char * prop = 0;
> + do {
> + if ((n = snprintf(&usb_alias[sizeof(USB_ALIAS)-1],
> +  USB_ALIAS_SIZE-(sizeof(USB_ALIAS)-1),
> +  "%d", usb_ctrl_offset)) > 0
> +    && (prop = fdt_get_alias(fdtbus_get_data(), usb_alias)) != NULL) {
> + if (OF_finddevice(prop) == phandle) {
> + break;
> + }
> + ++usb_ctrl_offset;
> + }
> + } while (n > 0 && prop);
> + if (n <= 0 || !prop) {
> + aprint_error_dev(self, "cannot identify usb instance\n");
> + return;
> + }
> + usb_ctrl_offset *= CM_USBCTL_SIZE;

The general approach seems fine, but wow is it hard to read.

What about something like this? This is in my opinion much more readable. Note: I’ve only compile-tested it

char alias_name[16];
int usb_instance = -1;
for (int i = 0; i < 2; i++) {
	int n = snprintf(alias_name, sizeof(alias_name), "usb%d", i);
	KASSERT(n > 0);
	const char *prop = fdt_get_alias(fdtbus_get_data(), alias_name);
	if (prop == NULL) {
		continue;
	}
	if (OF_finddevice(prop) == phandle) {
		usb_instance = i;
		break;
	}
}
if (usb_instance < 0) {
	aprint_error_dev(self, "cannot identify usb instance\n");
	return;
}

> + 
> + bus_space_handle_t handle;
> + if (bus_space_map(faa->faa_bst, phy_ctrl_addr, phy_ctrl_size, 0, &handle) != 0) {

This line is too long. This is fine while we are figuring stuff out, but before it can be committed, the code needs to follow KNF: https://cvsweb.netbsd.org/bsdweb.cgi/src/share/misc/style?rev=HEAD&content-type=text/x-cvsweb-markup 

> + aprint_error_dev(self, "couldn't map  control module USB registers\n");
> + return;
> + }
> +
> + uint32_t usb_ctrl = bus_space_read_4(faa->faa_bst, handle, usb_ctrl_offset);
> + usb_ctrl &= ~(CM_USBCTRL_CM_PWRDN | CM_USBCTRL_OTG_PWRDN);
> + usb_ctrl |= (CM_USBCTRL_OTGVDET_EN | CM_USBCTRL_OTGSESSENDEN);
> + bus_space_write_4(faa->faa_bst, handle, usb_ctrl_offset, usb_ctrl);
> +
> sc->sc_motg.sc_iot = faa->faa_bst;
> if (bus_space_map(sc->sc_motg.sc_iot, addr[0], size[0], 0,
>    &sc->sc_motg.sc_ioh) != 0) {
> Index: ti_otgreg.h
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/arm/ti/ti_otgreg.h,v
> retrieving revision 1.1
> diff -u -r1.1 ti_otgreg.h
> --- ti_otgreg.h 27 Oct 2019 16:31:26 -0000 1.1
> +++ ti_otgreg.h 12 Jul 2026 16:44:11 -0000
> @@ -72,3 +72,10 @@
> 
> #define USB_CORE_OFFSET 0x400
> #define USB_CORE_SIZE 0x400
> +
> +/* Control module register: usb_ctlN */
> +#define CM_USBCTL_SIZE 8
> +#define CM_USBCTRL_CM_PWRDN (1 << 0)
> +#define CM_USBCTRL_OTG_PWRDN (1 << 1)
> +#define CM_USBCTRL_OTGVDET_EN (1 << 19)
> +#define CM_USBCTRL_OTGSESSENDEN (1 << 20)
> 

In general, you could add some comments what the code does. You don’t need to go overboard, but imagine you are reading this code again in 5 years and wonder why it looks up device tree aliases.

- yuri