RE: commit 3497b9a5 (usb: dwc3: add power down scale setting) breaks imx8mp

From: Jun Li <jun.li@nxp.com>
To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: RE: commit 3497b9a5 (usb: dwc3: add power down scale setting) breaks imx8mp
Date: Fri, 6 Jan 2023 13:35:23 +0000	[thread overview]
Message-ID: <PA4PR04MB9640E88C1F4EF722682608D689FB9@PA4PR04MB9640.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <5ae757aa-5b0e-be81-e87c-134e2ba5205d@prevas.dk>


> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Sent: Friday, January 6, 2023 7:55 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jun Li
> <jun.li@nxp.com>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Subject: commit 3497b9a5 (usb: dwc3: add power down scale setting) breaks
> imx8mp
> 
> We have an imx8mp board with a lan7801 usb ethernet chip hardwired on the
> PCB, which is used as the host port for a Microchip KSZ9567 switch.
> 
> While trying to update the kernel to 6.1.y, I found something quite
> weird: When the switch was being probed for the second time (the first ends
> with a standard -EPROBE_DEFER), the board would spontaneously reset.
> 
> Now when I disable the switch driver in .config just to see how far I could
> otherwise get, the lan7801 device didn't appear until about 47 seconds after
> boot. Bisecting unambiguously points at 3497b9a5, and digging in, it's pretty
> obvious why that is bogus at least for imx8mp.
> 
> The .dtsi file lists IMX8MP_CLK_USB_ROOT as the "suspend" clk, and
> clk_get_rate() of that returns 500000000 ; divided by 16000 that's 31250,
> which certainly doesn't fit in the 13-bit field GCTL_PWRDNSCALE.
> But I assume the .dtsi file is wrong, because imx8mq.dtsi has
> 74bd5951dd3 (arm64: dts: imx8mq: correct usb controller clocks), and it seems
> likely from the commit log of 3497b9a5 that it was at least tested on imx8mq.
> 
> Now I have no idea if the right clock for imx8mp is also some 32kHz clk,
> but it would certainly make sense; unlike what the reference manual claims,
> it seems that the reset value of the GCTL register is 0x00112004, amounting
> to a pwrdwnscale value of 0x00100000>>19 == 2 == 32kHz/16kHz, and that could
> explain why things worked just fine without 3497b9a5.
> 
> Li Jun, please either revert 3497b9a5 or figure out if imx8mp.dtsi is broken
> and needs a fix similar to 74bd5951dd3.

iMX8MP suspend clock(with name IMX8MP_CLK_USB_ROOT) was 32K when 3497b9a5
was merged, a later iMX8MP clock driver patch change the clock to be root
clock gate(actually this is a shared clock gate for both suspend and root
clock), so break the iMX8MP USB as you are seeing, a fix patch set already
addressed this issue, please check and apply below 3 patches:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/dt-bindings/clock/imx8mp-clock.h?id=5c1f7f1090947d494c30042123e0ec846f696336
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clk/imx/clk-imx8mp.c?id=ed1f4ccfe947a3e1018a3bd7325134574c7ff9b3
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/freescale/imx8mp.dtsi?id=8a1ed98fe0f2e7669f0409de0f46f317b275f8be

Li Jun 
> 
> Rasmus