Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros <rog...@ti.com> writes:
>> Roger Quadros <rog...@ti.com> writes:
>>>>>> -        ret = dwc3_core_soft_reset(dwc);
>>>>>> +        ret = dwc3_core_get_phy(dwc);
>>>>>
>>>>> we can get_phy in dwc3_core_init() as it will get called on resume().
>>>>> This was the $subject of this patch.
>>>>
>>>> indeed. thanks :-)
>>>>
>>>
>>> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). 
>>> :P
>> 
>> bit of a chicken-and-egg problem. We need to setup the PHY interface
>> before getting the PHYs, but can't get PHY during resume. Maybe the best
>> way here would be to check for the pointers being valid. Something like:
>> 
>> if (!phy)
>>      get_phy();
>> 
>
> OK that should take care of not calling get_phy() on suspend.
> However there is one more issue with the approach
>
>> @@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>                      dwc->maximum_speed = USB_SPEED_HIGH;
>>      }
>>  
>> -    ret = dwc3_core_get_phy(dwc);
>> +    ret = dwc3_phy_setup(dwc);
>>      if (ret)
>>              goto err0;
>
> here we configure PHY related bits and register the ulpi interface.
>
>>  
>> -    ret = dwc3_core_soft_reset(dwc);
>> +    ret = dwc3_core_get_phy(dwc);
>>      if (ret)
>>              goto err0;
>>  
>
> we got the PHYs. all OK here.
>
>> -    ret = dwc3_phy_setup(dwc);
>> +    ret = dwc3_core_soft_reset(dwc);
>>      if (ret)
>>              goto err0;
>
> Now we do a soft reset. This means we loose the PHY configuration bits that 
> we did
> in dwc3_phy_setup. So we need to call dwc3_phy_setup again but not 
> re-register the ulpi interface.
> I can use a flag there so that dwc3_ulpi_init() is done only once.

sounds like it's better to extract out a smaller function that just
checks if we need ULPI bus and registers it, something akin to:

@@ -482,6 +482,21 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
        parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
 }
 
+static int dwc3_ulpi_init(struct dwc3 *dwc)
+{
+       int intf;
+
+       intf = DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3);
+
+       if (intf == DWC3_GHWPARAMS3_HSPHY_IFC_ULPI ||
+                       (intf == DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI &&
+                                       dwc->hsphy_interface &&
+                                       !strncmp(dwc->hsphy_interface, "ulpi", 
4)))
+               return dwc3_ulpi_init(dwc);
+
+       return 0;
+}
+
 /**
  * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -563,11 +578,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
                                break;
                }
                /* FALLTHROUGH */
-       case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
-               ret = dwc3_ulpi_init(dwc);
-               if (ret)
-                       return ret;
-               /* FALLTHROUGH */
        default:
                break;
        }

Then we just call that outside of any functions that get called during PM.

-- 
balbi

signature.asc
Description: PGP signature