[PATCH 0/9] chipidea fixes and features

[PATCH 0/9] chipidea fixes and features

[Michael Grzeschik]

Marc Kleine-Budde (2):
  usb: chipidea: pci: mark platformdata as static and __devinitdata
  usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings

Michael Grzeschik (7):
  usb: chipidea: ci13xxx_imx: add 2nd and 3rd clock to support imx5x and newer
  usb: chipidea: ci13xxx-imx: create dynamic platformdata
  usb: add phy connection by phy-mode
  usb: chipidea: add PTW and PTS handling
  usb: chipidea: udc: add force-full-speed option
  usb: chipidea: udc: remove unlocked ep_queue which can lead to an race
  usb: chipidea: udc: configure iso endpoints

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    4 +
 arch/arm/boot/dts/imx28.dtsi                       |    2 +
 drivers/of/Kconfig                                 |    4 +
 drivers/of/Makefile                                |    1 +
 drivers/of/of_usbphy.c                             |   49 ++++++++
 drivers/usb/chipidea/bits.h                        |    4 +
 drivers/usb/chipidea/ci.h                          |    2 +
 drivers/usb/chipidea/ci13xxx_imx.c                 |   84 ++++++++++---
 drivers/usb/chipidea/ci13xxx_pci.c                 |    6 +-
 drivers/usb/chipidea/core.c                        |   91 +++++++++++++-
 drivers/usb/chipidea/host.c                        |    4 +
 drivers/usb/chipidea/udc.c                         |  127 ++++++++++++--------
 include/linux/of_usbphy.h                          |   15 +++
 include/linux/usb/chipidea.h                       |   17 +++
 include/linux/usb/phy.h                            |    8 ++
 15 files changed, 337 insertions(+), 81 deletions(-)
 create mode 100644 drivers/of/of_usbphy.c
 create mode 100644 include/linux/of_usbphy.h

-- 
1.7.10.4

[PATCH 1/9] usb: chipidea: pci: mark platformdata as static and __devinitdata

[Michael Grzeschik]

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

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/chipidea/ci13xxx_pci.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_pci.c b/drivers/usb/chipidea/ci13xxx_pci.c
index 918e149..97d25c8 100644
--- a/drivers/usb/chipidea/ci13xxx_pci.c
+++ b/drivers/usb/chipidea/ci13xxx_pci.c
@@ -23,17 +23,17 @@
 /******************************************************************************
  * PCI block
  *****************************************************************************/
-struct ci13xxx_platform_data pci_platdata = {
+static struct ci13xxx_platform_data pci_platdata __devinitdata = {
 	.name		= UDC_DRIVER_NAME,
 	.capoffset	= DEF_CAPOFFSET,
 };
 
-struct ci13xxx_platform_data langwell_pci_platdata = {
+static struct ci13xxx_platform_data langwell_pci_platdata __devinitdata = {
 	.name		= UDC_DRIVER_NAME,
 	.capoffset	= 0,
 };
 
-struct ci13xxx_platform_data penwell_pci_platdata = {
+static struct ci13xxx_platform_data penwell_pci_platdata __devinitdata = {
 	.name		= UDC_DRIVER_NAME,
 	.capoffset	= 0,
 	.power_budget	= 200,
-- 
1.7.10.4

[PATCH 1/9] usb: chipidea: pci: mark platformdata as static and __devinitdata

[Alexander Shishkin]

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

> From: Marc Kleine-Budde <mkl@pengutronix.de>
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

static yes, __devinitdata -- probably not. These structs are used
everywhere throughout the driver currently, not only at initialization
time.

> ---
>  drivers/usb/chipidea/ci13xxx_pci.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci13xxx_pci.c b/drivers/usb/chipidea/ci13xxx_pci.c
> index 918e149..97d25c8 100644
> --- a/drivers/usb/chipidea/ci13xxx_pci.c
> +++ b/drivers/usb/chipidea/ci13xxx_pci.c
> @@ -23,17 +23,17 @@
>  /******************************************************************************
>   * PCI block
>   *****************************************************************************/
> -struct ci13xxx_platform_data pci_platdata = {
> +static struct ci13xxx_platform_data pci_platdata __devinitdata = {
>  	.name		= UDC_DRIVER_NAME,
>  	.capoffset	= DEF_CAPOFFSET,
>  };
>  
> -struct ci13xxx_platform_data langwell_pci_platdata = {
> +static struct ci13xxx_platform_data langwell_pci_platdata __devinitdata = {
>  	.name		= UDC_DRIVER_NAME,
>  	.capoffset	= 0,
>  };
>  
> -struct ci13xxx_platform_data penwell_pci_platdata = {
> +static struct ci13xxx_platform_data penwell_pci_platdata __devinitdata = {
>  	.name		= UDC_DRIVER_NAME,
>  	.capoffset	= 0,
>  	.power_budget	= 200,
> -- 
> 1.7.10.4

[PATCH 1/9] usb: chipidea: pci: mark platformdata as static and __devinitdata

[Marc Kleine-Budde]

On 11/16/2012 11:06 AM, Alexander Shishkin wrote:
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> static yes, __devinitdata -- probably not. These structs are used
> everywhere throughout the driver currently, not only at initialization
> time.

Yes, but a copy of the platform_data is used:

The PCI driver calls ci13xxx_add_device():
http://lxr.free-electrons.com/source/drivers/usb/chipidea/ci13xxx_pci.c#L85

which calls  platform_device_add_data():
http://lxr.free-electrons.com/source/drivers/usb/chipidea/core.c#L364

which makes a copy of the platform_data:
http://lxr.free-electrons.com/source/drivers/base/platform.c#L246

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: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/005f48e5/attachment.sig>

[PATCH 1/9] usb: chipidea: pci: mark platformdata as static and __devinitdata

[Alexander Shishkin]

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

> On 11/16/2012 11:06 AM, Alexander Shishkin wrote:
>> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>> 
>>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>>
>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> 
>> static yes, __devinitdata -- probably not. These structs are used
>> everywhere throughout the driver currently, not only at initialization
>> time.
>
> Yes, but a copy of the platform_data is used:

True, forgot about that. Apologies.

Regards,
--
Alex

[PATCH 1/9] usb: chipidea: pci: mark platformdata as static and __devinitdata

[Greg KH]

On Fri, Nov 16, 2012 at 12:06:57PM +0200, Alexander Shishkin wrote:
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
> > From: Marc Kleine-Budde <mkl@pengutronix.de>
> >
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> static yes, __devinitdata -- probably not. These structs are used
> everywhere throughout the driver currently, not only at initialization
> time.

__devinitdata means nothing anymore given that CONFIG_HOTPLUG is always
enabled.  It also will probably be removed for 3.8, so don't even worry
about adding these types of markings to variables.

thanks,

greg k-h

[PATCH 2/9] usb: chipidea: ci13xxx_imx: add 2nd and 3rd clock to support imx5x and newer

[Michael Grzeschik]

This patch adds support for a second and third clock to the chipidea driver. On
modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb",
"ipg" and "per") must be enabled in order to access the USB core.

In the original driver, the clock was requested without specifying the
connection id, further all mainline ARM archs with support for the chipidea
core (imx23, imx28) register their USB clock without a connection id.

This patch first renames the existing clk variable to clk_ahb. The connection
id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per"
are requested. As all archs don't specify a connection id, all clk_get return
the same clock. This ensures compatibility to existing USB support and adds
support for imx5x at the same time.

This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg"
and "per" clocks.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/ci13xxx_imx.c |   54 ++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 935de97..570aedf 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -32,7 +32,9 @@ struct ci13xxx_imx_data {
 	struct device_node *phy_np;
 	struct usb_phy *phy;
 	struct platform_device *ci_pdev;
-	struct clk *clk;
+	struct clk *clk_ahb;
+	struct clk *clk_ipg;
+	struct clk *clk_per;
 	struct regulator *reg_vbus;
 };
 
@@ -144,20 +146,48 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "pinctrl get/select failed, err=%ld\n",
 			PTR_ERR(pinctrl));
 
-	data->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(data->clk)) {
+	data->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(data->clk_ahb)) {
 		dev_err(&pdev->dev,
-			"Failed to get clock, err=%ld\n", PTR_ERR(data->clk));
-		return PTR_ERR(data->clk);
+			"Failed to get ahb clock, err=%ld\n", PTR_ERR(data->clk_ahb));
+		return PTR_ERR(data->clk_ahb);
 	}
 
-	ret = clk_prepare_enable(data->clk);
+	data->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(data->clk_ipg)) {
+		dev_err(&pdev->dev,
+			"Failed to get ipg clock, err=%ld\n", PTR_ERR(data->clk_ipg));
+		return PTR_ERR(data->clk_ipg);
+	}
+
+	data->clk_per = devm_clk_get(&pdev->dev, "per");
+	if (IS_ERR(data->clk_per)) {
+		dev_err(&pdev->dev,
+			"Failed to get per clock, err=%ld\n", PTR_ERR(data->clk_per));
+		return PTR_ERR(data->clk_per);
+	}
+
+	ret = clk_prepare_enable(data->clk_ahb);
 	if (ret) {
 		dev_err(&pdev->dev,
-			"Failed to prepare or enable clock, err=%d\n", ret);
+			"Failed to prepare or enable ahb clock, err=%d\n", ret);
 		return ret;
 	}
 
+	ret = clk_prepare_enable(data->clk_ipg);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Failed to prepare or enable ipg clock, err=%d\n", ret);
+		goto err_ipg_failed;
+	}
+
+	ret = clk_prepare_enable(data->clk_per);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Failed to prepare or enable per clock, err=%d\n", ret);
+		goto err_per_failed;
+	}
+
 	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
 	if (phy_np) {
 		data->phy_np = phy_np;
@@ -246,7 +276,11 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 put_np:
 	if (phy_np)
 		of_node_put(phy_np);
-	clk_disable_unprepare(data->clk);
+	clk_disable_unprepare(data->clk_per);
+err_per_failed:
+	clk_disable_unprepare(data->clk_ipg);
+err_ipg_failed:
+	clk_disable_unprepare(data->clk_ahb);
 
 	return ret;
 }
@@ -268,7 +302,9 @@ static int __devexit ci13xxx_imx_remove(struct platform_device *pdev)
 
 	of_node_put(data->phy_np);
 
-	clk_disable_unprepare(data->clk);
+	clk_disable_unprepare(data->clk_per);
+	clk_disable_unprepare(data->clk_ipg);
+	clk_disable_unprepare(data->clk_ahb);
 
 	platform_set_drvdata(pdev, NULL);
 
-- 
1.7.10.4

[PATCH 2/9] usb: chipidea: ci13xxx_imx: add 2nd and 3rd clock to support imx5x and newer

[Peter Chen]

On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote:
> This patch adds support for a second and third clock to the chipidea driver. On
> modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb",
> "ipg" and "per") must be enabled in order to access the USB core.
> 
> In the original driver, the clock was requested without specifying the
> connection id, further all mainline ARM archs with support for the chipidea
> core (imx23, imx28) register their USB clock without a connection id.
> 
> This patch first renames the existing clk variable to clk_ahb. The connection
> id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per"
> are requested. As all archs don't specify a connection id, all clk_get return
> the same clock. This ensures compatibility to existing USB support and adds
> support for imx5x at the same time.
> 
> This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg"
> and "per" clocks.
mx23, mx28, and mx6q has the same usb clock sources and different with
mxc (mx5x, mx3x).

I am not sure which method is better:
- Add dummy clock at clock.c
- Add platform information(id_table or something similar) at driver.

Add dummy clock may confuse some users, for example, mx6q has no
"per" and "ipg" clock at all.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/usb/chipidea/ci13xxx_imx.c |   54 ++++++++++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index 935de97..570aedf 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -32,7 +32,9 @@ struct ci13xxx_imx_data {
>  	struct device_node *phy_np;
>  	struct usb_phy *phy;
>  	struct platform_device *ci_pdev;
> -	struct clk *clk;
> +	struct clk *clk_ahb;
> +	struct clk *clk_ipg;
> +	struct clk *clk_per;
>  	struct regulator *reg_vbus;
>  };
>  
> @@ -144,20 +146,48 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>  		dev_warn(&pdev->dev, "pinctrl get/select failed, err=%ld\n",
>  			PTR_ERR(pinctrl));
>  
> -	data->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(data->clk)) {
> +	data->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(data->clk_ahb)) {
>  		dev_err(&pdev->dev,
> -			"Failed to get clock, err=%ld\n", PTR_ERR(data->clk));
> -		return PTR_ERR(data->clk);
> +			"Failed to get ahb clock, err=%ld\n", PTR_ERR(data->clk_ahb));
> +		return PTR_ERR(data->clk_ahb);
>  	}
>  
> -	ret = clk_prepare_enable(data->clk);
> +	data->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(data->clk_ipg)) {
> +		dev_err(&pdev->dev,
> +			"Failed to get ipg clock, err=%ld\n", PTR_ERR(data->clk_ipg));
> +		return PTR_ERR(data->clk_ipg);
> +	}
> +
> +	data->clk_per = devm_clk_get(&pdev->dev, "per");
> +	if (IS_ERR(data->clk_per)) {
> +		dev_err(&pdev->dev,
> +			"Failed to get per clock, err=%ld\n", PTR_ERR(data->clk_per));
> +		return PTR_ERR(data->clk_per);
> +	}
> +
> +	ret = clk_prepare_enable(data->clk_ahb);
>  	if (ret) {
>  		dev_err(&pdev->dev,
> -			"Failed to prepare or enable clock, err=%d\n", ret);
> +			"Failed to prepare or enable ahb clock, err=%d\n", ret);
>  		return ret;
>  	}
>  
> +	ret = clk_prepare_enable(data->clk_ipg);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to prepare or enable ipg clock, err=%d\n", ret);
> +		goto err_ipg_failed;
> +	}
> +
> +	ret = clk_prepare_enable(data->clk_per);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to prepare or enable per clock, err=%d\n", ret);
> +		goto err_per_failed;
> +	}
> +
>  	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
>  	if (phy_np) {
>  		data->phy_np = phy_np;
> @@ -246,7 +276,11 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>  put_np:
>  	if (phy_np)
>  		of_node_put(phy_np);
> -	clk_disable_unprepare(data->clk);
> +	clk_disable_unprepare(data->clk_per);
> +err_per_failed:
> +	clk_disable_unprepare(data->clk_ipg);
> +err_ipg_failed:
> +	clk_disable_unprepare(data->clk_ahb);
>  
>  	return ret;
>  }
> @@ -268,7 +302,9 @@ static int __devexit ci13xxx_imx_remove(struct platform_device *pdev)
>  
>  	of_node_put(data->phy_np);
>  
> -	clk_disable_unprepare(data->clk);
> +	clk_disable_unprepare(data->clk_per);
> +	clk_disable_unprepare(data->clk_ipg);
> +	clk_disable_unprepare(data->clk_ahb);
>  
>  	platform_set_drvdata(pdev, NULL);
>  
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Best Regards,
Peter Chen

