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

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

[Andre Przywara]

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@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;
-- 
2.14.4

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

[Jagan Teki]

On Wed, Jul 4, 2018 at 5:35 AM, Andre Przywara <andre.przywara@arm.com> 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@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);

Reviewed-by: Jagan Teki <jagan@openedev.com>

Tested on
- A64=> BPI-M64, Nanopi-A64

=> usb reset
resetting USB...
ohci_usb_remove: Input: mask = 0x10000, usb_clk_cfg = 0x30303
ohci_usb_remove: Output: usb_clk_cfg = 0x30303
EHCI failed to shut down host controller.
ohci_usb_remove: Input: mask = 0x20000, usb_clk_cfg = 0x30101
ohci_usb_remove: Input: mask updated = 0x30000
ohci_usb_remove: Output: usb_clk_cfg = 0x101
USB0:   USB EHCI 1.00
USB1:   USB OHCI 1.0
USB2:   USB EHCI 1.00
USB3:   USB OHCI 1.0
scanning bus 0 for devices... 1 USB Device(s) found
scanning bus 2 for devices... 2 USB Device(s) found
       scanning usb for storage devices... 0 Storage Device(s) found

- H5=> Orangepi PC2

- H3=> BPI-M2+

=> usb reset
resetting USB...
ohci_usb_remove: Input: mask = 0x10000, usb_clk_cfg = 0x70707
ohci_usb_remove: Input: mask updated = 0x10000
ohci_usb_remove: Output: usb_clk_cfg = 0x60707
EHCI failed to shut down host controller.
ohci_usb_remove: Input: mask = 0x20000, usb_clk_cfg = 0x60505
ohci_usb_remove: Input: mask updated = 0x20000
ohci_usb_remove: Output: usb_clk_cfg = 0x40505
EHCI failed to shut down host controller.
ohci_usb_remove: Input: mask = 0x40000, usb_clk_cfg = 0x40101
ohci_usb_remove: Input: mask updated = 0x40000
ohci_usb_remove: Output: usb_clk_cfg = 0x101
USB0:   USB EHCI 1.00
USB1:   USB OHCI 1.0
USB2:   USB EHCI 1.00
USB3:   USB OHCI 1.0
USB4:   USB EHCI 1.00
USB5:   USB OHCI 1.0
scanning bus 0 for devices... 1 USB Device(s) found
scanning bus 2 for devices... 1 USB Device(s) found
scanning bus 4 for devices... 1 USB Device(s) found
       scanning usb for storage devices... 0 Storage Device(s) found

All working fine, so

Tested-by: Jagan Teki <jagan@openedev.com>

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

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

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

[Andre Przywara]

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

Cheers,
Andre.

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

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

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

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

[Jagan Teki]

On Wed, Jul 4, 2018 at 8:03 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> 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@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.

I have tried this by managing global static, one side effect is when
only OHCI1 is enabled it will disable OHCI0 clock as well along with
OHCI1 clock and it shouldn't effect much I suppose. I have pasted code
snippet here just to review.

diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c
index 0ddbdbe460..2c4501788f 100644
--- a/drivers/usb/host/ohci-sunxi.c
+++ b/drivers/usb/host/ohci-sunxi.c
@@ -44,6 +44,8 @@ struct ohci_sunxi_priv {
     const struct ohci_sunxi_cfg *cfg;
 };

+static int nr_ctrl;
+
 static int ohci_usb_probe(struct udevice *dev)
 {
     struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
@@ -99,6 +101,7 @@ no_phy:
     priv->ahb_gate_mask <<= reg_mask * AHB_CLK_DIST;
     extra_ahb_gate_mask <<= reg_mask * AHB_CLK_DIST;
     priv->usb_gate_mask <<= reg_mask;
+    nr_ctrl++;

     setbits_le32(&priv->ccm->ahb_gate0,
              priv->ahb_gate_mask | extra_ahb_gate_mask);
@@ -130,7 +133,18 @@ 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);
+
+    if ((device_is_compatible(dev, "allwinner,sun50i-a64-ohci"))) {
+        if (!--nr_ctrl) {
+            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);
+        }
+    } else {
+        clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
+    }
+
     clrbits_le32(&priv->ccm->ahb_gate0, priv->ahb_gate_mask);

     return 0;

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

[Andre Przywara]

Hi,

On 04/07/18 16:51, Jagan Teki wrote:
> On Wed, Jul 4, 2018 at 8:03 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> 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@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.
> 
> I have tried this by managing global static, one side effect is when
> only OHCI1 is enabled it will disable OHCI0 clock as well along with
> OHCI1 clock and it shouldn't effect much I suppose.

Yes, that should just be 0 anyways. So you clear a cleared bit.

