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

[Marek Vasut]

On 07/03/2018 08:02 PM, Jagan Teki wrote:
> 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.

So the clock were enabled and yet there was a hang ? Why ?

I was under the impression that disabling clock was the problem, maybe
that is not the case ?

Are you sure you enabled all the clock ?

>>> => 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));

-- 
Best regards,
Marek Vasut