Message-ID: <c6400b3c-109b-4f40-9163-88ae2b53c73b@collabora.com>
Date: Tue, 30 Jul 2024 11:09:59 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>,
 Nícolas F. R. A. Prado <nfraprado@...labora.com>
Cc: 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, Macpaul Lin <macpaul.lin@...iatek.com>
Subject: Re: [PATCH] arm64: dts: mediatek: mt8195: Add missing clock for xhci1
 controller

Il 30/07/24 06:17, Chen-Yu Tsai ha scritto:
> On Tue, Jul 30, 2024 at 2:14 AM Nícolas F. R. A. Prado
> <nfraprado@...labora.com> wrote:
>>
>> 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.
> 
> Could you also try adding the USB3 PHY instead of disabling U3 on the
> controller? As:
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> index 2ee45752583c..61b3c202a8cd 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> @@ -1444,7 +1444,7 @@ xhci1: usb@...90000 {
>                                <0 0x11293e00 0 0x0100>;
>                          reg-names = "mac", "ippc";
>                          interrupts = <GIC_SPI 530 IRQ_TYPE_LEVEL_HIGH 0>;
> -                       phys = <&u2port1 PHY_TYPE_USB2>;
> +                       phys = <&u2port1 PHY_TYPE_USB2>, <&u3port1
> PHY_TYPE_USB3>;
>                          assigned-clocks = <&topckgen CLK_TOP_USB_TOP_1P>,
>                                            <&topckgen CLK_TOP_SSUSB_XHCI_1P>;
>                          assigned-clock-parents = <&topckgen
> CLK_TOP_UNIVPLL_D5_D4>,
> 
> I wanted to test this, but I couldn't actually reproduce the error.
> 
>> 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...
> 
> MacPaul seems to have one. He mentioned to me that he doesn't need the patch
> adding the PCIE clock, but needs "mediatek,force-mode" instead.
> 
> Looking at the comments around "mediatek,force-mode" in the driver, it
> seems that the PHY defaults to PCIe mode. A combination of not initializing
> the PHY to USB3 and turning on the PCIe related clock might be what is
> allowing the PHY to respond to the controller? This is just a guess though.
> 

I was just about to pick this patch, but now with this comment making a lot of
sense... I'm not sure that picking this is the right thing to do anymore :-)

Nicolas, can you please check if "mediatek,force-mode" makes it to work without
said clock?

If it works, this means that the PHY driver (or something else) is, and has always
been, at fault - so there's a bug elsewhere and must be fixed.

Cheers,
Angelo

>> 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.
> 
> I'll send the patches to fix up pure USB2 usage. Given we have conflicting
> reports on whether the PCIe clock is needed, I ask that you try the PHY
> assignment change first.
> 
> 
> Thanks
> ChenYu
> 
>> 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