[U-Boot] [PATCH] sunxi: A64: OHCI: prevent turning off shared USB clock
Andre Przywara
andre.przywara at arm.com
Wed Jul 4 16:11:49 UTC 2018
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;
Cheers,
Andre.
More information about the U-Boot
mailing list