[PATCH 2/9] usb: chipidea: ci13xxx_imx: add 2nd and 3rd clock to support imx5x and newer

[Sascha Hauer]

On Mon, Nov 26, 2012 at 05:29:31PM +0800, Peter Chen wrote:
> On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote:
> > This patch adds support for a second and third clock to the chipidea driver. On
> > modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb",
> > "ipg" and "per") must be enabled in order to access the USB core.
> > 
> > In the original driver, the clock was requested without specifying the
> > connection id, further all mainline ARM archs with support for the chipidea
> > core (imx23, imx28) register their USB clock without a connection id.
> > 
> > This patch first renames the existing clk variable to clk_ahb. The connection
> > id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per"
> > are requested. As all archs don't specify a connection id, all clk_get return
> > the same clock. This ensures compatibility to existing USB support and adds
> > support for imx5x at the same time.
> > 
> > This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg"
> > and "per" clocks.
> mx23, mx28, and mx6q has the same usb clock sources and different with
> mxc (mx5x, mx3x).
> 
> I am not sure which method is better:
> - Add dummy clock at clock.c
> - Add platform information(id_table or something similar) at driver.
> 
> Add dummy clock may confuse some users, for example, mx6q has no
> "per" and "ipg" clock at all.

The general idea is: The USB core has different input clocks. The driver
has to be provided with these input clocks. When i.MX6 does not have
these clocks, it only means that this SoC has no software controllable
gates for these clocks, thus they are not documented. Still this SoC
has these clocks.
This way we can describe the differences between the SoC purely on SoC
level without bothering the driver.
You might want to ask your hardware guys to get more information what
input clocks the USB core actually has, there actually is a lot of
guesswork in it due to missing/inconsistent/confusing documentation.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/9] usb: chipidea: ci13xxx_imx: add 2nd and 3rd clock to support imx5x and newer

[Peter Chen]

On Mon, Nov 26, 2012 at 11:22:32AM +0100, Sascha Hauer wrote:
> On Mon, Nov 26, 2012 at 05:29:31PM +0800, Peter Chen wrote:
> > On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote:
> > > This patch adds support for a second and third clock to the chipidea driver. On
> > > modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb",
> > > "ipg" and "per") must be enabled in order to access the USB core.
> > > 
> > > In the original driver, the clock was requested without specifying the
> > > connection id, further all mainline ARM archs with support for the chipidea
> > > core (imx23, imx28) register their USB clock without a connection id.
> > > 
> > > This patch first renames the existing clk variable to clk_ahb. The connection
> > > id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per"
> > > are requested. As all archs don't specify a connection id, all clk_get return
> > > the same clock. This ensures compatibility to existing USB support and adds
> > > support for imx5x at the same time.
> > > 
> > > This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg"
> > > and "per" clocks.
> > mx23, mx28, and mx6q has the same usb clock sources and different with
> > mxc (mx5x, mx3x).
> > 
> > I am not sure which method is better:
> > - Add dummy clock at clock.c
> > - Add platform information(id_table or something similar) at driver.
> > 
> > Add dummy clock may confuse some users, for example, mx6q has no
> > "per" and "ipg" clock at all.
> 
> The general idea is: The USB core has different input clocks. The driver
> has to be provided with these input clocks. When i.MX6 does not have
> these clocks, it only means that this SoC has no software controllable
> gates for these clocks, thus they are not documented. Still this SoC
> has these clocks.
> This way we can describe the differences between the SoC purely on SoC
> level without bothering the driver.
> You might want to ask your hardware guys to get more information what
> input clocks the USB core actually has, there actually is a lot of
> guesswork in it due to missing/inconsistent/confusing documentation.
Discussed with USB IC module owner, some feedback like below:
- You are correct, the input clock for controller is always same, there
are three sources:
	- ahb
	- xcvr_clk
	- xcvr_ser_clk
- ahb: the source is the system ipg and ahb source, but for usb subsystem,
there is NO separate control for ipg and ahb, there is only one bit
to control ipg and ahb clock together, just like we say usboh3_ipg_ahb bit
at CCM for mx51. The USB controller internal will have two clocks, one for
ipg (visit register), and the other is ahb (access DDR). 
>From the system point, we only need to control usboh3_ipg_ahb,
so, this clock is better as "usb_ahb". "ipg" clock for usb may only the
system ipg clock.

-xcvr_clk: it is the phy output clk, the software can't control it,
(for mx28/mx6q, it can be controlled from phy controller), the software
can only choose the PHY clk source.

-xcvr_ser_clk, it is used at serial phy mode, we need it as the default
phy mode is serial.
I find we take it as "usb_per" at mx5 platform, but this xcvr_ser_clk is
fixed (60M), and can't be adjusted (it is not different with 54M TLL clock).

So in order to consolidate i.mx usb clock, do you think below definitions
are ok:
usb_ahb: ahb clock used to visit register and access DDR
usb_ipg: system ipg clock (mx28 usb no ipg clock)
usb_per: serial phy clock, it is useless if serial phy is not supported, eg mx28, mx6q.
usb_phy: phy clock source, either from 24M or PLL.

So, in order to align with kinds of platforms, I suggest:
If the clock exists and share the same bit at CCM with others
(like usb_ahb and usb_ipg at some platforms), give the same clock num
but different connection id.
If the clock does not exists, just give a dummy clock but with 
connection id (like usb_per at imx6q).

-- 

Best Regards,
Peter Chen

[PATCH 2/9] usb: chipidea: ci13xxx_imx: add 2nd and 3rd clock to support imx5x and newer

[Sascha Hauer]

On Tue, Nov 27, 2012 at 02:50:26PM +0800, Peter Chen wrote:
> On Mon, Nov 26, 2012 at 11:22:32AM +0100, Sascha Hauer wrote:
> > On Mon, Nov 26, 2012 at 05:29:31PM +0800, Peter Chen wrote:
> > > On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote:
> > > > This patch adds support for a second and third clock to the chipidea driver. On
> > > > modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb",
> > > > "ipg" and "per") must be enabled in order to access the USB core.
> > > > 
> > > > In the original driver, the clock was requested without specifying the
> > > > connection id, further all mainline ARM archs with support for the chipidea
> > > > core (imx23, imx28) register their USB clock without a connection id.
> > > > 
> > > > This patch first renames the existing clk variable to clk_ahb. The connection
> > > > id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per"
> > > > are requested. As all archs don't specify a connection id, all clk_get return
> > > > the same clock. This ensures compatibility to existing USB support and adds
> > > > support for imx5x at the same time.
> > > > 
> > > > This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg"
> > > > and "per" clocks.
> > > mx23, mx28, and mx6q has the same usb clock sources and different with
> > > mxc (mx5x, mx3x).
> > > 
> > > I am not sure which method is better:
> > > - Add dummy clock at clock.c
> > > - Add platform information(id_table or something similar) at driver.
> > > 
> > > Add dummy clock may confuse some users, for example, mx6q has no
> > > "per" and "ipg" clock at all.
> > 
> > The general idea is: The USB core has different input clocks. The driver
> > has to be provided with these input clocks. When i.MX6 does not have
> > these clocks, it only means that this SoC has no software controllable
> > gates for these clocks, thus they are not documented. Still this SoC
> > has these clocks.
> > This way we can describe the differences between the SoC purely on SoC
> > level without bothering the driver.
> > You might want to ask your hardware guys to get more information what
> > input clocks the USB core actually has, there actually is a lot of
> > guesswork in it due to missing/inconsistent/confusing documentation.
> Discussed with USB IC module owner, some feedback like below:
> - You are correct, the input clock for controller is always same, there
> are three sources:
> 	- ahb
> 	- xcvr_clk
> 	- xcvr_ser_clk
> - ahb: the source is the system ipg and ahb source, but for usb subsystem,
> there is NO separate control for ipg and ahb, there is only one bit
> to control ipg and ahb clock together, just like we say usboh3_ipg_ahb bit
> at CCM for mx51. The USB controller internal will have two clocks, one for
> ipg (visit register), and the other is ahb (access DDR). 
> From the system point, we only need to control usboh3_ipg_ahb,
> so, this clock is better as "usb_ahb". "ipg" clock for usb may only the
> system ipg clock.

The i.MX27 has separate gates for ipg (PCCR1[25]) and ahb PCCR1[11])

> 
> -xcvr_clk: it is the phy output clk, the software can't control it,
> (for mx28/mx6q, it can be controlled from phy controller), the software
> can only choose the PHY clk source.
> 
> -xcvr_ser_clk, it is used at serial phy mode, we need it as the default
> phy mode is serial.
> I find we take it as "usb_per" at mx5 platform, but this xcvr_ser_clk is
> fixed (60M), and can't be adjusted (it is not different with 54M TLL clock).
> 
> So in order to consolidate i.mx usb clock, do you think below definitions
> are ok:
> usb_ahb: ahb clock used to visit register and access DDR
> usb_ipg: system ipg clock (mx28 usb no ipg clock)
> usb_per: serial phy clock, it is useless if serial phy is not supported, eg mx28, mx6q.
> usb_phy: phy clock source, either from 24M or PLL.
> 
> So, in order to align with kinds of platforms, I suggest:
> If the clock exists and share the same bit at CCM with others
> (like usb_ahb and usb_ipg at some platforms), give the same clock num
> but different connection id.

Yes.

> If the clock does not exists, just give a dummy clock but with 
> connection id (like usb_per at imx6q).

Yes.

Thanks for the informations, it's good to be able to get feedback from
the hardware people.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/9] usb: chipidea: ci13xxx-imx: create dynamic platformdata

[Michael Grzeschik]

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

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/ci13xxx_imx.c |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 570aedf..7b99c96 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -87,15 +87,6 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
 
 /* End of common functions shared by usbmisc drivers*/
 
-static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
-	.name			= "ci13xxx_imx",
-	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
-				  CI13XXX_PULLUP_ON_VBUS |
-				  CI13XXX_DISABLE_STREAMING |
-				  CI13XXX_REGS_SHARED,
-	.capoffset		= DEF_CAPOFFSET,
-};
-
 static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
 {
 
@@ -117,6 +108,7 @@ static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
 static int __devinit 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 ci13xxx	*ci;
 	struct device_node *phy_np;
@@ -129,6 +121,19 @@ static int __devinit 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 |
+		       CI13XXX_REGS_SHARED,
+
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
 		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
@@ -209,7 +214,7 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 	else
 		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,
@@ -234,7 +239,7 @@ static int __devinit 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 3/9] usb: chipidea: ci13xxx-imx: create dynamic platformdata

[Alexander Shishkin]

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

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

In more honest wording it should probably say: "makes using different
phys possible", but this is fine too. :)

>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/usb/chipidea/ci13xxx_imx.c |   27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index 570aedf..7b99c96 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -87,15 +87,6 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
>  
>  /* End of common functions shared by usbmisc drivers*/
>  
> -static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
> -	.name			= "ci13xxx_imx",
> -	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> -				  CI13XXX_PULLUP_ON_VBUS |
> -				  CI13XXX_DISABLE_STREAMING |
> -				  CI13XXX_REGS_SHARED,
> -	.capoffset		= DEF_CAPOFFSET,
> -};
> -
>  static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
>  {
>  
> @@ -117,6 +108,7 @@ static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
>  static int __devinit 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 ci13xxx	*ci;
>  	struct device_node *phy_np;
> @@ -129,6 +121,19 @@ static int __devinit 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 |
> +		       CI13XXX_REGS_SHARED,
> +
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data) {
>  		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
> @@ -209,7 +214,7 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>  	else
>  		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,
> @@ -234,7 +239,7 @@ static int __devinit 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 3/9] usb: chipidea: ci13xxx-imx: create dynamic platformdata

[Marc Kleine-Budde]

On 11/16/2012 11:14 AM, Alexander Shishkin wrote:
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
>> This patch removes the limitation of having only one instance of the
>> ci13xxx-imx platformdata and makes different configurations possible.
> 
> In more honest wording it should probably say: "makes using different
> phys possible", but this is fine too. :)

Granted, but later patches add support for different phy configurations. :D

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: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/3828570c/attachment.sig>

[PATCH 3/9] usb: chipidea: ci13xxx-imx: create dynamic platformdata

[Alexander Shishkin]

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

> This patch removes the limitation of having only one instance of the
> ci13xxx-imx platformdata and makes different configurations possible.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/usb/chipidea/ci13xxx_imx.c |   27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index 570aedf..7b99c96 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -87,15 +87,6 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
>  
>  /* End of common functions shared by usbmisc drivers*/
>  
> -static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
> -	.name			= "ci13xxx_imx",
> -	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> -				  CI13XXX_PULLUP_ON_VBUS |
> -				  CI13XXX_DISABLE_STREAMING |
> -				  CI13XXX_REGS_SHARED,
> -	.capoffset		= DEF_CAPOFFSET,
> -};
> -
>  static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
>  {
>  
> @@ -117,6 +108,7 @@ static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
>  static int __devinit 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 ci13xxx	*ci;
>  	struct device_node *phy_np;
> @@ -129,6 +121,19 @@ static int __devinit 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 |
> +		       CI13XXX_REGS_SHARED,

Btw, I think you want to use semicolon instead of comma here.

> +
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);

[PATCH 4/9] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings

[Michael Grzeschik]

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

Its necessary to limit a hostonly soc to its single role, since
debugging has shown that reading on the "CAP_DCCPARAMS" register inside
a host-only port, what ci_hdrc_gadget_init does, can lead to an
instable behaviour of the IC.

This patch allows the device tree to limit the chipidea to host or
peripheral mode only. Its uses the oftree name dr_mode, which is already
mainline and used i.e. in fsl-mph-dr-of.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    4 ++
 arch/arm/boot/dts/imx28.dtsi                       |    2 +
 drivers/usb/chipidea/ci13xxx_imx.c                 |    2 +
 drivers/usb/chipidea/core.c                        |   39 +++++++++++++++++---
 include/linux/usb/chipidea.h                       |    7 ++++
 5 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index 5778b9c..c83aea4 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -4,6 +4,9 @@ Required properties:
 - compatible: Should be "fsl,imx27-usb"
 - reg: Should contain registers location and length
 - interrupts: Should contain controller interrupt
+- dr_mode: indicates the working mode for "fsl,imx27-usb" compatible
+  controllers. Can be "host", "peripheral", or "otg". Defaults to
+  "otg" if not defined.
 
 Optional properties:
 - fsl,usbphy: phandler of usb phy that connects to the only one port
