[PATCH v6 0/3] imx: add usbmisc support

[PATCH v6 0/3] imx: add usbmisc support

[Richard Zhao]

usbmisc driver handles the SoC specific non-core usb registers.

Changes since last version:
 goto err when usbmisc_ops->init failed rather than return directly.

Richard Zhao (3):
  USB: chipidea: add imx usbmisc support
  ARM: imx6q: clk_register_clkdev usbmisc clock
  ARM: dts: imx6q-sabrelite: add usbmisc device

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +
 .../devicetree/bindings/usb/usbmisc-imx.txt        |   14 ++
 arch/arm/boot/dts/imx6q-sabrelite.dts              |    1 +
 arch/arm/boot/dts/imx6q.dtsi                       |   10 ++
 arch/arm/mach-imx/clk-imx6q.c                      |    1 +
 drivers/usb/chipidea/Makefile                      |    2 +-
 drivers/usb/chipidea/ci13xxx_imx.c                 |   64 ++++++++
 drivers/usb/chipidea/ci13xxx_imx.h                 |   28 ++++
 drivers/usb/chipidea/usbmisc_imx6q.c               |  162 ++++++++++++++++++++
 9 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
 create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h
 create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c

-- 
1.7.9.5

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Richard Zhao]

i.MX usb controllers shares non-core registers, which may include
SoC specific controls. We take it as a usbmisc device and usbmisc
driver set operations needed by ci13xxx_imx driver.

For example, Sabrelite board has bad over-current design, we can
usbmisc to disable over-current detect.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +
 .../devicetree/bindings/usb/usbmisc-imx.txt        |   14 ++
 drivers/usb/chipidea/Makefile                      |    2 +-
 drivers/usb/chipidea/ci13xxx_imx.c                 |   64 ++++++++
 drivers/usb/chipidea/ci13xxx_imx.h                 |   28 ++++
 drivers/usb/chipidea/usbmisc_imx6q.c               |  162 ++++++++++++++++++++
 6 files changed, 274 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
 create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h
 create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index 2c29041..5778b9c 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -7,7 +7,10 @@ Required properties:
 
 Optional properties:
 - fsl,usbphy: phandler of usb phy that connects to the only one port
+- fsl,usbmisc: phandler of non-core register device, with one argument
+  that indicate usb controller index
 - vbus-supply: regulator for vbus
+- disable-over-current: disable over current detect
 
 Examples:
 usb at 02184000 { /* USB OTG */
@@ -15,4 +18,6 @@ usb at 02184000 { /* USB OTG */
 	reg = <0x02184000 0x200>;
 	interrupts = <0 43 0x04>;
 	fsl,usbphy = <&usbphy1>;
+	fsl,usbmisc = <&usbmisc 0>;
+	disable-over-current;
 };
diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
new file mode 100644
index 0000000..97ce94e
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
@@ -0,0 +1,14 @@
+* Freescale i.MX non-core registers
+
+Required properties:
+- #index-cells: Cells used to descibe usb controller index. Should be <1>
+- compatible: Should be one of below:
+	"fsl,imx6q-usbmisc" for imx6q
+- reg: Should contain registers location and length
+
+Examples:
+usbmisc at 02184800 {
+	#index-cells = <1>;
+	compatible = "fsl,imx6q-usbmisc";
+	reg = <0x02184800 0x200>;
+};
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index 5c66d9c..57e510f 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -15,5 +15,5 @@ ifneq ($(CONFIG_PCI),)
 endif
 
 ifneq ($(CONFIG_OF_DEVICE),)
-	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
+	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o usbmisc_imx6q.o
 endif
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index ef60d06..96ac67b 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -22,6 +22,7 @@
 #include <linux/regulator/consumer.h>
 
 #include "ci.h"
+#include "ci13xxx_imx.h"
 
 #define pdev_to_phy(pdev) \
 	((struct usb_phy *)platform_get_drvdata(pdev))
@@ -34,6 +35,55 @@ struct ci13xxx_imx_data {
 	struct regulator *reg_vbus;
 };
 
+static const struct usbmisc_ops *usbmisc_ops;
+
+/* Common functions shared by usbmisc drivers */
+
+int usbmisc_set_ops(const struct usbmisc_ops *ops)
+{
+	if (usbmisc_ops)
+		return -EBUSY;
+
+	usbmisc_ops = ops;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usbmisc_set_ops);
+
+void usbmisc_unset_ops(const struct usbmisc_ops *ops)
+{
+	usbmisc_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(usbmisc_unset_ops);
+
+int usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev)
+{
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args args;
+	int ret;
+
+	usbdev->dev = dev;
+
+	ret = of_parse_phandle_with_args(np, "fsl,usbmisc", "#index-cells",
+					0, &args);
+	if (ret) {
+		dev_err(dev, "Failed to parse property fsl,usbmisc, errno %d\n",
+			ret);
+		memset(usbdev, 0, sizeof(*usbdev));
+		return ret;
+	}
+	usbdev->index = args.args[0];
+	of_node_put(args.np);
+
+	if (of_find_property(np, "disable-over-current", NULL))
+		usbdev->disable_oc = 1;
+
+	return 0;
+}
+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 |
@@ -51,6 +101,10 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 	struct regulator *reg_vbus;
 	int ret;
 
+	if (of_find_property(pdev->dev.of_node, "fsl,usbmisc", NULL)
+		&& !usbmisc_ops)
+		return -EPROBE_DEFER;
+
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
 		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
@@ -120,6 +174,16 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		*pdev->dev.dma_mask = DMA_BIT_MASK(32);
 		dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask);
 	}
+
+	if (usbmisc_ops && usbmisc_ops->init) {
+		ret = usbmisc_ops->init(&pdev->dev);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"usbmisc init failed, ret=%d\n", ret);
+			goto err;
+		}
+	}
+
 	plat_ci = ci13xxx_add_device(&pdev->dev,
 				pdev->resource, pdev->num_resources,
 				&ci13xxx_imx_platdata);
