Re: [PATCH] usb: dwc3: exynos: remove usb_phy_generic support

From: Bartlomiej Zolnierkiewicz
Date: Thu Aug 28 2014 - 14:11:16 EST

Next message: Paul E. McKenney: "[PATCH tip/core/rcu 0/3] Documentation updates for 3.18"
Previous message: David Damerell: "X86_RESERVE_LOW seems not to be respected in 3.13, 3.14"
In reply to: Felipe Balbi: "Re: [PATCH] usb: dwc3: exynos: remove usb_phy_generic support"
Next in thread: Greg Kroah-Hartman: "Re: [PATCH] usb: dwc3: exynos: remove usb_phy_generic support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

[ added Alan and Greg to cc: ]

Hi,

On Wednesday, August 27, 2014 11:42:25 PM Vivek Gautam wrote:
> Hi Baltlomiej,
>
>
> On Wed, Aug 27, 2014 at 1:22 PM, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@xxxxxxxxxxx> wrote:
> > dwc3 driver is using the new Exynos5 SoC series USB DRD PHY driver
> > (PHY_EXYNOS5_USBDRD which selects GENERIC_PHY) as can be seen by
> > looking at the following commits:
> >
> > 7a4cf0fde054 ("ARM: dts: Update DWC3 usb controller to use new
> > phy driver for exynos5250")
> >
> > f070267b5fc1 ("ARM: dts: Enable support for DWC3 controller for
> > exynos5420")
> >
> > Thus remove unused usb_phy_generic support from dwc3 Exynos glue
> > layer.
> >
> > [ The code that is being removed is harmful in the context of
> > multi_v7_defconfig and enabling "EHCI support for Samsung
> > S5P/EXYNOS SoC Series" (USB_EHCI_EXYNOS) + "OHCI support for
> > Samsung S5P/EXYNOS SoC Series" (USB_OHCI_EXYNOS) because "EHCI
> > support for OMAP3 and later chips" (USB_EHCI_HCD_OMAP) selects
> > "NOP USB Transceiver Driver" (NOP_USB_XCEIV). NOP USB driver
> > attaches itself to usb_phy_generic platform devices created by
> > dwc3 Exynos glue layer and later causes Exynos EHCI driver to
> > fail probe and Exynos OHCI driver to hang on probe (as observed
> > on Exynos5250 Arndale board). ]
>
> The issue with EHCI and OHCI on exynos platforms is that until now
> they also request
> usb-phy and only later if they don't find one, they go ahead and get a
> 'phy' generic.
>
> Fortunately we missed this issue with exynos_defconfig, and as you rightly
> mentioned with multi_v7_defconfig, the NOP_USB_XCEIV gets enabled and
> EHCI and OHCI exynos get no-op usb-phy, which actually blocks EHCI/OHCI from
> initializing the real PHYs.
>
> This issue is resolved with patches:
> [PATCH v2 1/2] usb: host: ehci-exynos: Remove unnecessary usb-phy support
> [PATCH v2 2/2] usb: host: ohci-exynos: Remove unnecessary usb-phy support
> wherein we are completely removing the usb-phy support from ehci/ohci-exynos.
> So with these patches we should not see the issue mentioned by you.

Indeed, your patches fix the issue.

Greg, could these two patches ([1] & [2]) get merged quickly, pretty please
(they were already acked by Alan)? They are not a mere cleanups because
they fix the actual problem with using multi_v7_defconfig which in turn has
been blocking Olof's defconfig update patch [3] for quite some time now.
Moreover these patches are limited to Exynos host drivers so they should be
pretty safe when it comes to potential regressions.

[1] http://www.spinics.net/lists/linux-usb/msg112294.html
[2] http://www.spinics.net/lists/linux-usb/msg112293.html
[3] http://www.spinics.net/lists/arm-kernel/msg349654.html

> Now for the DWC3 part, the usb-phy part was added to use register two
> separate no-op-xceivers of USB2_PHY type and USB3_PHY type,
> so that platforms with no separate PHY can still be able to use dwc3.

Is such support actually useful for Exynos platforms (it resides in
dwc3-exynos.c)? I think that all DWC3 Exynos users need corresponding
USB PHY support, no?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> > Cc: Olof Johansson <olof@xxxxxxxxx>
> > Cc: Kukjin Kim <kgene.kim@xxxxxxxxxxx>
> > Cc: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
> > Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> > ---
> > drivers/usb/dwc3/dwc3-exynos.c | 68 -----------------------------------------
> > 1 file changed, 1 insertion(+), 67 deletions(-)
> >
> > Index: b/drivers/usb/dwc3/dwc3-exynos.c
> > ===================================================================
> > --- a/drivers/usb/dwc3/dwc3-exynos.c 2014-08-25 14:57:04.991781925 +0200
> > +++ b/drivers/usb/dwc3/dwc3-exynos.c 2014-08-27 09:16:38.312617727 +0200
> > @@ -23,15 +23,12 @@
> > #include <linux/platform_data/dwc3-exynos.h>
> > #include <linux/dma-mapping.h>
> > #include <linux/clk.h>
> > -#include <linux/usb/otg.h>
> > -#include <linux/usb/usb_phy_generic.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > #include <linux/regulator/consumer.h>
> >
> > struct dwc3_exynos {
> > - struct platform_device *usb2_phy;
> > - struct platform_device *usb3_phy;
> > struct device *dev;
> >
> > struct clk *clk;
> > @@ -39,61 +36,6 @@ struct dwc3_exynos {
> > struct regulator *vdd10;
> > };
> >
> > -static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
> > -{
> > - struct usb_phy_generic_platform_data pdata;
> > - struct platform_device *pdev;
> > - int ret;
> > -
> > - memset(&pdata, 0x00, sizeof(pdata));
> > -
> > - pdev = platform_device_alloc("usb_phy_generic", PLATFORM_DEVID_AUTO);
> > - if (!pdev)
> > - return -ENOMEM;
> > -
> > - exynos->usb2_phy = pdev;
> > - pdata.type = USB_PHY_TYPE_USB2;
> > - pdata.gpio_reset = -1;
> > -
> > - ret = platform_device_add_data(exynos->usb2_phy, &pdata, sizeof(pdata));
> > - if (ret)
> > - goto err1;
> > -
> > - pdev = platform_device_alloc("usb_phy_generic", PLATFORM_DEVID_AUTO);
> > - if (!pdev) {
> > - ret = -ENOMEM;
> > - goto err1;
> > - }
> > -
> > - exynos->usb3_phy = pdev;
> > - pdata.type = USB_PHY_TYPE_USB3;
> > -
> > - ret = platform_device_add_data(exynos->usb3_phy, &pdata, sizeof(pdata));
> > - if (ret)
> > - goto err2;
> > -
> > - ret = platform_device_add(exynos->usb2_phy);
> > - if (ret)
> > - goto err2;
> > -
> > - ret = platform_device_add(exynos->usb3_phy);
> > - if (ret)
> > - goto err3;
> > -
> > - return 0;
> > -
> > -err3:
> > - platform_device_del(exynos->usb2_phy);
> > -
> > -err2:
> > - platform_device_put(exynos->usb3_phy);
> > -
> > -err1:
> > - platform_device_put(exynos->usb2_phy);
> > -
> > - return ret;
> > -}
> > -
> > static int dwc3_exynos_remove_child(struct device *dev, void *unused)
> > {
> > struct platform_device *pdev = to_platform_device(dev);
> > @@ -127,12 +69,6 @@ static int dwc3_exynos_probe(struct plat
> >
> > platform_set_drvdata(pdev, exynos);
> >
> > - ret = dwc3_exynos_register_phys(exynos);
> > - if (ret) {
> > - dev_err(dev, "couldn't register PHYs\n");
> > - return ret;
> > - }
> > -
> > clk = devm_clk_get(dev, "usbdrd30");
> > if (IS_ERR(clk)) {
> > dev_err(dev, "couldn't get clock\n");
> > @@ -194,8 +130,6 @@ static int dwc3_exynos_remove(struct pla
> > struct dwc3_exynos *exynos = platform_get_drvdata(pdev);
> >
> > device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child);
> > - platform_device_unregister(exynos->usb2_phy);
> > - platform_device_unregister(exynos->usb3_phy);
> >
> > clk_disable_unprepare(exynos->clk);
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Paul E. McKenney: "[PATCH tip/core/rcu 0/3] Documentation updates for 3.18"
Previous message: David Damerell: "X86_RESERVE_LOW seems not to be respected in 3.13, 3.14"
In reply to: Felipe Balbi: "Re: [PATCH] usb: dwc3: exynos: remove usb_phy_generic support"
Next in thread: Greg Kroah-Hartman: "Re: [PATCH] usb: dwc3: exynos: remove usb_phy_generic support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]