[U-Boot] [PATCH] sunxi: H3/H5/A64: OHCI: prevent turning off shared gates

[Andre Przywara]

Hi,

On 03/07/18 07:19, Jagan Teki wrote:
> On Tue, Jul 3, 2018 at 5:14 AM, Andre Przywara <andre.przywara at arm.com> wrote:
>> The USB host controllers on the H3, H5 and A64 have the oddity of
>> sharing some clock and reset gates, so both the OHCI and EHCI bits have
>> to be enabled to make only one of them working. We take care of this, and
>> initialisation works fine (due to setting already set bits).
>> However on shutdown we turn the clocks and reset gates off already when
>> deregistering one controller, so the other one is no longer functional.
>> In the result U-Boot complains just before launching the kernel and
>> then hangs.
>> Fix this by not turning off the clocks and resets on the OHCI side, so
>> that the EHCI controller has a chance to properly shut down.
>> This still isn't perfect, but at least prevents the hang.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>> Hi,
>>
>> commit b62cdbddedc3 ("sunxi: clock: Fix EHCI and OHCI clocks on A64")
>> introduced the proper reset and clock gates for the A64. While the patch
>> itself is correct, it broke Linux as soon as one actually enables USB0 in
>> the DT (in the moment we keep this "disabled" in U-Boot's DT version).
>>
>> I understand that this patch here is somewhat of a hack, but the proper
>> ref-counting is not easy to implement between the separate EHCI and OHCI
>> drivers. Those two files are doomed anyway, since driver model clocks
>> and reset drivers are already on the horizon:
>> https://github.com/apritzel/u-boot/commits/sunxi-dm-WIP
>> So lets fix this up for now so that we can use the Linux kernel DTs with
>> U-Boot itself.
>>
>> Cheers,
>> Andre.
>>
>>  drivers/usb/host/ohci-sunxi.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c
>> index 0ddbdbe460..42ffb6cbcb 100644
>> --- a/drivers/usb/host/ohci-sunxi.c
>> +++ b/drivers/usb/host/ohci-sunxi.c
>> @@ -128,10 +128,18 @@ static int ohci_usb_remove(struct udevice *dev)
>>         if (ret)
>>                 return ret;
>>
>> -       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);
>> +       /*
>> +        * For those SoCs that share the clock and reset gates with the EHCI
>> +        * controller, we should not turn them off here, to prevent the
>> +        * other one hanging (when the EHCI driver tries to shut itself down).
> 
> I've tried this "not to disable usb_clk_cfg" before sending "arm64:
> allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi" patch.
> But, I thought improper shutdown may effect other issue may be with
> Linux.

That's probably true. Did you try to prevent shutdown at all? Or just
for *one* of OHCI and EHCI?
My patch does it only for OHCI, so the EHCI still does the shutdown.
This somewhat relies on some shutdown ordering, though.

> Initially I found an issue of disabling OHCI1 gate clock during OHCI0
> shutdown. ohci driver is trying to clear BIT(16) for OHCI0, but it
> also clears BIT(17), which is for OHCI1. By adding proper shutdown
> support for musb and other things I came to a situation where all
> shutdown happen properly. But ofcourse I still see hang.

I believe with all those shared gates and dependencies we would need
refcounting per *bit*, for the gates, USB clocks and resets. As
mentioned, I am not sure it's worth the effort, if a DM clock driver
would solve this much more elegantly, and in one place.
I just need this fix here to make the kernel DTs work, so that I can
send those out.
After this I would revive the DM_CLK work.

Cheers,
Andre.

> => 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
> << hang  >>
> 
> Do you think this is still an issue with clock? considering proper
> shutdown sequence happened above.
>