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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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@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@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@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?

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@collabora.com>
>>
>>