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

[AngeloGioacchino Del Regno]

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?

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>