[U-Boot] [PATCH] sunxi: A64: OHCI: prevent turning off shared USB clock
Jagan Teki
jagannadh.teki at gmail.com
Wed Jul 4 18:15:21 UTC 2018
On Wed, Jul 4, 2018 at 9:41 PM, Andre Przywara <andre.przywara at 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 at 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 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.
>>
>> 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.
More information about the U-Boot
mailing list