[PATCH 0/5] chipidea-for-v3.10-v2: USB chipidea: make use of DT helpers in chipidea driver improve driver

[PATCH 0/5] chipidea-for-v3.10-v2: USB chipidea: make use of DT helpers in chipidea driver improve driver

[Marc Kleine-Budde]

Hello,

this series depends on the series "[PATCH 0/3] otg-for-v3.10-v2: separate phy
code and add DT helper" (a.k.a. tags/otg-for-v3.10-v2) posted earlier and is
intended for v3.10. The chipidea driver is converted to make use of the DT
helper functions.

No changes since v1, just rebased to otg-for-v3.10-v2, in case someone wants to
pull it.

regards,
Marc

---

The following changes since commit f5678b135967ea98256ee5df9a360b5769861d23:

  USB mxs-phy: Register phy with framework (2013-02-28 11:36:45 +0100)

are available in the git repository at:

  git://git.pengutronix.de/git/mkl/linux.git tags/chipidea-for-v3.10-v2

for you to fetch changes up to 11e56207b94d65f92acdee17c795753378c581c6:

  USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy (2013-02-28 11:36:48 +0100)

----------------------------------------------------------------
USB chipidea: make use of DT helpers in chipidea driver

Make use of DT helper functions for handling the dr_mode and phy_type property.

----------------------------------------------------------------
Michael Grzeschik (2):
      USB chipidea: ci13xxx-imx: create dynamic platformdata
      USB chipidea: add PTW and PTS handling

Sascha Hauer (3):
      USB chipidea: introduce dual role mode pdata flags
      USB chipidea i.MX: introduce dr_mode property
      USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    6 ++
 drivers/usb/chipidea/bits.h                        |   14 +++-
 drivers/usb/chipidea/ci13xxx_imx.c                 |   67 +++++++++++---------
 drivers/usb/chipidea/core.c                        |   61 ++++++++++++++++--
 include/linux/usb/chipidea.h                       |    3 +-
 5 files changed, 112 insertions(+), 39 deletions(-)

[PATCH 1/5] USB chipidea: ci13xxx-imx: create dynamic platformdata

[Marc Kleine-Budde]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>

This patch removes the limitation of having only one instance of the
ci13xxx-imx platformdata and makes different configurations possible.

Reviewed-by: Peter Chen <peter.chen@freescale.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/ci13xxx_imx.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 8c29122..69024e0 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -85,17 +85,10 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
 
 /* End of common functions shared by usbmisc drivers*/
 
-static struct ci13xxx_platform_data ci13xxx_imx_platdata  = {
-	.name			= "ci13xxx_imx",
-	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
-				  CI13XXX_PULLUP_ON_VBUS |
-				  CI13XXX_DISABLE_STREAMING,
-	.capoffset		= DEF_CAPOFFSET,
-};
-
 static int ci13xxx_imx_probe(struct platform_device *pdev)
 {
 	struct ci13xxx_imx_data *data;
+	struct ci13xxx_platform_data *pdata;
 	struct platform_device *plat_ci, *phy_pdev;
 	struct device_node *phy_np;
 	struct resource *res;
@@ -107,6 +100,18 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		&& !usbmisc_ops)
 		return -EPROBE_DEFER;
 
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX pdata!\n");
+		return -ENOMEM;
+	}
+
+	pdata->name = "ci13xxx_imx";
+	pdata->capoffset = DEF_CAPOFFSET;
+	pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER |
+		       CI13XXX_PULLUP_ON_VBUS |
+		       CI13XXX_DISABLE_STREAMING;
+
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
 		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
@@ -168,7 +173,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		reg_vbus = NULL;
 	}
 
-	ci13xxx_imx_platdata.phy = data->phy;
+	pdata->phy = data->phy;
 
 	if (!pdev->dev.dma_mask) {
 		pdev->dev.dma_mask = devm_kzalloc(&pdev->dev,
@@ -193,7 +198,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 
 	plat_ci = ci13xxx_add_device(&pdev->dev,
 				pdev->resource, pdev->num_resources,
-				&ci13xxx_imx_platdata);
+				pdata);
 	if (IS_ERR(plat_ci)) {
 		ret = PTR_ERR(plat_ci);
 		dev_err(&pdev->dev,
-- 
1.7.10.4

[PATCH 1/5] USB chipidea: ci13xxx-imx: create dynamic platformdata

[Felipe Balbi]

HI,

Hi,

On Thu, Feb 28, 2013 at 11:57:00AM +0100, Marc Kleine-Budde wrote:
> @@ -107,6 +100,18 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
>  		&& !usbmisc_ops)
>  		return -EPROBE_DEFER;
>  
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);

this isn't necessary. Look at how platform_device_add_data() is
implemented. It will duplicate whatever you pass to it. Which means you
can change ci13xxx_imx_platdata as much as you want ;-)

> @@ -168,7 +173,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
>  		reg_vbus = NULL;
>  	}
>  
> -	ci13xxx_imx_platdata.phy = data->phy;
> +	pdata->phy = data->phy;

it would be pretty cool to start seeing patches teaching chipidea core
how to handle its own resources (regulators, phys, clocks, etc).

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/2ebaec4f/attachment.sig>

[PATCH 2/5] USB chipidea: add PTW and PTS handling

[Marc Kleine-Budde]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>

This patch makes it possible to configure the PTW and PTS bits inside
the portsc register for host and device mode before the driver starts
and the phy can be addressed as hardware implementation is designed.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +++
 drivers/usb/chipidea/bits.h                        |   14 ++++++-
 drivers/usb/chipidea/ci13xxx_imx.c                 |    3 ++
 drivers/usb/chipidea/core.c                        |   39 ++++++++++++++++++++
 include/linux/usb/chipidea.h                       |    1 +
 5 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index 5778b9c..dd42ccd 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -5,6 +5,11 @@ Required properties:
 - reg: Should contain registers location and length
 - interrupts: Should contain controller interrupt
 
+Recommended properies:
+- phy_type: the type of the phy connected to the core. Should be one
+  of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this
+  property the PORTSC register won't be touched
+
 Optional properties:
 - fsl,usbphy: phandler of usb phy that connects to the only one port
 - fsl,usbmisc: phandler of non-core register device, with one argument
diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
index 050de85..d8ffc2f 100644
--- a/drivers/usb/chipidea/bits.h
+++ b/drivers/usb/chipidea/bits.h
@@ -48,10 +48,22 @@
 #define PORTSC_SUSP           BIT(7)
 #define PORTSC_HSP            BIT(9)
 #define PORTSC_PTC            (0x0FUL << 16)
+/* PTS and PTW for non lpm version only */
+#define PORTSC_PTS(d)         ((((d) & 0x3) << 30) | (((d) & 0x4) ? BIT(25) : 0))
+#define PORTSC_PTW            BIT(28)
 
 /* DEVLC */
 #define DEVLC_PSPD            (0x03UL << 25)
-#define    DEVLC_PSPD_HS      (0x02UL << 25)
+#define DEVLC_PSPD_HS         (0x02UL << 25)
+#define DEVLC_PTW             BIT(27)
+#define DEVLC_STS             BIT(28)
+#define DEVLC_PTS(d)          (((d) & 0x7) << 29)
+
+/* Encoding for DEVLC_PTS and PORTSC_PTS */
+#define PTS_UTMI              0
+#define PTS_ULPI              2
+#define PTS_SERIAL            3
+#define PTS_HSIC              4
 
 /* OTGSC */
 #define OTGSC_IDPU	      BIT(5)
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 69024e0..ebc1148 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -21,6 +21,7 @@
 #include <linux/clk.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/usb/of.h>
 
 #include "ci.h"
 #include "ci13xxx_imx.h"
@@ -112,6 +113,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		       CI13XXX_PULLUP_ON_VBUS |
 		       CI13XXX_DISABLE_STREAMING;
 
+	pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
+
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
 		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 57cae1f..04d68cb 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -67,6 +67,8 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/chipidea.h>
+#include <linux/usb/of.h>
+#include <linux/phy.h>
 
 #include "ci.h"
 #include "udc.h"
@@ -211,6 +213,41 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
 	return 0;
 }
 
