[U-Boot] [PATCH] sunxi: A64: OHCI: prevent turning off shared USB clock

[Andre Przywara]

Hi,

On 04/07/18 12:10, Marek Vasut wrote:
> On 07/04/2018 12:03 PM, Andre Przywara wrote:
>> Hi,
>>
>> On 04/07/18 08:14, Marek Vasut wrote:
>>> On 07/04/2018 02:05 AM, Andre Przywara wrote:
>>>> On the A64 the clock for the first USB controller is actually the parent
>>>> of the clock for the second controller, so turning them off in that order
>>>> makes the system hang.
>>>> Fix this by *not* turning off any clock for OHCI0, but both clocks when
>>>> OHCI1 is brought down.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>>>> ---
>>>> Hi,
>>>>
>>>> this is a new approach to fix the USB hang we see with mainline U-Boot.
>>>> Compared to the previous patch it just deals with the USB clock (the AHB
>>>> gate was a red herring), and it eventually turns both clocks off instead
>>>> of leaving them running. Please have a test on A64 boards!
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>>  drivers/usb/host/ohci-sunxi.c | 13 ++++++++++++-
>>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c
>>>> index 0ddbdbe460..8f108b48a8 100644
>>>> --- a/drivers/usb/host/ohci-sunxi.c
>>>> +++ b/drivers/usb/host/ohci-sunxi.c
>>>> @@ -114,6 +114,7 @@ no_phy:
>>>>  static int ohci_usb_remove(struct udevice *dev)
>>>>  {
>>>>  	struct ohci_sunxi_priv *priv = dev_get_priv(dev);
>>>> +	fdt_addr_t base_addr = devfdt_get_addr(dev);
>>>>  	int ret;
>>>>  
>>>>  	if (generic_phy_valid(&priv->phy)) {
>>>> @@ -130,7 +131,17 @@ static int ohci_usb_remove(struct udevice *dev)
>>>>  
>>>>  	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);
>>>> +	/*
>>>> +	 * On the A64 CLK_USB_OHCI0 is the parent of CLK_USB_OHCI1, so
>>>> +	 * we have to bring down none for OHCI0, but both for OHCI1.
>>>> +	 */
>>>> +	if (!priv->cfg->extra_usb_gate_mask || base_addr >= SUNXI_USB2_BASE) {
>>>> +		u32 usb_gate_mask = priv->usb_gate_mask;
>>>> +
>>>> +		usb_gate_mask |= priv->cfg->extra_usb_gate_mask;
>>>> +		clrbits_le32(&priv->ccm->usb_clk_cfg, usb_gate_mask);
>>>> +	}
>>>> +
>>>>  	clrbits_le32(&priv->ccm->ahb_gate0, priv->ahb_gate_mask);
>>>>  
>>>>  	return 0;
>>>>
>>>
>>> What about boards which only enable OHCI0 and do not enable OHCI1 ?
>>
>> Ah, c'mon, thanks for spoiling my short patch ;-)
>> The A64 has just two USB controllers, on dedicated pins, so mostly you
>> want both of them. But I see your point, the clocks would stay on if
>> only the first controller is ever dealt with. Maybe we can live with
>> that, at least for the next release?
>> Or do you have a clever idea how to deal with that? I think it's hard to
>> determine how many USB controller we have enabled?
> 
> I'd prefer approach which works in all cases.
> 
> Can't you check if both controllers are enabled by traversing the DT ?

Nah, that sound's awful. But I could count the number of controllers
during initialisation and store this in a static variable. That might
also help to avoid the ugly comparison against SUNXI_USB2_BASE.

Cheers,
Andre.