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

Hi,

On Monday 18 September 2017 07:50 PM, Andrzej Pietrasiewicz wrote:

Hi,

W dniu 18.09.2017 o 14:43, Felipe Balbi pisze:

quoted

Hi,

Andrzej Pietrasiewicz [off-list ref] writes:

quoted

quoted

quoted

quoted

quoted

+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.

quoted

->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.

quoted

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html