diff --git a/drivers/usb/chipidea/ci13xxx_imx.h b/drivers/usb/chipidea/ci13xxx_imx.h
new file mode 100644
index 0000000..2e88acc
--- /dev/null
+++ b/drivers/usb/chipidea/ci13xxx_imx.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/* Used to set SoC specific callbacks */
+struct usbmisc_ops {
+	/* It's called once when probe a usb device */
+	int (*init)(struct device *dev);
+};
+
+struct usbmisc_usb_device {
+	struct device *dev; /* usb controller device */
+	int index;
+
+	int disable_oc:1; /* over current detect disabled */
+};
+
+int usbmisc_set_ops(const struct usbmisc_ops *ops);
+void usbmisc_unset_ops(const struct usbmisc_ops *ops);
+int
+usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev);
diff --git a/drivers/usb/chipidea/usbmisc_imx6q.c b/drivers/usb/chipidea/usbmisc_imx6q.c
new file mode 100644
index 0000000..416e3fc
--- /dev/null
+++ b/drivers/usb/chipidea/usbmisc_imx6q.c
@@ -0,0 +1,162 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+
+#include "ci13xxx_imx.h"
+
+#define USB_DEV_MAX 4
+
+#define BM_OVER_CUR_DIS		BIT(7)
+
+struct imx6q_usbmisc {
+	void __iomem *base;
+	spinlock_t lock;
+	struct clk *clk;
+	struct usbmisc_usb_device usbdev[USB_DEV_MAX];
+};
+
+static struct imx6q_usbmisc *usbmisc;
+
+static struct usbmisc_usb_device *get_usbdev(struct device *dev)
+{
+	int i, ret;
+
+	for (i = 0; i < USB_DEV_MAX; i++) {
+		if (usbmisc->usbdev[i].dev == dev)
+			return &usbmisc->usbdev[i];
+		else if (!usbmisc->usbdev[i].dev)
+			break;
+	}
+
+	if (i >= USB_DEV_MAX)
+		return ERR_PTR(-EBUSY);
+
+	ret = usbmisc_get_init_data(dev, &usbmisc->usbdev[i]);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &usbmisc->usbdev[i];
+}
+
+static int usbmisc_imx6q_init(struct device *dev)
+{
+
+	struct usbmisc_usb_device *usbdev;
+	unsigned long flags;
+	u32 reg;
+
+	usbdev = get_usbdev(dev);
+	if (IS_ERR(usbdev))
+		return PTR_ERR(usbdev);
+
+	if (usbdev->disable_oc) {
+		spin_lock_irqsave(&usbmisc->lock, flags);
+		reg = readl(usbmisc->base + usbdev->index * 4);
+		writel(reg | BM_OVER_CUR_DIS,
+			usbmisc->base + usbdev->index * 4);
+		spin_unlock_irqrestore(&usbmisc->lock, flags);
+	}
+
+	return 0;
+}
+
+static const struct usbmisc_ops imx6q_usbmisc_ops = {
+	.init = usbmisc_imx6q_init,
+};
+
+static const struct of_device_id usbmisc_imx6q_dt_ids[] = {
+	{ .compatible = "fsl,imx6q-usbmisc"},
+	{ /* sentinel */ }
+};
+
+static int __devinit usbmisc_imx6q_probe(struct platform_device *pdev)
+{
+	struct resource	*res;
+	struct imx6q_usbmisc *data;
+	int ret;
+
+	if (usbmisc)
+		return -EBUSY;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	spin_lock_init(&data->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!data->base)
+		return -EADDRNOTAVAIL;
+
+	data->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->clk)) {
+		dev_err(&pdev->dev,
+			"failed to get clock, err=%ld\n", PTR_ERR(data->clk));
+		return PTR_ERR(data->clk);
+	}
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"clk_prepare_enable failed, err=%d\n", ret);
+		return ret;
+	}
+
+	ret = usbmisc_set_ops(&imx6q_usbmisc_ops);
+	if (ret) {
+		clk_disable_unprepare(data->clk);
+		return ret;
+	}
+
+	usbmisc = data;
+
+	return 0;
+}
+
+static int __devexit usbmisc_imx6q_remove(struct platform_device *pdev)
+{
+	usbmisc_unset_ops(&imx6q_usbmisc_ops);
+	clk_disable_unprepare(usbmisc->clk);
+	return 0;
+}
+
+static struct platform_driver usbmisc_imx6q_driver = {
+	.probe = usbmisc_imx6q_probe,
+	.remove = __devexit_p(usbmisc_imx6q_remove),
+	.driver = {
+		.name = "usbmisc_imx6q",
+		.owner = THIS_MODULE,
+		.of_match_table = usbmisc_imx6q_dt_ids,
+	 },
+};
+
+int __init usbmisc_imx6q_drv_init(void)
+{
+	return platform_driver_register(&usbmisc_imx6q_driver);
+}
+subsys_initcall(usbmisc_imx6q_drv_init);
+
+void __exit usbmisc_imx6q_drv_exit(void)
+{
+	platform_driver_unregister(&usbmisc_imx6q_driver);
+}
+module_exit(usbmisc_imx6q_drv_exit);
+
+MODULE_ALIAS("platform:usbmisc-imx6q");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("driver for imx6q usb non-core registers");
+MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
-- 
1.7.9.5

[PATCH v6 2/3] ARM: imx6q: clk_register_clkdev usbmisc clock

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-imx/clk-imx6q.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index ea89520..660fcbb 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -405,6 +405,7 @@ int __init mx6q_clocks_init(void)
 	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
 	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
 	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
+	clk_register_clkdev(clk[usboh3], NULL, "2184800.usbmisc");
 	clk_register_clkdev(clk[usbphy1], NULL, "20c9000.usbphy");
 	clk_register_clkdev(clk[usbphy2], NULL, "20ca000.usbphy");
 	clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
-- 
1.7.9.5

[PATCH v6 3/3] ARM: dts: imx6q-sabrelite: add usbmisc device

[Richard Zhao]

- add usbmisc device
- set property fsl,usbmisc for usb controllers
- set disable-over-current for imx6q-sabrelite usb otg

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts |    1 +
 arch/arm/boot/dts/imx6q.dtsi          |   10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 72f30f3..aee0568 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -63,6 +63,7 @@
 		aips-bus at 02100000 { /* AIPS2 */
 			usb at 02184000 { /* USB OTG */
 				vbus-supply = <&reg_usb_otg_vbus>;
+				disable-over-current;
 				status = "okay";
 			};
 
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index fd57079..7c27267 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -636,6 +636,7 @@
 				reg = <0x02184000 0x200>;
 				interrupts = <0 43 0x04>;
 				fsl,usbphy = <&usbphy1>;
+				fsl,usbmisc = <&usbmisc 0>;
 				status = "disabled";
 			};
 
@@ -644,6 +645,7 @@
 				reg = <0x02184200 0x200>;
 				interrupts = <0 40 0x04>;
 				fsl,usbphy = <&usbphy2>;
+				fsl,usbmisc = <&usbmisc 1>;
 				status = "disabled";
 			};
 
@@ -651,6 +653,7 @@
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
 				reg = <0x02184400 0x200>;
 				interrupts = <0 41 0x04>;
+				fsl,usbmisc = <&usbmisc 2>;
 				status = "disabled";
 			};
 
@@ -658,9 +661,16 @@
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
 				reg = <0x02184600 0x200>;
 				interrupts = <0 42 0x04>;
+				fsl,usbmisc = <&usbmisc 3>;
 				status = "disabled";
 			};
 
+			usbmisc: usbmisc at 02184800 {
+				#index-cells = <1>;
+				compatible = "fsl,imx6q-usbmisc";
+				reg = <0x02184800 0x200>;
+			};
+
 			ethernet at 02188000 {
 				compatible = "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
-- 
1.7.9.5

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Michael Grzeschik]

Hi,

On Tue, Aug 28, 2012 at 02:58:27PM +0800, Richard Zhao wrote:
> i.MX usb controllers shares non-core registers, which may include
> SoC specific controls. We take it as a usbmisc device and usbmisc
> driver set operations needed by ci13xxx_imx driver.
> 
> For example, Sabrelite board has bad over-current design, we can
> usbmisc to disable over-current detect.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +
>  .../devicetree/bindings/usb/usbmisc-imx.txt        |   14 ++
>  drivers/usb/chipidea/Makefile                      |    2 +-
>  drivers/usb/chipidea/ci13xxx_imx.c                 |   64 ++++++++
>  drivers/usb/chipidea/ci13xxx_imx.h                 |   28 ++++
>  drivers/usb/chipidea/usbmisc_imx6q.c               |  162 ++++++++++++++++++++
>  6 files changed, 274 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
>  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h
>  create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index 2c29041..5778b9c 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -7,7 +7,10 @@ Required properties:
>  
>  Optional properties:
>  - fsl,usbphy: phandler of usb phy that connects to the only one port
> +- fsl,usbmisc: phandler of non-core register device, with one argument
> +  that indicate usb controller index
>  - vbus-supply: regulator for vbus
> +- disable-over-current: disable over current detect
>  
>  Examples:
>  usb at 02184000 { /* USB OTG */
> @@ -15,4 +18,6 @@ usb at 02184000 { /* USB OTG */
>  	reg = <0x02184000 0x200>;
>  	interrupts = <0 43 0x04>;
>  	fsl,usbphy = <&usbphy1>;
> +	fsl,usbmisc = <&usbmisc 0>;
> +	disable-over-current;
>  };
> diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> new file mode 100644
> index 0000000..97ce94e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> @@ -0,0 +1,14 @@
> +* Freescale i.MX non-core registers
> +
> +Required properties:
> +- #index-cells: Cells used to descibe usb controller index. Should be <1>
> +- compatible: Should be one of below:
> +	"fsl,imx6q-usbmisc" for imx6q
> +- reg: Should contain registers location and length
> +
> +Examples:
> +usbmisc at 02184800 {
> +	#index-cells = <1>;
> +	compatible = "fsl,imx6q-usbmisc";
> +	reg = <0x02184800 0x200>;
> +};

i think we should have the devicetree-discuss Mailinglist in Cc: for
that.

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 v6 1/3] USB: chipidea: add imx usbmisc support

[Richard Zhao]

On Tue, Aug 28, 2012 at 04:51:51PM +0200, Michael Grzeschik wrote:
> Hi,
> 
> On Tue, Aug 28, 2012 at 02:58:27PM +0800, Richard Zhao wrote:
> > i.MX usb controllers shares non-core registers, which may include
> > SoC specific controls. We take it as a usbmisc device and usbmisc
> > driver set operations needed by ci13xxx_imx driver.
> > 
> > For example, Sabrelite board has bad over-current design, we can
> > usbmisc to disable over-current detect.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +
> >  .../devicetree/bindings/usb/usbmisc-imx.txt        |   14 ++
> >  drivers/usb/chipidea/Makefile                      |    2 +-
> >  drivers/usb/chipidea/ci13xxx_imx.c                 |   64 ++++++++
> >  drivers/usb/chipidea/ci13xxx_imx.h                 |   28 ++++
> >  drivers/usb/chipidea/usbmisc_imx6q.c               |  162 ++++++++++++++++++++
> >  6 files changed, 274 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> >  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h
> >  create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > index 2c29041..5778b9c 100644
> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -7,7 +7,10 @@ Required properties:
> >  
> >  Optional properties:
> >  - fsl,usbphy: phandler of usb phy that connects to the only one port
> > +- fsl,usbmisc: phandler of non-core register device, with one argument
> > +  that indicate usb controller index
> >  - vbus-supply: regulator for vbus
> > +- disable-over-current: disable over current detect
> >  
> >  Examples:
> >  usb at 02184000 { /* USB OTG */
> > @@ -15,4 +18,6 @@ usb at 02184000 { /* USB OTG */
> >  	reg = <0x02184000 0x200>;
> >  	interrupts = <0 43 0x04>;
> >  	fsl,usbphy = <&usbphy1>;
> > +	fsl,usbmisc = <&usbmisc 0>;
> > +	disable-over-current;
> >  };
> > diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > new file mode 100644
> > index 0000000..97ce94e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > @@ -0,0 +1,14 @@
> > +* Freescale i.MX non-core registers
> > +
> > +Required properties:
> > +- #index-cells: Cells used to descibe usb controller index. Should be <1>
> > +- compatible: Should be one of below:
> > +	"fsl,imx6q-usbmisc" for imx6q
> > +- reg: Should contain registers location and length
> > +
> > +Examples:
> > +usbmisc at 02184800 {
> > +	#index-cells = <1>;
> > +	compatible = "fsl,imx6q-usbmisc";
> > +	reg = <0x02184800 0x200>;
> > +};
> 
> i think we should have the devicetree-discuss Mailinglist in Cc: for
> that.
ccing devicetree-discuss ...

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

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Alexander Shishkin]

Richard Zhao <richard.zhao@freescale.com> writes:

> i.MX usb controllers shares non-core registers, which may include
> SoC specific controls. We take it as a usbmisc device and usbmisc
> driver set operations needed by ci13xxx_imx driver.
>
> For example, Sabrelite board has bad over-current design, we can
> usbmisc to disable over-current detect.

Why does this have to be part of the usb driver instead of SoC specific
code? It looks like you've created a whole new device/driver
infrastructure just to disable overcurrent for a specific board.

And the infrastructure boils down to a complex way of passing a callback
from imx driver to another imx driver, that only works if they are
probed in the right order. I don't see any point in doing it like this
other than inflating the device tree tables even further.

Why can't this be part of the SoC code like it is done, for example in
arch/arm/mach-omap2/control.c?

Regards,
--
Alex

> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +
>  .../devicetree/bindings/usb/usbmisc-imx.txt        |   14 ++
>  drivers/usb/chipidea/Makefile                      |    2 +-
>  drivers/usb/chipidea/ci13xxx_imx.c                 |   64 ++++++++
>  drivers/usb/chipidea/ci13xxx_imx.h                 |   28 ++++
>  drivers/usb/chipidea/usbmisc_imx6q.c               |  162 ++++++++++++++++++++
>  6 files changed, 274 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
>  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h
>  create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c
>
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index 2c29041..5778b9c 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -7,7 +7,10 @@ Required properties:
>  
>  Optional properties:
>  - fsl,usbphy: phandler of usb phy that connects to the only one port
> +- fsl,usbmisc: phandler of non-core register device, with one argument
> +  that indicate usb controller index
>  - vbus-supply: regulator for vbus
> +- disable-over-current: disable over current detect
>  
>  Examples:
>  usb at 02184000 { /* USB OTG */
> @@ -15,4 +18,6 @@ usb at 02184000 { /* USB OTG */
>  	reg = <0x02184000 0x200>;
>  	interrupts = <0 43 0x04>;
>  	fsl,usbphy = <&usbphy1>;
> +	fsl,usbmisc = <&usbmisc 0>;
> +	disable-over-current;
>  };
> diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> new file mode 100644
> index 0000000..97ce94e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> @@ -0,0 +1,14 @@
> +* Freescale i.MX non-core registers
> +
> +Required properties:
> +- #index-cells: Cells used to descibe usb controller index. Should be <1>
> +- compatible: Should be one of below:
> +	"fsl,imx6q-usbmisc" for imx6q
> +- reg: Should contain registers location and length
> +
> +Examples:
> +usbmisc at 02184800 {
> +	#index-cells = <1>;
> +	compatible = "fsl,imx6q-usbmisc";
> +	reg = <0x02184800 0x200>;
> +};
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index 5c66d9c..57e510f 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -15,5 +15,5 @@ ifneq ($(CONFIG_PCI),)
>  endif
>  
>  ifneq ($(CONFIG_OF_DEVICE),)
> -	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> +	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o usbmisc_imx6q.o
>  endif
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index ef60d06..96ac67b 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -22,6 +22,7 @@
>  #include <linux/regulator/consumer.h>
>  
>  #include "ci.h"
> +#include "ci13xxx_imx.h"
>  
>  #define pdev_to_phy(pdev) \
>  	((struct usb_phy *)platform_get_drvdata(pdev))
> @@ -34,6 +35,55 @@ struct ci13xxx_imx_data {
>  	struct regulator *reg_vbus;
>  };
>  
> +static const struct usbmisc_ops *usbmisc_ops;
> +
> +/* Common functions shared by usbmisc drivers */
> +
> +int usbmisc_set_ops(const struct usbmisc_ops *ops)
> +{
> +	if (usbmisc_ops)
> +		return -EBUSY;
> +
> +	usbmisc_ops = ops;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usbmisc_set_ops);
> +
> +void usbmisc_unset_ops(const struct usbmisc_ops *ops)
> +{
> +	usbmisc_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(usbmisc_unset_ops);
> +
> +int usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct of_phandle_args args;
> +	int ret;
> +
> +	usbdev->dev = dev;
> +
> +	ret = of_parse_phandle_with_args(np, "fsl,usbmisc", "#index-cells",
> +					0, &args);
> +	if (ret) {
> +		dev_err(dev, "Failed to parse property fsl,usbmisc, errno %d\n",
> +			ret);
> +		memset(usbdev, 0, sizeof(*usbdev));
> +		return ret;
> +	}
> +	usbdev->index = args.args[0];
> +	of_node_put(args.np);
> +
> +	if (of_find_property(np, "disable-over-current", NULL))
> +		usbdev->disable_oc = 1;
> +
> +	return 0;
> +}
> +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 |
> @@ -51,6 +101,10 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>  	struct regulator *reg_vbus;
>  	int ret;
>  
> +	if (of_find_property(pdev->dev.of_node, "fsl,usbmisc", NULL)
> +		&& !usbmisc_ops)
> +		return -EPROBE_DEFER;
> +
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data) {
>  		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
> @@ -120,6 +174,16 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
>  		*pdev->dev.dma_mask = DMA_BIT_MASK(32);
>  		dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask);
>  	}
> +
> +	if (usbmisc_ops && usbmisc_ops->init) {
> +		ret = usbmisc_ops->init(&pdev->dev);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"usbmisc init failed, ret=%d\n", ret);
> +			goto err;
> +		}
> +	}
> +
>  	plat_ci = ci13xxx_add_device(&pdev->dev,
>  				pdev->resource, pdev->num_resources,
>  				&ci13xxx_imx_platdata);
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.h b/drivers/usb/chipidea/ci13xxx_imx.h
> new file mode 100644
> index 0000000..2e88acc
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci13xxx_imx.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/* Used to set SoC specific callbacks */
> +struct usbmisc_ops {
> +	/* It's called once when probe a usb device */
> +	int (*init)(struct device *dev);
> +};
> +
> +struct usbmisc_usb_device {
> +	struct device *dev; /* usb controller device */
> +	int index;
> +
> +	int disable_oc:1; /* over current detect disabled */
> +};
> +
> +int usbmisc_set_ops(const struct usbmisc_ops *ops);
> +void usbmisc_unset_ops(const struct usbmisc_ops *ops);
> +int
> +usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev);
> diff --git a/drivers/usb/chipidea/usbmisc_imx6q.c b/drivers/usb/chipidea/usbmisc_imx6q.c
> new file mode 100644
> index 0000000..416e3fc
> --- /dev/null
> +++ b/drivers/usb/chipidea/usbmisc_imx6q.c
> @@ -0,0 +1,162 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +
> +#include "ci13xxx_imx.h"
> +
> +#define USB_DEV_MAX 4
> +
> +#define BM_OVER_CUR_DIS		BIT(7)
> +
> +struct imx6q_usbmisc {
> +	void __iomem *base;
> +	spinlock_t lock;
> +	struct clk *clk;
> +	struct usbmisc_usb_device usbdev[USB_DEV_MAX];
> +};
> +
> +static struct imx6q_usbmisc *usbmisc;
> +
> +static struct usbmisc_usb_device *get_usbdev(struct device *dev)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < USB_DEV_MAX; i++) {
> +		if (usbmisc->usbdev[i].dev == dev)
> +			return &usbmisc->usbdev[i];
> +		else if (!usbmisc->usbdev[i].dev)
> +			break;
> +	}
> +
> +	if (i >= USB_DEV_MAX)
> +		return ERR_PTR(-EBUSY);
> +
> +	ret = usbmisc_get_init_data(dev, &usbmisc->usbdev[i]);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return &usbmisc->usbdev[i];
> +}
> +
> +static int usbmisc_imx6q_init(struct device *dev)
> +{
> +
> +	struct usbmisc_usb_device *usbdev;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	usbdev = get_usbdev(dev);
> +	if (IS_ERR(usbdev))
> +		return PTR_ERR(usbdev);
> +
> +	if (usbdev->disable_oc) {
> +		spin_lock_irqsave(&usbmisc->lock, flags);
> +		reg = readl(usbmisc->base + usbdev->index * 4);
> +		writel(reg | BM_OVER_CUR_DIS,
> +			usbmisc->base + usbdev->index * 4);
> +		spin_unlock_irqrestore(&usbmisc->lock, flags);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct usbmisc_ops imx6q_usbmisc_ops = {
> +	.init = usbmisc_imx6q_init,
> +};
> +
> +static const struct of_device_id usbmisc_imx6q_dt_ids[] = {
> +	{ .compatible = "fsl,imx6q-usbmisc"},
> +	{ /* sentinel */ }
> +};
> +
> +static int __devinit usbmisc_imx6q_probe(struct platform_device *pdev)
> +{
> +	struct resource	*res;
> +	struct imx6q_usbmisc *data;
> +	int ret;
> +
> +	if (usbmisc)
> +		return -EBUSY;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&data->lock);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!data->base)
> +		return -EADDRNOTAVAIL;
> +
> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(data->clk)) {
> +		dev_err(&pdev->dev,
> +			"failed to get clock, err=%ld\n", PTR_ERR(data->clk));
> +		return PTR_ERR(data->clk);
> +	}
> +
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"clk_prepare_enable failed, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = usbmisc_set_ops(&imx6q_usbmisc_ops);
> +	if (ret) {
> +		clk_disable_unprepare(data->clk);
> +		return ret;
> +	}
> +
> +	usbmisc = data;
> +
> +	return 0;
> +}
> +
> +static int __devexit usbmisc_imx6q_remove(struct platform_device *pdev)
> +{
> +	usbmisc_unset_ops(&imx6q_usbmisc_ops);
> +	clk_disable_unprepare(usbmisc->clk);
> +	return 0;
> +}
> +
> +static struct platform_driver usbmisc_imx6q_driver = {
> +	.probe = usbmisc_imx6q_probe,
> +	.remove = __devexit_p(usbmisc_imx6q_remove),
> +	.driver = {
> +		.name = "usbmisc_imx6q",
> +		.owner = THIS_MODULE,
> +		.of_match_table = usbmisc_imx6q_dt_ids,
> +	 },
> +};
> +
> +int __init usbmisc_imx6q_drv_init(void)
> +{
> +	return platform_driver_register(&usbmisc_imx6q_driver);
> +}
> +subsys_initcall(usbmisc_imx6q_drv_init);
> +
> +void __exit usbmisc_imx6q_drv_exit(void)
> +{
> +	platform_driver_unregister(&usbmisc_imx6q_driver);
> +}
> +module_exit(usbmisc_imx6q_drv_exit);
> +
> +MODULE_ALIAS("platform:usbmisc-imx6q");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("driver for imx6q usb non-core registers");
> +MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
> -- 
> 1.7.9.5

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Sascha Hauer]

On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
> Richard Zhao <richard.zhao@freescale.com> writes:
> 
> > i.MX usb controllers shares non-core registers, which may include
> > SoC specific controls. We take it as a usbmisc device and usbmisc
> > driver set operations needed by ci13xxx_imx driver.
> >
> > For example, Sabrelite board has bad over-current design, we can
> > usbmisc to disable over-current detect.
> 
> Why does this have to be part of the usb driver instead of SoC specific
> code? It looks like you've created a whole new device/driver
> infrastructure just to disable overcurrent for a specific board.

Richards code indeed only handles overcurrent for a specific board, but
there are more bits to configure in the longer run: power pin
polarities, ULPI/serial mode select and some more.

> 
> And the infrastructure boils down to a complex way of passing a callback
> from imx driver to another imx driver, that only works if they are
> probed in the right order. I don't see any point in doing it like this
> other than inflating the device tree tables even further.
> 
> Why can't this be part of the SoC code like it is done, for example in
> arch/arm/mach-omap2/control.c?

The settings are board specific, so there must be some way to configure
them in the devicetree.

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 v6 1/3] USB: chipidea: add imx usbmisc support

[Richard Zhao]

On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
> Richard Zhao <richard.zhao@freescale.com> writes:
> 
> > i.MX usb controllers shares non-core registers, which may include
> > SoC specific controls. We take it as a usbmisc device and usbmisc
> > driver set operations needed by ci13xxx_imx driver.
> >
> > For example, Sabrelite board has bad over-current design, we can
> > usbmisc to disable over-current detect.
> 
> Why does this have to be part of the usb driver instead of SoC specific
> code? It looks like you've created a whole new device/driver
> infrastructure just to disable overcurrent for a specific board.
> 
> And the infrastructure boils down to a complex way of passing a callback
> from imx driver to another imx driver, that only works if they are
> probed in the right order. I don't see any point in doing it like this
> other than inflating the device tree tables even further.
The goal is to let ci13xxx_imx can call usbmisc function at runtime.
The disable over current can be set once at boot time, but there's other
callbacks which is not yet in the mainline have to be called dynamically.

