[PATCH 2/2] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Andrzej Pietrasiewicz]

Hi,

W dniu 18.09.2017 o 14:43, Felipe Balbi pisze:
> 
> Hi,
> 
> Andrzej Pietrasiewicz <andrzej.p at samsung.com> writes:
>>>>>> +static int exynos5_usbdrd_phy_reset(struct phy *phy)
>>>>>> +{
>>>>>> +	struct phy_usb_instance *inst = phy_get_drvdata(phy);
>>>>>> +	struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>>>>>> +
>>>>>> +	return exynos5420_usbdrd_phy_calibrate(phy_drd);
>>>>>> +}
>>>>>> +
>>>>>>     static const struct phy_ops exynos5_usbdrd_phy_ops = {
>>>>>>     	.init		= exynos5_usbdrd_phy_init,
>>>>>>     	.exit		= exynos5_usbdrd_phy_exit,
>>>>>>     	.power_on	= exynos5_usbdrd_phy_power_on,
>>>>>>     	.power_off	= exynos5_usbdrd_phy_power_off,
>>>>>> +	.reset		= exynos5_usbdrd_phy_reset,
>>>>>>     	.owner		= THIS_MODULE,
>>>>>>     };
>>>>>>     
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 03474d3..1d5836e 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>> @@ -156,9 +156,10 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>>>     		} else {
>>>>>>     			if (dwc->usb2_phy)
>>>>>>     				otg_set_vbus(dwc->usb2_phy->otg, true);
>>>>>> -			if (dwc->usb2_generic_phy)
>>>>>> +			if (dwc->usb2_generic_phy) {
>>>>>>     				phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>>>>> -
>>>>>> +				phy_reset(dwc->usb2_generic_phy);
>>>>>
>>>>> it doesn't look like this is the best place to reset the phy. Also,
>>>>
>>>> right, phy_reset is done during initialization before phy_power_on/phy_init or
>>>> in error cases.
>>>>
>>>>> ->reset() doesn't seem to match correctly with a calibration. That seems
>>>>> to be more fitting to a ->power_on() or ->init() implementation.
>>>>
>>>> yeah, the initial patch seems to calibrate in phy_init(). Not sure why it's
>>>> modified.
>>>
>>> The original patch used a hack like below, in xhci_plat_probe():
>>>
>>> +       /* Initialize and power-on USB 3.0 PHY */
>>> +       xhci->shared_hcd->phy->init_count = 0;
>>> +       ret = phy_init(xhci->shared_hcd->phy);
>>> +       if (ret)
>>> +               goto dealloc_usb3_hcd;
>>> +
>>> +       xhci->shared_hcd->phy->power_count = 0;
>>> +       ret = phy_power_on(xhci->shared_hcd->phy);
>>> +       if (ret) {
>>> +               phy_exit(xhci->shared_hcd->phy);
>>> +               goto dealloc_usb3_hcd;
>>> +       }
>>> +
>>>
>>> Manually setting init_count to 0 in order for the subsequent phy_init() to
>>> happen probably does not look good.
>>>
>>> The calibration is clearly needed. However, I don't have any strong opinions
>>> on from which place exactly to trigger the calibration process.
>>> The original patch did not make it upstream, but if that patch is ok,
>>> it is perfectly fine with me to drop my version and take that one instead.
>>
>> Me bad, I did not write about an important issue.
>> The calibration must happen after usb_add_hcd(), otherwise
>> usb_add_hcd() indirectly triggers overwriting the effects of calibration.
> 
> in that case, you should do that from xhci-plat indeed. I think the
> whole idea with init_count is just to make sure you don't initialize it
> twice.

As far as I understand the code in question the desired result is exactly the opposite:
to make sure it _does_ initialize twice, otherwise after the first initialization the
calibration results were lost. In other words, in the code snippet above,
in xhci_plat_probe() the phy_init() was creatively (ab)used in order to force
the calibration at a desired moment, while in the original invocation of phy_init()
the calibration result was merely a short-term side effect discarded soon afterwards.

> 
> One thing's for sure, ->reset() doesn't seem to be the matching callback
> for you to use and, given your explanation above, dwc3 doesn't seem to
> be the right place to fiddle with that.
> 
> Seems like we need an extension of the generic PHY framework to cope
> with your requirement.
>

Here are old patches from Vivek:

https://lkml.org/lkml/2014/9/2/166

In particular:

https://lkml.org/lkml/2014/9/2/170

Please see the discussion that follows the latter.

All in all, is adding the calibrate() method to phy_ops the way to go or not?

Andrzej