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

[Jagan Teki]

On Tue, Jul 3, 2018 at 3:41 PM, Andre Przywara <andre.przywara at arm.com> wrote:
> 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.

Yes I understand it, but here I think we can turn-off usb_clk_cfg no
need to turn-off gates as well.

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

This is what my next plan would be. Once we have dm_clk ready we can
use ehci/ohci generic drivers directly since we have PHY handling
driver.

thanks,
Jagan.