+static void hw_phymode_configure(struct ci13xxx *ci)
+{
+	u32 portsc, lpm;
+
+	switch (ci->platdata->phy_mode) {
+	case USBPHY_INTERFACE_MODE_UTMI:
+		portsc = PORTSC_PTS(PTS_UTMI);
+		lpm = DEVLC_PTS(PTS_UTMI);
+		break;
+	case USBPHY_INTERFACE_MODE_UTMIW:
+		portsc = PORTSC_PTS(PTS_UTMI) | PORTSC_PTW;
+		lpm = DEVLC_PTS(PTS_UTMI) | DEVLC_PTW;
+		break;
+	case USBPHY_INTERFACE_MODE_ULPI:
+		portsc = PORTSC_PTS(PTS_ULPI);
+		lpm = DEVLC_PTS(PTS_ULPI);
+		break;
+	case USBPHY_INTERFACE_MODE_SERIAL:
+		portsc = PORTSC_PTS(PTS_SERIAL);
+		lpm = DEVLC_PTS(PTS_SERIAL);
+		break;
+	case USBPHY_INTERFACE_MODE_HSIC:
+		portsc = PORTSC_PTS(PTS_HSIC);
+		lpm = DEVLC_PTS(PTS_HSIC);
+		break;
+	default:
+		return;
+	}
+
+	if (ci->hw_bank.lpm)
+		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
+	else
+		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
+}
+
 /**
  * hw_device_reset: resets chip (execute without interruption)
  * @ci: the controller
@@ -476,6 +513,8 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 			: CI_ROLE_GADGET;
 	}
 
+	hw_phymode_configure(ci);
+
 	ret = ci_role_start(ci, ci->role);
 	if (ret) {
 		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 544825d..1a2aa18 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -14,6 +14,7 @@ struct ci13xxx_platform_data {
 	uintptr_t	 capoffset;
 	unsigned	 power_budget;
 	struct usb_phy	*phy;
+	enum usb_phy_interface phy_mode;
 	unsigned long	 flags;
 #define CI13XXX_REGS_SHARED		BIT(0)
 #define CI13XXX_REQUIRE_TRANSCEIVER	BIT(1)
-- 
1.7.10.4

[PATCH 2/5] USB chipidea: add PTW and PTS handling

[Alexander Shishkin]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> From: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
> This patch makes it possible to configure the PTW and PTS bits inside
> the portsc register for host and device mode before the driver starts
> and the phy can be addressed as hardware implementation is designed.

Is anybody working on this? Now that the otg and phy bits are in
Felipe's next, we can think of applying these too. This needs some work,
though.

Firstly, it would be really nice to have the devicetree bit and imx bit
split to separate patches, so that if we're to revert one or the other,
we don't end up reverting both.

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +++
>  drivers/usb/chipidea/bits.h                        |   14 ++++++-
>  drivers/usb/chipidea/ci13xxx_imx.c                 |    3 ++
>  drivers/usb/chipidea/core.c                        |   39 ++++++++++++++++++++
>  include/linux/usb/chipidea.h                       |    1 +
>  5 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index 5778b9c..dd42ccd 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -5,6 +5,11 @@ Required properties:
>  - reg: Should contain registers location and length
>  - interrupts: Should contain controller interrupt
>  
> +Recommended properies:
> +- phy_type: the type of the phy connected to the core. Should be one
> +  of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this
> +  property the PORTSC register won't be touched
> +
>  Optional properties:
>  - fsl,usbphy: phandler of usb phy that connects to the only one port
>  - fsl,usbmisc: phandler of non-core register device, with one argument
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 050de85..d8ffc2f 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -48,10 +48,22 @@
>  #define PORTSC_SUSP           BIT(7)
>  #define PORTSC_HSP            BIT(9)
>  #define PORTSC_PTC            (0x0FUL << 16)
> +/* PTS and PTW for non lpm version only */
> +#define PORTSC_PTS(d)         ((((d) & 0x3) << 30) | (((d) & 0x4) ? BIT(25) : 0))
> +#define PORTSC_PTW            BIT(28)
>  
>  /* DEVLC */
>  #define DEVLC_PSPD            (0x03UL << 25)
> -#define    DEVLC_PSPD_HS      (0x02UL << 25)
> +#define DEVLC_PSPD_HS         (0x02UL << 25)
> +#define DEVLC_PTW             BIT(27)
> +#define DEVLC_STS             BIT(28)
> +#define DEVLC_PTS(d)          (((d) & 0x7) << 29)
> +
> +/* Encoding for DEVLC_PTS and PORTSC_PTS */
> +#define PTS_UTMI              0
> +#define PTS_ULPI              2
> +#define PTS_SERIAL            3
> +#define PTS_HSIC              4
>  
>  /* OTGSC */
>  #define OTGSC_IDPU	      BIT(5)
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index 69024e0..ebc1148 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -21,6 +21,7 @@
>  #include <linux/clk.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/usb/of.h>
>  
>  #include "ci.h"
>  #include "ci13xxx_imx.h"
> @@ -112,6 +113,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
>  		       CI13XXX_PULLUP_ON_VBUS |
>  		       CI13XXX_DISABLE_STREAMING;
>  
> +	pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
> +
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data) {
>  		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 57cae1f..04d68cb 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -67,6 +67,8 @@
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg.h>
>  #include <linux/usb/chipidea.h>
> +#include <linux/usb/of.h>
> +#include <linux/phy.h>

Is of.h actually needed here?

>  
>  #include "ci.h"
>  #include "udc.h"
> @@ -211,6 +213,41 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
>  	return 0;
>  }
>  
> +static void hw_phymode_configure(struct ci13xxx *ci)
> +{
> +	u32 portsc, lpm;
> +
> +	switch (ci->platdata->phy_mode) {
> +	case USBPHY_INTERFACE_MODE_UTMI:
> +		portsc = PORTSC_PTS(PTS_UTMI);
> +		lpm = DEVLC_PTS(PTS_UTMI);
> +		break;
> +	case USBPHY_INTERFACE_MODE_UTMIW:
> +		portsc = PORTSC_PTS(PTS_UTMI) | PORTSC_PTW;
> +		lpm = DEVLC_PTS(PTS_UTMI) | DEVLC_PTW;
> +		break;
> +	case USBPHY_INTERFACE_MODE_ULPI:
> +		portsc = PORTSC_PTS(PTS_ULPI);
> +		lpm = DEVLC_PTS(PTS_ULPI);
> +		break;
> +	case USBPHY_INTERFACE_MODE_SERIAL:
> +		portsc = PORTSC_PTS(PTS_SERIAL);
> +		lpm = DEVLC_PTS(PTS_SERIAL);
> +		break;
> +	case USBPHY_INTERFACE_MODE_HSIC:
> +		portsc = PORTSC_PTS(PTS_HSIC);
> +		lpm = DEVLC_PTS(PTS_HSIC);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (ci->hw_bank.lpm)
> +		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
> +	else
> +		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
> +}
> +
>  /**
>   * hw_device_reset: resets chip (execute without interruption)
>   * @ci: the controller
> @@ -476,6 +513,8 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  			: CI_ROLE_GADGET;
>  	}
>  
> +	hw_phymode_configure(ci);
> +
>  	ret = ci_role_start(ci, ci->role);
>  	if (ret) {
>  		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index 544825d..1a2aa18 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -14,6 +14,7 @@ struct ci13xxx_platform_data {
>  	uintptr_t	 capoffset;
>  	unsigned	 power_budget;
>  	struct usb_phy	*phy;
> +	enum usb_phy_interface phy_mode;
>  	unsigned long	 flags;
>  #define CI13XXX_REGS_SHARED		BIT(0)
>  #define CI13XXX_REQUIRE_TRANSCEIVER	BIT(1)
> -- 
> 1.7.10.4

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Marc Kleine-Budde]

From: Sascha Hauer <s.hauer@pengutronix.de>

Even if a chipidea core is otg capable the board may not. This allows
to explicitly set the core to host/peripheral mode. Without these
flags the driver falls back to the old behaviour.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/core.c  |   22 ++++++++++++++++------
 include/linux/usb/chipidea.h |    2 +-
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 04d68cb..ec27060 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -435,6 +435,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	struct resource	*res;
 	void __iomem	*base;
 	int		ret;
+	enum usb_dr_mode dr_mode;
 
 	if (!dev->platform_data) {
 		dev_err(dev, "platform data missing\n");
@@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	/* For now we treat dual-role as otg */
+	dr_mode = ci->platdata->dr_mode;
+	if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
+		dr_mode = USB_DR_MODE_OTG;
+
 	/* initialize role(s) before the interrupt is requested */
-	ret = ci_hdrc_host_init(ci);
-	if (ret)
-		dev_info(dev, "doesn't support host\n");
+	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
+		ret = ci_hdrc_host_init(ci);
+		if (ret)
+			dev_info(dev, "doesn't support host\n");
+	}
 
-	ret = ci_hdrc_gadget_init(ci);
-	if (ret)
-		dev_info(dev, "doesn't support gadget\n");
+	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
+		ret = ci_hdrc_gadget_init(ci);
+		if (ret)
+			dev_info(dev, "doesn't support gadget\n");
+	}
 
 	if (!ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]) {
 		dev_err(dev, "no supported roles\n");
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 1a2aa18..b314647 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -20,7 +20,7 @@ struct ci13xxx_platform_data {
 #define CI13XXX_REQUIRE_TRANSCEIVER	BIT(1)
 #define CI13XXX_PULLUP_ON_VBUS		BIT(2)
 #define CI13XXX_DISABLE_STREAMING	BIT(3)
-
+	enum usb_dr_mode	dr_mode;
 #define CI13XXX_CONTROLLER_RESET_EVENT		0
 #define CI13XXX_CONTROLLER_STOPPED_EVENT	1
 	void	(*notify_event) (struct ci13xxx *ci, unsigned event);
-- 
1.7.10.4

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Felipe Balbi]

Hi,

On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote:
> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	/* For now we treat dual-role as otg */
> +	dr_mode = ci->platdata->dr_mode;
> +	if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
> +		dr_mode = USB_DR_MODE_OTG;
> +
>  	/* initialize role(s) before the interrupt is requested */
> -	ret = ci_hdrc_host_init(ci);
> -	if (ret)
> -		dev_info(dev, "doesn't support host\n");
> +	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {

this is not something you should be passing via pdata; chipidea core
should know how to read this data by itself. Meaning that chipidea core
should be taught about devicetree. But make it optional since now all
users use DT.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/5eda4a64/attachment.sig>

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Alexander Shishkin]

On 8 March 2013 18:33, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote:
>> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>               return -ENODEV;
>>       }
>>
>> +     /* For now we treat dual-role as otg */
>> +     dr_mode = ci->platdata->dr_mode;
>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
>> +             dr_mode = USB_DR_MODE_OTG;
>> +
>>       /* initialize role(s) before the interrupt is requested */
>> -     ret = ci_hdrc_host_init(ci);
>> -     if (ret)
>> -             dev_info(dev, "doesn't support host\n");
>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>
> this is not something you should be passing via pdata; chipidea core
> should know how to read this data by itself. Meaning that chipidea core
> should be taught about devicetree. But make it optional since now all
> users use DT.

