chipidea fixes and features

From: Marc Kleine-Budde <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

Signed-off-by Michael Grzeschik <[email protected]>

---
 drivers/usb/chipidea/ci13xxx_pci.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/usb/chipidea/ci13xxx_pci.c +3 -3

index 918e149..97d25c8 100644
@@ -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,

Signature · 1 line

1.7.10.4

Quoted 4 lines · Michael Grzeschik

From: Marc Kleine-Budde <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

Signed-off-by Michael Grzeschik <[email protected]>

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

Quoted 4 lines · Alexander Shishkin

Quoted 2 lines · Michael Grzeschik

From: Marc Kleine-Budde <[email protected]>

Quoted 6 lines

Quoted 2 lines

Signed-off-by Marc Kleine-Budde <[email protected]>

Signed-off-by Michael Grzeschik <[email protected]>

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:

Quoted 4 lines · Michael Grzeschik

From: Marc Kleine-Budde <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

Signed-off-by Michael Grzeschik <[email protected]>

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

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 drivers/usb/chipidea/ci13xxx_imx.c |   54 ++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 9 deletions(-)

drivers/usb/chipidea/ci13xxx_imx.c +45 -9

index 935de97..570aedf 100644
@@ -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);
 

Signature · 6 lines

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

Quoted 8 lines · 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.

Quoted 8 lines

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.

Quoted 20 lines · Peter Chen

Quoted 8 lines · 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.

Quoted 8 lines

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

Quoted 6 lines

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.

From the system point, we only need to control usboh3_ipg_ahb,

Quoted 28 lines · Sascha Hauer

Quoted 20 lines · Peter Chen

Quoted 8 lines · 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.

Quoted 8 lines

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

Quoted 6 lines

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.

Quoted 10 lines

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

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 drivers/usb/chipidea/ci13xxx_imx.c |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

drivers/usb/chipidea/ci13xxx_imx.c +16 -11

index 570aedf..7b99c96 100644
@@ -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,

Signature · 1 line

1.7.10.4

Quoted 2 lines · Michael Grzeschik

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

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 drivers/usb/chipidea/ci13xxx_imx.c |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

drivers/usb/chipidea/ci13xxx_imx.c +3 -9

index 570aedf..7b99c96 100644
@@ -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) {

drivers/usb/chipidea/ci13xxx_imx.c +1 -0

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

From: Marc Kleine-Budde <[email protected]>

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.

drivers/usb/chipidea/ci13xxx_imx.c +2 -0

@@ -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);

drivers/usb/chipidea/core.c +8 -0

index b50b77a..3e3e159 100644
@@ -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;

Quoted 6 lines · Michael Grzeschik

From: Marc Kleine-Budde <[email protected]>

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]

Quoted 41 lines

drivers/usb/chipidea/ci13xxx_imx.c +2 -0

@@ -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);

drivers/usb/chipidea/core.c +15 -0

index b50b77a..3e3e159 100644
@@ -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;

drivers/usb/chipidea/core.c void ci13xxx_remove_device(struct platform_device *pdev) · line 541

Quoted 3 lines

drivers/usb/chipidea/core.c +3 -0

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

Quoted 6 lines · Michael Grzeschik

From: Marc Kleine-Budde <[email protected]>

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]

Quoted 34 lines

drivers/usb/chipidea/ci13xxx_imx.c +2 -0

@@ -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);

drivers/usb/chipidea/core.c +8 -0

index b50b77a..3e3e159 100644
@@ -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;

drivers/usb/chipidea/core.c void ci13xxx_remove_device(struct platform_device *pdev) · line 541

Quoted 10 lines

drivers/usb/chipidea/core.c +10 -0

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

Quoted 6 lines · Alexander Shishkin

Michael Grzeschik <[email protected]> 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.

Quoted 1 line

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.

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

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

drivers/of/Kconfig +4 -0

index dfba3e6..28f99fb 100644
@@ -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

drivers/of/Makefile +1 -0

index e027f44..fdcaf51 100644
@@ -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

drivers/of/of_usbphy.c new file +49 -0

new file mode 100644
index 0000000..2e71f7b
@@ -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);

include/linux/of_usbphy.h new file +15 -0

new file mode 100644
index 0000000..9a4132d
@@ -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 */

include/linux/usb/phy.h +8 -0

index a29ae1e..150eb69 100644
@@ -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 */

Signature · 1 line

1.7.10.4

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

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

drivers/of/Kconfig +4 -0

index dfba3e6..28f99fb 100644
@@ -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

drivers/of/Makefile +1 -0

index e027f44..fdcaf51 100644
@@ -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

drivers/of/of_usbphy.c new file +49 -0

new file mode 100644
index 0000000..2e71f7b
@@ -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);

include/linux/of_usbphy.h new file +15 -0

