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

Tue Jul 3 21:22:23 UTC 2018

On 07/03/2018 03:52 PM, Tom Rini wrote:
> On Tue, Jul 03, 2018 at 06:06:37PM +0530, Jagan Teki wrote:
>> On Tue, Jul 3, 2018 at 3:10 AM, Tom Rini <trini at konsulko.com> 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.
>>>
>>>>> 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.
>>>
>>>>> This particular condition occur when we enable both the
>>>>> controllers, so this patch will disable OHCI1 and EHCI1 for
>>>>> bananapi-m64 and nanopi-a64 boards. It will re-enable the same
>>>>> once the issue got fixed.
>>>>>
>>>>> Log:
>>>>> => usb reset
>>>>> resetting USB...
>>>>>
>>>>> PHY0: mask = 0x101, usb_clk_cfg = 0x30202
>>>>> sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
>>>>> EHCI failed to shut down host controller.
>>>>> ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
>>>>> ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
>>>>> ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
>>>>>
>>>>> PHY1: mask = 0x202, usb_clk_cfg = 0x0
>>>>> ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
>>>>
>>>> Why is this usb reset so noisy ?
>>>
>>> ... I would assume additional debug messages.
>>>
>>>>> << hang here >>
>>>>
>>>> Please fix this properly, this patch is pure attempt to hide a bug.
>>>> NAK from me.
>>>
>>> Well, the point of this patch as Jagan says is to hide the bug for the
>>> release so that Linux can boot, which is an important case.
>>>
>>> Jagan, can you bisect down to when this started happening?  I assume
>>> it's a recent'ish thing.  Thanks!
>>
>> commit b62cdbddedc3 ("sunxi: clock: Fix EHCI and OHCI clocks on A64")
>> is effecting this change, but functionally this commit is proper but
>> handling clock directly in drivers look not effective particularly in
>> the case of A64 where sharing of clocks and gates between OHCI and
>> EHCI.
>>
>> Here are the possible changes to look at it.
>>
>> 1)  with this patch, disabling [oe]hci1 controllers on two boards (bpi
>> and nanopi):
>>       this change specific to two boards, rest of A64 boards are OK.
>>
>> 2)  turn-off gate and clocks for H3/H5/A64 from Andre [1]
>>       this would change all H3/H5/A64 shutdown behavior not to disable
>> gates and clocks during .remove
>>
>> 3)  turn-off clock only for A64
>>      this would fix, two board problems in 1) and also specific to A64.
>>
>> 4)  add counter mechanism to disable all clock at once, suggested by Marek
>>      either we can add counter mechanism to A64 or for all other SOC.
>> this need to test in all Allwinner boards if the counter is globally
>> managed in ohci-sunxi.c
>>
>> Again, there is an ongoing work for syncing all DT changes from Linux,
>> by Andre. 1) change will update later in the release. rest of 2), 3)
>> and 4) will make changes in driver [eo]hci-sunxi.c and this drivers
>> indeed remove once we have dm clock which would also planned for
>> upcoming releases.
>>
>> [1] https://patchwork.ozlabs.org/patch/938279/
> 
> Adding a few more people to the list.  It looks like b62cdbddedc3 was in
> response to fef73766d9ad.  So, this close to the release, what do we
> need to do to get things back to the state things were in for v2018.05?
> And then what are the combinations that aren't working and need to be
> addressed again in v2018.09 so that we can make forward progress?

Without going into much detail here, the clock dependency for two
companion controllers on those A64 is weird, and we hack it somehow
since we don't have a DM_CLK driver for sunxi (yet, see below). That
works for init, since we just set already set bits. For shutdown, we
*happen* to take down the AHB gate clocks and resets correctly, because
the order of shutdown matches the dependency (EHCI first, the OHCI). Not
sure if this is intentional, though. It's fragile, but works.

What we don't (and can't easily) model is another oddity: the USB clock
for [OE]HCI0 is actually the parent of [OE]HCI1. So if we need to bring
down *two* controllers and do it in the natural order, the second one is
dead before it can be disabled properly.

This was somewhat hidden before, since we had only one controller in
operation. b62cdbddedc3 somewhat fixed that, but the DT for the Pine64
(which was the test vehicle) has the controller still disabled. Enabling
this (what I did) or using other boards (BananaPi and NanoPi from Jagan)
triggered this bug though.
AFAICS this parent relation only affects the A64 with its two
controllers, so:
- We could just fix it for now by *not* disabling the USB clocks (and
only those, gates and reset still go down) at all. This is basically one
part of my patch from yesterday (the second part is not needed).
- We do more effort and skip disabling for OHCI0, but disable both
clocks for OHCI1. This still relies on the order, but would probably
shut down the controllers. A bit hackish to implement, though.
I will try the second solution now and let you decide on the list.

Long term:
The proper solution is a DT based DM_CLK driver, which models it like
Linux does. Work is underway[1], but this somewhat opens a can of worms
(needs DM support for UART, MMC, a DM_RESET driver, pinctrl ...), so it
takes a bit of time.
Fixing the current ad-hoc solution somewhat properly with ref-counting
is not easy (two driver files) and not really worthwhile, I believe, but
we can make it work like described above until the proper solution comes
into play.

Cheers,
Andre.