@@ -20,4 +23,5 @@ usb at 02184000 { /* USB OTG */
 	fsl,usbphy = <&usbphy1>;
 	fsl,usbmisc = <&usbmisc 0>;
 	disable-over-current;
+	dr_mode= "otg";
 };
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 55c57ea..2a6dd21 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -874,6 +874,7 @@
 			clocks = <&clks 60>;
 			fsl,usbphy = <&usbphy0>;
 			status = "disabled";
+			dr_mode = "otg";
 		};
 
 		usb1: usb at 80090000 {
@@ -883,6 +884,7 @@
 			clocks = <&clks 61>;
 			fsl,usbphy = <&usbphy1>;
 			status = "disabled";
+			dr_mode = "host";
 		};
 
 		dflpt at 800c0000 {
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 7b99c96..ee4dab0 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -237,6 +237,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		}
 	}
 
+	ci13xxx_get_dr_mode(pdev->dev.of_node, pdata);
+
 	plat_ci = ci13xxx_add_device(&pdev->dev,
 				pdev->resource, pdev->num_resources,
 				pdata);
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index b50b77a..3e3e159 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -63,6 +63,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_platform.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
@@ -521,6 +522,23 @@ void ci13xxx_remove_device(struct platform_device *pdev)
 }
 EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
 
+void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
+{
+	const unsigned char *dr_mode;
+
+	dr_mode = of_get_property(of_node, "dr_mode", NULL);
+	if (!dr_mode)
+		return;
+
+	if (!strcmp(dr_mode, "host"))
+		pdata->flags |= CI13XXX_DR_MODE_HOST;
+	else if (!strcmp(dr_mode, "peripheral"))
+		pdata->flags |= CI13XXX_DR_MODE_PERIPHERAL;
+	else if (!strcmp(dr_mode, "otg"))
+		pdata->flags |= CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL;
+}
+EXPORT_SYMBOL_GPL(ci13xxx_get_dr_mode);
+
 static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 {
 	struct device	*dev = &pdev->dev;
@@ -591,13 +609,22 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 	hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, OTGSC_INT_STATUS_BITS);
 
 	/* initialize role(s) before the interrupt is requested */
-	ret = ci_hdrc_host_init(ci);
-	if (ret)
-		dev_info(dev, "doesn't support host\n");
+	/* default to otg */
+	if (!(ci->platdata->flags & CI13XXX_DR_MODE_MASK))
+		ci->platdata->flags |= CI13XXX_DR_MODE_HOST |
+			CI13XXX_DR_MODE_PERIPHERAL;
+
+	if (ci->platdata->flags & CI13XXX_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 (ci->platdata->flags & CI13XXX_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 544825d..906d259 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -19,6 +19,10 @@ struct ci13xxx_platform_data {
 #define CI13XXX_REQUIRE_TRANSCEIVER	BIT(1)
 #define CI13XXX_PULLUP_ON_VBUS		BIT(2)
 #define CI13XXX_DISABLE_STREAMING	BIT(3)
+#define CI13XXX_DR_MODE_HOST		BIT(4)
+#define CI13XXX_DR_MODE_PERIPHERAL	BIT(5)
+#define CI13XXX_DR_MODE_MASK \
+	(CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL)
 
 #define CI13XXX_CONTROLLER_RESET_EVENT		0
 #define CI13XXX_CONTROLLER_STOPPED_EVENT	1
@@ -35,4 +39,7 @@ struct platform_device *ci13xxx_add_device(struct device *dev,
 /* Remove ci13xxx device */
 void ci13xxx_remove_device(struct platform_device *pdev);
 
+/* Parse of-tree "dr_mode" property */
+void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
+
 #endif
-- 
1.7.10.4

[PATCH 4/9] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings

[Alexander Shishkin]

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

> From: Marc Kleine-Budde <mkl@pengutronix.de>
>
> Its necessary to limit a hostonly soc to its single role, since
> debugging has shown that reading on the "CAP_DCCPARAMS" register inside
> a host-only port, what ci_hdrc_gadget_init does, can lead to an
> instable behaviour of the IC.

Probably typos: should be "it's" and "unstable".

[snip]

> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -237,6 +237,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	ci13xxx_get_dr_mode(pdev->dev.of_node, pdata);
> +
>  	plat_ci = ci13xxx_add_device(&pdev->dev,
>  				pdev->resource, pdev->num_resources,
>  				pdata);
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index b50b77a..3e3e159 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -63,6 +63,7 @@
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/of_platform.h>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg.h>
> @@ -521,6 +522,23 @@ void ci13xxx_remove_device(struct platform_device *pdev)
>  }
>  EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
>  
> +void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
> +{
> +	const unsigned char *dr_mode;
> +
> +	dr_mode = of_get_property(of_node, "dr_mode", NULL);
> +	if (!dr_mode)
> +		return;
> +
> +	if (!strcmp(dr_mode, "host"))
> +		pdata->flags |= CI13XXX_DR_MODE_HOST;
> +	else if (!strcmp(dr_mode, "peripheral"))
> +		pdata->flags |= CI13XXX_DR_MODE_PERIPHERAL;
> +	else if (!strcmp(dr_mode, "otg"))
> +		pdata->flags |= CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL;
> +}
> +EXPORT_SYMBOL_GPL(ci13xxx_get_dr_mode);
> +

I'd prefer this function to live in ci13xxx_imx, since that's where it's
used and it doesn't really need anything from core.c anyway. Or maybe it
would make sense to make it even more generic (for other devitetree
users), since you're saying that other drivers are using this already.

Looks good to me otherwise.

Regards,
--
Alex

[PATCH 4/9] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings

[Marc Kleine-Budde]

On 11/16/2012 12:53 PM, Alexander Shishkin wrote:
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>
>> Its necessary to limit a hostonly soc to its single role, since
>> debugging has shown that reading on the "CAP_DCCPARAMS" register inside
>> a host-only port, what ci_hdrc_gadget_init does, can lead to an
>> instable behaviour of the IC.
> 
> Probably typos: should be "it's" and "unstable".
> 
> [snip]
> 
>> --- a/drivers/usb/chipidea/ci13xxx_imx.c
>> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
>> @@ -237,6 +237,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>>  		}
>>  	}
>>  
>> +	ci13xxx_get_dr_mode(pdev->dev.of_node, pdata);
>> +
>>  	plat_ci = ci13xxx_add_device(&pdev->dev,
>>  				pdev->resource, pdev->num_resources,
>>  				pdata);
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index b50b77a..3e3e159 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -63,6 +63,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/slab.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/of_platform.h>
>>  #include <linux/usb/ch9.h>
>>  #include <linux/usb/gadget.h>
>>  #include <linux/usb/otg.h>
>> @@ -521,6 +522,23 @@ void ci13xxx_remove_device(struct platform_device *pdev)
>>  }
>>  EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
>>  
>> +void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
>> +{
>> +	const unsigned char *dr_mode;
>> +
>> +	dr_mode = of_get_property(of_node, "dr_mode", NULL);
>> +	if (!dr_mode)
>> +		return;
>> +
>> +	if (!strcmp(dr_mode, "host"))
>> +		pdata->flags |= CI13XXX_DR_MODE_HOST;
>> +	else if (!strcmp(dr_mode, "peripheral"))
>> +		pdata->flags |= CI13XXX_DR_MODE_PERIPHERAL;
>> +	else if (!strcmp(dr_mode, "otg"))
>> +		pdata->flags |= CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL;
>> +}
>> +EXPORT_SYMBOL_GPL(ci13xxx_get_dr_mode);
>> +
> 
> I'd prefer this function to live in ci13xxx_imx, since that's where it's
> used and it doesn't really need anything from core.c anyway. Or maybe it
> would make sense to make it even more generic (for other devitetree
> users), since you're saying that other drivers are using this already.

+1

Move it next to the phy-mode and make it work the same way (i.e. return
an enum).

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: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/f54165f8/attachment-0001.sig>

[PATCH 4/9] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings

[Peter Chen]

On Fri, Nov 16, 2012 at 01:53:09PM +0200, Alexander Shishkin wrote:
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
> > From: Marc Kleine-Budde <mkl@pengutronix.de>
> >
> > Its necessary to limit a hostonly soc to its single role, since
> > debugging has shown that reading on the "CAP_DCCPARAMS" register inside
> > a host-only port, what ci_hdrc_gadget_init does, can lead to an
> > instable behaviour of the IC.
> 
> Probably typos: should be "it's" and "unstable".
> 
> [snip]
> 
> > --- a/drivers/usb/chipidea/ci13xxx_imx.c
> > +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > @@ -237,6 +237,8 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >  
> > +	ci13xxx_get_dr_mode(pdev->dev.of_node, pdata);
> > +
> >  	plat_ci = ci13xxx_add_device(&pdev->dev,
> >  				pdev->resource, pdev->num_resources,
> >  				pdata);
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index b50b77a..3e3e159 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -63,6 +63,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/of_platform.h>
> >  #include <linux/usb/ch9.h>
> >  #include <linux/usb/gadget.h>
> >  #include <linux/usb/otg.h>
> > @@ -521,6 +522,23 @@ void ci13xxx_remove_device(struct platform_device *pdev)
> >  }
> >  EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
> >  
> > +void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
> > +{
> > +	const unsigned char *dr_mode;
> > +
> > +	dr_mode = of_get_property(of_node, "dr_mode", NULL);
> > +	if (!dr_mode)
> > +		return;
> > +
> > +	if (!strcmp(dr_mode, "host"))
> > +		pdata->flags |= CI13XXX_DR_MODE_HOST;
> > +	else if (!strcmp(dr_mode, "peripheral"))
> > +		pdata->flags |= CI13XXX_DR_MODE_PERIPHERAL;
> > +	else if (!strcmp(dr_mode, "otg"))
> > +		pdata->flags |= CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL;
> > +}
> > +EXPORT_SYMBOL_GPL(ci13xxx_get_dr_mode);
> > +
> 
> I'd prefer this function to live in ci13xxx_imx, since that's where it's
> used and it doesn't really need anything from core.c anyway. Or maybe it
> would make sense to make it even more generic (for other devitetree
> users), since you're saying that other drivers are using this already.
> 
> Looks good to me otherwise.

I think it supplies a way that the platform can override the usb role,
only freescale uses it, but also other platforms may use it. It is a
generic feature, other chipidea users may need it.

> 
> Regards,
> --
> Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Best Regards,
Peter Chen

[PATCH 4/9] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings

[Alexander Shishkin]

Peter Chen <peter.chen@freescale.com> writes:

> On Fri, Nov 16, 2012 at 01:53:09PM +0200, Alexander Shishkin wrote:
>> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>> I'd prefer this function to live in ci13xxx_imx, since that's where it's
>> used and it doesn't really need anything from core.c anyway. Or maybe it
>> would make sense to make it even more generic (for other devitetree
>> users), since you're saying that other drivers are using this already.
>> 
>> Looks good to me otherwise.
>
> I think it supplies a way that the platform can override the usb role,
> only freescale uses it, but also other platforms may use it. It is a
> generic feature, other chipidea users may need it.

I'm not objecting against the flags, I just don't like the function in
chipidea core. Platform code can just use the flags. It can be either
part of the platform driver or a generic dt helper, in case there are
more users, it doesn't belong to chipidea core.

Regards,
--
Alex

[PATCH 5/9] usb: add phy connection by phy-mode

[Michael Grzeschik]

This patch makes it possible to set the connection of the usbphy to the
soc. It is derived from the oftree bindings for the ethernetphy and adds
similar helperfunctions.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/of/Kconfig        |    4 ++++
 drivers/of/Makefile       |    1 +
 drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_usbphy.h |   15 ++++++++++++++
 include/linux/usb/phy.h   |    8 ++++++++
 5 files changed, 77 insertions(+)
 create mode 100644 drivers/of/of_usbphy.c
 create mode 100644 include/linux/of_usbphy.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..28f99fb 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -67,6 +67,10 @@ config OF_MDIO
 	help
 	  OpenFirmware MDIO bus (Ethernet PHY) accessors
 
+config OF_USBPHY
+	depends on USB
+	def_bool y
+
 config OF_PCI
 	def_tristate PCI
 	depends on PCI
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..fdcaf51 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
 obj-$(CONFIG_OF_NET)	+= of_net.o
+obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
 obj-$(CONFIG_OF_SELFTEST) += selftest.o
 obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
