| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <be8a0047-3a37-4e25-b4aa-ab34adff4418@notapiano>
Date: Mon, 29 Jul 2024 14:14:47 -0400
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Chen-Yu Tsai <wenst@...omium.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, kernel@...labora.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] arm64: dts: mediatek: mt8195: Add missing clock for
xhci1 controller
On Mon, Jul 29, 2024 at 02:34:27PM +0200, AngeloGioacchino Del Regno wrote:
> Il 29/07/24 12:48, Chen-Yu Tsai ha scritto:
> > On Mon, Jul 29, 2024 at 4:54 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@...labora.com> 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@...labora.com> 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@...labora.com> 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@...labora.com> 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@...labora.com> #KernelCI
> > > > > > > > > Closes: https://lore.kernel.org/all/9fce9838-ef87-4d1b-b3df-63e1ddb0ec51@notapiano/
> > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > > > > > > >
> > > > > > > > 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.
> >
>
> I can accept a patch for Tomato with a Fixes tag. Yes.
>
> > > 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.
Huh, that's surprising. FWIW I just reproduced with kernel next-20240729,
upstream defconfig (besides a CONFIG_USB_ONBOARD_DEV=n to be able to boot from
my USB drive), and the pcie1 node in mt8195-cherry.dtsi disabled. Hardware is
mt8195-cherry-tomato-r2.
Also, I just tested adding
diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
index fe5400e17b0f..a60c4d1419df 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
@@ -1401,6 +1401,7 @@ &xhci0 {
&xhci1 {
status = "okay";
+ mediatek,u3p-dis-msk = <0x1>;
rx-fifo-depth = <3072>;
vusb33-supply = <&mt6359_vusb_ldo_reg>;
vbus-supply = <&usb_vbus>;
And that fixed the issue as well. So as far as fixing the error on Tomato, this
patch works too, and makes more sense.
However I agree with Angelo that a board that does use USB3 on this controller
would still need the original patch adding the PCIE clock to work. But such a
board doesn't currently exist, does it? And we don't actually know if USB3
really works in that case. Or why this clock is needed. So there are a few
unknowns...
Anyway, I really don't know what option would be better. Just let me know if I
should resend a patch or CC me in any alternative patch.
Thanks,
Nícolas
> >
>
> Sometimes the noise actually opens some eyes around (be it mine or whoever else's),
> so as long as it is constructive, I don't see it as noise.
>
> In short: no worries! :-)
>
> > > 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?
> >
>
> Yeah, that'd make sense, though I'm not sure if the driver can cope with that: in
> that case, we'd obviously need "two" patches and not one :-)
>
> Cheers!
>
> >
> > 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@...90000 {
> > > > > > > > > <&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@...labora.com>
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > >
>
>
>
Powered by blists - more mailing lists