Re: [PATCH 2/2] usb: misc: onboard_hub: add support for XMOS XVF3500

From: Matthias Kaehlcke <mka@chromium.org>
To: Javier Carrasco <javier.carrasco@wolfvision.net>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-sound@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH 2/2] usb: misc: onboard_hub: add support for XMOS XVF3500
Date: Tue, 30 Jan 2024 16:11:29 +0000	[thread overview]
Message-ID: <ZbkfsVr-1pOTa1ic@google.com> (raw)
In-Reply-To: <20240130-onboard_xvf3500-v1-2-51b5398406cb@wolfvision.net>

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 = &microchip_usb424_data, },
>  	{ .compatible = "usb424,2514", .data = &microchip_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
> 