new file mode 100644
index 0000000..2e71f7b
--- /dev/null
+++ b/drivers/of/of_usbphy.c
@@ -0,0 +1,49 @@
+/*
+ * OF helpers for network devices.
+ *
+ * This file is released under the GPLv2
+ *
+ * Initially copied out of drivers/of/of_net.c
+ */
+#include <linux/etherdevice.h>
+#include <linux/kernel.h>
+#include <linux/of_usbphy.h>
+#include <linux/usb/phy.h>
+#include <linux/export.h>
+
+/**
+ * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
+ * into the device tree binding of 'phy-mode', so that USB
+ * device driver can get phy interface from device tree.
+ */
+static const char *usbphy_modes[] = {
+	[USBPHY_INTERFACE_MODE_NA]	= "",
+	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
+	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
+	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
+	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
+};
+
+/**
+ * of_get_phy_mode - Get phy mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy-mode',
+ * and return its index in phy_modes table, or errno in error case.
+ */
+const int of_get_usbphy_mode(struct device_node *np)
+{
+	const char *pm;
+	int err, i;
+
+	err = of_property_read_string(np, "phy-mode", &pm);
+	if (err < 0)
+		return err;
+
+	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
+		if (!strcasecmp(pm, usbphy_modes[i]))
+			return i;
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
new file mode 100644
index 0000000..9a4132d
--- /dev/null
+++ b/include/linux/of_usbphy.h
@@ -0,0 +1,15 @@
+/*
+ * OF helpers for usb devices.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_USBPHY_H
+#define __LINUX_OF_USBPHY_H
+
+#ifdef CONFIG_OF_USBPHY
+#include <linux/of.h>
+extern const int of_get_usbphy_mode(struct device_node *np);
+#endif
+
+#endif /* __LINUX_OF_USBPHY_H */
diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index a29ae1e..150eb69 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -12,6 +12,14 @@
 #include <linux/notifier.h>
 #include <linux/usb.h>
 
+enum usb_phy_interface {
+	USBPHY_INTERFACE_MODE_NA,
+	USBPHY_INTERFACE_MODE_UTMI,
+	USBPHY_INTERFACE_MODE_UTMIW,
+	USBPHY_INTERFACE_MODE_ULPI,
+	USBPHY_INTERFACE_MODE_SERIAL,
+};
+
 enum usb_phy_events {
 	USB_EVENT_NONE,         /* no events or cable disconnected */
 	USB_EVENT_VBUS,         /* vbus valid event */
-- 
1.7.10.4

[PATCH 5/9] usb: add phy connection by phy-mode

[Alexander Shishkin]

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

> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h

This looks a bit like Felipe's call, cc'ing him.

>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */
> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> +};
> +
> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */
> +const int of_get_usbphy_mode(struct device_node *np)
> +{
> +	const char *pm;
> +	int err, i;
> +
> +	err = of_property_read_string(np, "phy-mode", &pm);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> +		if (!strcasecmp(pm, usbphy_modes[i]))
> +			return i;
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
> new file mode 100644
> index 0000000..9a4132d
> --- /dev/null
> +++ b/include/linux/of_usbphy.h
> @@ -0,0 +1,15 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_USBPHY_H
> +#define __LINUX_OF_USBPHY_H
> +
> +#ifdef CONFIG_OF_USBPHY
> +#include <linux/of.h>
> +extern const int of_get_usbphy_mode(struct device_node *np);
> +#endif
> +
> +#endif /* __LINUX_OF_USBPHY_H */
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a29ae1e..150eb69 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -12,6 +12,14 @@
>  #include <linux/notifier.h>
>  #include <linux/usb.h>
>  
> +enum usb_phy_interface {
> +	USBPHY_INTERFACE_MODE_NA,
> +	USBPHY_INTERFACE_MODE_UTMI,
> +	USBPHY_INTERFACE_MODE_UTMIW,
> +	USBPHY_INTERFACE_MODE_ULPI,
> +	USBPHY_INTERFACE_MODE_SERIAL,
> +};
> +
>  enum usb_phy_events {
>  	USB_EVENT_NONE,         /* no events or cable disconnected */
>  	USB_EVENT_VBUS,         /* vbus valid event */
> -- 
> 1.7.10.4

[PATCH 5/9] usb: add phy connection by phy-mode

[Felipe Balbi]

Hi,

On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

I think Kishon had sent some patches related to this. Kishon ?

> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */
> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> +};
> +
> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */
> +const int of_get_usbphy_mode(struct device_node *np)
> +{
> +	const char *pm;
> +	int err, i;
> +
> +	err = of_property_read_string(np, "phy-mode", &pm);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> +		if (!strcasecmp(pm, usbphy_modes[i]))
> +			return i;
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
> new file mode 100644
> index 0000000..9a4132d
> --- /dev/null
> +++ b/include/linux/of_usbphy.h
> @@ -0,0 +1,15 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_USBPHY_H
> +#define __LINUX_OF_USBPHY_H
> +
> +#ifdef CONFIG_OF_USBPHY
> +#include <linux/of.h>
> +extern const int of_get_usbphy_mode(struct device_node *np);
> +#endif
> +
> +#endif /* __LINUX_OF_USBPHY_H */
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a29ae1e..150eb69 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -12,6 +12,14 @@
>  #include <linux/notifier.h>
>  #include <linux/usb.h>
>  
> +enum usb_phy_interface {
> +	USBPHY_INTERFACE_MODE_NA,
> +	USBPHY_INTERFACE_MODE_UTMI,
> +	USBPHY_INTERFACE_MODE_UTMIW,
> +	USBPHY_INTERFACE_MODE_ULPI,
> +	USBPHY_INTERFACE_MODE_SERIAL,
> +};
> +
>  enum usb_phy_events {
>  	USB_EVENT_NONE,         /* no events or cable disconnected */
>  	USB_EVENT_VBUS,         /* vbus valid event */
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
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/20121116/91ed2506/attachment-0001.sig>

[PATCH 5/9] usb: add phy connection by phy-mode

[Felipe Balbi]

Hi,

On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */

provide proper kernel-doc

> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> +};

In fact, these would be better off as constants:

#define USBPHY_INTERFACE_MODE_UTMI	1
#define USBPHY_INTERFACE_MODE_UTMIW	2
...

> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */

why do you pass a string instead of passing an Integer ? This makes no
sense to me.

> +const int of_get_usbphy_mode(struct device_node *np)
> +{
> +	const char *pm;
> +	int err, i;
> +
> +	err = of_property_read_string(np, "phy-mode", &pm);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> +		if (!strcasecmp(pm, usbphy_modes[i]))
> +			return i;
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
> new file mode 100644
> index 0000000..9a4132d
> --- /dev/null
> +++ b/include/linux/of_usbphy.h
> @@ -0,0 +1,15 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_USBPHY_H
> +#define __LINUX_OF_USBPHY_H
> +
> +#ifdef CONFIG_OF_USBPHY
> +#include <linux/of.h>
> +extern const int of_get_usbphy_mode(struct device_node *np);
> +#endif
> +
> +#endif /* __LINUX_OF_USBPHY_H */
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a29ae1e..150eb69 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -12,6 +12,14 @@
>  #include <linux/notifier.h>
>  #include <linux/usb.h>
>  
> +enum usb_phy_interface {
> +	USBPHY_INTERFACE_MODE_NA,
> +	USBPHY_INTERFACE_MODE_UTMI,
> +	USBPHY_INTERFACE_MODE_UTMIW,
> +	USBPHY_INTERFACE_MODE_ULPI,
> +	USBPHY_INTERFACE_MODE_SERIAL,
> +};

no documentation at all here... What does UTMIW mean ? Why do you mean
the NA mode ?

-- 
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/20121116/d3cecaa9/attachment.sig>

[PATCH 5/9] usb: add phy connection by phy-mode

[Marc Kleine-Budde]

On 11/16/2012 12:31 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
>> This patch makes it possible to set the connection of the usbphy to the
>> soc. It is derived from the oftree bindings for the ethernetphy and adds
>> similar helperfunctions.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>  drivers/of/Kconfig        |    4 ++++
>>  drivers/of/Makefile       |    1 +
>>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of_usbphy.h |   15 ++++++++++++++
>>  include/linux/usb/phy.h   |    8 ++++++++
>>  5 files changed, 77 insertions(+)
>>  create mode 100644 drivers/of/of_usbphy.c
>>  create mode 100644 include/linux/of_usbphy.h
>>
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index dfba3e6..28f99fb 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -67,6 +67,10 @@ config OF_MDIO
>>  	help
>>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>>  
>> +config OF_USBPHY
>> +	depends on USB
>> +	def_bool y
>> +
>>  config OF_PCI
>>  	def_tristate PCI
>>  	depends on PCI
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index e027f44..fdcaf51 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>>  obj-$(CONFIG_OF_NET)	+= of_net.o
>> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
>> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
>> new file mode 100644
>> index 0000000..2e71f7b
>> --- /dev/null
>> +++ b/drivers/of/of_usbphy.c
>> @@ -0,0 +1,49 @@
>> +/*
>> + * OF helpers for network devices.
>> + *
>> + * This file is released under the GPLv2
>> + *
>> + * Initially copied out of drivers/of/of_net.c
>> + */
>> +#include <linux/etherdevice.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of_usbphy.h>
>> +#include <linux/usb/phy.h>
>> +#include <linux/export.h>
>> +
>> +/**
>> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
>> + * into the device tree binding of 'phy-mode', so that USB
>> + * device driver can get phy interface from device tree.
>> + */
> 
> provide proper kernel-doc
> 
>> +static const char *usbphy_modes[] = {
>> +	[USBPHY_INTERFACE_MODE_NA]	= "",
>> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
>> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
>> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
>> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
>> +};
> 
> In fact, these would be better off as constants:
> 
> #define USBPHY_INTERFACE_MODE_UTMI	1
> #define USBPHY_INTERFACE_MODE_UTMIW	2
> ...

Why are defines better than an enum? BTW: this code is a copy of the
ethernet phy of-helper code.

> 
>> +/**
>> + * of_get_phy_mode - Get phy mode for given device_node
>> + * @np:	Pointer to the given device_node
>> + *
>> + * The function gets phy interface string from property 'phy-mode',
>> + * and return its index in phy_modes table, or errno in error case.
>> + */
> 
> why do you pass a string instead of passing an Integer ? This makes no
> sense to me.

This code returns an integer or rather an enum....see header file below.

>> +const int of_get_usbphy_mode(struct device_node *np)
>> +{
>> +	const char *pm;
>> +	int err, i;
>> +
>> +	err = of_property_read_string(np, "phy-mode", &pm);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
>> +		if (!strcasecmp(pm, usbphy_modes[i]))
>> +			return i;
>> +
>> +	return -ENODEV;
>> +}
>> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
>> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
>> new file mode 100644
>> index 0000000..9a4132d
>> --- /dev/null
>> +++ b/include/linux/of_usbphy.h
>> @@ -0,0 +1,15 @@
>> +/*
>> + * OF helpers for usb devices.
>> + *
>> + * This file is released under the GPLv2
>> + */
>> +
>> +#ifndef __LINUX_OF_USBPHY_H
>> +#define __LINUX_OF_USBPHY_H
>> +
>> +#ifdef CONFIG_OF_USBPHY
>> +#include <linux/of.h>
>> +extern const int of_get_usbphy_mode(struct device_node *np);
>> +#endif
>> +
>> +#endif /* __LINUX_OF_USBPHY_H */
>> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
>> index a29ae1e..150eb69 100644
>> --- a/include/linux/usb/phy.h
>> +++ b/include/linux/usb/phy.h
>> @@ -12,6 +12,14 @@
>>  #include <linux/notifier.h>
>>  #include <linux/usb.h>
>>  
>> +enum usb_phy_interface {
>> +	USBPHY_INTERFACE_MODE_NA,
>> +	USBPHY_INTERFACE_MODE_UTMI,
>> +	USBPHY_INTERFACE_MODE_UTMIW,
>> +	USBPHY_INTERFACE_MODE_ULPI,
>> +	USBPHY_INTERFACE_MODE_SERIAL,
>> +};
> 
> no documentation at all here... What does UTMIW mean ? Why do you mean
> the NA mode ?
> 

Good point, W means wide.

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: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/3a70183b/attachment.sig>

[PATCH 5/9] usb: add phy connection by phy-mode

[Felipe Balbi]

Hi,

On Fri, Nov 16, 2012 at 12:44:19PM +0100, Marc Kleine-Budde wrote:
> >> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> >> new file mode 100644
> >> index 0000000..2e71f7b
> >> --- /dev/null
> >> +++ b/drivers/of/of_usbphy.c
> >> @@ -0,0 +1,49 @@
> >> +/*
> >> + * OF helpers for network devices.
> >> + *
> >> + * This file is released under the GPLv2
> >> + *
> >> + * Initially copied out of drivers/of/of_net.c
> >> + */
> >> +#include <linux/etherdevice.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/of_usbphy.h>
> >> +#include <linux/usb/phy.h>
> >> +#include <linux/export.h>
> >> +
> >> +/**
> >> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> >> + * into the device tree binding of 'phy-mode', so that USB
> >> + * device driver can get phy interface from device tree.
> >> + */
> > 
> > provide proper kernel-doc
> > 
> >> +static const char *usbphy_modes[] = {
> >> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> >> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> >> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> >> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> >> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> >> +};
> > 
> > In fact, these would be better off as constants:
> > 
> > #define USBPHY_INTERFACE_MODE_UTMI	1
> > #define USBPHY_INTERFACE_MODE_UTMIW	2
> > ...
> 
> Why are defines better than an enum? BTW: this code is a copy of the

because with enums can change value if you add another one in the middle
and it's really easy to miss that sort of thing during review and cause
regressions to many DTS files.

> ethernet phy of-helper code.

so ?

> >> +/**
> >> + * of_get_phy_mode - Get phy mode for given device_node
> >> + * @np:	Pointer to the given device_node
> >> + *
> >> + * The function gets phy interface string from property 'phy-mode',
> >> + * and return its index in phy_modes table, or errno in error case.
> >> + */
> > 
> > why do you pass a string instead of passing an Integer ? This makes no
> > sense to me.
> 
> This code returns an integer or rather an enum....see header file below.

I mean through DT, should've been more explicit. The dts files should
pass an integer, not a string. Then you don't need this silly conversion
helper.

-- 
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/20121116/5eb3366f/attachment.sig>

[PATCH 5/9] usb: add phy connection by phy-mode

[Marc Kleine-Budde]

On 11/16/2012 02:41 PM, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Nov 16, 2012 at 12:44:19PM +0100, Marc Kleine-Budde wrote:
>>>> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
>>>> new file mode 100644
>>>> index 0000000..2e71f7b
>>>> --- /dev/null
>>>> +++ b/drivers/of/of_usbphy.c
>>>> @@ -0,0 +1,49 @@
>>>> +/*
>>>> + * OF helpers for network devices.
>>>> + *
>>>> + * This file is released under the GPLv2
>>>> + *
>>>> + * Initially copied out of drivers/of/of_net.c
>>>> + */
>>>> +#include <linux/etherdevice.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/of_usbphy.h>
>>>> +#include <linux/usb/phy.h>
>>>> +#include <linux/export.h>
>>>> +
>>>> +/**
>>>> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
>>>> + * into the device tree binding of 'phy-mode', so that USB
>>>> + * device driver can get phy interface from device tree.
>>>> + */
>>>
>>> provide proper kernel-doc
>>>
>>>> +static const char *usbphy_modes[] = {
>>>> +	[USBPHY_INTERFACE_MODE_NA]	= "",
>>>> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
>>>> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
>>>> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
>>>> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
>>>> +};
>>>
>>> In fact, these would be better off as constants:
>>>
>>> #define USBPHY_INTERFACE_MODE_UTMI	1
>>> #define USBPHY_INTERFACE_MODE_UTMIW	2
>>> ...
>>
>> Why are defines better than an enum? BTW: this code is a copy of the
> 
> because with enums can change value if you add another one in the middle
> and it's really easy to miss that sort of thing during review and cause
> regressions to many DTS files.

Now I get it, you want to use integers instead of strings in the DT. Why
do you want to have Integers in the DT? It seems to work with strings
pretty well for the Ethernet phys, why invent a new concept here?

>> ethernet phy of-helper code.
> 
> so ?

reviewed core, proven to work, if the concept works for Ethernet phys it
should work with USB phys, too.

> 
>>>> +/**
>>>> + * of_get_phy_mode - Get phy mode for given device_node
>>>> + * @np:	Pointer to the given device_node
>>>> + *
>>>> + * The function gets phy interface string from property 'phy-mode',
>>>> + * and return its index in phy_modes table, or errno in error case.
>>>> + */
>>>
>>> why do you pass a string instead of passing an Integer ? This makes no
>>> sense to me.
>>
>> This code returns an integer or rather an enum....see header file below.
> 
> I mean through DT, should've been more explicit. The dts files should
> pass an integer, not a string. Then you don't need this silly conversion
> helper.

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: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/4a6c9851/attachment.sig>

[PATCH 5/9] usb: add phy connection by phy-mode

[Peter Chen]

On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */
> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
"fsls"? or should be "serial"?
> +};
> +

Best Regards,
Peter Chen

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Michael Grzeschik]

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: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/bits.h        |    3 +++
 drivers/usb/chipidea/ci.h          |    2 ++
 drivers/usb/chipidea/ci13xxx_imx.c |    1 +
 drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/host.c        |    4 +++
 include/linux/usb/chipidea.h       |    9 +++++++
 6 files changed, 66 insertions(+)

diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
index 4b6ae3e..3cded5f 100644
--- a/drivers/usb/chipidea/bits.h
+++ b/drivers/usb/chipidea/bits.h
@@ -48,6 +48,9 @@
 #define PORTSC_SUSP           BIT(7)
 #define PORTSC_HSP            BIT(9)
 #define PORTSC_PTC            (0x0FUL << 16)
+#define PORTSC_PTS            (BIT(31) | BIT(30))
+#define PORTSC_PTW            BIT(28)
+#define PORTSC_STS            BIT(29)
 
 /* DEVLC */
 #define DEVLC_PSPD            (0x03UL << 25)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index cd42b59..1439e51 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -314,6 +314,8 @@ static inline u32 hw_test_and_write(struct ci13xxx *ci, enum ci13xxx_regs reg,
 	return (val & mask) >> ffs_nr(mask);
 }
 
+void hw_portsc_configure(struct ci13xxx *ci);
+
 int hw_device_reset(struct ci13xxx *ci, u32 mode);
 
 int hw_port_test_set(struct ci13xxx *ci, u8 mode);
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index ee4dab0..a8257b8 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -133,6 +133,7 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		       CI13XXX_PULLUP_ON_VBUS |
 		       CI13XXX_DISABLE_STREAMING |
 		       CI13XXX_REGS_SHARED,
+	ci13xxx_get_dr_flags(pdev->dev.of_node, pdata);
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 3e3e159..7e80f1b 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -68,6 +68,8 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/chipidea.h>
+#include <linux/of_usbphy.h>
+#include <linux/phy.h>
 
 #include "ci.h"
 #include "udc.h"
@@ -213,6 +215,23 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
 	return 0;
 }
 
