From: Pascal Stumpf Subject: Re: uaudio: Handle certain devices with multiple clock domains, v2 To: Alexandre Ratchov Cc: tech@openbsd.org Date: Fri, 14 Nov 2025 17:53:46 +0100 On Fri, 14 Nov 2025 17:20:25 +0100, Alexandre Ratchov wrote: > On Thu, Nov 13, 2025 at 02:11:18PM +0100, Alexandre Ratchov wrote: > > uaudio(4) doesn't support devices with multiple "clock domains". They > > would be almost multiple independent devices in a single piece of > > hardware. > > > > Physical clocks cost money, so I suspect that most devices have one > > physical clock only, i.e. that the different clocks the device exposes > > are synchronous. If so, uaudio(4) could handle them with the diff > > below. > > > > To test this, first use a know working uaudio(4) device to validate > > your setup (usb host controller, hub etc). Once you feel audio is > > stable, try the device with the multiple clock domains on the same > > port. > > > > I'm insisting on this because there are still usb-related bugs, that > > would prevent understanding the cause of a possible failure. > > > > Thanks for all the feedback. > > Here's a better version of the diff. It assumes that the clocks are > equivalent if they have the same attributes and at least one common > rate. > > This version addresses devices where certain rates are disabled in one > direction i.e. ones that failed with "clock rates differ" with the > previous version of the diff. If your devices just worked with the > previous diff, no need to test this one. Thanks! This now attaches the Lenovo Thunderbolt 3 Dock integrated audio which did not work before: uaudio0 at uhub5 port 2 configuration 1 interface 1 "Lenovo ThinkPad Thunderbolt 3 Dock USB Audio" rev 2.01/0.95 addr 5 uaudio0: warning: assuming common clock uaudio0: class v2, full-speed, sync, channels: 2 play, 1 rec, 3 ctls audio1 at uaudio0 Both playback and recording are working fine. > Index: uaudio.c > =================================================================== > RCS file: /cvs/src/sys/dev/usb/uaudio.c,v > diff -u -p -u -p -r1.180 uaudio.c > --- uaudio.c 2 Nov 2025 14:35:20 -0000 1.180 > +++ uaudio.c 14 Nov 2025 15:54:47 -0000 > @@ -263,7 +263,7 @@ struct uaudio_softc { > /* > * Current clock, UAC v2.0 only > */ > - struct uaudio_unit *clock; > + struct uaudio_unit *pclock, *rclock; > > /* > * Number of input and output terminals > @@ -1023,7 +1023,7 @@ uaudio_alt_getrates(struct uaudio_softc > case UAUDIO_V1: > return p->v1_rates; > case UAUDIO_V2: > - u = sc->clock; > + u = p->mode == AUMODE_PLAY ? sc->pclock : sc->rclock; > while (1) { > switch (u->type) { > case UAUDIO_AC_CLKSRC: > @@ -1050,11 +1050,8 @@ uaudio_alt_getrates(struct uaudio_softc > * return the clock source unit > */ > struct uaudio_unit * > -uaudio_clock(struct uaudio_softc *sc) > +uaudio_clock(struct uaudio_unit *u) > { > - struct uaudio_unit *u; > - > - u = sc->clock; > while (1) { > if (u == NULL) { > DPRINTF("%s: NULL clock pointer\n", __func__); > @@ -1081,13 +1078,20 @@ uaudio_clock(struct uaudio_softc *sc) > * Return the rates bitmap of the given parameters setting > */ > int > -uaudio_getrates(struct uaudio_softc *sc, struct uaudio_params *p) > +uaudio_getrates(struct uaudio_softc *sc, int mode, struct uaudio_params *p) > { > + int rates; > + > switch (sc->version) { > case UAUDIO_V1: > return p->v1_rates; > case UAUDIO_V2: > - return uaudio_alt_getrates(sc, p->palt ? p->palt : p->ralt); > + rates = ~0; > + if (p->palt != NULL && (mode & AUMODE_PLAY)) > + rates &= uaudio_alt_getrates(sc, p->palt); > + if (p->ralt != NULL && (mode & AUMODE_RECORD)) > + rates &= uaudio_alt_getrates(sc, p->ralt); > + return rates; > } > return 0; > } > @@ -1181,6 +1185,38 @@ uaudio_feature_addent(struct uaudio_soft > } > > /* > + * Compare two clock source units. According to UAC2 each clock > + * source defines its own clock domain. However there is hardware > + * exposing multiple clock units that are clocked by the same source > + * and consequently are equivalent to single domain. > + */ > +int > +uaudio_clock_equiv(struct uaudio_softc *sc, > + struct uaudio_unit *u, struct uaudio_unit *v) > +{ > + if (u == NULL || v == NULL) > + return 1; > + > + u = uaudio_clock(u); > + if (u == NULL) > + return 0; > + > + v = uaudio_clock(v); > + if (v == NULL) > + return 0; > + > + if (u->term != v->term) { > + printf("%s: clock attributes differ\n", DEVNAME(sc)); > + return 0; > + } > + > + if (u != v) > + printf("%s: warning: assuming common clock\n", DEVNAME(sc)); > + > + return 1; > +} > + > +/* > * For the given unit, parse the list of its sources and recursively > * call uaudio_process_unit() for each. > */ > @@ -1887,7 +1923,7 @@ uaudio_conf_print(struct uaudio_softc *s > rates = p->v1_rates; > break; > case UAUDIO_V2: > - rates = uaudio_getrates(sc, p); > + rates = uaudio_getrates(sc, AUMODE_PLAY | AUMODE_RECORD, p); > break; > } > printf("pchan = %d, s%dle%d, rchan = %d, s%dle%d, rates:", > @@ -2264,34 +2300,39 @@ uaudio_process_ac(struct uaudio_softc *s > } > > if (sc->version == UAUDIO_V2) { > + > /* > - * Find common clock unit. We assume all terminals > - * belong to the same clock domain (ie are connected > - * to the same source) > + * Find the clocks for the usb-streaming terminal units > */ > - sc->clock = NULL; > + > for (u = sc->unit_list; u != NULL; u = u->unit_next) { > - if (u->type != UAUDIO_AC_INPUT && > - u->type != UAUDIO_AC_OUTPUT) > - continue; > - if (sc->clock == NULL) { > + if (u->type == UAUDIO_AC_INPUT && (u->term >> 8) == 1) { > if (u->clock == NULL) { > - printf("%s: terminal with no clock\n", > - DEVNAME(sc)); > + printf("%s: no play clock\n", DEVNAME(sc)); > return 0; > } > - sc->clock = u->clock; > - } else if (u->clock != sc->clock) { > - printf("%s: only one clock domain supported\n", > - DEVNAME(sc)); > - return 0; > + sc->pclock = u->clock; > + break; > } > } > > - if (sc->clock == NULL) { > - printf("%s: no clock found\n", DEVNAME(sc)); > - return 0; > + for (u = sc->unit_list; u != NULL; u = u->unit_next) { > + if (u->type == UAUDIO_AC_OUTPUT && (u->term >> 8) == 1) { > + if (u->clock == NULL) { > + printf("%s: no rec clock\n", DEVNAME(sc)); > + return 0; > + } > + sc->rclock = u->clock; > + break; > + } > } > + > + DPRINTF("%s: pclock = %d, rclock = %d\n", __func__, > + sc->pclock ? sc->pclock->id : -1, > + sc->rclock ? sc->rclock->id : -1); > + > + if (!uaudio_clock_equiv(sc, sc->pclock, sc->rclock)) > + return 0; > } > return 1; > } > @@ -3118,7 +3159,7 @@ uaudio_stream_open(struct uaudio_softc * > req_buf[1] = sc->rate >> 8; > req_buf[2] = sc->rate >> 16; > req_buf[3] = sc->rate >> 24; > - clock = uaudio_clock(sc); > + clock = uaudio_clock(dir == AUMODE_PLAY ? sc->pclock : sc->rclock); > if (clock == NULL) { > printf("%s: can't get clock\n", DEVNAME(sc)); > goto failed; > @@ -3879,7 +3920,8 @@ uaudio_attach(struct device *parent, str > sc->names = NULL; > sc->alts = NULL; > sc->params_list = NULL; > - sc->clock = NULL; > + sc->rclock = NULL; > + sc->pclock = NULL; > sc->params = NULL; > sc->rate = 0; > sc->mode = 0; > @@ -4059,7 +4101,7 @@ uaudio_set_params(void *self, int setmod > best_mode = p; > > /* test if params match the requested rate */ > - if ((uaudio_getrates(sc, p) & (1 << rateindex)) == 0) > + if ((uaudio_getrates(sc, setmode, p) & (1 << rateindex)) == 0) > continue; > if (best_rate == NULL) > best_rate = p; > @@ -4105,7 +4147,7 @@ uaudio_set_params(void *self, int setmod > * Recalculate rate index, because the chosen parameters > * may not support the requested one > */ > - rateindex = uaudio_rates_indexof(uaudio_getrates(sc, p), rate); > + rateindex = uaudio_rates_indexof(uaudio_getrates(sc, setmode, p), rate); > if (rateindex < 0) > return ENOTTY; > >