Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231005113624.8329-1-piyush.mehta@amd.com>
In-Reply-To: <20231005113624.8329-1-piyush.mehta@amd.com>
From:   Peter Geis <pgwipeout@gmail.com>
Date:   Thu, 5 Oct 2023 13:23:05 -0400
Message-ID: <CAMdYzYq87xuZ8fTrGhcV5sdCyuGFGyEAPwJOJSoXE2O_pVZUag@mail.gmail.com>
Subject: Re: [PATCH V2] usb: dwc3: core: disable 3.0 clock when operating in
 2.0 device mode
To:     Piyush Mehta <piyush.mehta@amd.com>
Cc:     gregkh@linuxfoundation.org, michal.simek@amd.com,
        Thinh.Nguyen@synopsys.com, robh@kernel.org,
        herve.codina@bootlin.com, yangbin@rock-chips.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        git@amd.com, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 5, 2023 at 7:36=E2=80=AFAM Piyush Mehta <piyush.mehta@amd.com> =
wrote:
>
> The GUCTL1.DEV_FORCE_20_CLK_FOR_30_CLK bit enable the feature of internal
> 2.0(utmi/ulpi) clock to be routed as the 3.0 (pipe) clock. This feature i=
s
> applicable when core is operating in 2.0 device mode.
>
> When this bit is set in host mode and core is in 2.0 device mode (maximum
> speed =3D high-speed) then usb super speed devices not detected on host.
>
> To address the above issue added usb device mode conditional check.

Good Afternoon,

This will outright break the patch it attempts to fix. This was
originally done to work around hardware where a dwc3 core was attached
only to a 2.0 phy and lacked 3.0 clocks. While I empathize with the
intentions of the hardware designers, it works perfectly well for host
mode as well as device mode. As I didn't have access to the register
mappings, I was unaware of the original intention of this register
beyond what was in the downstream commit.

If this is affecting hardware in the wild, I would surmise the
firmware for that hardware is incorrectly reporting the dwc3
implementation max speed as USB_SPEED_HIGH or USB_SPEED_FULL when it
should be USB_SPEED_SUPER or USB_SPEED_SUPER_PLUS. If this is the
case, we have fixed this issue in other ways both in newer kernels and
in firmware so the risk of this affecting users is slim. If this is
just to make the software behave the way the hardware designers
originally intended, then I have to respectfully nack this patch.

Very Respectfully,
Peter Geis

>
> Cc: stable@vger.kernel.org
> Fixes: 62b20e6e0dde ("usb: dwc3: core: do not use 3.0 clock when operatin=
g in 2.0 mode")
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> ---
> DWC3 Register Map Link:
> https://docs.xilinx.com/r/en-US/ug1087-zynq-ultrascale-registers/GUCTL1-U=
SB3_XHCI-Register
> Register Name   GUCTL1
> Bit: 26
> Bit Name: DEV_FORCE_20_CLK_FOR_30_CLK
>
> Change in V2:
> - Added CC stable kernel email.
>
> Link: https://lore.kernel.org/all/20231005102725.8458-1-piyush.mehta@amd.=
com/
> ---
>  drivers/usb/dwc3/core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9c6bf054f15d..0cf1fe60628b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1202,6 +1202,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>                         reg |=3D DWC3_GUCTL1_PARKMODE_DISABLE_HS;
>
>                 if (DWC3_VER_IS_WITHIN(DWC3, 290A, ANY) &&
> +                   (dwc->dr_mode =3D=3D USB_DR_MODE_PERIPHERAL) &&
>                     (dwc->maximum_speed =3D=3D USB_SPEED_HIGH ||
>                      dwc->maximum_speed =3D=3D USB_SPEED_FULL))
>                         reg |=3D DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK;
> --
> 2.17.1
>