[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Jagan Teki]

On Tue, Jul 3, 2018 at 11:26 PM, Marek Vasut <marex at denx.de> wrote:
> On 07/03/2018 07:09 PM, Jagan Teki wrote:
>> On Tue, Jul 3, 2018 at 10:14 PM, Marek Vasut <marex at denx.de> wrote:
>>> On 07/03/2018 05:22 PM, Andre Przywara wrote:
>>>> Hi,
>>>>
>>>> On 02/07/18 22:57, Marek Vasut wrote:
>>>>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>>>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>>>>> on the specific controller will disable. Usually this shutdown
>>>>>>>> happen during U-Boot proper handoff to Linux.
>>>>>>>
>>>>>>> No, 'usb reset' can be triggered by the user any time.
>>>>>>
>>>>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>>>>> case in question.
>>>>>
>>>>> No, that's not true. The USB controllers are torn down when starting the
>>>>> OS, which is a different thing from usb reset, which brings them back up
>>>>> and rescans the bus too.
>>>>>
>>>>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>>>>> the controller is unable to access the register space
>>>>>>>> so the Linux boot hangs at this place.
>>>>>>>
>>>>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>>>>
>>>>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>>>>> in the dts files.
>>>>>
>>>>> How did you reach that conclusion about the DTS files ? There is a bug
>>>>> in Linux, but it's likely in the driver which doesn't enable the clock
>>>>> before accessing the registers.
>>>>>
>>>>> But maybe the description here is completely confusing, since the output
>>>>> down below would indicate this hang is still in U-Boot.
>>>>
>>>> Yes, it is. There is no bug in Linux.
>>>>
>>>> U-Boot trips over its own feet when bringing down the USB controllers:
>>>> It shutdowns one part (EHCI or OHCI) on the register level, then turns
>>>> off the clocks and reset gates. But because they are shared between
>>>> controllers, this turns off the other controller as well. Then it tries
>>>> to bring down the the second part (OHCI or EHCI, respectively) on the
>>>> USB register level, which hangs, because the AHB clock is already off.
>>>> As this just happens quite late, it looks like U-Boot already said
>>>> goodbye, but it actually hasn't completely finished.
>>>> So Linux is completely fine and the bug is entirely in U-Boot.
>>>> My patch [1] tries to paper^Wsolve this in a different way, though it
>>>> isn't perfect either. I think there is a bit more to it than I assumed
>>>> yesterday, so I need to go back to the code later tonight to see what's
>>>> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
>>>> about EHCI and OHCI).
>>>
>>> Well, please keep poking.
>>>
>>> Maybe a dumb idea, but what about enabling the clock at the beginning of
>>> remove function to guarantee they are ON and then disabling the clock at
>>> the end of the function. Would that work maybe ? ie.
>>>
>>> .remove() {
>>>         clk_enable(..);
>>>         readl()/writel()/...
>>>         clk_disable(..);
>>> }
>>
>> I've verified clock bits before disabling, and bits are enabled as
>> usual. and even verified your idea of enabling before disable[2]
>
> Verified ... with what result ?

Same hang, with properly disabling clock during OHCI0 shutdown.

>
>> => usb reset
>> resetting USB...
>> ohci_usb_remove: input mask = 0x10000, input usb_clk_cfg = 0x30303
>> ohci_usb_remove: usb_clk_cfg = 0x20303
>> EHCI failed to shut down host controller.

<< hang here >>

>>
>> [2] https://paste.ubuntu.com/p/V9KKxMx6Cj/
>
> (no need to use pastebin to share 20 line patch, in fact it doesn't help
> the ML archives at all)

Sorry, copying same here.

--- a/drivers/usb/host/ohci-sunxi.c
+++ b/drivers/usb/host/ohci-sunxi.c
@@ -128,10 +128,14 @@ static int ohci_usb_remove(struct udevice *dev)
        if (ret)
                return ret;

+       printf("%s: mask = 0x%x, usb_clk_cfg = 0x%x\n", __func__,
+               priv->usb_gate_mask, readl(&priv->ccm->usb_clk_cfg));
+       setbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
        if (priv->cfg->has_reset)
                clrbits_le32(priv->reset0_cfg, priv->ahb_gate_mask);
-       clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
        clrbits_le32(&priv->ccm->ahb_gate0, priv->ahb_gate_mask);
+       clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
+       printf("%s: usb_clk_cfg = 0x%x\n", __func__,
readl(&priv->ccm->usb_clk_cfg));