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

[Jagan Teki]

On Tue, Jul 3, 2018 at 11:46 PM, Marek Vasut <marex at denx.de> wrote:
> 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 ?

ie what I really not understand.

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

Yes I'm sure about it. usb_clk_cfg = 0x30303 is final out of clock
init. It iterated twice during initialization sequence.
- 1st iteration bit 0, 8 were enabled by PHY driver and bit 16 enabled by OHCI0
- 2nd iteration bit 1, 9 were enabled by PHY driver and bit 17 enabled by OHCI1