On Wed, Jul 4, 2018 at 9:41 PM, Andre Przywara <andre.przyw...@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.przyw...@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.przyw...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot