On Mon, Jul 29, 2024 at 4:54 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote: > > Il 29/07/24 10:07, Chen-Yu Tsai ha scritto: > > On Mon, Jul 29, 2024 at 3:59 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote: > >> > >> Il 26/07/24 17:11, Chen-Yu Tsai ha scritto: > >>> On Fri, Jul 26, 2024 at 8:11 PM AngeloGioacchino Del Regno > >>> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote: > >>>> > >>>> Il 25/07/24 12:34, Chen-Yu Tsai ha scritto: > >>>>> Hi, > >>>>> > >>>>> On Mon, Jul 22, 2024 at 11:27 PM Nícolas F. R. A. Prado > >>>>> <nfraprado@xxxxxxxxxxxxx> wrote: > >>>>>> > >>>>>> Currently if the xhci1 controller happens to probe before the pcie1 > >>>>>> controller then it fails with the following errors: > >>>>>> > >>>>>> xhci-mtk 11290000.usb: clocks are not stable (0x1003d0f) > >>>>>> xhci-mtk 11290000.usb: can't setup: -110 > >>>>>> xhci-mtk: probe of 11290000.usb failed with error -110 > >>>>>> > >>>>>> The issue has been tracked down to the CLK_INFRA_AO_PCIE_P1_TL_96M > >>>>>> clock, although exactly why this pcie clock is needed for the usb > >>>>>> controller is still unknown. Add the clock to the xhci1 controller so it > >>>>>> always probes successfully and use a placeholder clock name for it. > >>>>>> > >>>>>> Reported-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx> #KernelCI > >>>>>> Closes: https://lore.kernel.org/all/9fce9838-ef87-4d1b-b3df-63e1ddb0ec51@notapiano/ > >>>>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx> > >>>>> > >>>>> So I asked MediaTek about this, and it seems the correct thing to do is > >>>>> disable USB 3 on this host controller using the following snippet. The > >>>>> snippet is copy-pasted from our issue tracker and won't apply directly. > >>>>> > >>>>> This is also seen in mt8395-kontron-3-5-sbc-i1200.dts, on which xhci1 > >>>>> is used only for USB 2.0 on an M.2 slot. > >>>>> > >>>> > >>>> Uhm, okay, but why should USB3 be disabled on a controller that supports USB3? > >>>> > >>>> I agree about disabling it on specific boards that use only the USB 2.0 lines of > >>>> this controller, but doing that globally looks wrong... and looks like being a > >>>> workaround for an error that gets solved with adding a clock as well. > >>>> > >>>> In short, the question is: why would that be the correct thing to do? > >>> > >>> We can disable it in mt8195-cherry.dtsi then? > >> > >> That device does not use this controller, so yes we can disable it, but that still > >> doesn't resolve the issue pointed out by Nicolas...! > > > > No. I mean disable USB3 on this port. Also see the next paragraph. > > > > Yes, sorry I was meaning the same - but I effectively wrote "disable controller" > instead, my bad. > > >> Please note that the issue that he sees doesn't happen only on Tomato, but also on > >> other MediaTek MT8195/MT8395 boards - and applying this commit makes disabling the > >> controller, or disabling the USB 3 lines on the controller, kinda redundant, as > >> this will effectively fix probing it... but again, fixing the actual issue with > >> this controller is something that must be done. > > > > If those other boards use the XHCI1 USB3 lines for ... USB3, then the USB3 > > PHY should also be tied to XHCI1, right now due to the Cherry Chromebook > > design, only the USB2 PHY is tied to it. > > > > Yes, I am aware of that. > > >> Disabling the controller on Tomato is a different topic - here we are discussing > >> about fixing the issue, and that will happen, again, on any board that has this > >> controller enabled with USB3 lines. :-) > >> > >> So, unless there is any specific reason for which applying this commit is a bad > >> idea, or any alternative fix to this that is better than the proposed one, and > >> not a workaround... I'm applying this one. > > > > Didn't I just relay what MediaTek says is the correct fix? Disable USB3 > > for this port on devices where the serial pairs are used for PCIe instead > > of USB3. > > > > I think there must've been some misunderstanding here. > > Yes you did relay what MediaTek is the correct fix, and I agree that the USB3 must > be disabled on devices where those serial pairs are used for PCIe instead of USB, > or on devices where those are completely unused. OK. I will send a patch for Tomato as you asked. > This, though, will fix the issue only on those devices (because we are disabling > those lines entirely, so depending on how we see it, this might not be a fix but > rather a workaround). I would say that is a more accurate description of the hardware, so a fix. > If we don't apply this fix, any board that uses those pairs for USB 3 instead will > still show the same "clocks are not stable" error, leaving them with a broken port. > > And I believe that because the clocks are not routed externally but rather are > internal to the SoC, so, if INFRA_AO_PCIE_P1_TL_96M is necessary for that USB 3 > port to work, a board that intends to use those pairs for USB3 would still need > this exact clock to actually get that port to work. I couldn't reproduce the issue by disabling pcie1 as Nicolas mentioned. I don't have any more to add to this though. Sorry for the noise. > As for Tomato itself - I agree that we must add the u3p-dis-msk=0x1 flag, yes, > and we will, but I'm purely talking about - again - an eventual board that would > not have that property because USB3 is exposed/used for real. I think it would make more sense to fix the `phys = ` statement in mt8195.dtsi to list both the USB2 and USB3 PHYs. At the board level, for boards only using USB2, we would have the overriding `phys = ` statement alongside the `mediatek,u3p-dis-mask` property. Does that make sense to you? Thanks ChenYu > Cheers, > Angelo > > > > > Regards > > ChenYu > > > >> Cheers, > >> Angelo > >> > >>> > >>> ChenYu > >>> > >>>> Cheers, > >>>> Angelo > >>>> > >>>>> > >>>>> ChenYu > >>>>> > >>>>> index 8b7307cdefc6..2dac9f706a58 > >>>>> --- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi > >>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi > >>>>> @@ -1447,6 +1447,7 @@ > >>>>> "xhci_ck"; > >>>>> mediatek,syscon-wakeup = <&pericfg 0x400 104>; > >>>>> wakeup-source; > >>>>> + mediatek,u3p-dis-msk = <0x1>; > >>>>> status = "disabled"; > >>>>> }; > >>>>> > >>>>>> --- > >>>>>> arch/arm64/boot/dts/mediatek/mt8195.dtsi | 10 ++++++++-- > >>>>>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi > >>>>>> index 2ee45752583c..cc5169871f1c 100644 > >>>>>> --- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi > >>>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi > >>>>>> @@ -1453,9 +1453,15 @@ xhci1: usb@11290000 { > >>>>>> <&topckgen CLK_TOP_SSUSB_P1_REF>, > >>>>>> <&apmixedsys CLK_APMIXED_USB1PLL>, > >>>>>> <&clk26m>, > >>>>>> - <&pericfg_ao CLK_PERI_AO_SSUSB_1P_XHCI>; > >>>>>> + <&pericfg_ao CLK_PERI_AO_SSUSB_1P_XHCI>, > >>>>>> + /* > >>>>>> + * This clock is required due to a hardware > >>>>>> + * bug. The 'frmcnt_ck' clock name is used as a > >>>>>> + * placeholder. > >>>>>> + */ > >>>>>> + <&infracfg_ao CLK_INFRA_AO_PCIE_P1_TL_96M>; > >>>>>> clock-names = "sys_ck", "ref_ck", "mcu_ck", "dma_ck", > >>>>>> - "xhci_ck"; > >>>>>> + "xhci_ck", "frmcnt_ck"; > >>>>>> mediatek,syscon-wakeup = <&pericfg 0x400 104>; > >>>>>> wakeup-source; > >>>>>> status = "disabled"; > >>>>>> > >>>>>> --- > >>>>>> base-commit: dee7f101b64219f512bb2f842227bd04c14efe30 > >>>>>> change-id: 20240722-usb-1129-probe-pci-clk-fix-ef8646f46aac > >>>>>> > >>>>>> Best regards, > >>>>>> -- > >>>>>> Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx> > >>>>>> > >>>>>> > >>>> > >>>> > >>>> > >> >
