Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b82bec27-f750-19e7-f133-3ea615d2a438@redhat.com>
Date:   Wed, 26 Jul 2023 21:49:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v2] usb: dwc3: pci: skip BYT GPIO lookup table for
 hardwired phy
To:     Gratian Crisan <gratian.crisan@ni.com>, Thinh.Nguyen@synopsys.com,
        gregkh@linuxfoundation.org, felipe.balbi@linux.intel.com
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        gratian@gmail.com
References: <20230726184555.218091-2-gratian.crisan@ni.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230726184555.218091-2-gratian.crisan@ni.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@ni.com>
> ---
> V1 -> V2: Remove redundant NULL check

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

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