+void hw_portsc_configure(struct ci13xxx *ci)
+{
+	if (ci->platdata->flags & CI13XXX_PORTSC_PTW_16BIT)
+		hw_write(ci, OP_PORTSC, PORTSC_PTW, 0x1 << ffs_nr(PORTSC_PTW));
+
+	if (ci->platdata->flags & CI13XXX_PORTSC_PTS_UTMI) {
+		hw_write(ci, OP_PORTSC, PORTSC_PTS, 0x0 << ffs_nr(PORTSC_PTS));
+		hw_write(ci, OP_PORTSC, PORTSC_STS, 0x0 << ffs_nr(PORTSC_STS));
+	} else if (ci->platdata->flags & CI13XXX_PORTSC_PTS_ULPI) {
+		hw_write(ci, OP_PORTSC, PORTSC_PTS, 0x2 << ffs_nr(PORTSC_PTS));
+		hw_write(ci, OP_PORTSC, PORTSC_STS, 0x0 << ffs_nr(PORTSC_STS));
+	} else if (ci->platdata->flags & CI13XXX_PORTSC_PTS_FSLS) {
+		hw_write(ci, OP_PORTSC, PORTSC_PTS, 0x3 << ffs_nr(PORTSC_PTS));
+		hw_write(ci, OP_PORTSC, PORTSC_STS, 0x1 << ffs_nr(PORTSC_STS));
+	}
+}
+
 /**
  * hw_device_reset: resets chip (execute without interruption)
  * @ci: the controller
@@ -237,6 +256,8 @@ int hw_device_reset(struct ci13xxx *ci, u32 mode)
 	if (ci->platdata->flags & CI13XXX_DISABLE_STREAMING)
 		hw_write(ci, OP_USBMODE, USBMODE_CI_SDIS, USBMODE_CI_SDIS);
 
+	hw_portsc_configure(ci);
+
 	/* USBMODE should be configured step by step */
 	hw_write(ci, OP_USBMODE, USBMODE_CM, USBMODE_CM_IDLE);
 	hw_write(ci, OP_USBMODE, USBMODE_CM, mode);
@@ -522,6 +543,32 @@ void ci13xxx_remove_device(struct platform_device *pdev)
 }
 EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
 
+void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
+{
+	int interface = of_get_usbphy_mode(of_node);
+
+	switch (interface) {
+	case USBPHY_INTERFACE_MODE_UTMI:
+		pdata->flags |= CI13XXX_PORTSC_PTS_UTMI;
+		break;
+	case USBPHY_INTERFACE_MODE_UTMIW:
+		pdata->flags |= CI13XXX_PORTSC_PTS_UTMI |
+			CI13XXX_PORTSC_PTW_16BIT;
+		break;
+	case USBPHY_INTERFACE_MODE_ULPI:
+		pdata->flags |= CI13XXX_PORTSC_PTS_ULPI;
+		break;
+	case USBPHY_INTERFACE_MODE_SERIAL:
+		pdata->flags |= CI13XXX_PORTSC_PTS_FSLS;
+		break;
+	case USBPHY_INTERFACE_MODE_NA:
+	default:
+		pr_err("no phy interface defined\n");
+	}
+
+}
+EXPORT_SYMBOL_GPL(ci13xxx_get_dr_flags);
+
 void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
 {
 	const unsigned char *dr_mode;
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index ebff9f4..b23ee1d 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -106,6 +106,10 @@ static int host_start(struct ci13xxx *ci)
 	if (usb_disabled())
 		return -ENODEV;
 
+	hw_portsc_configure(ci);
+
+	mdelay(10);
+
 	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
 	if (!hcd)
 		return -ENOMEM;
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 906d259..cafca23 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -21,6 +21,12 @@ struct ci13xxx_platform_data {
 #define CI13XXX_DISABLE_STREAMING	BIT(3)
 #define CI13XXX_DR_MODE_HOST		BIT(4)
 #define CI13XXX_DR_MODE_PERIPHERAL	BIT(5)
+#define CI13XXX_PORTSC_PTW_8BIT		BIT(6)
+#define CI13XXX_PORTSC_PTW_16BIT	BIT(7)
+#define CI13XXX_PORTSC_PTS_UTMI		BIT(8)
+#define CI13XXX_PORTSC_PTS_ULPI		BIT(9)
+#define CI13XXX_PORTSC_PTS_FSLS		BIT(10)
+
 #define CI13XXX_DR_MODE_MASK \
 	(CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL)
 
@@ -42,4 +48,7 @@ void ci13xxx_remove_device(struct platform_device *pdev);
 /* Parse of-tree "dr_mode" property */
 void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
 
+/* Parse of-tree "flags" */
+void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
+
 #endif
-- 
1.7.10.4

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Alexander Shishkin]

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

> 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: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/usb/chipidea/bits.h        |    3 +++
>  drivers/usb/chipidea/ci.h          |    2 ++
>  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
>  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
>  drivers/usb/chipidea/host.c        |    4 +++
>  include/linux/usb/chipidea.h       |    9 +++++++
>  6 files changed, 66 insertions(+)
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 4b6ae3e..3cded5f 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -48,6 +48,9 @@
>  #define PORTSC_SUSP           BIT(7)
>  #define PORTSC_HSP            BIT(9)
>  #define PORTSC_PTC            (0x0FUL << 16)
> +#define PORTSC_PTS            (BIT(31) | BIT(30))
> +#define PORTSC_PTW            BIT(28)
> +#define PORTSC_STS            BIT(29)
>  
>  /* DEVLC */
>  #define DEVLC_PSPD            (0x03UL << 25)
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index cd42b59..1439e51 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -314,6 +314,8 @@ static inline u32 hw_test_and_write(struct ci13xxx *ci, enum ci13xxx_regs reg,
>  	return (val & mask) >> ffs_nr(mask);
>  }
>  
> +void hw_portsc_configure(struct ci13xxx *ci);
> +
>  int hw_device_reset(struct ci13xxx *ci, u32 mode);
>  
>  int hw_port_test_set(struct ci13xxx *ci, u8 mode);
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index ee4dab0..a8257b8 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -133,6 +133,7 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>  		       CI13XXX_PULLUP_ON_VBUS |
>  		       CI13XXX_DISABLE_STREAMING |
>  		       CI13XXX_REGS_SHARED,
> +	ci13xxx_get_dr_flags(pdev->dev.of_node, pdata);

It will be a bit easier to read if this one and ci13xxx_get_dr_mode()
calls are next to one another. Also my comment about get_dr_mode()
probably applies here too.

Regards,
--
Alex

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Alexander Shishkin]

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

> 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: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/usb/chipidea/bits.h        |    3 +++
>  drivers/usb/chipidea/ci.h          |    2 ++
>  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
>  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
>  drivers/usb/chipidea/host.c        |    4 +++
>  include/linux/usb/chipidea.h       |    9 +++++++
>  6 files changed, 66 insertions(+)
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 4b6ae3e..3cded5f 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -48,6 +48,9 @@
>  #define PORTSC_SUSP           BIT(7)
>  #define PORTSC_HSP            BIT(9)
>  #define PORTSC_PTC            (0x0FUL << 16)
> +#define PORTSC_PTS            (BIT(31) | BIT(30))
> +#define PORTSC_PTW            BIT(28)
> +#define PORTSC_STS            BIT(29)

Hm, my spec says these are actually in DEVLC register and only have this
meaning in device mode. And in portsc these bits fall in device address
bitfield. Can you refer me to your spec?

Regards,
--
Alex

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Michael Grzeschik]

On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
> > 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: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  drivers/usb/chipidea/bits.h        |    3 +++
> >  drivers/usb/chipidea/ci.h          |    2 ++
> >  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
> >  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
> >  drivers/usb/chipidea/host.c        |    4 +++
> >  include/linux/usb/chipidea.h       |    9 +++++++
> >  6 files changed, 66 insertions(+)
> >
> > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> > index 4b6ae3e..3cded5f 100644
> > --- a/drivers/usb/chipidea/bits.h
> > +++ b/drivers/usb/chipidea/bits.h
> > @@ -48,6 +48,9 @@
> >  #define PORTSC_SUSP           BIT(7)
> >  #define PORTSC_HSP            BIT(9)
> >  #define PORTSC_PTC            (0x0FUL << 16)
> > +#define PORTSC_PTS            (BIT(31) | BIT(30))
> > +#define PORTSC_PTW            BIT(28)
> > +#define PORTSC_STS            BIT(29)
> 
> Hm, my spec says these are actually in DEVLC register and only have this
> meaning in device mode. And in portsc these bits fall in device address
> bitfield. Can you refer me to your spec?

You can find it here:
http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
Page 4947

We as well do have the original documentation from Synopsys in which there
there is an DEVLC register in the index, but nowhere else in the Datasheet.

June 2011 - Doc.Rev. 2.40a
USB 2.0 High Speed Atlantic Controller

Unfortunately, its not possible for me to share this document.

Thanks,
Michael
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Alexander Shishkin]

Michael Grzeschik <mgr@pengutronix.de> writes:

> On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
>> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>> 
>> > 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: Marc Kleine-Budde <mkl@pengutronix.de>
>> > ---
>> >  drivers/usb/chipidea/bits.h        |    3 +++
>> >  drivers/usb/chipidea/ci.h          |    2 ++
>> >  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
>> >  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
>> >  drivers/usb/chipidea/host.c        |    4 +++
>> >  include/linux/usb/chipidea.h       |    9 +++++++
>> >  6 files changed, 66 insertions(+)
>> >
>> > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>> > index 4b6ae3e..3cded5f 100644
>> > --- a/drivers/usb/chipidea/bits.h
>> > +++ b/drivers/usb/chipidea/bits.h
>> > @@ -48,6 +48,9 @@
>> >  #define PORTSC_SUSP           BIT(7)
>> >  #define PORTSC_HSP            BIT(9)
>> >  #define PORTSC_PTC            (0x0FUL << 16)
>> > +#define PORTSC_PTS            (BIT(31) | BIT(30))
>> > +#define PORTSC_PTW            BIT(28)
>> > +#define PORTSC_STS            BIT(29)
>> 
>> Hm, my spec says these are actually in DEVLC register and only have this
>> meaning in device mode. And in portsc these bits fall in device address
>> bitfield. Can you refer me to your spec?
>
> You can find it here:
> http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
> Page 4947

Oh, but see, the offset is 0x184, which in chipidea spec (the version
that I have) corresponds to DEVLC and not PORTSC. So in this driver's
terminology it's DEVLC too, at least currently.

So have you tested this code and did it make any difference?

> We as well do have the original documentation from Synopsys in which there
> there is an DEVLC register in the index, but nowhere else in the Datasheet.

Maybe it's a leftover from the old terminology, which would explain the
confusion.

> June 2011 - Doc.Rev. 2.40a
> USB 2.0 High Speed Atlantic Controller

Yes, mine is from 2009.

Regards,
--
Alex

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Michael Grzeschik]

On Fri, Nov 16, 2012 at 03:34:23PM +0200, Alexander Shishkin wrote:
> Michael Grzeschik <mgr@pengutronix.de> writes:
> 
> > On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
> >> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> >> 
> >> > 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: Marc Kleine-Budde <mkl@pengutronix.de>
> >> > ---
> >> >  drivers/usb/chipidea/bits.h        |    3 +++
> >> >  drivers/usb/chipidea/ci.h          |    2 ++
> >> >  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
> >> >  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
> >> >  drivers/usb/chipidea/host.c        |    4 +++
> >> >  include/linux/usb/chipidea.h       |    9 +++++++
> >> >  6 files changed, 66 insertions(+)
> >> >
> >> > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> >> > index 4b6ae3e..3cded5f 100644
> >> > --- a/drivers/usb/chipidea/bits.h
> >> > +++ b/drivers/usb/chipidea/bits.h
> >> > @@ -48,6 +48,9 @@
> >> >  #define PORTSC_SUSP           BIT(7)
> >> >  #define PORTSC_HSP            BIT(9)
> >> >  #define PORTSC_PTC            (0x0FUL << 16)
> >> > +#define PORTSC_PTS            (BIT(31) | BIT(30))
> >> > +#define PORTSC_PTW            BIT(28)
> >> > +#define PORTSC_STS            BIT(29)
> >> 
> >> Hm, my spec says these are actually in DEVLC register and only have this
> >> meaning in device mode. And in portsc these bits fall in device address
> >> bitfield. Can you refer me to your spec?
> >
> > You can find it here:
> > http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
> > Page 4947
> 
> Oh, but see, the offset is 0x184, which in chipidea spec (the version
> that I have) corresponds to DEVLC and not PORTSC. So in this driver's
> terminology it's DEVLC too, at least currently.
> 
> So have you tested this code and did it make any difference?

Yes, i have tested this code with MX25, MX28, MX35 and MX53. In every
SoCs Datasheet the PORTSC register is defined on PORTBASE+0x184. Without
this proper configuration its not possible to communicate with the PHY.

> > We as well do have the original documentation from Synopsys in which there
> > there is an DEVLC register in the index, but nowhere else in the Datasheet.
> 
> Maybe it's a leftover from the old terminology, which would explain the
> confusion.