Thanks
Richard
> 
> Why can't this be part of the SoC code like it is done, for example in
> arch/arm/mach-omap2/control.c?
> 
> Regards,
> --
> Alex
> 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    5 +
> >  .../devicetree/bindings/usb/usbmisc-imx.txt        |   14 ++
> >  drivers/usb/chipidea/Makefile                      |    2 +-
> >  drivers/usb/chipidea/ci13xxx_imx.c                 |   64 ++++++++
> >  drivers/usb/chipidea/ci13xxx_imx.h                 |   28 ++++
> >  drivers/usb/chipidea/usbmisc_imx6q.c               |  162 ++++++++++++++++++++
> >  6 files changed, 274 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> >  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h
> >  create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > index 2c29041..5778b9c 100644
> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -7,7 +7,10 @@ Required properties:
> >  
> >  Optional properties:
> >  - fsl,usbphy: phandler of usb phy that connects to the only one port
> > +- fsl,usbmisc: phandler of non-core register device, with one argument
> > +  that indicate usb controller index
> >  - vbus-supply: regulator for vbus
> > +- disable-over-current: disable over current detect
> >  
> >  Examples:
> >  usb at 02184000 { /* USB OTG */
> > @@ -15,4 +18,6 @@ usb at 02184000 { /* USB OTG */
> >  	reg = <0x02184000 0x200>;
> >  	interrupts = <0 43 0x04>;
> >  	fsl,usbphy = <&usbphy1>;
> > +	fsl,usbmisc = <&usbmisc 0>;
> > +	disable-over-current;
> >  };
> > diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > new file mode 100644
> > index 0000000..97ce94e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > @@ -0,0 +1,14 @@
> > +* Freescale i.MX non-core registers
> > +
> > +Required properties:
> > +- #index-cells: Cells used to descibe usb controller index. Should be <1>
> > +- compatible: Should be one of below:
> > +	"fsl,imx6q-usbmisc" for imx6q
> > +- reg: Should contain registers location and length
> > +
> > +Examples:
> > +usbmisc at 02184800 {
> > +	#index-cells = <1>;
> > +	compatible = "fsl,imx6q-usbmisc";
> > +	reg = <0x02184800 0x200>;
> > +};
> > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> > index 5c66d9c..57e510f 100644
> > --- a/drivers/usb/chipidea/Makefile
> > +++ b/drivers/usb/chipidea/Makefile
> > @@ -15,5 +15,5 @@ ifneq ($(CONFIG_PCI),)
> >  endif
> >  
> >  ifneq ($(CONFIG_OF_DEVICE),)
> > -	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> > +	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o usbmisc_imx6q.o
> >  endif
> > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> > index ef60d06..96ac67b 100644
> > --- a/drivers/usb/chipidea/ci13xxx_imx.c
> > +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/regulator/consumer.h>
> >  
> >  #include "ci.h"
> > +#include "ci13xxx_imx.h"
> >  
> >  #define pdev_to_phy(pdev) \
> >  	((struct usb_phy *)platform_get_drvdata(pdev))
> > @@ -34,6 +35,55 @@ struct ci13xxx_imx_data {
> >  	struct regulator *reg_vbus;
> >  };
> >  
> > +static const struct usbmisc_ops *usbmisc_ops;
> > +
> > +/* Common functions shared by usbmisc drivers */
> > +
> > +int usbmisc_set_ops(const struct usbmisc_ops *ops)
> > +{
> > +	if (usbmisc_ops)
> > +		return -EBUSY;
> > +
> > +	usbmisc_ops = ops;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(usbmisc_set_ops);
> > +
> > +void usbmisc_unset_ops(const struct usbmisc_ops *ops)
> > +{
> > +	usbmisc_ops = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(usbmisc_unset_ops);
> > +
> > +int usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev)
> > +{
> > +	struct device_node *np = dev->of_node;
> > +	struct of_phandle_args args;
> > +	int ret;
> > +
> > +	usbdev->dev = dev;
> > +
> > +	ret = of_parse_phandle_with_args(np, "fsl,usbmisc", "#index-cells",
> > +					0, &args);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to parse property fsl,usbmisc, errno %d\n",
> > +			ret);
> > +		memset(usbdev, 0, sizeof(*usbdev));
> > +		return ret;
> > +	}
> > +	usbdev->index = args.args[0];
> > +	of_node_put(args.np);
> > +
> > +	if (of_find_property(np, "disable-over-current", NULL))
> > +		usbdev->disable_oc = 1;
> > +
> > +	return 0;
> > +}
> > +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 |
> > @@ -51,6 +101,10 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
> >  	struct regulator *reg_vbus;
> >  	int ret;
> >  
> > +	if (of_find_property(pdev->dev.of_node, "fsl,usbmisc", NULL)
> > +		&& !usbmisc_ops)
> > +		return -EPROBE_DEFER;
> > +
> >  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> >  	if (!data) {
> >  		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
> > @@ -120,6 +174,16 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
> >  		*pdev->dev.dma_mask = DMA_BIT_MASK(32);
> >  		dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask);
> >  	}
> > +
> > +	if (usbmisc_ops && usbmisc_ops->init) {
> > +		ret = usbmisc_ops->init(&pdev->dev);
> > +		if (ret) {
> > +			dev_err(&pdev->dev,
> > +				"usbmisc init failed, ret=%d\n", ret);
> > +			goto err;
> > +		}
> > +	}
> > +
> >  	plat_ci = ci13xxx_add_device(&pdev->dev,
> >  				pdev->resource, pdev->num_resources,
> >  				&ci13xxx_imx_platdata);
> > diff --git a/drivers/usb/chipidea/ci13xxx_imx.h b/drivers/usb/chipidea/ci13xxx_imx.h
> > new file mode 100644
> > index 0000000..2e88acc
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/ci13xxx_imx.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +/* Used to set SoC specific callbacks */
> > +struct usbmisc_ops {
> > +	/* It's called once when probe a usb device */
> > +	int (*init)(struct device *dev);
> > +};
> > +
> > +struct usbmisc_usb_device {
> > +	struct device *dev; /* usb controller device */
> > +	int index;
> > +
> > +	int disable_oc:1; /* over current detect disabled */
> > +};
> > +
> > +int usbmisc_set_ops(const struct usbmisc_ops *ops);
> > +void usbmisc_unset_ops(const struct usbmisc_ops *ops);
> > +int
> > +usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev);
> > diff --git a/drivers/usb/chipidea/usbmisc_imx6q.c b/drivers/usb/chipidea/usbmisc_imx6q.c
> > new file mode 100644
> > index 0000000..416e3fc
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/usbmisc_imx6q.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +
> > +#include "ci13xxx_imx.h"
> > +
> > +#define USB_DEV_MAX 4
> > +
> > +#define BM_OVER_CUR_DIS		BIT(7)
> > +
> > +struct imx6q_usbmisc {
> > +	void __iomem *base;
> > +	spinlock_t lock;
> > +	struct clk *clk;
> > +	struct usbmisc_usb_device usbdev[USB_DEV_MAX];
> > +};
> > +
> > +static struct imx6q_usbmisc *usbmisc;
> > +
> > +static struct usbmisc_usb_device *get_usbdev(struct device *dev)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < USB_DEV_MAX; i++) {
> > +		if (usbmisc->usbdev[i].dev == dev)
> > +			return &usbmisc->usbdev[i];
> > +		else if (!usbmisc->usbdev[i].dev)
> > +			break;
> > +	}
> > +
> > +	if (i >= USB_DEV_MAX)
> > +		return ERR_PTR(-EBUSY);
> > +
> > +	ret = usbmisc_get_init_data(dev, &usbmisc->usbdev[i]);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	return &usbmisc->usbdev[i];
> > +}
> > +
> > +static int usbmisc_imx6q_init(struct device *dev)
> > +{
> > +
> > +	struct usbmisc_usb_device *usbdev;
> > +	unsigned long flags;
> > +	u32 reg;
> > +
> > +	usbdev = get_usbdev(dev);
> > +	if (IS_ERR(usbdev))
> > +		return PTR_ERR(usbdev);
> > +
> > +	if (usbdev->disable_oc) {
> > +		spin_lock_irqsave(&usbmisc->lock, flags);
> > +		reg = readl(usbmisc->base + usbdev->index * 4);
> > +		writel(reg | BM_OVER_CUR_DIS,
> > +			usbmisc->base + usbdev->index * 4);
> > +		spin_unlock_irqrestore(&usbmisc->lock, flags);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct usbmisc_ops imx6q_usbmisc_ops = {
> > +	.init = usbmisc_imx6q_init,
> > +};
> > +
> > +static const struct of_device_id usbmisc_imx6q_dt_ids[] = {
> > +	{ .compatible = "fsl,imx6q-usbmisc"},
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static int __devinit usbmisc_imx6q_probe(struct platform_device *pdev)
> > +{
> > +	struct resource	*res;
> > +	struct imx6q_usbmisc *data;
> > +	int ret;
> > +
> > +	if (usbmisc)
> > +		return -EBUSY;
> > +
> > +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_init(&data->lock);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	data->base = devm_request_and_ioremap(&pdev->dev, res);
> > +	if (!data->base)
> > +		return -EADDRNOTAVAIL;
> > +
> > +	data->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(data->clk)) {
> > +		dev_err(&pdev->dev,
> > +			"failed to get clock, err=%ld\n", PTR_ERR(data->clk));
> > +		return PTR_ERR(data->clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(data->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"clk_prepare_enable failed, err=%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = usbmisc_set_ops(&imx6q_usbmisc_ops);
> > +	if (ret) {
> > +		clk_disable_unprepare(data->clk);
> > +		return ret;
> > +	}
> > +
> > +	usbmisc = data;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __devexit usbmisc_imx6q_remove(struct platform_device *pdev)
> > +{
> > +	usbmisc_unset_ops(&imx6q_usbmisc_ops);
> > +	clk_disable_unprepare(usbmisc->clk);
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver usbmisc_imx6q_driver = {
> > +	.probe = usbmisc_imx6q_probe,
> > +	.remove = __devexit_p(usbmisc_imx6q_remove),
> > +	.driver = {
> > +		.name = "usbmisc_imx6q",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = usbmisc_imx6q_dt_ids,
> > +	 },
> > +};
> > +
> > +int __init usbmisc_imx6q_drv_init(void)
> > +{
> > +	return platform_driver_register(&usbmisc_imx6q_driver);
> > +}
> > +subsys_initcall(usbmisc_imx6q_drv_init);
> > +
> > +void __exit usbmisc_imx6q_drv_exit(void)
> > +{
> > +	platform_driver_unregister(&usbmisc_imx6q_driver);
> > +}
> > +module_exit(usbmisc_imx6q_drv_exit);
> > +
> > +MODULE_ALIAS("platform:usbmisc-imx6q");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("driver for imx6q usb non-core registers");
> > +MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
> > -- 
> > 1.7.9.5
> --
> 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
> 

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Alexander Shishkin]

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

> On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
>> Richard Zhao <richard.zhao@freescale.com> writes:
>> 
>> > i.MX usb controllers shares non-core registers, which may include
>> > SoC specific controls. We take it as a usbmisc device and usbmisc
>> > driver set operations needed by ci13xxx_imx driver.
>> >
>> > For example, Sabrelite board has bad over-current design, we can
>> > usbmisc to disable over-current detect.
>> 
>> Why does this have to be part of the usb driver instead of SoC specific
>> code? It looks like you've created a whole new device/driver
>> infrastructure just to disable overcurrent for a specific board.
>
> Richards code indeed only handles overcurrent for a specific board, but
> there are more bits to configure in the longer run: power pin
> polarities, ULPI/serial mode select and some more.

Sounds to me like these things that should be taken care of by the phy
driver, which will likely be simpler from both driver's and devicetree's
perspective.

>
>> 
>> And the infrastructure boils down to a complex way of passing a callback
>> from imx driver to another imx driver, that only works if they are
>> probed in the right order. I don't see any point in doing it like this
>> other than inflating the device tree tables even further.
>> 
>> Why can't this be part of the SoC code like it is done, for example in
>> arch/arm/mach-omap2/control.c?
>
> The settings are board specific, so there must be some way to configure
> them in the devicetree.

But I'm sure there's a way to control board-specific settings/kludges
from devicetree?

Regards,
--
Alex

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Richard Zhao]

On Wed, Aug 29, 2012 at 01:18:10PM +0300, Alexander Shishkin wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
> >> Richard Zhao <richard.zhao@freescale.com> writes:
> >> 
> >> > i.MX usb controllers shares non-core registers, which may include
> >> > SoC specific controls. We take it as a usbmisc device and usbmisc
> >> > driver set operations needed by ci13xxx_imx driver.
> >> >
> >> > For example, Sabrelite board has bad over-current design, we can
> >> > usbmisc to disable over-current detect.
> >> 
> >> Why does this have to be part of the usb driver instead of SoC specific
> >> code? It looks like you've created a whole new device/driver
> >> infrastructure just to disable overcurrent for a specific board.
> >
> > Richards code indeed only handles overcurrent for a specific board, but
> > there are more bits to configure in the longer run: power pin
> > polarities, ULPI/serial mode select and some more.
> 
> Sounds to me like these things that should be taken care of by the phy
> driver, which will likely be simpler from both driver's and devicetree's
> perspective.
No, it's controlled by a set of usb misc registers, which is not in phy
IP or usb controller IP.

Thanks
Richard
> 
> >
> >> 
> >> And the infrastructure boils down to a complex way of passing a callback
> >> from imx driver to another imx driver, that only works if they are
> >> probed in the right order. I don't see any point in doing it like this
> >> other than inflating the device tree tables even further.
> >> 
> >> Why can't this be part of the SoC code like it is done, for example in
> >> arch/arm/mach-omap2/control.c?
> >
> > The settings are board specific, so there must be some way to configure
> > them in the devicetree.
> 
> But I'm sure there's a way to control board-specific settings/kludges
> from devicetree?
> 
> Regards,
> --
> Alex
> 

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Sascha Hauer]

On Wed, Aug 29, 2012 at 01:18:10PM +0300, Alexander Shishkin wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
> >> Richard Zhao <richard.zhao@freescale.com> writes:
> >> 
> >> > i.MX usb controllers shares non-core registers, which may include
> >> > SoC specific controls. We take it as a usbmisc device and usbmisc
> >> > driver set operations needed by ci13xxx_imx driver.
> >> >
> >> > For example, Sabrelite board has bad over-current design, we can
> >> > usbmisc to disable over-current detect.
> >> 
> >> Why does this have to be part of the usb driver instead of SoC specific
> >> code? It looks like you've created a whole new device/driver
> >> infrastructure just to disable overcurrent for a specific board.
> >
> > Richards code indeed only handles overcurrent for a specific board, but
> > there are more bits to configure in the longer run: power pin
> > polarities, ULPI/serial mode select and some more.
> 
> Sounds to me like these things that should be taken care of by the phy
> driver, which will likely be simpler from both driver's and devicetree's
> perspective.

Most i.MX SoCs have three instances of the chipidea core. These cores
share a single register space for controlling the mentioned bits (the
usbmisc register space). The usbmisc looks different on the different
SoCs.
Indeed they control some phy specific aspects, but the phy itself may
also be an external ULPI or UTMI phy with a separate driver. So if we
integrate the usbmisc into the phy wouldn't that mean that it has to
be integrated into all possible phy drivers?

>From a devicetrees perspective it makes sense to integrate the flags
into the chipidea nodes, because there is one node per chipidea core,
but only one usbmisc unit for all ports on the SoC. So we can do a:

	chipidea at ofs {
		disable-overcurrent = <1>;
	};

instead of

	usbmisc at ofs {
		disable-overcurrent-port0 = <1>;
		disable-overcurrent-port1 = <0>;
		...
	};

> 
> >
> >> 
> >> And the infrastructure boils down to a complex way of passing a callback
> >> from imx driver to another imx driver, that only works if they are
> >> probed in the right order. I don't see any point in doing it like this
> >> other than inflating the device tree tables even further.
> >> 
> >> Why can't this be part of the SoC code like it is done, for example in
> >> arch/arm/mach-omap2/control.c?
> >
> > The settings are board specific, so there must be some way to configure
> > them in the devicetree.
> 
> But I'm sure there's a way to control board-specific settings/kludges
> from devicetree?

Hm, yes. That's what Richard does, right? I may be misunderstanding you
here.

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 v6 1/3] USB: chipidea: add imx usbmisc support

[Marc Kleine-Budde]

On 08/29/2012 01:01 PM, Sascha Hauer wrote:
> On Wed, Aug 29, 2012 at 01:18:10PM +0300, Alexander Shishkin wrote:
>> Sascha Hauer <s.hauer@pengutronix.de> writes:
>>
>>> On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
>>>> Richard Zhao <richard.zhao@freescale.com> writes:
>>>>
>>>>> i.MX usb controllers shares non-core registers, which may include
>>>>> SoC specific controls. We take it as a usbmisc device and usbmisc
>>>>> driver set operations needed by ci13xxx_imx driver.
>>>>>
>>>>> For example, Sabrelite board has bad over-current design, we can
>>>>> usbmisc to disable over-current detect.
>>>>
>>>> Why does this have to be part of the usb driver instead of SoC specific
>>>> code? It looks like you've created a whole new device/driver
>>>> infrastructure just to disable overcurrent for a specific board.
>>>
>>> Richards code indeed only handles overcurrent for a specific board, but
>>> there are more bits to configure in the longer run: power pin
>>> polarities, ULPI/serial mode select and some more.

We already have a patch adding a usbmisc_imx53_init() function to that
driver.

>> Sounds to me like these things that should be taken care of by the phy
>> driver, which will likely be simpler from both driver's and devicetree's
>> perspective.
> 
> Most i.MX SoCs have three instances of the chipidea core. These cores
> share a single register space for controlling the mentioned bits (the
> usbmisc register space). The usbmisc looks different on the different
> SoCs.
> Indeed they control some phy specific aspects, but the phy itself may
> also be an external ULPI or UTMI phy with a separate driver. So if we
> integrate the usbmisc into the phy wouldn't that mean that it has to
> be integrated into all possible phy drivers?
> 
> From a devicetrees perspective it makes sense to integrate the flags
> into the chipidea nodes, because there is one node per chipidea core,
> but only one usbmisc unit for all ports on the SoC. So we can do a:
> 
> 	chipidea at ofs {
> 		disable-overcurrent = <1>;
> 	};
> 
> instead of
> 
> 	usbmisc at ofs {
> 		disable-overcurrent-port0 = <1>;
> 		disable-overcurrent-port1 = <0>;
> 		...
> 	};

+1

IMHO looks much cleaner.

>>>> And the infrastructure boils down to a complex way of passing a callback
>>>> from imx driver to another imx driver, that only works if they are
>>>> probed in the right order. I don't see any point in doing it like this
>>>> other than inflating the device tree tables even further.
>>>>
>>>> Why can't this be part of the SoC code like it is done, for example in
>>>> arch/arm/mach-omap2/control.c?
>>>
>>> The settings are board specific, so there must be some way to configure
>>> them in the devicetree.
>>
>> But I'm sure there's a way to control board-specific settings/kludges
>> from devicetree?
> 
> Hm, yes. That's what Richard does, right? I may be misunderstanding you
> here.
> 
> Sascha

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/20120829/a2f19d86/attachment.sig>

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Richard Zhao]

On Wed, Aug 29, 2012 at 10:00:32PM +0200, Marc Kleine-Budde wrote:
> On 08/29/2012 01:01 PM, Sascha Hauer wrote:
> > On Wed, Aug 29, 2012 at 01:18:10PM +0300, Alexander Shishkin wrote:
> >> Sascha Hauer <s.hauer@pengutronix.de> writes:
> >>
> >>> On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
> >>>> Richard Zhao <richard.zhao@freescale.com> writes:
> >>>>
> >>>>> i.MX usb controllers shares non-core registers, which may include
> >>>>> SoC specific controls. We take it as a usbmisc device and usbmisc
> >>>>> driver set operations needed by ci13xxx_imx driver.
> >>>>>
> >>>>> For example, Sabrelite board has bad over-current design, we can
> >>>>> usbmisc to disable over-current detect.
> >>>>
> >>>> Why does this have to be part of the usb driver instead of SoC specific
> >>>> code? It looks like you've created a whole new device/driver
> >>>> infrastructure just to disable overcurrent for a specific board.
> >>>
> >>> Richards code indeed only handles overcurrent for a specific board, but
> >>> there are more bits to configure in the longer run: power pin
> >>> polarities, ULPI/serial mode select and some more.
> 
> We already have a patch adding a usbmisc_imx53_init() function to that
> driver.
> 
> >> Sounds to me like these things that should be taken care of by the phy
> >> driver, which will likely be simpler from both driver's and devicetree's
> >> perspective.
> > 
> > Most i.MX SoCs have three instances of the chipidea core. These cores
> > share a single register space for controlling the mentioned bits (the
> > usbmisc register space). The usbmisc looks different on the different
> > SoCs.
> > Indeed they control some phy specific aspects, but the phy itself may
> > also be an external ULPI or UTMI phy with a separate driver. So if we
> > integrate the usbmisc into the phy wouldn't that mean that it has to
> > be integrated into all possible phy drivers?
> > 
> > From a devicetrees perspective it makes sense to integrate the flags
> > into the chipidea nodes, because there is one node per chipidea core,
> > but only one usbmisc unit for all ports on the SoC. So we can do a:
> > 
> > 	chipidea at ofs {
> > 		disable-overcurrent = <1>;
> > 	};
> > 
> > instead of
> > 
> > 	usbmisc at ofs {
> > 		disable-overcurrent-port0 = <1>;
> > 		disable-overcurrent-port1 = <0>;
> > 		...
> > 	};
> 
> +1
> 
> IMHO looks much cleaner.
So, Marc agree on the patch too. Maybe you can give a reviewed-by? :)

Hi Alex,

What do you think?

Thanks
Richard
> 
> >>>> And the infrastructure boils down to a complex way of passing a callback
> >>>> from imx driver to another imx driver, that only works if they are
> >>>> probed in the right order. I don't see any point in doing it like this
> >>>> other than inflating the device tree tables even further.
> >>>>
> >>>> Why can't this be part of the SoC code like it is done, for example in
> >>>> arch/arm/mach-omap2/control.c?
> >>>
> >>> The settings are board specific, so there must be some way to configure
> >>> them in the devicetree.
> >>
> >> But I'm sure there's a way to control board-specific settings/kludges
> >> from devicetree?
> > 
> > Hm, yes. That's what Richard does, right? I may be misunderstanding you
> > here.
> > 
> > Sascha
> 
> 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   |
> 

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Alexander Shishkin]

Richard Zhao <linuxzsc@gmail.com> writes:

> On Wed, Aug 29, 2012 at 10:00:32PM +0200, Marc Kleine-Budde wrote:
>> On 08/29/2012 01:01 PM, Sascha Hauer wrote:
>> > On Wed, Aug 29, 2012 at 01:18:10PM +0300, Alexander Shishkin wrote:
>> >> Sascha Hauer <s.hauer@pengutronix.de> writes:
>> >>
>> >>> On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
>> >>>> Richard Zhao <richard.zhao@freescale.com> writes:
>> >>>>
>> >>>>> i.MX usb controllers shares non-core registers, which may include
>> >>>>> SoC specific controls. We take it as a usbmisc device and usbmisc
>> >>>>> driver set operations needed by ci13xxx_imx driver.
>> >>>>>
>> >>>>> For example, Sabrelite board has bad over-current design, we can
>> >>>>> usbmisc to disable over-current detect.
>> >>>>
>> >>>> Why does this have to be part of the usb driver instead of SoC specific
>> >>>> code? It looks like you've created a whole new device/driver
>> >>>> infrastructure just to disable overcurrent for a specific board.
>> >>>
>> >>> Richards code indeed only handles overcurrent for a specific board, but
>> >>> there are more bits to configure in the longer run: power pin
>> >>> polarities, ULPI/serial mode select and some more.
>> 
>> We already have a patch adding a usbmisc_imx53_init() function to that
>> driver.
>> 
>> >> Sounds to me like these things that should be taken care of by the phy
>> >> driver, which will likely be simpler from both driver's and devicetree's
>> >> perspective.
>> > 
>> > Most i.MX SoCs have three instances of the chipidea core. These cores
>> > share a single register space for controlling the mentioned bits (the
>> > usbmisc register space). The usbmisc looks different on the different
>> > SoCs.
>> > Indeed they control some phy specific aspects, but the phy itself may
>> > also be an external ULPI or UTMI phy with a separate driver. So if we
>> > integrate the usbmisc into the phy wouldn't that mean that it has to
>> > be integrated into all possible phy drivers?
>> > 
>> > From a devicetrees perspective it makes sense to integrate the flags
>> > into the chipidea nodes, because there is one node per chipidea core,
>> > but only one usbmisc unit for all ports on the SoC. So we can do a:
>> > 
>> > 	chipidea at ofs {
>> > 		disable-overcurrent = <1>;
>> > 	};
>> > 
>> > instead of
>> > 
>> > 	usbmisc at ofs {
>> > 		disable-overcurrent-port0 = <1>;
>> > 		disable-overcurrent-port1 = <0>;
>> > 		...
>> > 	};
>> 
>> +1
>> 
>> IMHO looks much cleaner.
> So, Marc agree on the patch too. Maybe you can give a reviewed-by? :)
>
> Hi Alex,
>
> What do you think?

Weeell, as long as this is contained in platform specific code and arm
bits, I can tolerate it.

What about the other two patches? I guess they can go through arm tree
so we don't have to send it through Greg, right?

Regards,
--
Alex

[PATCH v6 1/3] USB: chipidea: add imx usbmisc support

[Shawn Guo]

On Tue, Sep 11, 2012 at 01:42:01PM +0300, Alexander Shishkin wrote:
> What about the other two patches? I guess they can go through arm tree
> so we don't have to send it through Greg, right?
> 
Right, those two will go via arm-soc tree once the first one gets
applied on usb tree.

-- 
Regards,
Shawn