And I don't think I like the idea of chipidea core calling into device
tree code directly.

Regards,
--
Alex

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Marc Kleine-Budde]

On 03/08/2013 05:46 PM, Alexander Shishkin wrote:
> On 8 March 2013 18:33, Felipe Balbi <balbi@ti.com> wrote:
>> Hi,
>>
>> On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote:
>>> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>>               return -ENODEV;
>>>       }
>>>
>>> +     /* For now we treat dual-role as otg */
>>> +     dr_mode = ci->platdata->dr_mode;
>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
>>> +             dr_mode = USB_DR_MODE_OTG;
>>> +
>>>       /* initialize role(s) before the interrupt is requested */
>>> -     ret = ci_hdrc_host_init(ci);
>>> -     if (ret)
>>> -             dev_info(dev, "doesn't support host\n");
>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>
>> this is not something you should be passing via pdata; chipidea core
>> should know how to read this data by itself. Meaning that chipidea core
>> should be taught about devicetree. But make it optional since now all
>> users use DT.
> 
> And I don't think I like the idea of chipidea core calling into device
> tree code directly.

Hmmm....this means draw :)

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/2919a15a/attachment.sig>

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Alexander Shishkin]

On 8 March 2013 18:52, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 03/08/2013 05:46 PM, Alexander Shishkin wrote:
>> On 8 March 2013 18:33, Felipe Balbi <balbi@ti.com> wrote:
>>> Hi,
>>>
>>> On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote:
>>>> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>>>               return -ENODEV;
>>>>       }
>>>>
>>>> +     /* For now we treat dual-role as otg */

Btw, if we do this, Peter's otg code will try to access OTGSC, which
is not what we want on non-otg devices, so we'll need a clear
distinction between the two.

>>>> +     dr_mode = ci->platdata->dr_mode;
>>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
>>>> +             dr_mode = USB_DR_MODE_OTG;
>>>> +
>>>>       /* initialize role(s) before the interrupt is requested */
>>>> -     ret = ci_hdrc_host_init(ci);
>>>> -     if (ret)
>>>> -             dev_info(dev, "doesn't support host\n");
>>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>
>>> this is not something you should be passing via pdata; chipidea core
>>> should know how to read this data by itself. Meaning that chipidea core
>>> should be taught about devicetree. But make it optional since now all
>>> users use DT.
>>
>> And I don't think I like the idea of chipidea core calling into device
>> tree code directly.
>
> Hmmm....this means draw :)

Well, we could go for something like

ci_hdrc-$(CONFIG_OF) += of.o

and try to contain the damage there, maybe? Ideas? I would very much
like to keep the clutter away from the core probe if possible.

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Felipe Balbi]

Hi,

On Fri, Mar 08, 2013 at 10:55:46PM +0200, Alexander Shishkin wrote:
> >>>> +     dr_mode = ci->platdata->dr_mode;
> >>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
> >>>> +             dr_mode = USB_DR_MODE_OTG;
> >>>> +
> >>>>       /* initialize role(s) before the interrupt is requested */
> >>>> -     ret = ci_hdrc_host_init(ci);
> >>>> -     if (ret)
> >>>> -             dev_info(dev, "doesn't support host\n");
> >>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> >>>
> >>> this is not something you should be passing via pdata; chipidea core
> >>> should know how to read this data by itself. Meaning that chipidea core
> >>> should be taught about devicetree. But make it optional since now all
> >>> users use DT.
> >>
> >> And I don't think I like the idea of chipidea core calling into device
> >> tree code directly.
> >
> > Hmmm....this means draw :)
> 
> Well, we could go for something like
> 
> ci_hdrc-$(CONFIG_OF) += of.o
> 
> and try to contain the damage there, maybe? Ideas? I would very much
> like to keep the clutter away from the core probe if possible.

damage, what damage ? DeviceTree is quite real and drivers need to cope
with it. If not all platforms support devicetree, make it optional. It's
easy enough to make the choice based on device.of_node being valid or
not.

At the end of the day, it's the chipidea IP which needs dr_mode, not the
glue. Passing the responsability of decoding dr_mode to the glue is
moronic. It's just like asking the glue to control chipidea's clocks.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130310/e1c46c88/attachment.sig>

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Alexander Shishkin]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Fri, Mar 08, 2013 at 10:55:46PM +0200, Alexander Shishkin wrote:
>> >>>> +     dr_mode = ci->platdata->dr_mode;
>> >>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
>> >>>> +             dr_mode = USB_DR_MODE_OTG;
>> >>>> +
>> >>>>       /* initialize role(s) before the interrupt is requested */
>> >>>> -     ret = ci_hdrc_host_init(ci);
>> >>>> -     if (ret)
>> >>>> -             dev_info(dev, "doesn't support host\n");
>> >>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>> >>>
>> >>> this is not something you should be passing via pdata; chipidea core
>> >>> should know how to read this data by itself. Meaning that chipidea core
>> >>> should be taught about devicetree. But make it optional since now all
>> >>> users use DT.
>> >>
>> >> And I don't think I like the idea of chipidea core calling into device
>> >> tree code directly.
>> >
>> > Hmmm....this means draw :)
>> 
>> Well, we could go for something like
>> 
>> ci_hdrc-$(CONFIG_OF) += of.o
>> 
>> and try to contain the damage there, maybe? Ideas? I would very much
>> like to keep the clutter away from the core probe if possible.
>
> damage, what damage ? DeviceTree is quite real and drivers need to cope
> with it. If not all platforms support devicetree, make it optional. It's
> easy enough to make the choice based on device.of_node being valid or
> not.

We have dr_mode and phy_mode (so far). The latter is simple, but the
former one needs to see some special cases, based on its setting. Now,
if we're a pci device, for example, we don't have phandles and stuff and
we will still get this information via platform data.

