On 11/01/18 11:31, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@xxxxxx> writes:
>>> Roger Quadros <rogerq@xxxxxx> 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.
>
Right. Seems like we've covered everything. I'll send a patch in a while.
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki