LinuxLists.cc - [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

2014-01-21 10:12:22

Subject: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
do not return from probe if the USB PHY library returns -ENODEV as that
indicates the platform does not have PHY.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
drivers/usb/dwc3/core.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a49217a..e009d4e 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -411,32 +411,26 @@ static int dwc3_probe(struct platform_device *pdev)

if (IS_ERR(dwc->usb2_phy)) {
ret = PTR_ERR(dwc->usb2_phy);
-
- /*
- * if -ENXIO is returned, it means PHY layer wasn't
- * enabled, so it makes no sense to return -EPROBE_DEFER
- * in that case, since no PHY driver will ever probe.
- */
- if (ret == -ENXIO)
+ if (ret == -ENXIO || ret == -ENODEV) {
+ dwc->usb2_phy = NULL;
+ } else if (ret == -EPROBE_DEFER) {
return ret;
-
- dev_err(dev, "no usb2 phy configured\n");
- return -EPROBE_DEFER;
+ } else {
+ dev_err(dev, "no usb2 phy configured\n");
+ return ret;
+ }
}

if (IS_ERR(dwc->usb3_phy)) {
ret = PTR_ERR(dwc->usb3_phy);
-
- /*
- * if -ENXIO is returned, it means PHY layer wasn't
- * enabled, so it makes no sense to return -EPROBE_DEFER
- * in that case, since no PHY driver will ever probe.
- */
- if (ret == -ENXIO)
+ if (ret == -ENXIO || ret == -ENODEV) {
+ dwc->usb3_phy = NULL;
+ } else if (ret == -EPROBE_DEFER) {
return ret;
-
- dev_err(dev, "no usb3 phy configured\n");
- return -EPROBE_DEFER;
+ } else {
+ dev_err(dev, "no usb3 phy configured\n");
+ return ret;
+ }
}

dwc->xhci_resources[0].start = res->start;
--
1.7.10.4

2014-01-21 10:12:36

by Kishon Vijay Abraham I

[permalink] [raw]

Subject: [PATCH v4 2/2] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
power_on and power_off the following APIs are used phy_init(), phy_exit(),
phy_power_on() and phy_power_off().

However using the old USB phy library wont be removed till the PHYs of all
other SoC's using dwc3 core is adapted to the Generic PHY Framework.

Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
Changes from v3:
* avoided using quirks

Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++-
drivers/usb/dwc3/core.c | 60 ++++++++++++++++++++++++
drivers/usb/dwc3/core.h | 7 +++
3 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index e807635..471366d 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -6,11 +6,13 @@ Required properties:
- compatible: must be "snps,dwc3"
- reg : Address and length of the register set for the device
- interrupts: Interrupts used by the dwc3 controller.
+
+Optional properties:
- usb-phy : array of phandle for the PHY device. The first element
in the array is expected to be a handle to the USB2/HS PHY and
the second element is expected to be a handle to the USB3/SS PHY
-
-Optional properties:
+ - phys: from the *Generic PHY* bindings
+ - phy-names: from the *Generic PHY* bindings
- tx-fifo-resize: determines if the FIFO *has* to be reallocated.

This is usually a subnode to DWC3 glue to which it is connected.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index e009d4e..036d589 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -82,6 +82,11 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)

usb_phy_init(dwc->usb2_phy);
usb_phy_init(dwc->usb3_phy);
+ if (dwc->usb2_generic_phy)
+ phy_init(dwc->usb2_generic_phy);
+ if (dwc->usb3_generic_phy)
+ phy_init(dwc->usb3_generic_phy);
+
mdelay(100);

/* Clear USB3 PHY reset */
@@ -343,6 +348,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
{
usb_phy_shutdown(dwc->usb2_phy);
usb_phy_shutdown(dwc->usb3_phy);
+ if (dwc->usb2_generic_phy)
+ phy_exit(dwc->usb2_generic_phy);
+ if (dwc->usb3_generic_phy)
+ phy_exit(dwc->usb3_generic_phy);
+
}

#define DWC3_ALIGN_MASK (16 - 1)
@@ -433,6 +443,32 @@ static int dwc3_probe(struct platform_device *pdev)
}
}

+ dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
+ if (IS_ERR(dwc->usb2_generic_phy)) {
+ ret = PTR_ERR(dwc->usb2_generic_phy);
+ if (ret == -ENOSYS || ret == -ENODEV) {
+ dwc->usb2_generic_phy = NULL;
+ } else if (ret == -EPROBE_DEFER) {
+ return ret;
+ } else {
+ dev_err(dev, "no usb2 phy configured\n");
+ return ret;
+ }
+ }
+
+ dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
+ if (IS_ERR(dwc->usb3_generic_phy)) {
+ ret = PTR_ERR(dwc->usb3_generic_phy);
+ if (ret == -ENOSYS || ret == -ENODEV) {
+ dwc->usb3_generic_phy = NULL;
+ } else if (ret == -EPROBE_DEFER) {
+ return ret;
+ } else {
+ dev_err(dev, "no usb3 phy configured\n");
+ return ret;
+ }
+ }
+
dwc->xhci_resources[0].start = res->start;
dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
DWC3_XHCI_REGS_END;
@@ -482,6 +518,11 @@ static int dwc3_probe(struct platform_device *pdev)
usb_phy_set_suspend(dwc->usb2_phy, 0);
usb_phy_set_suspend(dwc->usb3_phy, 0);

+ if (dwc->usb2_generic_phy)
+ phy_power_on(dwc->usb2_generic_phy);
+ if (dwc->usb3_generic_phy)
+ phy_power_on(dwc->usb3_generic_phy);
+
ret = dwc3_event_buffers_setup(dwc);
if (ret) {
dev_err(dwc->dev, "failed to setup event buffers\n");
@@ -565,6 +606,10 @@ err2:
err1:
usb_phy_set_suspend(dwc->usb2_phy, 1);
usb_phy_set_suspend(dwc->usb3_phy, 1);
+ if (dwc->usb2_generic_phy)
+ phy_power_off(dwc->usb2_generic_phy);
+ if (dwc->usb3_generic_phy)
+ phy_power_off(dwc->usb3_generic_phy);
dwc3_core_exit(dwc);

err0:
@@ -580,6 +625,11 @@ static int dwc3_remove(struct platform_device *pdev)
usb_phy_set_suspend(dwc->usb2_phy, 1);
usb_phy_set_suspend(dwc->usb3_phy, 1);

+ if (dwc->usb2_generic_phy)
+ phy_power_off(dwc->usb2_generic_phy);
+ if (dwc->usb3_generic_phy)
+ phy_power_off(dwc->usb3_generic_phy);
+
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);

@@ -677,6 +727,11 @@ static int dwc3_suspend(struct device *dev)
usb_phy_shutdown(dwc->usb3_phy);
usb_phy_shutdown(dwc->usb2_phy);

+ if (dwc->usb2_generic_phy)
+ phy_exit(dwc->usb2_generic_phy);
+ if (dwc->usb3_generic_phy)
+ phy_exit(dwc->usb3_generic_phy);
+
return 0;
}

@@ -688,6 +743,11 @@ static int dwc3_resume(struct device *dev)
usb_phy_init(dwc->usb3_phy);
usb_phy_init(dwc->usb2_phy);

+ if (dwc->usb2_generic_phy)
+ phy_init(dwc->usb2_generic_phy);
+ if (dwc->usb3_generic_phy)
+ phy_init(dwc->usb3_generic_phy);
+
spin_lock_irqsave(&dwc->lock, flags);

dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f8af8d4..01ec7d7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -31,6 +31,8 @@
#include <linux/usb/gadget.h>
#include <linux/usb/otg.h>

+#include <linux/phy/phy.h>
+
/* Global constants */
#define DWC3_EP0_BOUNCE_SIZE 512
#define DWC3_ENDPOINTS_NUM 32
@@ -613,6 +615,8 @@ struct dwc3_scratchpad_array {
* @dr_mode: requested mode of operation
* @usb2_phy: pointer to USB2 PHY
* @usb3_phy: pointer to USB3 PHY
+ * @usb2_generic_phy: pointer to USB2 PHY
+ * @usb3_generic_phy: pointer to USB3 PHY
* @dcfg: saved contents of DCFG register
* @gctl: saved contents of GCTL register
* @is_selfpowered: true when we are selfpowered
@@ -665,6 +669,9 @@ struct dwc3 {
struct usb_phy *usb2_phy;
struct usb_phy *usb3_phy;

+ struct phy *usb2_generic_phy;
+ struct phy *usb3_generic_phy;
+
void __iomem *regs;
size_t regs_size;

--
1.7.10.4

2014-01-21 13:53:43

by Roger Quadros

[permalink] [raw]

Subject: Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

On 01/21/2014 12:11 PM, Kishon Vijay Abraham I wrote:
> Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
> do not return from probe if the USB PHY library returns -ENODEV as that
> indicates the platform does not have PHY.
>
> Signed-off-by: Kishon Vijay Abraham I <[email protected]>

Reviewed-by: Roger Quadros <[email protected]>

cheers,
-roger

> ---
> drivers/usb/dwc3/core.c | 34 ++++++++++++++--------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a49217a..e009d4e 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -411,32 +411,26 @@ static int dwc3_probe(struct platform_device *pdev)
>
> if (IS_ERR(dwc->usb2_phy)) {
> ret = PTR_ERR(dwc->usb2_phy);
> -
> - /*
> - * if -ENXIO is returned, it means PHY layer wasn't
> - * enabled, so it makes no sense to return -EPROBE_DEFER
> - * in that case, since no PHY driver will ever probe.
> - */
> - if (ret == -ENXIO)
> + if (ret == -ENXIO || ret == -ENODEV) {
> + dwc->usb2_phy = NULL;
> + } else if (ret == -EPROBE_DEFER) {
> return ret;
> -
> - dev_err(dev, "no usb2 phy configured\n");
> - return -EPROBE_DEFER;
> + } else {
> + dev_err(dev, "no usb2 phy configured\n");
> + return ret;
> + }
> }
>
> if (IS_ERR(dwc->usb3_phy)) {
> ret = PTR_ERR(dwc->usb3_phy);
> -
> - /*
> - * if -ENXIO is returned, it means PHY layer wasn't
> - * enabled, so it makes no sense to return -EPROBE_DEFER
> - * in that case, since no PHY driver will ever probe.
> - */
> - if (ret == -ENXIO)
> + if (ret == -ENXIO || ret == -ENODEV) {
> + dwc->usb3_phy = NULL;
> + } else if (ret == -EPROBE_DEFER) {
> return ret;
> -
> - dev_err(dev, "no usb3 phy configured\n");
> - return -EPROBE_DEFER;
> + } else {
> + dev_err(dev, "no usb3 phy configured\n");
> + return ret;
> + }
> }
>
> dwc->xhci_resources[0].start = res->start;
>

2014-01-21 14:00:50

by Roger Quadros

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

Hi Kishon,

On 01/21/2014 12:11 PM, Kishon Vijay Abraham I wrote:
> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
> power_on and power_off the following APIs are used phy_init(), phy_exit(),
> phy_power_on() and phy_power_off().
>
> However using the old USB phy library wont be removed till the PHYs of all
> other SoC's using dwc3 core is adapted to the Generic PHY Framework.
>
> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
> ---
> Changes from v3:
> * avoided using quirks
>
> Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++-
> drivers/usb/dwc3/core.c | 60 ++++++++++++++++++++++++
> drivers/usb/dwc3/core.h | 7 +++
> 3 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e807635..471366d 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -6,11 +6,13 @@ Required properties:
> - compatible: must be "snps,dwc3"
> - reg : Address and length of the register set for the device
> - interrupts: Interrupts used by the dwc3 controller.
> +
> +Optional properties:
> - usb-phy : array of phandle for the PHY device. The first element
> in the array is expected to be a handle to the USB2/HS PHY and
> the second element is expected to be a handle to the USB3/SS PHY
> -
> -Optional properties:
> + - phys: from the *Generic PHY* bindings
> + - phy-names: from the *Generic PHY* bindings
> - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>
> This is usually a subnode to DWC3 glue to which it is connected.
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index e009d4e..036d589 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -82,6 +82,11 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>
> usb_phy_init(dwc->usb2_phy);
> usb_phy_init(dwc->usb3_phy);
> + if (dwc->usb2_generic_phy)
> + phy_init(dwc->usb2_generic_phy);

What if phy_init() fails? You need to report and fail. Same applies for all PHY apis in this patch.

> + if (dwc->usb3_generic_phy)
> + phy_init(dwc->usb3_generic_phy);
> +
> mdelay(100);
>
> /* Clear USB3 PHY reset */
> @@ -343,6 +348,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
> {
> usb_phy_shutdown(dwc->usb2_phy);
> usb_phy_shutdown(dwc->usb3_phy);
> + if (dwc->usb2_generic_phy)
> + phy_exit(dwc->usb2_generic_phy);
> + if (dwc->usb3_generic_phy)
> + phy_exit(dwc->usb3_generic_phy);
> +
> }
>
> #define DWC3_ALIGN_MASK (16 - 1)
> @@ -433,6 +443,32 @@ static int dwc3_probe(struct platform_device *pdev)
> }
> }
>
> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> + if (IS_ERR(dwc->usb2_generic_phy)) {
> + ret = PTR_ERR(dwc->usb2_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV) {
> + dwc->usb2_generic_phy = NULL;
> + } else if (ret == -EPROBE_DEFER) {
> + return ret;
> + } else {
> + dev_err(dev, "no usb2 phy configured\n");
> + return ret;
> + }
> + }
> +
> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> + if (IS_ERR(dwc->usb3_generic_phy)) {
> + ret = PTR_ERR(dwc->usb3_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV) {
> + dwc->usb3_generic_phy = NULL;
> + } else if (ret == -EPROBE_DEFER) {
> + return ret;
> + } else {
> + dev_err(dev, "no usb3 phy configured\n");
> + return ret;
> + }
> + }
> +
> dwc->xhci_resources[0].start = res->start;
> dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
> DWC3_XHCI_REGS_END;
> @@ -482,6 +518,11 @@ static int dwc3_probe(struct platform_device *pdev)
> usb_phy_set_suspend(dwc->usb2_phy, 0);
> usb_phy_set_suspend(dwc->usb3_phy, 0);
>
> + if (dwc->usb2_generic_phy)
> + phy_power_on(dwc->usb2_generic_phy);
> + if (dwc->usb3_generic_phy)
> + phy_power_on(dwc->usb3_generic_phy);
> +

Is it OK to power on the phy before phy_init()?

I suggest to move phy_init() from core_soft_reset() to here, just before phy_power_on().

> ret = dwc3_event_buffers_setup(dwc);
> if (ret) {
> dev_err(dwc->dev, "failed to setup event buffers\n");
> @@ -565,6 +606,10 @@ err2:
> err1:
> usb_phy_set_suspend(dwc->usb2_phy, 1);
> usb_phy_set_suspend(dwc->usb3_phy, 1);
> + if (dwc->usb2_generic_phy)
> + phy_power_off(dwc->usb2_generic_phy);
> + if (dwc->usb3_generic_phy)
> + phy_power_off(dwc->usb3_generic_phy);
> dwc3_core_exit(dwc);
>
> err0:
> @@ -580,6 +625,11 @@ static int dwc3_remove(struct platform_device *pdev)
> usb_phy_set_suspend(dwc->usb2_phy, 1);
> usb_phy_set_suspend(dwc->usb3_phy, 1);
>
> + if (dwc->usb2_generic_phy)
> + phy_power_off(dwc->usb2_generic_phy);
> + if (dwc->usb3_generic_phy)
> + phy_power_off(dwc->usb3_generic_phy);
> +
> pm_runtime_put_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
> @@ -677,6 +727,11 @@ static int dwc3_suspend(struct device *dev)
> usb_phy_shutdown(dwc->usb3_phy);
> usb_phy_shutdown(dwc->usb2_phy);
>
> + if (dwc->usb2_generic_phy)
> + phy_exit(dwc->usb2_generic_phy);
> + if (dwc->usb3_generic_phy)
> + phy_exit(dwc->usb3_generic_phy);
> +
> return 0;
> }
>
> @@ -688,6 +743,11 @@ static int dwc3_resume(struct device *dev)
> usb_phy_init(dwc->usb3_phy);
> usb_phy_init(dwc->usb2_phy);
>
> + if (dwc->usb2_generic_phy)
> + phy_init(dwc->usb2_generic_phy);
> + if (dwc->usb3_generic_phy)
> + phy_init(dwc->usb3_generic_phy);
> +
> spin_lock_irqsave(&dwc->lock, flags);
>
> dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);

cheers,
-roger

2014-01-21 14:49:01

Hi,

On Mon, Jan 27, 2014 at 05:08:55PM +0200, Heikki Krogerus wrote:
> > > Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
> > > do not return from probe if the USB PHY library returns -ENODEV as that
> >
> > this isn't correct, they all have PHYs, some of them might not be
> > controllable.
> >
> > > indicates the platform does not have PHY.
> >
> > not really, that indicates the current platform tried to grab a PHY and
> > the PHY doesn't exist. If there's anybody with a non-controllable PHY
> > and someone gives me a really good reason for not using the generic
> > no-op PHY, then we should add a flag and we could:
> >
> > if (!likely(dwc->flags & DWC3_USB2PHY_DRIVER_NOT_NEEDED))
> > dwc3_grab_phys(dwc);
>
> Why would you need to know if the PHY drivers are needed or not
> explicitly in your controller driver?

because, one way or another, they all do need it. Except for quirky ones
like AM437x where a USB3 IP was hardwired into USB2-only mode, so it
really lacks a USB3 PHY.

> > But I really want to see the argument against using no-op. As far as I
> > could see, everybody needs a PHY driver one way or another, some
> > platforms just haven't sent any PHY driver upstream and have their own
> > hacked up solution to avoid using the PHY layer.
>
> Not true in our case. Platforms using Intel's SoCs and chip sets may
> or may not have controllable USB PHY. Quite often they don't. The
> Baytrails have usually ULPI PHY for USB2, but that does not mean they
> provide any vendor specific functions or any need for a driver in any
> case.

that's different from what I heard.

> Are we talking about the old USB PHY library or the new PHY framework
> with the no-op PHY driver?
>
> Well, in any case, I don't understand what is the purpose of the no-op
> PHY driver. What are you drying to achieve with that?

I'm trying to avoid supporting 500 different combinations for a single
driver. We already support old USB PHY layer and generic PHY layer, now
they both need to be made optional. The old USB PHY layer also provides
a no-op, now called phy-generic, which is actually pretty useful.

On top of all that, I'm sure you'll subscribe to the idea of preventing
dwc3 from becoming another MUSB which supports several different
configurations and none work correctly. I much prefer to take baby steps
which are well thought-out and very well exercised, so I'll be very
pedantic about proof of testing.

An invasive change such as $subject needs to be very well-tested in
different architectures with different Kconfig choices before I'd feel
comfortable applying it.

Also, the assumptions made in $subject are just plain wrong, which
gets me really iffy about applying it or not.

cheers

--
balbi

Attachments:

(No filename) (2.69 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-01-28 15:32:45

by Heikki Krogerus

[permalink] [raw]

Subject: Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

Hi,

On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
> > Why would you need to know if the PHY drivers are needed or not
> > explicitly in your controller driver?
>
> because, one way or another, they all do need it. Except for quirky ones
> like AM437x where a USB3 IP was hardwired into USB2-only mode, so it
> really lacks a USB3 PHY.

The Baytrail board I deal with has completely autonomous PHYs. But my
question is, why would you need to care about the PHYs in your
controller driver? Why can't you leave the responsibility of them to
the upper layers and trust what they tell you?

If there is no USB3 PHY for dwc3 then, the driver gets something like
-ENODEV and just continues. There is no need to know about the
details.

For the controller drivers the PHYs are just a resource like any
other. The controller drivers can't have any responsibility of
them. They should not care if PHY drivers are available for them or
not, or even if the PHY framework is available or not.

> > > But I really want to see the argument against using no-op. As far as I
> > > could see, everybody needs a PHY driver one way or another, some
> > > platforms just haven't sent any PHY driver upstream and have their own
> > > hacked up solution to avoid using the PHY layer.
> >
> > Not true in our case. Platforms using Intel's SoCs and chip sets may
> > or may not have controllable USB PHY. Quite often they don't. The
> > Baytrails have usually ULPI PHY for USB2, but that does not mean they
> > provide any vendor specific functions or any need for a driver in any
> > case.
>
> that's different from what I heard.

I don't know where you got that impression, but it's not true. The
Baytrail SoCs for example don't have internal USB PHYs, which means
the manufacturers using it can select what they want. So we have
boards where PHY driver(s) is needed and boards where it isn't.

The problem is that we just don't always know all the details about
the platform. If the PHY is ULPI PHY, we can do runtime detection, but
we can't even rely on always having ULPI.

> > Are we talking about the old USB PHY library or the new PHY framework
> > with the no-op PHY driver?
> >
> > Well, in any case, I don't understand what is the purpose of the no-op
> > PHY driver. What are you drying to achieve with that?
>
> I'm trying to avoid supporting 500 different combinations for a single
> driver. We already support old USB PHY layer and generic PHY layer, now
> they both need to be made optional.

This is really good to get. We have some projects where we are dealing
with more embedded environments, like IVI, where the kernel should be
stripped of everything useless. Since the PHYs are autonomous, we
should be able to disable the PHY libraries/frameworks.

But I still don't understand what is the benefit in the no-op? You
really need to explain this. What you have now in dwc3 is expectations
regarding the PHY, which put a lot of pressure on upper layers to
satisfy the driver. The no-op is just an extra thing that you have to
provide when you fail to determine if the system requires a PHY driver
or not, or if you know that it doesn't, plus an additional check for
the case where the PHY lib/framework is not enabled. I don't see the
value in it.

> The old USB PHY layer also provides
> a no-op, now called phy-generic, which is actually pretty useful.
>
> On top of all that, I'm sure you'll subscribe to the idea of preventing
> dwc3 from becoming another MUSB which supports several different
> configurations and none work correctly. I much prefer to take baby steps
> which are well thought-out and very well exercised, so I'll be very
> pedantic about proof of testing.

I think our goals are the same. I just want to also minimize the need
for any platform specific extra work from the upper layers regarding
the PHYs.

Thanks,

--
heikki

2014-01-28 16:32:18

Hi,

On Monday 24 February 2014 04:35 PM, Roger Quadros wrote:
> On 02/24/2014 11:51 AM, Kishon Vijay Abraham I wrote:
>> Hi Roger,
>>
>> On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
>>> On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
>>>> Hi Roger,
>>>>
>>>> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>>>>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>>>>>> For the controller drivers the PHYs are just a resource like any
>>>>>>>>> other. The controller drivers can't have any responsibility of
>>>>>>>>> them. They should not care if PHY drivers are available for them or
>>>>>>>>> not, or even if the PHY framework is available or not.
>>>>>>>>
>>>>>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>>>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>>>>>> resource; which might be fine, but we need to make sure we don't
>>>>>>>> continue probing when a PHY is missing in a platform that certainly
>>>>>>>> needs a PHY.
>>>>>>>
>>>>>>> Yes, true. In my head I was comparing the PHY only to resources like
>>>>>>> gpios, clocks, dma channels, etc. that are often optional to the
>>>>>>> drivers.
>>>>>>>
>>>>>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>>>>>
>>>>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>>>>>> case.
>>>>>>>>>>
>>>>>>>>>> that's different from what I heard.
>>>>>>>>>
>>>>>>>>> I don't know where you got that impression, but it's not true. The
>>>>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>>>>>> the manufacturers using it can select what they want. So we have
>>>>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>>>>>
>>>>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>>>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>>>>>> difficult task.
>>>>>>>
>>>>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>>>>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>>>>>
>>>>>>> <snip>
>>>>>>>
>>>>>>>>> This is really good to get. We have some projects where we are dealing
>>>>>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>>>>>> should be able to disable the PHY libraries/frameworks.
>>>>>>>>
>>>>>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>>>>>> something like that.
>>>>>>>>
>>>>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>>>>>> as an indication ?
>>>>>>>
>>>>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>>>>>> layer?
>>>>>>
>>>>>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>>>>>
>>>>>>>> I mean, I need to know that a particular platform depends on a PHY
>>>>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>>>>>> isn't needed ;-)
>>>>>>>
>>>>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>>>>>> to tell us that. If the PHY driver that the platform depends is not
>>>>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>>>>>> returning -EPROBE_DEFER.
>>>>>>
>>>>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>>>>>> not. Consider when the phy_provider_register fails, there is no way to know if
>>>>>> PHY driver is available or not. There are a few cases where PHY layer returns
>>>>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>>>>>> available and failed or not available at all. It would be best for us to leave
>>>>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>>>>>
>>>>>
>>>>> Just to summarize this thread on what we need
>>>>
>>>> Thanks for summarizing.
>>>>>
>>>>> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
>>>>> It should be as generic as possible.
>>>>>
>>>>> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
>>>>>
>>>>> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
>>>>>
>>>>> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
>>>>> e.g. init error.
>>>>>
>>>>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
>>>>>
>>>>> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
>>>>> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.
>>>>
>>>> The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
>>>> checks for PHY in the glue layer.
>>>
>>> Sorry, I didn't get you. My reasoning was that since OMAP platform has this strict requirement of requiring
>>> explicit PHY control in order to work, we must do the sanity checks in OMAP specific code and not in the dwc3 core code. It has nothing to do with how hardware is laid out.
>>
>> What kind of sanity check do you think can be done in OMAP code? We don't use
>> any of the PHY API's in glue code. If we add the same PHY APIs in glue code it
>> will be duplication of the same code without much value besides breaking the
>> design guideline of the software to be modelled similar to hardware.
>>
>
> I wasn't saying about using PHY APIs in glue code, but just doing the basic sanity checks like
> presence of PHY layer, the required USB PHY DT nodes and the required drivers for the platform.

hmm.. instead of doing sanity checks we can just select them in Kconfig like
what I suggested before no? We could check if there is a phandle on the child
dwc3 node though.

Thanks
Kishon

2014-02-24 15:51:16

by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

On Mon, Feb 24, 2014 at 03:21:05PM +0530, Kishon Vijay Abraham I wrote:
> Hi Roger,
>
> On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
> > On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
> >> Hi Roger,
> >>
> >> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
> >>> Hi,
> >>>
> >>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
> >>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
> >>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
> >>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
> >>>>>>> For the controller drivers the PHYs are just a resource like any
> >>>>>>> other. The controller drivers can't have any responsibility of
> >>>>>>> them. They should not care if PHY drivers are available for them or
> >>>>>>> not, or even if the PHY framework is available or not.
> >>>>>>
> >>>>>> huh? If memory isn't available you don't continue probing, right ? If
> >>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
> >>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
> >>>>>> to the driver, what you're asking here is to treat PHY as a _different_
> >>>>>> resource; which might be fine, but we need to make sure we don't
> >>>>>> continue probing when a PHY is missing in a platform that certainly
> >>>>>> needs a PHY.
> >>>>>
> >>>>> Yes, true. In my head I was comparing the PHY only to resources like
> >>>>> gpios, clocks, dma channels, etc. that are often optional to the
> >>>>> drivers.
> >>>>>
> >>>>>>>>>> But I really want to see the argument against using no-op. As far as I
> >>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
> >>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
> >>>>>>>>>> hacked up solution to avoid using the PHY layer.
> >>>>>>>>>
> >>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
> >>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
> >>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
> >>>>>>>>> provide any vendor specific functions or any need for a driver in any
> >>>>>>>>> case.
> >>>>>>>>
> >>>>>>>> that's different from what I heard.
> >>>>>>>
> >>>>>>> I don't know where you got that impression, but it's not true. The
> >>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
> >>>>>>> the manufacturers using it can select what they want. So we have
> >>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
> >>>>>>
> >>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
> >>>>>> You have an external PIPE3 interface ? That's quite an achievement,
> >>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
> >>>>>> difficult task.
> >>>>>
> >>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
> >>>>> I'm sorry about that. Need to concentrate on what I'm writing.
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>>> This is really good to get. We have some projects where we are dealing
> >>>>>>> with more embedded environments, like IVI, where the kernel should be
> >>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
> >>>>>>> should be able to disable the PHY libraries/frameworks.
> >>>>>>
> >>>>>> hmmm, in that case it's a lot easier to treat. We can use
> >>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
> >>>>>> something like that.
> >>>>>>
> >>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
> >>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
> >>>>>> as an indication ?
> >>>>>
> >>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
> >>>>> layer?
> >>>>
> >>>> right, but the PHY is connected to the dwc3 core and not to the glue.
> >>>>>
> >>>>>> I mean, I need to know that a particular platform depends on a PHY
> >>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
> >>>>>> isn't needed ;-)
> >>>>>
> >>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
> >>>>> to tell us that. If the PHY driver that the platform depends is not
> >>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
> >>>>> returning -EPROBE_DEFER.
> >>>>
> >>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
> >>>> not. Consider when the phy_provider_register fails, there is no way to know if
> >>>> PHY driver is available or not. There are a few cases where PHY layer returns
> >>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
> >>>> available and failed or not available at all. It would be best for us to leave
> >>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
> >>>>
> >>>
> >>> Just to summarize this thread on what we need
> >>
> >> Thanks for summarizing.
> >>>
> >>> 1) dwc3 core shouldn't worry about platform specific stuff i.e.
> >>> PHY needed or not. It should be as generic as possible.
> >>>
> >>> 2) dwc3 core should continue probe even if PHY layer is not
> >>> enabled, as not all platforms need it.
> >>>
> >>> 3) dwc3 core should continue probe if PHY device is not available.
> >>> (-ENODEV?)
> >>>
> >>> 4) dwc3 core should error out on any error condition if PHY device
> >>> is available and caused some error, e.g. init error.
> >>>
> >>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available
> >>> but device driver is not yet loaded.
> >>>
> >>> 6) platform glue should do the necessary sanity checks for
> >>> availability of all resources like PHY device, PHY layer, etc,
> >>> before populating the dwc3 device. e.g. in OMAP5 case we could
> >>> check if both usb2 and usb3 PHY nodes are available in the DT and
> >>> PHY layer is enabled, from dwc3-omap.c? In J6 case we could check
> >>> that at least usb2 phy node is there for the High-Speed only
> >>> controller, and so on.
> >>
> >> The PHY is connected to the dwc3 core. So I'm not sure if we should
> >> be doing checks for PHY in the glue layer.
> >
> > Sorry, I didn't get you. My reasoning was that since OMAP platform
> > has this strict requirement of requiring explicit PHY control in
> > order to work, we must do the sanity checks in OMAP specific code
> > and not in the dwc3 core code. It has nothing to do with how
> > hardware is laid out.
>
> What kind of sanity check do you think can be done in OMAP code? We don't use
> any of the PHY API's in glue code. If we add the same PHY APIs in glue code it
> will be duplication of the same code without much value besides breaking the
> design guideline of the software to be modelled similar to hardware.
>
> However in Kconfig of dwc3 glue we can add 'select GENERIC_PHY, select
> PHY_OMAP_USB2, select OMAP_USB3' I guess.

ick, please don't. We have suffered too much from selects being
sprinkled all over the place.

--
balbi

Attachments:

(No filename) (6.95 kB)
signature.asc (819.00 B)
Digital signature Download all attachments