So, what we'll end up with is some glue drivers (that don't have device
tree) passing all sorts of stuff via platform data and others just
expecting the chipidea to take care of it. That's inconsistent at best.

> At the end of the day, it's the chipidea IP which needs dr_mode, not the
> glue. Passing the responsability of decoding dr_mode to the glue is
> moronic. It's just like asking the glue to control chipidea's clocks.

Now, now. There's something to be said about stuffing core drivers with
support for all sorts of resource management protocols du jour, but
we'll leave that for another day.

As for the clocks, if they are external to chipidea controller, the
latter has no business messing with them. It's like asking chipidea to
do power management on your SoC for you. :)

Regards,
--
Alex

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Felipe Balbi]

Hi,

On Thu, Mar 28, 2013 at 01:13:00PM +0200, Alexander Shishkin wrote:
> >> >>>> +     dr_mode = ci->platdata->dr_mode;
> >> >>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
> >> >>>> +             dr_mode = USB_DR_MODE_OTG;
> >> >>>> +
> >> >>>>       /* initialize role(s) before the interrupt is requested */
> >> >>>> -     ret = ci_hdrc_host_init(ci);
> >> >>>> -     if (ret)
> >> >>>> -             dev_info(dev, "doesn't support host\n");
> >> >>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> >> >>>
> >> >>> this is not something you should be passing via pdata; chipidea core
> >> >>> should know how to read this data by itself. Meaning that chipidea core
> >> >>> should be taught about devicetree. But make it optional since now all
> >> >>> users use DT.
> >> >>
> >> >> And I don't think I like the idea of chipidea core calling into device
> >> >> tree code directly.
> >> >
> >> > Hmmm....this means draw :)
> >> 
> >> Well, we could go for something like
> >> 
> >> ci_hdrc-$(CONFIG_OF) += of.o
> >> 
> >> and try to contain the damage there, maybe? Ideas? I would very much
> >> like to keep the clutter away from the core probe if possible.
> >
> > damage, what damage ? DeviceTree is quite real and drivers need to cope
> > with it. If not all platforms support devicetree, make it optional. It's
> > easy enough to make the choice based on device.of_node being valid or
> > not.
> 
> We have dr_mode and phy_mode (so far). The latter is simple, but the
> former one needs to see some special cases, based on its setting. Now,
> if we're a pci device, for example, we don't have phandles and stuff and
> we will still get this information via platform data.

fair enough:

if (pdev->dev.of_node)
	chipidea_init_from_dt(ci);
else
	chipidea_init_from_pdata(ci);

> So, what we'll end up with is some glue drivers (that don't have device
> tree) passing all sorts of stuff via platform data and others just
> expecting the chipidea to take care of it. That's inconsistent at best.

it's not inconsistent at all.

Some drivers pass data through DT and some pass data through pdata.

Regardless of which driver type you have, chipidea core still needs to
fetch the data, either by of_property_*() calls or by reading
pdata->$field.

I wouldn't call it inconsistency, it's just coping with both ways of
receiving data.

> > At the end of the day, it's the chipidea IP which needs dr_mode, not the
> > glue. Passing the responsability of decoding dr_mode to the glue is
> > moronic. It's just like asking the glue to control chipidea's clocks.
> 
> Now, now. There's something to be said about stuffing core drivers with
> support for all sorts of resource management protocols du jour, but
> we'll leave that for another day.
> 
> As for the clocks, if they are external to chipidea controller, the
> latter has no business messing with them. It's like asking chipidea to

heh, that's not what I said...

> do power management on your SoC for you. :)

right, asking other layers to do your work is stupid, that's exactly
what I said. Shuving DT knowledge in glue layer just so chipidea core
only understands pdata is stupid. You end up allocating memory twice to
hold the same data (once for DT and once for the pdata copies of it).

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130328/2d748425/attachment.sig>

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Alexander Shishkin]

Felipe Balbi <balbi@ti.com> writes:

> Hi,

Hi,

> On Thu, Mar 28, 2013 at 01:13:00PM +0200, Alexander Shishkin wrote:
>> >> >>>> +     dr_mode = ci->platdata->dr_mode;
>> >> >>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
>> >> >>>> +             dr_mode = USB_DR_MODE_OTG;
>> >> >>>> +
>> >> >>>>       /* initialize role(s) before the interrupt is requested */
>> >> >>>> -     ret = ci_hdrc_host_init(ci);
>> >> >>>> -     if (ret)
>> >> >>>> -             dev_info(dev, "doesn't support host\n");
>> >> >>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>> >> >>>
>> >> >>> this is not something you should be passing via pdata; chipidea core
>> >> >>> should know how to read this data by itself. Meaning that chipidea core
>> >> >>> should be taught about devicetree. But make it optional since now all
>> >> >>> users use DT.
>> >> >>
>> >> >> And I don't think I like the idea of chipidea core calling into device
>> >> >> tree code directly.
>> >> >
>> >> > Hmmm....this means draw :)
>> >> 
>> >> Well, we could go for something like
>> >> 
>> >> ci_hdrc-$(CONFIG_OF) += of.o
>> >> 
>> >> and try to contain the damage there, maybe? Ideas? I would very much
>> >> like to keep the clutter away from the core probe if possible.
>> >
>> > damage, what damage ? DeviceTree is quite real and drivers need to cope
>> > with it. If not all platforms support devicetree, make it optional. It's
>> > easy enough to make the choice based on device.of_node being valid or
>> > not.
>> 
>> We have dr_mode and phy_mode (so far). The latter is simple, but the
>> former one needs to see some special cases, based on its setting. Now,
>> if we're a pci device, for example, we don't have phandles and stuff and
>> we will still get this information via platform data.
>
> fair enough:
>
> if (pdev->dev.of_node)
> 	chipidea_init_from_dt(ci);
> else
> 	chipidea_init_from_pdata(ci);

You mean, you want to have two instances of the similar logic? Don't
forget that they might fail to fetch certain phandles and still
continue, but failing to fetch other phandles will be fatal for
probe(). The above snipped can also be shortened to

  chipidea_just_do_the_right_thing(ci); /* I'd like that, btw */

The devil is in the details.

Then, I hate to bring it up, but what do you do for acpi devices? PnP
devices? PCMCIA devices?