new file mode 100644
index 0000000..9a4132d
@@ -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 */

include/linux/usb/phy.h +8 -0

index a29ae1e..150eb69 100644
@@ -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 */

Signature · 6 lines

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

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

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

drivers/of/Kconfig +4 -0

index dfba3e6..28f99fb 100644
@@ -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

drivers/of/Makefile +1 -0

index e027f44..fdcaf51 100644
@@ -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

drivers/of/of_usbphy.c new file +8 -0

new file mode 100644
index 0000000..2e71f7b
@@ -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>

drivers/of/of_usbphy.c new file +16 -0

+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);

include/linux/of_usbphy.h new file +15 -0

new file mode 100644
index 0000000..9a4132d
@@ -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 */

include/linux/usb/phy.h +0 -0

index a29ae1e..150eb69 100644

Hi,

Quoted 57 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

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

drivers/of/Kconfig +4 -0

index dfba3e6..28f99fb 100644
@@ -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

drivers/of/Makefile +1 -0

index e027f44..fdcaf51 100644
@@ -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

drivers/of/of_usbphy.c new file +8 -0

new file mode 100644
index 0000000..2e71f7b
@@ -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>

drivers/of/of_usbphy.c line 18

Quoted 10 lines

drivers/of/of_usbphy.c new file +10 -0

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

drivers/of/of_usbphy.c line 25

Quoted 7 lines

drivers/of/of_usbphy.c new file +7 -0

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

drivers/of/of_usbphy.c line 33

Quoted 8 lines

drivers/of/of_usbphy.c new file +7 -0

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

Quoted 45 lines

drivers/of/of_usbphy.c new file +16 -0

+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);

include/linux/of_usbphy.h new file +15 -0

new file mode 100644
index 0000000..9a4132d
@@ -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 */

include/linux/usb/phy.h +0 -0

index a29ae1e..150eb69 100644
@@ -12,6 +12,14 @@
 #include <linux/notifier.h>
 #include <linux/usb.h>
 

include/linux/usb/phy.h line 21

Quoted 7 lines

include/linux/usb/phy.h +7 -0

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

Quoted 37 lines

Quoted 14 lines

drivers/of/of_usbphy.c new file +8 -0

new file mode 100644
index 0000000..2e71f7b
@@ -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>

drivers/of/of_usbphy.c line 18

Quoted 10 lines

drivers/of/of_usbphy.c new file +10 -0

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

drivers/of/of_usbphy.c line 25

Quoted 7 lines

drivers/of/of_usbphy.c new file +7 -0

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

Quoted 3 lines

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

Quoted 8 lines

Quoted 7 lines

drivers/of/of_usbphy.c new file +7 -0

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

Quoted 2 lines

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.

Hi,

Quoted 41 lines · Marc Kleine-Budde

Quoted 40 lines

Quoted 14 lines

drivers/of/of_usbphy.c new file +8 -0

new file mode 100644
index 0000000..2e71f7b
@@ -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>

drivers/of/of_usbphy.c line 18

Quoted 10 lines

drivers/of/of_usbphy.c new file +10 -0

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

drivers/of/of_usbphy.c line 25

Quoted 7 lines

drivers/of/of_usbphy.c new file +7 -0

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

Quoted 1 line

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.

Quoted 1 line

ethernet phy of-helper code.

so ?

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

drivers/of/of_usbphy.c new file +7 -0

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

Quoted 4 lines

Quoted 2 lines

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.

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

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

drivers/of/Kconfig +4 -0

index dfba3e6..28f99fb 100644
@@ -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

drivers/of/Makefile +1 -0

index e027f44..fdcaf51 100644
@@ -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

drivers/of/of_usbphy.c new file +14 -0

