Re: [PATCH] arm64: dts: mediatek: mt8195: Add missing clock for xhci1 controller

[AngeloGioacchino Del Regno]

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...!

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.

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.

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>