> I have pasted code
> snippet here just to review.
> 
> diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c
> index 0ddbdbe460..2c4501788f 100644
> --- a/drivers/usb/host/ohci-sunxi.c
> +++ b/drivers/usb/host/ohci-sunxi.c
> @@ -44,6 +44,8 @@ struct ohci_sunxi_priv {
>      const struct ohci_sunxi_cfg *cfg;
>  };
> 
> +static int nr_ctrl;
> +
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>      struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
> @@ -99,6 +101,7 @@ no_phy:
>      priv->ahb_gate_mask <<= reg_mask * AHB_CLK_DIST;
>      extra_ahb_gate_mask <<= reg_mask * AHB_CLK_DIST;
>      priv->usb_gate_mask <<= reg_mask;
> +    nr_ctrl++;
> 
>      setbits_le32(&priv->ccm->ahb_gate0,
>               priv->ahb_gate_mask | extra_ahb_gate_mask);
> @@ -130,7 +133,18 @@ 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);
> +
> +    if ((device_is_compatible(dev, "allwinner,sun50i-a64-ohci"))) {
> +        if (!--nr_ctrl) {
> +            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);
> +        }
> +    } else {
> +        clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
> +    }
> +

It can actually be simpler:
This is what I need to test for v2, based on the idea that the last one
turns out the lights:
------------------
@@ -44,6 +44,8 @@ struct ohci_sunxi_priv {
        const struct ohci_sunxi_cfg *cfg;
 };

+static fdt_addr_t last_ohci_addr = 0;
+
 static int ohci_usb_probe(struct udevice *dev)
 {
        struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
@@ -53,6 +55,9 @@ static int ohci_usb_probe(struct udevice *dev)
        u8 reg_mask = 0;
        int phys, ret;

+       if ((fdt_addr_t)regs > last_ohci_addr)
+               last_ohci_addr = (fdt_addr_t)regs;
+
    priv->cfg = (const struct ohci_sunxi_cfg *)dev_get_driver_data(dev);
        priv->ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
        if (IS_ERR(priv->ccm))
@@ -135,7 +140,7 @@ static int ohci_usb_remove(struct udevice *dev)
         * 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) {
+       if (!priv->cfg->extra_usb_gate_mask || base_addr ==
last_ohci_addr) {
                u32 usb_gate_mask = priv->usb_gate_mask;

                usb_gate_mask |= priv->cfg->extra_usb_gate_mask;


Cheers,
Andre.

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

[Jagan Teki]

On Wed, Jul 4, 2018 at 9:41 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 04/07/18 16:51, Jagan Teki wrote:
>> On Wed, Jul 4, 2018 at 8:03 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>> 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@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.
>>
>> I have tried this by managing global static, one side effect is when
>> only OHCI1 is enabled it will disable OHCI0 clock as well along with
>> OHCI1 clock and it shouldn't effect much I suppose.
>
> Yes, that should just be 0 anyways. So you clear a cleared bit.
>
>> I have pasted code
>> snippet here just to review.
>>
>> diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c
>> index 0ddbdbe460..2c4501788f 100644
>> --- a/drivers/usb/host/ohci-sunxi.c
>> +++ b/drivers/usb/host/ohci-sunxi.c
>> @@ -44,6 +44,8 @@ struct ohci_sunxi_priv {
>>      const struct ohci_sunxi_cfg *cfg;
>>  };
>>
>> +static int nr_ctrl;
>> +
>>  static int ohci_usb_probe(struct udevice *dev)
>>  {
>>      struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
>> @@ -99,6 +101,7 @@ no_phy:
>>      priv->ahb_gate_mask <<= reg_mask * AHB_CLK_DIST;
>>      extra_ahb_gate_mask <<= reg_mask * AHB_CLK_DIST;
>>      priv->usb_gate_mask <<= reg_mask;
>> +    nr_ctrl++;
>>
>>      setbits_le32(&priv->ccm->ahb_gate0,
>>               priv->ahb_gate_mask | extra_ahb_gate_mask);
>> @@ -130,7 +133,18 @@ 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);
>> +
>> +    if ((device_is_compatible(dev, "allwinner,sun50i-a64-ohci"))) {
>> +        if (!--nr_ctrl) {
>> +            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);
>> +        }
>> +    } else {
>> +        clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
>> +    }
>> +
>
> It can actually be simpler:
> This is what I need to test for v2, based on the idea that the last one
> turns out the lights:
> ------------------
> @@ -44,6 +44,8 @@ struct ohci_sunxi_priv {
>         const struct ohci_sunxi_cfg *cfg;
>  };
>
> +static fdt_addr_t last_ohci_addr = 0;
> +
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>         struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
> @@ -53,6 +55,9 @@ static int ohci_usb_probe(struct udevice *dev)
>         u8 reg_mask = 0;
>         int phys, ret;
>
> +       if ((fdt_addr_t)regs > last_ohci_addr)
> +               last_ohci_addr = (fdt_addr_t)regs;
> +
>     priv->cfg = (const struct ohci_sunxi_cfg *)dev_get_driver_data(dev);
>         priv->ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>         if (IS_ERR(priv->ccm))
> @@ -135,7 +140,7 @@ static int ohci_usb_remove(struct udevice *dev)
>          * 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) {
> +       if (!priv->cfg->extra_usb_gate_mask || base_addr ==
> last_ohci_addr) {
>                 u32 usb_gate_mask = priv->usb_gate_mask;
>
>                 usb_gate_mask |= priv->cfg->extra_usb_gate_mask;

Better to send another version with this changes, I'm fine with this
and tested on A64 and H3 boards.