Right now, the core is a platform driver. It gets all the information
from platform data. That's all it needs for its purpose, and all the
platform specific details are abstracted away. It's the purpose of the
glue layer's existance to fetch all the relevant bits from the glue
driver knows where and supply it in a *consistent* manner to the core.

Note, it's totally different for regulators or clocks or phys. It is
totally unacceptable to pass objects around between glue and core and
glue shouldn't have to deal with those. And, of course, you can request
all those in the core code in a platform-agnostic manner.

>> So, what we'll end up with is some glue drivers (that don't have device
>> tree) passing all sorts of stuff via platform data and others just
>> expecting the chipidea to take care of it. That's inconsistent at best.
>
> it's not inconsistent at all.
>
> Some drivers pass data through DT and some pass data through pdata.
>
> Regardless of which driver type you have, chipidea core still needs to
> fetch the data, either by of_property_*() calls or by reading
> pdata->$field.
>
> I wouldn't call it inconsistency, it's just coping with both ways of
> receiving data.
>
>> > At the end of the day, it's the chipidea IP which needs dr_mode, not the
>> > glue. Passing the responsability of decoding dr_mode to the glue is
>> > moronic. It's just like asking the glue to control chipidea's clocks.
>> 
>> Now, now. There's something to be said about stuffing core drivers with
>> support for all sorts of resource management protocols du jour, but
>> we'll leave that for another day.
>> 
>> As for the clocks, if they are external to chipidea controller, the
>> latter has no business messing with them. It's like asking chipidea to
>
> heh, that's not what I said...
>
>> do power management on your SoC for you. :)
>
> right, asking other layers to do your work is stupid, that's exactly
> what I said. Shuving DT knowledge in glue layer just so chipidea core
> only understands pdata is stupid.

Why? Especially if the glue drivers have to fetch stuff for their own
needs from DT anyway. Might easily happen.

> You end up allocating memory twice to
> hold the same data (once for DT and once for the pdata copies of it).

It would have been one valid reason for teaching chipidea core about DT,
if we could duplicate *all* of the pdata fields in DT. Otherwise we
still need pdata. And supposing that we can (which we can't) do that,
and supposing that extra 32 bytes of memory actually matter, it still
doesn't justify the extra code in the core to deal with DT. I'm still
not convinced.

Regards,
--
Alex

[PATCH 3/5] USB chipidea: introduce dual role mode pdata flags

[Felipe Balbi]

Hi,

On Thu, Mar 28, 2013 at 03:18:14PM +0200, Alexander Shishkin wrote:
> >> >> >>>> +     dr_mode = ci->platdata->dr_mode;
> >> >> >>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
> >> >> >>>> +             dr_mode = USB_DR_MODE_OTG;
> >> >> >>>> +
> >> >> >>>>       /* initialize role(s) before the interrupt is requested */
> >> >> >>>> -     ret = ci_hdrc_host_init(ci);
> >> >> >>>> -     if (ret)
> >> >> >>>> -             dev_info(dev, "doesn't support host\n");
> >> >> >>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> >> >> >>>
> >> >> >>> this is not something you should be passing via pdata; chipidea core
> >> >> >>> should know how to read this data by itself. Meaning that chipidea core
> >> >> >>> should be taught about devicetree. But make it optional since now all
> >> >> >>> users use DT.
> >> >> >>
> >> >> >> And I don't think I like the idea of chipidea core calling into device
> >> >> >> tree code directly.
> >> >> >
> >> >> > Hmmm....this means draw :)
> >> >> 
> >> >> Well, we could go for something like
> >> >> 
> >> >> ci_hdrc-$(CONFIG_OF) += of.o
> >> >> 
> >> >> and try to contain the damage there, maybe? Ideas? I would very much
> >> >> like to keep the clutter away from the core probe if possible.
> >> >
> >> > damage, what damage ? DeviceTree is quite real and drivers need to cope
> >> > with it. If not all platforms support devicetree, make it optional. It's
> >> > easy enough to make the choice based on device.of_node being valid or
> >> > not.
> >> 
> >> We have dr_mode and phy_mode (so far). The latter is simple, but the
> >> former one needs to see some special cases, based on its setting. Now,
> >> if we're a pci device, for example, we don't have phandles and stuff and
> >> we will still get this information via platform data.
> >
> > fair enough:
> >
> > if (pdev->dev.of_node)
> > 	chipidea_init_from_dt(ci);
> > else
> > 	chipidea_init_from_pdata(ci);
> 
> You mean, you want to have two instances of the similar logic? Don't
> forget that they might fail to fetch certain phandles and still
> continue, but failing to fetch other phandles will be fatal for
> probe(). The above snipped can also be shortened to
> 
>   chipidea_just_do_the_right_thing(ci); /* I'd like that, btw */
> 
> The devil is in the details.
> 
> Then, I hate to bring it up, but what do you do for acpi devices? PnP
> devices? PCMCIA devices?

those look like PCI devices right ? What's the problem with them ?

> Right now, the core is a platform driver. It gets all the information
> from platform data. That's all it needs for its purpose, and all the
> platform specific details are abstracted away. It's the purpose of the
> glue layer's existance to fetch all the relevant bits from the glue
> driver knows where and supply it in a *consistent* manner to the core.

I still that e.g. requesting regulators in glue and passing a regulator
pointer through platform_data is really, really wrong.

> Note, it's totally different for regulators or clocks or phys. It is
> totally unacceptable to pass objects around between glue and core and
> glue shouldn't have to deal with those. And, of course, you can request
> all those in the core code in a platform-agnostic manner.

how ? If your regulator is bound to the glue, how will you
regulator_get() from core driver ?

> >> So, what we'll end up with is some glue drivers (that don't have device
> >> tree) passing all sorts of stuff via platform data and others just
> >> expecting the chipidea to take care of it. That's inconsistent at best.
> >
> > it's not inconsistent at all.
> >
> > Some drivers pass data through DT and some pass data through pdata.
> >
> > Regardless of which driver type you have, chipidea core still needs to
> > fetch the data, either by of_property_*() calls or by reading
> > pdata->$field.
> >
> > I wouldn't call it inconsistency, it's just coping with both ways of
> > receiving data.
> >
> >> > At the end of the day, it's the chipidea IP which needs dr_mode, not the
> >> > glue. Passing the responsability of decoding dr_mode to the glue is
> >> > moronic. It's just like asking the glue to control chipidea's clocks.
> >> 
> >> Now, now. There's something to be said about stuffing core drivers with
> >> support for all sorts of resource management protocols du jour, but
> >> we'll leave that for another day.
> >> 
> >> As for the clocks, if they are external to chipidea controller, the
> >> latter has no business messing with them. It's like asking chipidea to
> >
> > heh, that's not what I said...
> >
> >> do power management on your SoC for you. :)
> >
> > right, asking other layers to do your work is stupid, that's exactly
> > what I said. Shuving DT knowledge in glue layer just so chipidea core
> > only understands pdata is stupid.
> 
> Why? Especially if the glue drivers have to fetch stuff for their own
> needs from DT anyway. Might easily happen.

