Re: [PATCH v2] usb: dwc3: pci: skip BYT GPIO lookup table for hardwired phy

[Hans de Goede]

Hi,

On 7/26/23 20:45, Gratian Crisan wrote:
> Hardware based on the Bay Trail / BYT SoCs require an external ULPI phy for
> USB device-mode. The phy chip usually has its 'reset' and 'chip select'
> lines connected to GPIOs described by ACPI fwnodes in the DSDT table.
> 
> Because of hardware with missing ACPI resources for the 'reset' and 'chip
> select' GPIOs commit 5741022cbdf3 ("usb: dwc3: pci: Add GPIO lookup table
> on platforms without ACPI GPIO resources") introduced a fallback
> gpiod_lookup_table with hard-coded mappings for Bay Trail devices.
> 
> However there are existing Bay Trail based devices, like the National
> Instruments cRIO-903x series, where the phy chip has its 'reset' and
> 'chip-select' lines always asserted in hardware via resistor pull-ups. On
> this hardware the phy chip is always enabled and the ACPI dsdt table is
> missing information not only for the 'chip-select' and 'reset' lines but
> also for the BYT GPIO controller itself "INT33FC".
> 
> With the introduction of the gpiod_lookup_table initializing the USB
> device-mode on these hardware now errors out. The error comes from the
> gpiod_get_optional() calls in dwc3_pci_quirks() which will now return an
> -ENOENT error due to the missing ACPI entry for the INT33FC gpio controller
> used in the aforementioned table.
> 
> This hardware used to work before because gpiod_get_optional() will return
> NULL instead of -ENOENT if no GPIO has been assigned to the requested
> function. The dwc3_pci_quirks() code for setting the 'cs' and 'reset' GPIOs
> was then skipped (due to the NULL return). This is the correct behavior in
> cases where the phy chip is hardwired and there are no GPIOs to control.
> 
> Since the gpiod_lookup_table relies on the presence of INT33FC fwnode
> in ACPI tables only add the table if we know the entry for the INT33FC
> gpio controller is present. This allows Bay Trail based devices with
> hardwired dwc3 ULPI phys to continue working.
> 
> Fixes: 5741022cbdf3 ("usb: dwc3: pci: Add GPIO lookup table on platforms without ACPI GPIO resources")
> Signed-off-by: Gratian Crisan <gratian.crisan@xxxxxx>
> ---
> V1 -> V2: Remove redundant NULL check

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Regards,

Hans




> 
>  drivers/usb/dwc3/dwc3-pci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index 44a04c9b2073..6604845c397c 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -233,10 +233,12 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc,
>  
>  			/*
>  			 * A lot of BYT devices lack ACPI resource entries for
> -			 * the GPIOs, add a fallback mapping to the reference
> +			 * the GPIOs. If the ACPI entry for the GPIO controller
> +			 * is present add a fallback mapping to the reference
>  			 * design GPIOs which all boards seem to use.
>  			 */
> -			gpiod_add_lookup_table(&platform_bytcr_gpios);
> +			if (acpi_dev_present("INT33FC", NULL, -1))
> +				gpiod_add_lookup_table(&platform_bytcr_gpios);
>  
>  			/*
>  			 * These GPIOs will turn on the USB2 PHY. Note that we have to