| Message ID | 20240130-onboard_xvf3500-v1-2-51b5398406cb@wolfvision.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: misc: onboard_usb_hub: add support for XMOS XVF3500 | expand |
Hi Javier, I understand your motivation for using the onboard_usb_hub driver for powering up a non-hub device, it feels a bit hacky to use it as is though. Re-using the driver might be the right thing to do, but then it should probably be renamed to onboard_usb_dev (or similar) and do the hub specific bits as special case. Greg, do you have any thoughts on this? Also there is an implication that might not matter for the XVF3500, but could for other non-hub devices with wakeup support: by default the driver powers the hub/device down during suspend, unless (in case of a hub) wakeup capable devices are connected. This behavior can be changed through sysfs, but the default would still be unexpected. In hindsight I think the default should have been to keep the hub powered. Not sure if it's an option to change the default at this point, since folks might rely on it (I know systems that do, but those could be adapted). We could possibly change the behavior for non-hub devices and keep the device powered if wakeup is supported and enabled m. On Tue, Jan 30, 2024 at 01:26:57PM +0100, Javier Carrasco wrote: > The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit > multicore controller for voice processing. > > This device requires a specific power sequence, which consists of > enabling the regulators that control the 3V3 and 1V0 device supplies, > and a reset de-assertion after a delay of at least 100ns. Such power > sequence is already supported by the onboard_hub driver, and it can be > reused for non-hub USB devices as well. > > Once in normal operation, the XVF3500 registers itself as a USB device, > and it does not require any device-specific operations in the driver. > > [1] https://www.xmos.com/xvf3500/ > > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > --- > drivers/usb/misc/onboard_usb_hub.c | 2 ++ > drivers/usb/misc/onboard_usb_hub.h | 6 ++++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > index 0dd2b032c90b..f16ea3053f84 100644 > --- a/drivers/usb/misc/onboard_usb_hub.c > +++ b/drivers/usb/misc/onboard_usb_hub.c > @@ -366,6 +366,7 @@ static struct platform_driver onboard_hub_driver = { > #define VENDOR_ID_REALTEK 0x0bda > #define VENDOR_ID_TI 0x0451 > #define VENDOR_ID_VIA 0x2109 > +#define VENDOR_ID_XMOS 0x20B1 > > /* > * Returns the onboard_hub platform device that is associated with the USB > @@ -458,6 +459,7 @@ static const struct usb_device_id onboard_hub_id_table[] = { > { USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */ > { USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */ > { USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */ > + { USB_DEVICE(VENDOR_ID_XMOS, 0x0013) }, /* XMOS XVF3500 */ > {} > }; > MODULE_DEVICE_TABLE(usb, onboard_hub_id_table); > diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h > index f360d5cf8d8a..1809c0923b98 100644 > --- a/drivers/usb/misc/onboard_usb_hub.h > +++ b/drivers/usb/misc/onboard_usb_hub.h > @@ -56,6 +56,11 @@ static const struct onboard_hub_pdata vialab_vl817_data = { > .num_supplies = 1, > }; > > +static const struct onboard_hub_pdata xmos_xvf3500_data = { > + .reset_us = 1, > + .num_supplies = 2, > +}; > + > static const struct of_device_id onboard_hub_match[] = { > { .compatible = "usb424,2412", .data = µchip_usb424_data, }, > { .compatible = "usb424,2514", .data = µchip_usb424_data, }, > @@ -77,6 +82,7 @@ static const struct of_device_id onboard_hub_match[] = { > { .compatible = "usbbda,5414", .data = &realtek_rts5411_data, }, > { .compatible = "usb2109,817", .data = &vialab_vl817_data, }, > { .compatible = "usb2109,2817", .data = &vialab_vl817_data, }, > + { .compatible = "usb20b1,0013", .data = &xmos_xvf3500_data, }, > {} > }; > > > -- > 2.39.2 >
On Tue, Jan 30, 2024 at 04:11:29PM +0000, Matthias Kaehlcke wrote: > Hi Javier, > > I understand your motivation for using the onboard_usb_hub driver > for powering up a non-hub device, it feels a bit hacky to use it > as is though. Re-using the driver might be the right thing to do, > but then it should probably be renamed to onboard_usb_dev (or > similar) and do the hub specific bits as special case. > > Greg, do you have any thoughts on this? Yeah, this worries me, adding non-hub support to this driver feels odd. Why can't this all just be done in an individual driver for this device itself? Javier, what kernel driver supports this device to communicate with it? Having 2 different drivers poke the same device is just going to cause problems in the end, as there is no way to communicate between the two, right? thanks, greg k-h
On Tue, Jan 30, 2024 at 08:19:40AM -0800, Greg Kroah-Hartman wrote: > On Tue, Jan 30, 2024 at 04:11:29PM +0000, Matthias Kaehlcke wrote: > > Hi Javier, > > > > I understand your motivation for using the onboard_usb_hub driver > > for powering up a non-hub device, it feels a bit hacky to use it > > as is though. Re-using the driver might be the right thing to do, > > but then it should probably be renamed to onboard_usb_dev (or > > similar) and do the hub specific bits as special case. > > > > Greg, do you have any thoughts on this? > > Yeah, this worries me, adding non-hub support to this driver feels odd. It is odd as long as this driver claims to be hub-specific, but truth is that the hub-specific bits are a small part of the driver, I think it might be worthwhile to consider adapting the driver to other devices if there is no clear better solution. A possible alternative could be a separate onboard_usb_dev driver for non-hub devices, with a similar structure as the onboard_hub driver, but without the hub-specific bits. > Why can't this all just be done in an individual driver for this device > itself? I suppose the reason is the good old chicken-egg situation that the (USB) driver is only instantiated after the device has bee powered on, which is what the driver is supposed to take care of. For the onboard_hub driver this was solved by having a platform driver that is instantiated by the parent hub if the onboard hub has a device tree entry. Probably something similar would be needed for an individual driver, and the generic hub driver would have to call the equivalent of onboard_hub_create_pdevs() for all drivers of this type (or a wrapper that does this). m.
On 30.01.24 18:26, Matthias Kaehlcke wrote: > On Tue, Jan 30, 2024 at 08:19:40AM -0800, Greg Kroah-Hartman wrote: >> On Tue, Jan 30, 2024 at 04:11:29PM +0000, Matthias Kaehlcke wrote: >>> Hi Javier, >>> >>> I understand your motivation for using the onboard_usb_hub driver >>> for powering up a non-hub device, it feels a bit hacky to use it >>> as is though. Re-using the driver might be the right thing to do, >>> but then it should probably be renamed to onboard_usb_dev (or >>> similar) and do the hub specific bits as special case. >>> >>> Greg, do you have any thoughts on this? >> >> Yeah, this worries me, adding non-hub support to this driver feels odd. > > It is odd as long as this driver claims to be hub-specific, but truth > is that the hub-specific bits are a small part of the driver, I think > it might be worthwhile to consider adapting the driver to other devices > if there is no clear better solution. The driver was programmed for hubs from the beginning, and that makes non-hub additions look weird, but I wonder if the other way round would have been seen less odd: a generic onboard_usb_dev that ended up being extended to support hub-specific capabilities. As Matthias pointed out, most of the driver is generic, but I have to admit that adding a non-hub device directly (without any renaming) does not look 100% right. > A possible alternative could be a separate onboard_usb_dev driver for > non-hub devices, with a similar structure as the onboard_hub driver, > but without the hub-specific bits. > I was thinking about that possibility too, but the new driver would be a renamed onboard_usb_hub with less functionality. Its only added value would be that it keeps onboard_usb_hub only for hubs. But if that is exactly the goal, I have nothing against an onboard_usb_dev driver for the next patch version. Adding a device-specific driver for such a generic power sequence and resume/suspend support might not be the best approach, though, especially if more USB devices with similar needs arise. >> Why can't this all just be done in an individual driver for this device >> itself? > > I suppose the reason is the good old chicken-egg situation that the (USB) > driver is only instantiated after the device has bee powered on, which is > what the driver is supposed to take care of. For the onboard_hub driver > this was solved by having a platform driver that is instantiated by the > parent hub if the onboard hub has a device tree entry. Probably something > similar would be needed for an individual driver, and the generic hub > driver would have to call the equivalent of onboard_hub_create_pdevs() > for all drivers of this type (or a wrapper that does this). > > m. The XVF3500 does not have kernel support so far, and once it reaches normal operation and registers itself as a USB device, the communication is achieved in userspace. What this device needs from a driver is only the power sequence and eventually the resume/suspend support, which makes it a good candidate for generic implementations. Best regards, Javier Carrasco
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c index 0dd2b032c90b..f16ea3053f84 100644 --- a/drivers/usb/misc/onboard_usb_hub.c +++ b/drivers/usb/misc/onboard_usb_hub.c @@ -366,6 +366,7 @@ static struct platform_driver onboard_hub_driver = { #define VENDOR_ID_REALTEK 0x0bda #define VENDOR_ID_TI 0x0451 #define VENDOR_ID_VIA 0x2109 +#define VENDOR_ID_XMOS 0x20B1 /* * Returns the onboard_hub platform device that is associated with the USB @@ -458,6 +459,7 @@ static const struct usb_device_id onboard_hub_id_table[] = { { USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */ { USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */ { USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */ + { USB_DEVICE(VENDOR_ID_XMOS, 0x0013) }, /* XMOS XVF3500 */ {} }; MODULE_DEVICE_TABLE(usb, onboard_hub_id_table); diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h index f360d5cf8d8a..1809c0923b98 100644 --- a/drivers/usb/misc/onboard_usb_hub.h +++ b/drivers/usb/misc/onboard_usb_hub.h @@ -56,6 +56,11 @@ static const struct onboard_hub_pdata vialab_vl817_data = { .num_supplies = 1, }; +static const struct onboard_hub_pdata xmos_xvf3500_data = { + .reset_us = 1, + .num_supplies = 2, +}; + static const struct of_device_id onboard_hub_match[] = { { .compatible = "usb424,2412", .data = µchip_usb424_data, }, { .compatible = "usb424,2514", .data = µchip_usb424_data, }, @@ -77,6 +82,7 @@ static const struct of_device_id onboard_hub_match[] = { { .compatible = "usbbda,5414", .data = &realtek_rts5411_data, }, { .compatible = "usb2109,817", .data = &vialab_vl817_data, }, { .compatible = "usb2109,2817", .data = &vialab_vl817_data, }, + { .compatible = "usb20b1,0013", .data = &xmos_xvf3500_data, }, {} };
The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit multicore controller for voice processing. This device requires a specific power sequence, which consists of enabling the regulators that control the 3V3 and 1V0 device supplies, and a reset de-assertion after a delay of at least 100ns. Such power sequence is already supported by the onboard_hub driver, and it can be reused for non-hub USB devices as well. Once in normal operation, the XVF3500 registers itself as a USB device, and it does not require any device-specific operations in the driver. [1] https://www.xmos.com/xvf3500/ Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> --- drivers/usb/misc/onboard_usb_hub.c | 2 ++ drivers/usb/misc/onboard_usb_hub.h | 6 ++++++ 2 files changed, 8 insertions(+)