why ? Because it's poor encapsulation. Why would you give another entity
knowledge about yourself ?

> > You end up allocating memory twice to
> > hold the same data (once for DT and once for the pdata copies of it).
> 
> It would have been one valid reason for teaching chipidea core about DT,
> if we could duplicate *all* of the pdata fields in DT. Otherwise we
> still need pdata. And supposing that we can (which we can't) do that,
> and supposing that extra 32 bytes of memory actually matter, it still
> doesn't justify the extra code in the core to deal with DT. I'm still
> not convinced.

fair enough, it's your headache in the end of the day anyway. When you
get bug reports such as one we saw recently of clocks being left on even
though probe failed, you'll understand.

You have worked with MUSB before and should already know that giving too
much knowledge to your glue layers is a recipe for disaster.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130402/43647640/attachment.sig>

[PATCH 4/5] USB chipidea i.MX: introduce dr_mode property

[Marc Kleine-Budde]

From: Sascha Hauer <s.hauer@pengutronix.de>

The dr_mode devicetree property allows to explicitly specify the
host/peripheral/otg mode. This is necessary for boards without proper
ID pin handling.

Reviewed-by: Peter Chen <peter.chen@freescale.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 Documentation/devicetree/bindings/usb/ci13xxx-imx.txt |    1 +
 drivers/usb/chipidea/ci13xxx_imx.c                    |    1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index dd42ccd..493a414 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -9,6 +9,7 @@ Recommended properies:
 - phy_type: the type of the phy connected to the core. Should be one
   of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this
   property the PORTSC register won't be touched
+- dr_mode: One of "host", "peripheral" or "otg". Defaults to "otg"
 
 Optional properties:
 - fsl,usbphy: phandler of usb phy that connects to the only one port
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index ebc1148..b598bb8 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -114,6 +114,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		       CI13XXX_DISABLE_STREAMING;
 
 	pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
+	pdata->dr_mode = of_usb_get_dr_mode(pdev->dev.of_node);
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
-- 
1.7.10.4

[PATCH 5/5] USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy

[Marc Kleine-Budde]

From: Sascha Hauer <s.hauer@pengutronix.de>

This patch replaces the hand crafted code to retrieve the phy's phandle from
the DT by the helper function devm_usb_get_phy_by_phandle() which has been
added in commit:

    "5d3c28b usb: otg: add device tree support to otg library"

Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Peter Chen <peter.chen@freescale.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/ci13xxx_imx.c |   38 ++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index b598bb8..6e720ce 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -30,7 +30,6 @@
 	((struct usb_phy *)platform_get_drvdata(pdev))
 
 struct ci13xxx_imx_data {
-	struct device_node *phy_np;
 	struct usb_phy *phy;
 	struct platform_device *ci_pdev;
 	struct clk *clk;
@@ -90,12 +89,12 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 {
 	struct ci13xxx_imx_data *data;
 	struct ci13xxx_platform_data *pdata;
-	struct platform_device *plat_ci, *phy_pdev;
-	struct device_node *phy_np;
+	struct platform_device *plat_ci;
 	struct resource *res;
 	struct regulator *reg_vbus;
 	struct pinctrl *pinctrl;
 	int ret;
+	struct usb_phy *phy;
 
 	if (of_find_property(pdev->dev.of_node, "fsl,usbmisc", NULL)
 		&& !usbmisc_ops)
@@ -147,19 +146,20 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
-	if (phy_np) {
-		data->phy_np = phy_np;
-		phy_pdev = of_find_device_by_node(phy_np);
-		if (phy_pdev) {
-			struct usb_phy *phy;
-			phy = pdev_to_phy(phy_pdev);
-			if (phy &&
-			    try_module_get(phy_pdev->dev.driver->owner)) {
-				usb_phy_init(phy);
-				data->phy = phy;
-			}
+	phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
+	if (PTR_ERR(phy) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto err_clk;
+	}
+
+	if (!IS_ERR(phy)) {
+		ret = usb_phy_init(phy);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to init phy: %d\n", ret);
+			goto err_clk;
 		}
+
+		data->phy = phy;
 	}
 
 	/* we only support host now, so enable vbus here */
@@ -170,7 +170,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"Failed to enable vbus regulator, err=%d\n",
 				ret);
-			goto put_np;
+			goto err_clk;
 		}
 		data->reg_vbus = reg_vbus;
 	} else {
@@ -222,9 +222,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 err:
 	if (reg_vbus)
 		regulator_disable(reg_vbus);
-put_np:
-	if (phy_np)
-		of_node_put(phy_np);
+err_clk:
 	clk_disable_unprepare(data->clk);
 	return ret;
 }
@@ -244,8 +242,6 @@ static int ci13xxx_imx_remove(struct platform_device *pdev)
 		module_put(data->phy->dev->driver->owner);
 	}
 
-	of_node_put(data->phy_np);
-
 	clk_disable_unprepare(data->clk);
 
 	platform_set_drvdata(pdev, NULL);
-- 
1.7.10.4

[PATCH 5/5] USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy

[Felipe Balbi]

