Hi,
On Monday 18 September 2017 07:50 PM, Andrzej Pietrasiewicz wrote:
> Hi,
>
> W dniu 18.09.2017 o 14:43, Felipe Balbi pisze:
>>
>> Hi,
>>
>> Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx> 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?
Adding calibrate is fine but doing init() and power_on() in one driver and
calibrate() in another doesn't look correct. Why not let xhci do init() and
power_on() of phy instead of dwc3?
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