ACK

> > June 2011 - Doc.Rev. 2.40a
> > USB 2.0 High Speed Atlantic Controller
> Yes, mine is from 2009.

Probably worth to get updatet.

Michael
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Alexander Shishkin]

Michael Grzeschik <mgr@pengutronix.de> writes:

> On Fri, Nov 16, 2012 at 03:34:23PM +0200, Alexander Shishkin wrote:
>> Michael Grzeschik <mgr@pengutronix.de> writes:
>> 
>> > On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
>> >> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>> >> 
>> >> > 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: Marc Kleine-Budde <mkl@pengutronix.de>
>> >> > ---
>> >> >  drivers/usb/chipidea/bits.h        |    3 +++
>> >> >  drivers/usb/chipidea/ci.h          |    2 ++
>> >> >  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
>> >> >  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
>> >> >  drivers/usb/chipidea/host.c        |    4 +++
>> >> >  include/linux/usb/chipidea.h       |    9 +++++++
>> >> >  6 files changed, 66 insertions(+)
>> >> >
>> >> > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>> >> > index 4b6ae3e..3cded5f 100644
>> >> > --- a/drivers/usb/chipidea/bits.h
>> >> > +++ b/drivers/usb/chipidea/bits.h
>> >> > @@ -48,6 +48,9 @@
>> >> >  #define PORTSC_SUSP           BIT(7)
>> >> >  #define PORTSC_HSP            BIT(9)
>> >> >  #define PORTSC_PTC            (0x0FUL << 16)
>> >> > +#define PORTSC_PTS            (BIT(31) | BIT(30))
>> >> > +#define PORTSC_PTW            BIT(28)
>> >> > +#define PORTSC_STS            BIT(29)
>> >> 
>> >> Hm, my spec says these are actually in DEVLC register and only have this
>> >> meaning in device mode. And in portsc these bits fall in device address
>> >> bitfield. Can you refer me to your spec?
>> >
>> > You can find it here:
>> > http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
>> > Page 4947
>> 
>> Oh, but see, the offset is 0x184, which in chipidea spec (the version
>> that I have) corresponds to DEVLC and not PORTSC. So in this driver's
>> terminology it's DEVLC too, at least currently.
>> 
>> So have you tested this code and did it make any difference?
>
> Yes, i have tested this code with MX25, MX28, MX35 and MX53. In every
> SoCs Datasheet the PORTSC register is defined on PORTBASE+0x184. Without
> this proper configuration its not possible to communicate with the PHY.

No, I mean, you're writing DEVLC (using present driver's terminology)
bits to PORTSC register. It *shouldn't* work. I suppose, it does
something, but not exactly what you intended.

Regards,
--
Alex

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Matthieu CASTET]

Alexander Shishkin a ?crit :
> Michael Grzeschik <mgr@pengutronix.de> writes:
> 
>> On Fri, Nov 16, 2012 at 03:34:23PM +0200, Alexander Shishkin wrote:
>>> Michael Grzeschik <mgr@pengutronix.de> writes:
>>>
>>>> On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
>>>>> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>>>>>
>>>>>> 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: Marc Kleine-Budde <mkl@pengutronix.de>
>>>>>> ---
>>>>>>  drivers/usb/chipidea/bits.h        |    3 +++
>>>>>>  drivers/usb/chipidea/ci.h          |    2 ++
>>>>>>  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
>>>>>>  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
>>>>>>  drivers/usb/chipidea/host.c        |    4 +++
>>>>>>  include/linux/usb/chipidea.h       |    9 +++++++
>>>>>>  6 files changed, 66 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>>>>>> index 4b6ae3e..3cded5f 100644
>>>>>> --- a/drivers/usb/chipidea/bits.h
>>>>>> +++ b/drivers/usb/chipidea/bits.h
>>>>>> @@ -48,6 +48,9 @@
>>>>>>  #define PORTSC_SUSP           BIT(7)
>>>>>>  #define PORTSC_HSP            BIT(9)
>>>>>>  #define PORTSC_PTC            (0x0FUL << 16)
>>>>>> +#define PORTSC_PTS            (BIT(31) | BIT(30))
>>>>>> +#define PORTSC_PTW            BIT(28)
>>>>>> +#define PORTSC_STS            BIT(29)
>>>>> Hm, my spec says these are actually in DEVLC register and only have this
>>>>> meaning in device mode. And in portsc these bits fall in device address
>>>>> bitfield. Can you refer me to your spec?
>>>> You can find it here:
>>>> http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
>>>> Page 4947
>>> Oh, but see, the offset is 0x184, which in chipidea spec (the version
>>> that I have) corresponds to DEVLC and not PORTSC. So in this driver's
>>> terminology it's DEVLC too, at least currently.
>>>
>>> So have you tested this code and did it make any difference?
>> Yes, i have tested this code with MX25, MX28, MX35 and MX53. In every
>> SoCs Datasheet the PORTSC register is defined on PORTBASE+0x184. Without
>> this proper configuration its not possible to communicate with the PHY.
> 
> No, I mean, you're writing DEVLC (using present driver's terminology)
> bits to PORTSC register. It *shouldn't* work. I suppose, it does
> something, but not exactly what you intended.
> 
I confirm in our datasheet we have PTW, PTS, STS in this register  PORTSCx with
this mapping.

In later design that support lpm, these bits have to move in another register
because ehci 1.1 use them [1].

So this code should only do that when !ci->hw_bank.lpm.
In case of ci->hw_bank.lpm, I don't know the new register mapping.


Matthieu



[1]
/* EHCI 1.1 addendum */
#define PORTSC_SUSPEND_STS_ACK 0
#define PORTSC_SUSPEND_STS_NYET 1
#define PORTSC_SUSPEND_STS_STALL 2
#define PORTSC_SUSPEND_STS_ERR 3

#define PORT_DEV_ADDR   (0x7f<<25)      /* device address */
#define PORT_SSTS   (0x3<<23)       /* suspend status */

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Alexander Shishkin]

Matthieu CASTET <matthieu.castet@parrot.com> writes:

> Alexander Shishkin a ?crit :
>> Michael Grzeschik <mgr@pengutronix.de> writes:
>> 
>>> On Fri, Nov 16, 2012 at 03:34:23PM +0200, Alexander Shishkin wrote:
>>>> Michael Grzeschik <mgr@pengutronix.de> writes:
>>>>
>>>>> On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
>>>>>> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>>>>>>
>>>>>>> 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: Marc Kleine-Budde <mkl@pengutronix.de>
>>>>>>> ---
>>>>>>>  drivers/usb/chipidea/bits.h        |    3 +++
>>>>>>>  drivers/usb/chipidea/ci.h          |    2 ++
>>>>>>>  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
>>>>>>>  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
>>>>>>>  drivers/usb/chipidea/host.c        |    4 +++
>>>>>>>  include/linux/usb/chipidea.h       |    9 +++++++
>>>>>>>  6 files changed, 66 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>>>>>>> index 4b6ae3e..3cded5f 100644
>>>>>>> --- a/drivers/usb/chipidea/bits.h
>>>>>>> +++ b/drivers/usb/chipidea/bits.h
>>>>>>> @@ -48,6 +48,9 @@
>>>>>>>  #define PORTSC_SUSP           BIT(7)
>>>>>>>  #define PORTSC_HSP            BIT(9)
>>>>>>>  #define PORTSC_PTC            (0x0FUL << 16)
>>>>>>> +#define PORTSC_PTS            (BIT(31) | BIT(30))
>>>>>>> +#define PORTSC_PTW            BIT(28)
>>>>>>> +#define PORTSC_STS            BIT(29)
>>>>>> Hm, my spec says these are actually in DEVLC register and only have this
>>>>>> meaning in device mode. And in portsc these bits fall in device address
>>>>>> bitfield. Can you refer me to your spec?
>>>>> You can find it here:
>>>>> http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
>>>>> Page 4947
>>>> Oh, but see, the offset is 0x184, which in chipidea spec (the version
>>>> that I have) corresponds to DEVLC and not PORTSC. So in this driver's
>>>> terminology it's DEVLC too, at least currently.
>>>>
>>>> So have you tested this code and did it make any difference?
>>> Yes, i have tested this code with MX25, MX28, MX35 and MX53. In every
>>> SoCs Datasheet the PORTSC register is defined on PORTBASE+0x184. Without
>>> this proper configuration its not possible to communicate with the PHY.
>> 
>> No, I mean, you're writing DEVLC (using present driver's terminology)
>> bits to PORTSC register. It *shouldn't* work. I suppose, it does
>> something, but not exactly what you intended.
>> 
> I confirm in our datasheet we have PTW, PTS, STS in this register  PORTSCx with
> this mapping.
>
> In later design that support lpm, these bits have to move in another register
> because ehci 1.1 use them [1].

Ahh, I didn't realize imxes were nolpm. Now it makes more sense. This
means that, as you suggested in the other mail, these portsc/devlc
accesses should be done conditionally based on ci->hw_bank.lpm.

Regards,
--
Alex

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Michael Grzeschik]

On Fri, Nov 16, 2012 at 05:39:42PM +0200, Alexander Shishkin wrote:
> Matthieu CASTET <matthieu.castet@parrot.com> writes:
> 
> > Alexander Shishkin a ?crit :
> >> Michael Grzeschik <mgr@pengutronix.de> writes:
> >> 
> >>> On Fri, Nov 16, 2012 at 03:34:23PM +0200, Alexander Shishkin wrote:
> >>>> Michael Grzeschik <mgr@pengutronix.de> writes:
> >>>>
> >>>>> On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
> >>>>>> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> >>>>>>
> >>>>>>> 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: Marc Kleine-Budde <mkl@pengutronix.de>
> >>>>>>> ---
> >>>>>>>  drivers/usb/chipidea/bits.h        |    3 +++
> >>>>>>>  drivers/usb/chipidea/ci.h          |    2 ++
> >>>>>>>  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
> >>>>>>>  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
> >>>>>>>  drivers/usb/chipidea/host.c        |    4 +++
> >>>>>>>  include/linux/usb/chipidea.h       |    9 +++++++
> >>>>>>>  6 files changed, 66 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> >>>>>>> index 4b6ae3e..3cded5f 100644
> >>>>>>> --- a/drivers/usb/chipidea/bits.h
> >>>>>>> +++ b/drivers/usb/chipidea/bits.h
> >>>>>>> @@ -48,6 +48,9 @@
> >>>>>>>  #define PORTSC_SUSP           BIT(7)
> >>>>>>>  #define PORTSC_HSP            BIT(9)
> >>>>>>>  #define PORTSC_PTC            (0x0FUL << 16)
> >>>>>>> +#define PORTSC_PTS            (BIT(31) | BIT(30))
> >>>>>>> +#define PORTSC_PTW            BIT(28)
> >>>>>>> +#define PORTSC_STS            BIT(29)
> >>>>>> Hm, my spec says these are actually in DEVLC register and only have this
> >>>>>> meaning in device mode. And in portsc these bits fall in device address
> >>>>>> bitfield. Can you refer me to your spec?
> >>>>> You can find it here:
> >>>>> http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
> >>>>> Page 4947
> >>>> Oh, but see, the offset is 0x184, which in chipidea spec (the version
> >>>> that I have) corresponds to DEVLC and not PORTSC. So in this driver's
> >>>> terminology it's DEVLC too, at least currently.
> >>>>
> >>>> So have you tested this code and did it make any difference?
> >>> Yes, i have tested this code with MX25, MX28, MX35 and MX53. In every
> >>> SoCs Datasheet the PORTSC register is defined on PORTBASE+0x184. Without
> >>> this proper configuration its not possible to communicate with the PHY.
> >> 
> >> No, I mean, you're writing DEVLC (using present driver's terminology)
> >> bits to PORTSC register. It *shouldn't* work. I suppose, it does
> >> something, but not exactly what you intended.
> >> 
> > I confirm in our datasheet we have PTW, PTS, STS in this register  PORTSCx with
> > this mapping.
> >
> > In later design that support lpm, these bits have to move in another register
> > because ehci 1.1 use them [1].
> 
> Ahh, I didn't realize imxes were nolpm. Now it makes more sense. This
> means that, as you suggested in the other mail, these portsc/devlc
> accesses should be done conditionally based on ci->hw_bank.lpm.

To handle lpm devices in this patch, we need some register layout of the
lpm core. Probably only te DEVLC register. Is there some documentation
available?

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Matthieu CASTET]

Michael Grzeschik a ?crit :
> On Fri, Nov 16, 2012 at 05:39:42PM +0200, Alexander Shishkin wrote:
>> Matthieu CASTET <matthieu.castet@parrot.com> writes:
>>
>>> Alexander Shishkin a ?crit :
>>>> Michael Grzeschik <mgr@pengutronix.de> writes:
>>>>
>>>>> On Fri, Nov 16, 2012 at 03:34:23PM +0200, Alexander Shishkin wrote:
>>>>>> Michael Grzeschik <mgr@pengutronix.de> writes:
>>>>>>
>>>>>>> On Fri, Nov 16, 2012 at 02:45:39PM +0200, Alexander Shishkin wrote:
>>>>>>>> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>>>>>>>>
>>>>>>>>> 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: Marc Kleine-Budde <mkl@pengutronix.de>
>>>>>>>>> ---
>>>>>>>>>  drivers/usb/chipidea/bits.h        |    3 +++
>>>>>>>>>  drivers/usb/chipidea/ci.h          |    2 ++
>>>>>>>>>  drivers/usb/chipidea/ci13xxx_imx.c |    1 +
>>>>>>>>>  drivers/usb/chipidea/core.c        |   47 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>  drivers/usb/chipidea/host.c        |    4 +++
>>>>>>>>>  include/linux/usb/chipidea.h       |    9 +++++++
>>>>>>>>>  6 files changed, 66 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>>>>>>>>> index 4b6ae3e..3cded5f 100644
>>>>>>>>> --- a/drivers/usb/chipidea/bits.h
>>>>>>>>> +++ b/drivers/usb/chipidea/bits.h
>>>>>>>>> @@ -48,6 +48,9 @@
>>>>>>>>>  #define PORTSC_SUSP           BIT(7)
>>>>>>>>>  #define PORTSC_HSP            BIT(9)
>>>>>>>>>  #define PORTSC_PTC            (0x0FUL << 16)
>>>>>>>>> +#define PORTSC_PTS            (BIT(31) | BIT(30))
>>>>>>>>> +#define PORTSC_PTW            BIT(28)
>>>>>>>>> +#define PORTSC_STS            BIT(29)
>>>>>>>> Hm, my spec says these are actually in DEVLC register and only have this
>>>>>>>> meaning in device mode. And in portsc these bits fall in device address
>>>>>>>> bitfield. Can you refer me to your spec?
>>>>>>> You can find it here:
>>>>>>> http://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf?fpsp=1
>>>>>>> Page 4947
>>>>>> Oh, but see, the offset is 0x184, which in chipidea spec (the version
>>>>>> that I have) corresponds to DEVLC and not PORTSC. So in this driver's
>>>>>> terminology it's DEVLC too, at least currently.
>>>>>>
>>>>>> So have you tested this code and did it make any difference?
>>>>> Yes, i have tested this code with MX25, MX28, MX35 and MX53. In every
>>>>> SoCs Datasheet the PORTSC register is defined on PORTBASE+0x184. Without
>>>>> this proper configuration its not possible to communicate with the PHY.
>>>> No, I mean, you're writing DEVLC (using present driver's terminology)
>>>> bits to PORTSC register. It *shouldn't* work. I suppose, it does
>>>> something, but not exactly what you intended.
>>>>
>>> I confirm in our datasheet we have PTW, PTS, STS in this register  PORTSCx with
>>> this mapping.
>>>
>>> In later design that support lpm, these bits have to move in another register
>>> because ehci 1.1 use them [1].
>> Ahh, I didn't realize imxes were nolpm. Now it makes more sense. This
>> means that, as you suggested in the other mail, these portsc/devlc
>> accesses should be done conditionally based on ci->hw_bank.lpm.
> 
> To handle lpm devices in this patch, we need some register layout of the
> lpm core. Probably only te DEVLC register. Is there some documentation
> available?