On Thu, Feb 28, 2013 at 11:57:04AM +0100, Marc Kleine-Budde wrote:
> @@ -147,19 +146,20 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> -	if (phy_np) {
> -		data->phy_np = phy_np;
> -		phy_pdev = of_find_device_by_node(phy_np);
> -		if (phy_pdev) {
> -			struct usb_phy *phy;
> -			phy = pdev_to_phy(phy_pdev);
> -			if (phy &&
> -			    try_module_get(phy_pdev->dev.driver->owner)) {
> -				usb_phy_init(phy);
> -				data->phy = phy;
> -			}
> +	phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);

very nice, but should be done at chipidea core.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/14486a35/attachment.sig>

[PATCH 5/5] USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy

[Marc Kleine-Budde]

On 03/08/2013 05:33 PM, Felipe Balbi wrote:
> On Thu, Feb 28, 2013 at 11:57:04AM +0100, Marc Kleine-Budde wrote:
>> @@ -147,19 +146,20 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> -	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
>> -	if (phy_np) {
>> -		data->phy_np = phy_np;
>> -		phy_pdev = of_find_device_by_node(phy_np);
>> -		if (phy_pdev) {
>> -			struct usb_phy *phy;
>> -			phy = pdev_to_phy(phy_pdev);
>> -			if (phy &&
>> -			    try_module_get(phy_pdev->dev.driver->owner)) {
>> -				usb_phy_init(phy);
>> -				data->phy = phy;
>> -			}
>> +	phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
> 
> very nice, but should be done at chipidea core.

Any suggestions for the phandle name? "chipidea,usbphy"?

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/e9867bb3/attachment.sig>

[PATCH 5/5] USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy

[Felipe Balbi]

On Fri, Mar 08, 2013 at 05:39:53PM +0100, Marc Kleine-Budde wrote:
> On 03/08/2013 05:33 PM, Felipe Balbi wrote:
> > On Thu, Feb 28, 2013 at 11:57:04AM +0100, Marc Kleine-Budde wrote:
> >> @@ -147,19 +146,20 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
> >>  		return ret;
> >>  	}
> >>  
> >> -	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> >> -	if (phy_np) {
> >> -		data->phy_np = phy_np;
> >> -		phy_pdev = of_find_device_by_node(phy_np);
> >> -		if (phy_pdev) {
> >> -			struct usb_phy *phy;
> >> -			phy = pdev_to_phy(phy_pdev);
> >> -			if (phy &&
> >> -			    try_module_get(phy_pdev->dev.driver->owner)) {
> >> -				usb_phy_init(phy);
> >> -				data->phy = phy;
> >> -			}
> >> +	phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
> > 
> > very nice, but should be done at chipidea core.
> 
> Any suggestions for the phandle name? "chipidea,usbphy"?

isn't usbphy or phy enough ? Why do you need the prefix ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130310/d4932277/attachment-0001.sig>

[PATCH 0/5] chipidea-for-v3.10-v1: USB chipidea: make use of DT helpers in chipidea driver improve driver

[Marc Kleine-Budde]

Hello,

this series depends on the series "[PATCH 0/3] otg-for-v3.10-v1: separate phy
code and add DT helper" (a.k.a. tags/otg-for-v3.10-v1) posted earlier and is
intended for v3.10. The chipidea driver is converted to make use of the DT
helper functions.

regards,
Marc


The following changes since commit 8a4c9f8489b2fce9e7bf0eb43fdb160ab51adc2c:

  USB mxs-phy: Register phy with framework (2013-02-27 13:48:21 +0100)

are available in the git repository at:

  git://git.pengutronix.de/git/mkl/linux.git tags/chipidea-for-v3.10-v1

for you to fetch changes up to 5d837224ea36da50f3e744a65fb23af1fa0a480d:

  USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy (2013-02-27 13:49:07 +0100)

----------------------------------------------------------------
USB chipidea: make use of DT helpers in chipidea driver

Make use of DT helper functions for handling the dr_mode and phy_type property.

----------------------------------------------------------------
Michael Grzeschik (2):
      USB chipidea: ci13xxx-imx: create dynamic platformdata
      USB chipidea: add PTW and PTS handling

Sascha Hauer (3):
      USB chipidea: introduce dual role mode pdata flags
      USB chipidea i.MX: introduce dr_mode property
      USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    6 ++
 drivers/usb/chipidea/bits.h                        |   14 +++-
 drivers/usb/chipidea/ci13xxx_imx.c                 |   67 +++++++++++---------
 drivers/usb/chipidea/core.c                        |   61 ++++++++++++++++--
 include/linux/usb/chipidea.h                       |    3 +-
 5 files changed, 112 insertions(+), 39 deletions(-)

[PATCH 1/5] USB chipidea: ci13xxx-imx: create dynamic platformdata

[Marc Kleine-Budde]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>

This patch removes the limitation of having only one instance of the
ci13xxx-imx platformdata and makes different configurations possible.

Reviewed-by: Peter Chen <peter.chen@freescale.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/ci13xxx_imx.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 8c29122..69024e0 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -85,17 +85,10 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
 
 /* End of common functions shared by usbmisc drivers*/
 
-static struct ci13xxx_platform_data ci13xxx_imx_platdata  = {
-	.name			= "ci13xxx_imx",
-	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
-				  CI13XXX_PULLUP_ON_VBUS |
-				  CI13XXX_DISABLE_STREAMING,
-	.capoffset		= DEF_CAPOFFSET,
-};
-
 static int ci13xxx_imx_probe(struct platform_device *pdev)
 {
 	struct ci13xxx_imx_data *data;
+	struct ci13xxx_platform_data *pdata;
 	struct platform_device *plat_ci, *phy_pdev;
 	struct device_node *phy_np;
 	struct resource *res;
@@ -107,6 +100,18 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		&& !usbmisc_ops)
 		return -EPROBE_DEFER;
 
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX pdata!\n");
+		return -ENOMEM;
+	}
+
+	pdata->name = "ci13xxx_imx";
+	pdata->capoffset = DEF_CAPOFFSET;
+	pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER |
+		       CI13XXX_PULLUP_ON_VBUS |
+		       CI13XXX_DISABLE_STREAMING;
+
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
 		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
@@ -168,7 +173,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		reg_vbus = NULL;
 	}
 
-	ci13xxx_imx_platdata.phy = data->phy;
+	pdata->phy = data->phy;
 
 	if (!pdev->dev.dma_mask) {
 		pdev->dev.dma_mask = devm_kzalloc(&pdev->dev,
@@ -193,7 +198,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 
 	plat_ci = ci13xxx_add_device(&pdev->dev,
 				pdev->resource, pdev->num_resources,
-				&ci13xxx_imx_platdata);
+				pdata);
 	if (IS_ERR(plat_ci)) {
 		ret = PTR_ERR(plat_ci);
 		dev_err(&pdev->dev,
-- 
1.7.10.4