[PATCH] arm64: dts: mediatek: mt8195: Add missing clock for xhci1 controller
Chen-Yu Tsai
wenst at chromium.org
Mon Jul 29 03:48:05 PDT 2024
On Mon, Jul 29, 2024 at 4:54 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno at collabora.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 at collabora.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 at collabora.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 at collabora.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 at collabora.com> #KernelCI
> >>>>>> Closes: https://lore.kernel.org/all/9fce9838-ef87-4d1b-b3df-63e1ddb0ec51@notapiano/
> >>>>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.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.
> 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 at 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 at collabora.com>
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>>>
> >>
>
More information about the linux-arm-kernel
mailing list