In include/linux/usb/langwell_udc.h before it was removed, you could find the
mapping :

    u32 devlc;      /* control LPM and each USB port behavior */
/* bits 31:29, parallel transceiver select */
#define LPM_PTS(d)  (((d)>>29)&7)
#define LPM_STS     BIT(28) /* serial transceiver select */
#define LPM_PTW     BIT(27) /* parallel transceiver width */
#define LPM_PSPD(d) (((d)>>25)&3)   /* bits 26:25, port speed */
#define LPM_PSPD_MASK   (BIT(26) | BIT(25))
#define LPM_SPEED_FULL  0
#define LPM_SPEED_LOW   1
#define LPM_SPEED_HIGH  2
#define LPM_SRT     BIT(24) /* shorten reset time */
#define LPM_PFSC    BIT(23) /* port force full speed connect */
#define LPM_PHCD    BIT(22) /* PHY low power suspend clock disable */
#define LPM_STL     BIT(16) /* STALL reply to LPM token */
#define LPM_BA(d)   \
    (((d)>>1)&0x7ff)    /* bits 11:1, BmAttributes */
#define LPM_NYT_ACK BIT(0)  /* NYET/ACK reply to LPM token */

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Peter Chen]

On Wed, Nov 14, 2012 at 05:19:07PM +0100, Michael Grzeschik wrote:
>  /**
>   * hw_device_reset: resets chip (execute without interruption)
>   * @ci: the controller
> @@ -237,6 +256,8 @@ int hw_device_reset(struct ci13xxx *ci, u32 mode)
>  	if (ci->platdata->flags & CI13XXX_DISABLE_STREAMING)
>  		hw_write(ci, OP_USBMODE, USBMODE_CI_SDIS, USBMODE_CI_SDIS);
>  
> +	hw_portsc_configure(ci);
> +
>  	/* USBMODE should be configured step by step */
>  	hw_write(ci, OP_USBMODE, USBMODE_CM, USBMODE_CM_IDLE);
>  	hw_write(ci, OP_USBMODE, USBMODE_CM, mode);
> @@ -522,6 +543,32 @@ void ci13xxx_remove_device(struct platform_device *pdev)
>  }
>  EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
>  
> +void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
> +{
> +	int interface = of_get_usbphy_mode(of_node);
> +
> +	switch (interface) {
> +	case USBPHY_INTERFACE_MODE_UTMI:
> +		pdata->flags |= CI13XXX_PORTSC_PTS_UTMI;
> +		break;
> +	case USBPHY_INTERFACE_MODE_UTMIW:
> +		pdata->flags |= CI13XXX_PORTSC_PTS_UTMI |
> +			CI13XXX_PORTSC_PTW_16BIT;
> +		break;
> +	case USBPHY_INTERFACE_MODE_ULPI:
> +		pdata->flags |= CI13XXX_PORTSC_PTS_ULPI;
> +		break;
> +	case USBPHY_INTERFACE_MODE_SERIAL:
> +		pdata->flags |= CI13XXX_PORTSC_PTS_FSLS;
> +		break;
> +	case USBPHY_INTERFACE_MODE_NA:
> +	default:
> +		pr_err("no phy interface defined\n");
> +	}
> +
> +}
> +EXPORT_SYMBOL_GPL(ci13xxx_get_dr_flags);
> +
>  void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
>  {
>  	const unsigned char *dr_mode;
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index ebff9f4..b23ee1d 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -106,6 +106,10 @@ static int host_start(struct ci13xxx *ci)
>  	if (usb_disabled())
>  		return -ENODEV;
>  
> +	hw_portsc_configure(ci);
> +
> +	mdelay(10);
is usleep_range ok?
Why host needs 10ms delay, but peripheral mode doesn't?

> +
>  	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
>  	if (!hcd)
>  		return -ENOMEM;
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index 906d259..cafca23 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -21,6 +21,12 @@ struct ci13xxx_platform_data {
>  #define CI13XXX_DISABLE_STREAMING	BIT(3)
>  #define CI13XXX_DR_MODE_HOST		BIT(4)
>  #define CI13XXX_DR_MODE_PERIPHERAL	BIT(5)
> +#define CI13XXX_PORTSC_PTW_8BIT		BIT(6)
> +#define CI13XXX_PORTSC_PTW_16BIT	BIT(7)
> +#define CI13XXX_PORTSC_PTS_UTMI		BIT(8)
> +#define CI13XXX_PORTSC_PTS_ULPI		BIT(9)
> +#define CI13XXX_PORTSC_PTS_FSLS		BIT(10)
> +
>  #define CI13XXX_DR_MODE_MASK \
>  	(CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL)
>  
> @@ -42,4 +48,7 @@ void ci13xxx_remove_device(struct platform_device *pdev);
>  /* Parse of-tree "dr_mode" property */
>  void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
>  
> +/* Parse of-tree "flags" */
> +void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
> +
>  #endif
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Best Regards,
Peter Chen

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Michael Grzeschik]

On Tue, Nov 27, 2012 at 09:12:30AM +0800, Peter Chen wrote:
> On Wed, Nov 14, 2012 at 05:19:07PM +0100, Michael Grzeschik wrote:
> >  /**
> >   * hw_device_reset: resets chip (execute without interruption)
> >   * @ci: the controller
> > @@ -237,6 +256,8 @@ int hw_device_reset(struct ci13xxx *ci, u32 mode)
> >  	if (ci->platdata->flags & CI13XXX_DISABLE_STREAMING)
> >  		hw_write(ci, OP_USBMODE, USBMODE_CI_SDIS, USBMODE_CI_SDIS);
> >  
> > +	hw_portsc_configure(ci);
> > +

This can be dropped.

> >  	/* USBMODE should be configured step by step */
> >  	hw_write(ci, OP_USBMODE, USBMODE_CM, USBMODE_CM_IDLE);
> >  	hw_write(ci, OP_USBMODE, USBMODE_CM, mode);
> > @@ -522,6 +543,32 @@ void ci13xxx_remove_device(struct platform_device *pdev)
> >  }
> >  EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
> >  
> > +void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
> > +{
> > +	int interface = of_get_usbphy_mode(of_node);
> > +
> > +	switch (interface) {
> > +	case USBPHY_INTERFACE_MODE_UTMI:
> > +		pdata->flags |= CI13XXX_PORTSC_PTS_UTMI;
> > +		break;
> > +	case USBPHY_INTERFACE_MODE_UTMIW:
> > +		pdata->flags |= CI13XXX_PORTSC_PTS_UTMI |
> > +			CI13XXX_PORTSC_PTW_16BIT;
> > +		break;
> > +	case USBPHY_INTERFACE_MODE_ULPI:
> > +		pdata->flags |= CI13XXX_PORTSC_PTS_ULPI;
> > +		break;
> > +	case USBPHY_INTERFACE_MODE_SERIAL:
> > +		pdata->flags |= CI13XXX_PORTSC_PTS_FSLS;
> > +		break;
> > +	case USBPHY_INTERFACE_MODE_NA:
> > +	default:
> > +		pr_err("no phy interface defined\n");
> > +	}
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(ci13xxx_get_dr_flags);
> > +
> >  void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata)
> >  {
> >  	const unsigned char *dr_mode;
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index ebff9f4..b23ee1d 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -106,6 +106,10 @@ static int host_start(struct ci13xxx *ci)
> >  	if (usb_disabled())
> >  		return -ENODEV;
> >  
> > +	hw_portsc_configure(ci);
> > +
> > +	mdelay(10);
> is usleep_range ok?

I will change that.

> Why host needs 10ms delay, but peripheral mode doesn't?

I didn't figure out why this is needed. However, we prefer to move this
hunk to be in ci_hdrc_probe just before ci_role_start gets called. So
hw_portsc_configure will be called only there in the beginning. As the
phy setup doesn't change, we need no further call and can take the delay
unconditionally of the role.

> > +
> >  	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> >  	if (!hcd)
> >  		return -ENOMEM;
> > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> > index 906d259..cafca23 100644
> > --- a/include/linux/usb/chipidea.h
> > +++ b/include/linux/usb/chipidea.h
> > @@ -21,6 +21,12 @@ struct ci13xxx_platform_data {
> >  #define CI13XXX_DISABLE_STREAMING	BIT(3)
> >  #define CI13XXX_DR_MODE_HOST		BIT(4)
> >  #define CI13XXX_DR_MODE_PERIPHERAL	BIT(5)
> > +#define CI13XXX_PORTSC_PTW_8BIT		BIT(6)
> > +#define CI13XXX_PORTSC_PTW_16BIT	BIT(7)
> > +#define CI13XXX_PORTSC_PTS_UTMI		BIT(8)
> > +#define CI13XXX_PORTSC_PTS_ULPI		BIT(9)
> > +#define CI13XXX_PORTSC_PTS_FSLS		BIT(10)
> > +
> >  #define CI13XXX_DR_MODE_MASK \
> >  	(CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL)
> >  
> > @@ -42,4 +48,7 @@ void ci13xxx_remove_device(struct platform_device *pdev);
> >  /* Parse of-tree "dr_mode" property */
> >  void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
> >  
> > +/* Parse of-tree "flags" */
> > +void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
> > +
> >  #endif
> > -- 
> > 1.7.10.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 6/9] usb: chipidea: add PTW and PTS handling

[Peter Chen]

On Tue, Nov 27, 2012 at 10:54:32AM +0100, Michael Grzeschik wrote:
> 
> I didn't figure out why this is needed. However, we prefer to move this
> hunk to be in ci_hdrc_probe just before ci_role_start gets called. So
> hw_portsc_configure will be called only there in the beginning. As the
> phy setup doesn't change, we need no further call and can take the delay
> unconditionally of the role.

I agree, as configure phy is common future, it is ok at ci_hdrc_probe.
Below initialization sequence is ok:

configure_phy
delay for phy stable
role_start(init)

> 
> > > +
> > >  	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> > >  	if (!hcd)
> > >  		return -ENOMEM;
> > > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> > > index 906d259..cafca23 100644
> > > --- a/include/linux/usb/chipidea.h
> > > +++ b/include/linux/usb/chipidea.h
> > > @@ -21,6 +21,12 @@ struct ci13xxx_platform_data {
> > >  #define CI13XXX_DISABLE_STREAMING	BIT(3)
> > >  #define CI13XXX_DR_MODE_HOST		BIT(4)
> > >  #define CI13XXX_DR_MODE_PERIPHERAL	BIT(5)
> > > +#define CI13XXX_PORTSC_PTW_8BIT		BIT(6)
> > > +#define CI13XXX_PORTSC_PTW_16BIT	BIT(7)
> > > +#define CI13XXX_PORTSC_PTS_UTMI		BIT(8)
> > > +#define CI13XXX_PORTSC_PTS_ULPI		BIT(9)
> > > +#define CI13XXX_PORTSC_PTS_FSLS		BIT(10)
> > > +
> > >  #define CI13XXX_DR_MODE_MASK \
> > >  	(CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL)
> > >  
> > > @@ -42,4 +48,7 @@ void ci13xxx_remove_device(struct platform_device *pdev);
> > >  /* Parse of-tree "dr_mode" property */
> > >  void ci13xxx_get_dr_mode(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
> > >  
> > > +/* Parse of-tree "flags" */
> > > +void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_data *pdata);
> > > +
> > >  #endif
> > > -- 
> > > 1.7.10.4
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > > the body of a message to majordomo at vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 

Best Regards,
Peter Chen

[PATCH 7/9] usb: chipidea: udc: add force-full-speed option

[Michael Grzeschik]

This patch makes it possible to set the chipidea udc
into full-speed only mode. It can be set by the oftree
property "force-full-speed".

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/bits.h  |    1 +
 drivers/usb/chipidea/core.c  |    5 +++++
 include/linux/usb/chipidea.h |    1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
index 3cded5f..85b6b09 100644
--- a/drivers/usb/chipidea/bits.h
+++ b/drivers/usb/chipidea/bits.h
@@ -51,6 +51,7 @@
 #define PORTSC_PTS            (BIT(31) | BIT(30))
 #define PORTSC_PTW            BIT(28)
 #define PORTSC_STS            BIT(29)
+#define PORTSC_PFSC           BIT(24)
 
 /* DEVLC */
 #define DEVLC_PSPD            (0x03UL << 25)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 7e80f1b..bc99cea 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -230,6 +230,9 @@ void hw_portsc_configure(struct ci13xxx *ci)
 		hw_write(ci, OP_PORTSC, PORTSC_PTS, 0x3 << ffs_nr(PORTSC_PTS));
 		hw_write(ci, OP_PORTSC, PORTSC_STS, 0x1 << ffs_nr(PORTSC_STS));
 	}
+
+	if (ci->platdata->flags & CI13XXX_PORTSC_PFSC)
+		hw_write(ci, OP_PORTSC, PORTSC_PFSC, 0x1 << ffs_nr(PORTSC_PFSC));
 }
 
 /**
@@ -566,6 +569,8 @@ void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_d
 		pr_err("no phy interface defined\n");
 	}
 
+	if (of_find_property(of_node, "force-full-speed", NULL))
+		pdata->flags |= CI13XXX_PORTSC_PFSC;
 }
 EXPORT_SYMBOL_GPL(ci13xxx_get_dr_flags);
 
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index cafca23..9ffe105 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -26,6 +26,7 @@ struct ci13xxx_platform_data {
 #define CI13XXX_PORTSC_PTS_UTMI		BIT(8)
 #define CI13XXX_PORTSC_PTS_ULPI		BIT(9)
 #define CI13XXX_PORTSC_PTS_FSLS		BIT(10)
+#define CI13XXX_PORTSC_PFSC		BIT(11)
 
 #define CI13XXX_DR_MODE_MASK \
 	(CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL)
-- 
1.7.10.4

[PATCH 7/9] usb: chipidea: udc: add force-full-speed option

[Alexander Shishkin]

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

> This patch makes it possible to set the chipidea udc
> into full-speed only mode. It can be set by the oftree
> property "force-full-speed".
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/usb/chipidea/bits.h  |    1 +
>  drivers/usb/chipidea/core.c  |    5 +++++
>  include/linux/usb/chipidea.h |    1 +
>  3 files changed, 7 insertions(+)
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 3cded5f..85b6b09 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -51,6 +51,7 @@
>  #define PORTSC_PTS            (BIT(31) | BIT(30))
>  #define PORTSC_PTW            BIT(28)
>  #define PORTSC_STS            BIT(29)
> +#define PORTSC_PFSC           BIT(24)

Same for this one, it's in DEVLC in my spec.

>  
>  /* DEVLC */
>  #define DEVLC_PSPD            (0x03UL << 25)
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 7e80f1b..bc99cea 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -230,6 +230,9 @@ void hw_portsc_configure(struct ci13xxx *ci)
>  		hw_write(ci, OP_PORTSC, PORTSC_PTS, 0x3 << ffs_nr(PORTSC_PTS));
>  		hw_write(ci, OP_PORTSC, PORTSC_STS, 0x1 << ffs_nr(PORTSC_STS));
>  	}
> +
> +	if (ci->platdata->flags & CI13XXX_PORTSC_PFSC)
> +		hw_write(ci, OP_PORTSC, PORTSC_PFSC, 0x1 << ffs_nr(PORTSC_PFSC));
>  }
>  
>  /**
> @@ -566,6 +569,8 @@ void ci13xxx_get_dr_flags(struct device_node *of_node, struct ci13xxx_platform_d
>  		pr_err("no phy interface defined\n");
>  	}
>  
> +	if (of_find_property(of_node, "force-full-speed", NULL))
> +		pdata->flags |= CI13XXX_PORTSC_PFSC;
>  }
>  EXPORT_SYMBOL_GPL(ci13xxx_get_dr_flags);
>  
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index cafca23..9ffe105 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -26,6 +26,7 @@ struct ci13xxx_platform_data {
>  #define CI13XXX_PORTSC_PTS_UTMI		BIT(8)
>  #define CI13XXX_PORTSC_PTS_ULPI		BIT(9)
>  #define CI13XXX_PORTSC_PTS_FSLS		BIT(10)
> +#define CI13XXX_PORTSC_PFSC		BIT(11)

Btw, these can be more descriptive and don't need _PORTSC_ prefix, like
"CI13XXX_FORCE_FULLSPEED".

Regards,
--
Alex

[PATCH 7/9] usb: chipidea: udc: add force-full-speed option

[Matthieu CASTET]

Alexander Shishkin a ?crit :
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
>> This patch makes it possible to set the chipidea udc
>> into full-speed only mode. It can be set by the oftree
>> property "force-full-speed".
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>  drivers/usb/chipidea/bits.h  |    1 +
>>  drivers/usb/chipidea/core.c  |    5 +++++
>>  include/linux/usb/chipidea.h |    1 +
>>  3 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>> index 3cded5f..85b6b09 100644
>> --- a/drivers/usb/chipidea/bits.h
>> +++ b/drivers/usb/chipidea/bits.h
>> @@ -51,6 +51,7 @@
>>  #define PORTSC_PTS            (BIT(31) | BIT(30))
>>  #define PORTSC_PTW            BIT(28)
>>  #define PORTSC_STS            BIT(29)
>> +#define PORTSC_PFSC           BIT(24)
> 
> Same for this one, it's in DEVLC in my spec.
> 
Yes it depends it device support lpm or not.

We have the same problem with hw_port_is_high_speed [1]. Mapping is different
according lpm support for some register.

Matthieu

[1]
drivers/usb/chipidea/udc.c
static int hw_port_is_high_speed(struct ci13xxx *ci)
{
    return ci->hw_bank.lpm ? hw_read(ci, OP_DEVLC, DEVLC_PSPD) :
        hw_read(ci, OP_PORTSC, PORTSC_HSP);
}

[PATCH 8/9] usb: chipidea: udc: remove unlocked ep_queue which can lead to an race

[Michael Grzeschik]

Its not nessecary to call ep_queue unlocked while in the
own driver. So we move the function to an unlocked version
and call it conditionaly.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/udc.c |  115 +++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 0ed2e1a..2999b0d 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -648,6 +648,68 @@ static void isr_get_status_complete(struct usb_ep *ep, struct usb_request *req)
 }
 
 /**
+ * _ep_queue: queues (submits) an I/O request to an endpoint
+ *
+ * Caller must hold lock
+ */
+static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
+		    gfp_t __maybe_unused gfp_flags)
+{
+	struct ci13xxx_ep  *mEp  = container_of(ep,  struct ci13xxx_ep, ep);
+	struct ci13xxx_req *mReq = container_of(req, struct ci13xxx_req, req);
+	struct ci13xxx *ci = mEp->ci;
+	int retval = 0;
+
+	if (ep == NULL || req == NULL || mEp->ep.desc == NULL)
+		return -EINVAL;
+
+	if (mEp->type == USB_ENDPOINT_XFER_CONTROL) {
+		if (req->length)
+			mEp = (ci->ep0_dir == RX) ?
+			       ci->ep0out : ci->ep0in;
+		if (!list_empty(&mEp->qh.queue)) {
+			_ep_nuke(mEp);
+			retval = -EOVERFLOW;
+			dev_warn(mEp->ci->dev, "endpoint ctrl %X nuked\n",
+				 _usb_addr(mEp));
+		}
+	}
+
+	/* first nuke then test link, e.g. previous status has not sent */
+	if (!list_empty(&mReq->queue)) {
+		retval = -EBUSY;
+		dev_err(mEp->ci->dev, "request already in queue\n");
+		goto done;
+	}
+
+	if (req->length > 4 * CI13XXX_PAGE_SIZE) {
+		req->length = 4 * CI13XXX_PAGE_SIZE;
+		retval = -EMSGSIZE;
+		dev_warn(mEp->ci->dev, "request length truncated\n");
+	}
+
+	dbg_queue(_usb_addr(mEp), req, retval);
+
+	/* push request */
+	mReq->req.status = -EINPROGRESS;
+	mReq->req.actual = 0;
+
+	retval = _hardware_enqueue(mEp, mReq);
+
+	if (retval == -EALREADY) {
+		dbg_event(_usb_addr(mEp), "QUEUE", retval);
+		retval = 0;
+	}
+	if (!retval)
+		list_add_tail(&mReq->queue, &mEp->qh.queue);
+
+ done:
+	return retval;
+}
+
+
+
+/**
  * isr_get_status_response: get_status request response
  * @ci: ci struct
  * @setup: setup request packet
@@ -694,9 +756,7 @@ __acquires(mEp->lock)
 	}
 	/* else do nothing; reserved for future use */
 
