Re: PR 60404: Beaglebone Black: nonfunctional usb wifi

Hi,

> Am 12.07.2026 um 19:05 schrieb Brook Milligan =
<[email protected]>:
>=20
> 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.
>=20
> I would greatly appreciate feedback before I commit this patch.
> Although all input is welcome, there are several questions I would
> appreciate answers for.
>=20
> 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=E2=80=99ve removed it.

>=20
> 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=20
still not working. Also add the new crash.

>=20
> Thanks a lot for your help.  I look forward to feedback.
>=20
> Cheers,
> Brook
>=20
>=20
> Index: ti_motg.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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 @@
>=20
> #include <dev/fdt/fdtvar.h>
>=20
> +#include <libfdt.h>
> +
> #ifdef USB_DEBUG
> #ifndef MOTG_DEBUG
> #define motgdebug 0
> @@ -165,6 +167,47 @@
>=20
> aprint_normal_dev(self, "interrupting on %s\n", intrstr);
>=20
> + bus_addr_t phy_ctrl_addr;
> + bus_size_t phy_ctrl_size;
> + int phy_phandle =3D fdtbus_get_phandle(phandle, "phys");

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

> + int cm_phandle =3D 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] =3D USB_ALIAS;
> + int usb_ctrl_offset =3D 0;
> + int n =3D 0;
> + const char * prop =3D 0;
> + do {
> + if ((n =3D snprintf(&usb_alias[sizeof(USB_ALIAS)-1],
> +  USB_ALIAS_SIZE-(sizeof(USB_ALIAS)-1),
> +  "%d", usb_ctrl_offset)) > 0
> +    && (prop =3D fdt_get_alias(fdtbus_get_data(), usb_alias)) !=3D =
NULL) {
> + if (OF_finddevice(prop) =3D=3D phandle) {
> + break;
> + }
> + ++usb_ctrl_offset;
> + }
> + } while (n > 0 && prop);
> + if (n <=3D 0 || !prop) {
> + aprint_error_dev(self, "cannot identify usb instance\n");
> + return;
> + }
> + usb_ctrl_offset *=3D 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=E2=80=99ve only compile-tested it

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

> +=20
> + bus_space_handle_t handle;
> + if (bus_space_map(faa->faa_bst, phy_ctrl_addr, phy_ctrl_size, 0, =
&handle) !=3D 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=3DHEAD&conte=
nt-type=3Dtext/x-cvsweb-markup=20

> + aprint_error_dev(self, "couldn't map  control module USB =
registers\n");
> + return;
> + }
> +
> + uint32_t usb_ctrl =3D bus_space_read_4(faa->faa_bst, handle, =
usb_ctrl_offset);
> + usb_ctrl &=3D ~(CM_USBCTRL_CM_PWRDN | CM_USBCTRL_OTG_PWRDN);
> + usb_ctrl |=3D (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 =3D faa->faa_bst;
> if (bus_space_map(sc->sc_motg.sc_iot, addr[0], size[0], 0,
>    &sc->sc_motg.sc_ioh) !=3D 0) {
> Index: ti_otgreg.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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 @@
>=20
> #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)
>=20

In general, you could add some comments what the code does. You don=E2=80=99=
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