new file mode 100644
index 0000000..2e71f7b
@@ -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>
+
+/**

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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)

drivers/usb/chipidea/ci.h +2 -0

index cd42b59..1439e51 100644
@@ -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);

drivers/usb/chipidea/ci13xxx_imx.c +0 -0

index ee4dab0..a8257b8 100644

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

drivers/usb/chipidea/bits.h +0 -0

index 4b6ae3e..3cded5f 100644

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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?

Quoted 17 lines · Alexander Shishkin

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

Quoted 15 lines

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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

Quoted 34 lines · Michael Grzeschik

Quoted 17 lines · Alexander Shishkin

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

Quoted 15 lines

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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?

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 2 lines

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

Yes, mine is from 2009.

Quoted 44 lines · Alexander Shishkin

Quoted 34 lines · Michael Grzeschik

Quoted 17 lines · Alexander Shishkin

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

Quoted 15 lines

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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?

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 42 lines · Michael Grzeschik

Quoted 41 lines · Alexander Shishkin

Quoted 32 lines · Michael Grzeschik

Quoted 17 lines · Alexander Shishkin

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

Quoted 14 lines

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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?

Quoted 3 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Alexander Shishkin a ?crit :

Quoted 44 lines

Quoted 42 lines · Michael Grzeschik

Quoted 41 lines · Alexander Shishkin

Quoted 32 lines · Michael Grzeschik

Quoted 17 lines · Alexander Shishkin

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

Quoted 14 lines

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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?

Quoted 3 lines

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.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 57 lines · Matthieu CASTET

Alexander Shishkin a ?crit :

Quoted 44 lines

Quoted 42 lines · Michael Grzeschik

Quoted 41 lines · Alexander Shishkin

Quoted 32 lines · Michael Grzeschik

Quoted 17 lines · Alexander Shishkin

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

Quoted 14 lines

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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?

Quoted 3 lines

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.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 60 lines

Quoted 58 lines · Matthieu CASTET

Alexander Shishkin a ?crit :

Quoted 44 lines

Quoted 42 lines · Michael Grzeschik

Quoted 41 lines · Alexander Shishkin

Quoted 32 lines · Michael Grzeschik

Quoted 17 lines · Alexander Shishkin

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 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(+)

Quoted 14 lines

drivers/usb/chipidea/bits.h line 53

Quoted 11 lines

drivers/usb/chipidea/bits.h +3 -0

index 4b6ae3e..3cded5f 100644
@@ -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?

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 3 lines

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?

/**
* hw_device_reset: resets chip (execute without interruption)
* @ci: the controller

drivers/usb/chipidea/core.c +28 -0

@@ -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;

drivers/usb/chipidea/host.c +0 -0

index ebff9f4..b23ee1d 100644

drivers/usb/chipidea/host.c +1 -0

+
 	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
 	if (!hcd)
 		return -ENOMEM;

include/linux/usb/chipidea.h +9 -0

index 906d259..cafca23 100644
@@ -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

Signature · 6 lines

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

drivers/usb/chipidea/core.c int hw_device_reset(struct ci13xxx *ci, u32 mode) · line 260

Quoted 9 lines · Michael Grzeschik

/**
* hw_device_reset: resets chip (execute without interruption)
* @ci: the controller

drivers/usb/chipidea/core.c +2 -0

@@ -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);
+

Quoted 44 lines

drivers/usb/chipidea/core.c +26 -0

 	/* 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;

drivers/usb/chipidea/host.c +0 -0

index ebff9f4..b23ee1d 100644
@@ -106,6 +106,10 @@ static int host_start(struct ci13xxx *ci)
 	if (usb_disabled())
 		return -ENODEV;
 

drivers/usb/chipidea/host.c static int host_start(struct ci13xxx *ci) · line 111

Quoted 3 lines

drivers/usb/chipidea/host.c +3 -0

+	hw_portsc_configure(ci);
+
+	mdelay(10);

is usleep_range ok?

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

Quoted 37 lines

drivers/usb/chipidea/host.c +1 -0

+
 	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
 	if (!hcd)
 		return -ENOMEM;

include/linux/usb/chipidea.h +9 -0

index 906d259..cafca23 100644
@@ -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

Signature · 6 lines

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

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.

Quoted 38 lines

Quoted 38 lines

drivers/usb/chipidea/host.c +1 -0

+
 	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
 	if (!hcd)
 		return -ENOMEM;

include/linux/usb/chipidea.h +9 -0

index 906d259..cafca23 100644
@@ -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

Signature · 6 lines

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

Signature · 4 lines

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 |

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 drivers/usb/chipidea/bits.h  |    1 +
 drivers/usb/chipidea/core.c  |    5 +++++
 include/linux/usb/chipidea.h |    1 +
 3 files changed, 7 insertions(+)

drivers/usb/chipidea/bits.h +0 -0

index 3cded5f..85b6b09 100644

drivers/usb/chipidea/bits.h +0 -0

 /* DEVLC */
 #define DEVLC_PSPD            (0x03UL << 25)

drivers/usb/chipidea/core.c +5 -0

index 7e80f1b..bc99cea 100644
@@ -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);
 

include/linux/usb/chipidea.h +0 -0

index cafca23..9ffe105 100644

Quoted 12 lines · 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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 drivers/usb/chipidea/bits.h  |    1 +
 drivers/usb/chipidea/core.c  |    5 +++++
 include/linux/usb/chipidea.h |    1 +
 3 files changed, 7 insertions(+)

drivers/usb/chipidea/bits.h line 54

Quoted 9 lines

drivers/usb/chipidea/bits.h +1 -0

index 3cded5f..85b6b09 100644
@@ -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.

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.

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 <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
 drivers/usb/chipidea/udc.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/usb/chipidea/udc.c +0 -0

index 2999b0d..0bc4308 100644

drivers/usb/chipidea/udc.c +3 -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);
 

drivers/usb/chipidea/udc.c +1 -0

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