-	spin_unlock(mEp->lock);
-	retval = usb_ep_queue(&mEp->ep, req, gfp_flags);
-	spin_lock(mEp->lock);
+	retval = _ep_queue(&mEp->ep, req, gfp_flags);
 	if (retval)
 		goto err_free_buf;
 
@@ -743,8 +803,6 @@ isr_setup_status_complete(struct usb_ep *ep, struct usb_request *req)
  * This function returns an error code
  */
 static int isr_setup_status_phase(struct ci13xxx *ci)
-__releases(mEp->lock)
-__acquires(mEp->lock)
 {
 	int retval;
 	struct ci13xxx_ep *mEp;
@@ -753,9 +811,7 @@ __acquires(mEp->lock)
 	ci->status->context = ci;
 	ci->status->complete = isr_setup_status_complete;
 
-	spin_unlock(mEp->lock);
-	retval = usb_ep_queue(&mEp->ep, ci->status, GFP_ATOMIC);
-	spin_lock(mEp->lock);
+	retval = _ep_queue(&mEp->ep, ci->status, GFP_ATOMIC);
 
 	return retval;
 }
@@ -1152,8 +1208,6 @@ static int ep_queue(struct usb_ep *ep, struct usb_request *req,
 		    gfp_t __maybe_unused gfp_flags)
 {
 	struct ci13xxx_ep  *mEp  = container_of(ep,  struct ci13xxx_ep, ep);
-	struct ci13xxx_req *mReq = container_of(req, struct ci13xxx_req, req);
-	struct ci13xxx *ci = mEp->ci;
 	int retval = 0;
 	unsigned long flags;
 
@@ -1162,47 +1216,8 @@ static int ep_queue(struct usb_ep *ep, struct usb_request *req,
 
 	spin_lock_irqsave(mEp->lock, flags);
 
-	if (mEp->type == USB_ENDPOINT_XFER_CONTROL) {
-		if (req->length)
-			mEp = (ci->ep0_dir == RX) ?
-			       ci->ep0out : ci->ep0in;
-		if (!list_empty(&mEp->qh.queue)) {
-			_ep_nuke(mEp);
-			retval = -EOVERFLOW;
-			dev_warn(mEp->ci->dev, "endpoint ctrl %X nuked\n",
-				 _usb_addr(mEp));
-		}
-	}
-
-	/* first nuke then test link, e.g. previous status has not sent */
-	if (!list_empty(&mReq->queue)) {
-		retval = -EBUSY;
-		dev_err(mEp->ci->dev, "request already in queue\n");
-		goto done;
-	}
-
-	if (req->length > 4 * CI13XXX_PAGE_SIZE) {
-		req->length = 4 * CI13XXX_PAGE_SIZE;
-		retval = -EMSGSIZE;
-		dev_warn(mEp->ci->dev, "request length truncated\n");
-	}
-
-	dbg_queue(_usb_addr(mEp), req, retval);
+	retval = _ep_queue(ep, req, gfp_flags);
 
-	/* push request */
-	mReq->req.status = -EINPROGRESS;
-	mReq->req.actual = 0;
-
-	retval = _hardware_enqueue(mEp, mReq);
-
-	if (retval == -EALREADY) {
-		dbg_event(_usb_addr(mEp), "QUEUE", retval);
-		retval = 0;
-	}
-	if (!retval)
-		list_add_tail(&mReq->queue, &mEp->qh.queue);
-
- done:
 	spin_unlock_irqrestore(mEp->lock, flags);
 	return retval;
 }
-- 
1.7.10.4

[PATCH 8/9] usb: chipidea: udc: remove unlocked ep_queue which can lead to an race

[Alexander Shishkin]

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

> Its not nessecary to call ep_queue unlocked while in the
> own driver. So we move the function to an unlocked version
> and call it conditionaly.

What do you mean by "conditionally" here? Also, when you say race, you
might as well mention what code is racing what to make things clearer.

Otherwise looks good, assuming that you have tested this with lockdep
enabled.

Regards,
--
Alex

[PATCH 9/9] usb: chipidea: udc: configure iso endpoints

[Michael Grzeschik]

The implementation is derived from the fsl_udc_core code in
fsl_ep_enable and makes basic iso handling possible.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/usb/chipidea/udc.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 2999b0d..0bc4308 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1058,6 +1058,9 @@ static int ep_enable(struct usb_ep *ep,
 	int retval = 0;
 	unsigned long flags;
 
+	unsigned short max = 0;
+	unsigned char mult = 0;
+
 	if (ep == NULL || desc == NULL)
 		return -EINVAL;
 
@@ -1075,6 +1078,7 @@ static int ep_enable(struct usb_ep *ep,
 	mEp->type = usb_endpoint_type(desc);
 
 	mEp->ep.maxpacket = usb_endpoint_maxp(desc);
+	max = usb_endpoint_maxp(desc);
 
 	dbg_event(_usb_addr(mEp), "ENABLE", 0);
 
@@ -1082,8 +1086,12 @@ static int ep_enable(struct usb_ep *ep,
 
 	if (mEp->type == USB_ENDPOINT_XFER_CONTROL)
 		mEp->qh.ptr->cap |=  QH_IOS;
-	else if (mEp->type == USB_ENDPOINT_XFER_ISOC)
-		mEp->qh.ptr->cap &= ~QH_MULT;
+	else if (mEp->type == USB_ENDPOINT_XFER_ISOC) {
+		/* Calculate transactions needed for high bandwidth iso */
+		mult = (unsigned char)(1 + ((max >> 11) & 0x03));
+		max = max & 0x7ff;	/* bit 0~10 */
+		mEp->qh.ptr->cap |= (mult << 30);
+	}
 	else
 		mEp->qh.ptr->cap &= ~QH_ZLT;
 
-- 
1.7.10.4

[PATCH 9/9] usb: chipidea: udc: configure iso endpoints

[Sergei Shtylyov]

Hello.

On 11/14/2012 07:19 PM, Michael Grzeschik wrote:

> The implementation is derived from the fsl_udc_core code in
> fsl_ep_enable and makes basic iso handling possible.

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/usb/chipidea/udc.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 2999b0d..0bc4308 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1058,6 +1058,9 @@ static int ep_enable(struct usb_ep *ep,
>  	int retval = 0;
>  	unsigned long flags;
>  
> +	unsigned short max = 0;

   It doesn't look like this initialization is necessary.

> +	unsigned char mult = 0;
> +
>  	if (ep == NULL || desc == NULL)
>  		return -EINVAL;
>  
> @@ -1075,6 +1078,7 @@ static int ep_enable(struct usb_ep *ep,
>  	mEp->type = usb_endpoint_type(desc);
>  
>  	mEp->ep.maxpacket = usb_endpoint_maxp(desc);
> +	max = usb_endpoint_maxp(desc);
>  
>  	dbg_event(_usb_addr(mEp), "ENABLE", 0);
>  
> @@ -1082,8 +1086,12 @@ static int ep_enable(struct usb_ep *ep,
>  
>  	if (mEp->type == USB_ENDPOINT_XFER_CONTROL)
>  		mEp->qh.ptr->cap |=  QH_IOS;
> -	else if (mEp->type == USB_ENDPOINT_XFER_ISOC)
> -		mEp->qh.ptr->cap &= ~QH_MULT;
> +	else if (mEp->type == USB_ENDPOINT_XFER_ISOC) {
> +		/* Calculate transactions needed for high bandwidth iso */
> +		mult = (unsigned char)(1 + ((max >> 11) & 0x03));
> +		max = max & 0x7ff;	/* bit 0~10 */

	max &= 0x7ff;

> +		mEp->qh.ptr->cap |= (mult << 30);

   Parens not needed.

> +	}
>  	else
>  		mEp->qh.ptr->cap &= ~QH_ZLT;
>  
>