[PATCH v2 0/2] imx: add usbmisc support

[PATCH v2 0/2] imx: add usbmisc support

[Richard Zhao]

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

It's based on Greg's usb-next.

Changes since last version:
 - rework to let usb-imxXXX drivers register ops callbacks

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

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 +
 .../devicetree/bindings/usb/usbmisc-imx.txt        |   12 ++
 arch/arm/boot/dts/imx6q-sabrelite.dts              |    4 +
 arch/arm/boot/dts/imx6q.dtsi                       |   19 ++-
 arch/arm/mach-imx/clk-imx6q.c                      |    1 +
 drivers/usb/chipidea/Makefile                      |    2 +-
 drivers/usb/chipidea/ci13xxx_imx.c                 |   23 +++
 drivers/usb/chipidea/usbmisc_imx6q.c               |  161 ++++++++++++++++++++
 8 files changed, 219 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
 create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c

-- 
1.7.9.5

[PATCH v2 1/2] 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>
---
 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 +
 .../devicetree/bindings/usb/usbmisc-imx.txt        |   12 ++
 drivers/usb/chipidea/Makefile                      |    2 +-
 drivers/usb/chipidea/ci13xxx_imx.c                 |   23 +++
 drivers/usb/chipidea/usbmisc_imx6q.c               |  161 ++++++++++++++++++++
 5 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
 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..06105ce 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -8,6 +8,7 @@ Required properties:
 Optional properties:
 - fsl,usbphy: phandler of usb phy that connects to the only one port
 - vbus-supply: regulator for vbus
+- disable-over-current: disable over current detect
 
 Examples:
 usb at 02184000 { /* USB OTG */
@@ -15,4 +16,5 @@ usb at 02184000 { /* USB OTG */
 	reg = <0x02184000 0x200>;
 	interrupts = <0 43 0x04>;
 	fsl,usbphy = <&usbphy1>;
+	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..4fa500d
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
@@ -0,0 +1,12 @@
+* Freescale i.MX non-core registers
+
+Required properties:
+- compatible: Should be one of below:
+	"fsl,imx6q-usbmisc" for imx6q
+- reg: Should contain registers location and length
+
+Examples:
+usbmisc at 02184800 {
+	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..e790c0e 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,25 @@ struct ci13xxx_imx_data {
 	struct regulator *reg_vbus;
 };
 
+static const struct usbmisc_ops *usbmisc_ops;
+
+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);
+
 static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
 	.name			= "ci13xxx_imx",
 	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
@@ -120,6 +140,9 @@ 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);
 	}
+
+	usbmisc_ops->init(&pdev->dev);
+
 	plat_ci = ci13xxx_add_device(&pdev->dev,
 				pdev->resource, pdev->num_resources,
 				&ci13xxx_imx_platdata);
diff --git a/drivers/usb/chipidea/usbmisc_imx6q.c b/drivers/usb/chipidea/usbmisc_imx6q.c
new file mode 100644
index 0000000..9f69a8c
--- /dev/null
+++ b/drivers/usb/chipidea/usbmisc_imx6q.c
@@ -0,0 +1,161 @@
+/*
+ * 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;
+	struct clk *clk;
+	struct device *usb_dev[USB_DEV_MAX];
+	spinlock_t lock;
+
+	int disable_oc:USB_DEV_MAX;
+};
+
+static struct imx6q_usbmisc *usbmisc;
+
+static int get_index(struct device *dev)
+{
+	int i, id = -1;
+
+	for (i = 0; i < USB_DEV_MAX; i ++) {
+		if (usbmisc->usb_dev[i] == dev) {
+			id = i;
+			break;
+		}
+	}
+
+	if (id != -1)
+		return id;
+
+	id = of_alias_get_id(dev->of_node, "usb");
+	if (id < 0)
+		dev_err(dev, "failed to get alias id, errno %d\n", id);
+	return id;
+}
+
+static int usbmisc_imx6q_init(struct device *usb_dev)
+{
+
+	int id;
+	u32 reg;
+	struct device_node *usb_np;
+	unsigned long flags;
+
+	id = get_index(usb_dev);
+	if (id < 0)
+		return id;
+
+	usb_np = usb_dev->of_node;
+	if (of_find_property(usb_np, "disable-over-current", NULL)) {
+		spin_lock_irqsave(&usbmisc->lock, flags);
+		usbmisc->disable_oc |= BIT(id);
+		reg = readl(usbmisc->base + id * 4);
+		writel(reg | BM_OVER_CUR_DIS, usbmisc->base + id * 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)
+		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,
+	 },
+};
+
+static int __init usbmisc_imx6q_drv_init(void)
+{
+	return platform_driver_register(&usbmisc_imx6q_driver);
+}
+subsys_initcall(usbmisc_imx6q_drv_init);
+
+static 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("i.MX6Q non-core usb register handling");
+MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
-- 
1.7.9.5

[PATCH v2 2/2] ARM: dts: imx6q-sabrelite: add usbmisc device

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts |    4 ++++
 arch/arm/boot/dts/imx6q.dtsi          |   19 +++++++++++++++----
 arch/arm/mach-imx/clk-imx6q.c         |    1 +
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index d42e851..c66ff9d 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -62,12 +62,16 @@
 		aips-bus at 02100000 { /* AIPS2 */
 			usb at 02184000 { /* USB OTG */
 				vbus-supply = <&reg_usb_otg_vbus>;
+				disable-over-current;
 				status = "okay";
 			};
 
 			usb at 02184200 { /* USB1 */
 				status = "okay";
 			};
+			usbmisc at 02184800 {
+				status = "okay";
+			};
 
 			ethernet at 02188000 {
 				phy-mode = "rgmii";
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index c25d495..42b0c39 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -19,6 +19,11 @@
 		serial2 = &uart3;
 		serial3 = &uart4;
 		serial4 = &uart5;
+
+		usb0 = &usbotg;
+		usb1 = &usbhost1;
+		usb2 = &usbhost2;
+		usb3 = &usbhost3;
 	};
 
 	cpus {
@@ -624,7 +629,7 @@
 				reg = <0x0217c000 0x4000>;
 			};
 
-			usb at 02184000 { /* USB OTG */
+			usbotg: usb at 02184000 {
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
 				reg = <0x02184000 0x200>;
 				interrupts = <0 43 0x04>;
@@ -632,7 +637,7 @@
 				status = "disabled";
 			};
 
-			usb at 02184200 { /* USB1 */
+			usbhost1: usb at 02184200 {
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
 				reg = <0x02184200 0x200>;
 				interrupts = <0 40 0x04>;
@@ -640,20 +645,26 @@
 				status = "disabled";
 			};
 
-			usb at 02184400 { /* USB2 */
+			usbhost2: usb at 02184400 {
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
 				reg = <0x02184400 0x200>;
 				interrupts = <0 41 0x04>;
 				status = "disabled";
 			};
 
-			usb at 02184600 { /* USB3 */
+			usbhost3: usb at 02184600 {
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
 				reg = <0x02184600 0x200>;
 				interrupts = <0 42 0x04>;
 				status = "disabled";
 			};
 
+			usbmisc at 02184800 {
+				compatible = "fsl,imx6q-usbmisc";
+				reg = <0x02184800 0x200>;
+				status = "disabled";
+			};
+
 			ethernet at 02188000 {
 				compatible = "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
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 v2 1/2] USB: chipidea: add imx usbmisc support

[Sascha Hauer]

On Wed, Jul 18, 2012 at 06:29:06PM +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>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 +
>  .../devicetree/bindings/usb/usbmisc-imx.txt        |   12 ++
>  drivers/usb/chipidea/Makefile                      |    2 +-
>  drivers/usb/chipidea/ci13xxx_imx.c                 |   23 +++
>  drivers/usb/chipidea/usbmisc_imx6q.c               |  161 ++++++++++++++++++++
>  5 files changed, 199 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
>  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..06105ce 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -8,6 +8,7 @@ Required properties:
>  Optional properties:
>  - fsl,usbphy: phandler of usb phy that connects to the only one port
>  - vbus-supply: regulator for vbus
> +- disable-over-current: disable over current detect
>  
>  Examples:
>  usb at 02184000 { /* USB OTG */
> @@ -15,4 +16,5 @@ usb at 02184000 { /* USB OTG */
>  	reg = <0x02184000 0x200>;
>  	interrupts = <0 43 0x04>;
>  	fsl,usbphy = <&usbphy1>;
> +	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..4fa500d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> @@ -0,0 +1,12 @@
> +* Freescale i.MX non-core registers
> +
> +Required properties:
> +- compatible: Should be one of below:
> +	"fsl,imx6q-usbmisc" for imx6q
> +- reg: Should contain registers location and length
> +
> +Examples:
> +usbmisc at 02184800 {
> +	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..e790c0e 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,25 @@ struct ci13xxx_imx_data {
>  	struct regulator *reg_vbus;
>  };
>  
> +static const struct usbmisc_ops *usbmisc_ops;
> +
> +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);
> +
>  static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
>  	.name			= "ci13xxx_imx",
>  	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> @@ -120,6 +140,9 @@ 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);
>  	}
> +
> +	usbmisc_ops->init(&pdev->dev);

usbmisc_ops can be NULL and also can return an error.

> +
>  	plat_ci = ci13xxx_add_device(&pdev->dev,
>  				pdev->resource, pdev->num_resources,
>  				&ci13xxx_imx_platdata);
> diff --git a/drivers/usb/chipidea/usbmisc_imx6q.c b/drivers/usb/chipidea/usbmisc_imx6q.c
> new file mode 100644
> index 0000000..9f69a8c
> --- /dev/null
> +++ b/drivers/usb/chipidea/usbmisc_imx6q.c
> @@ -0,0 +1,161 @@
> +/*
> + * 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;
> +	struct clk *clk;
> +	struct device *usb_dev[USB_DEV_MAX];
> +	spinlock_t lock;
> +
> +	int disable_oc:USB_DEV_MAX;
> +};

Please add a per-port struct instead of adding multiple arrays into
struct imx6q_usbmisc.

Also, I think this per port struct shouldn't be imx6q specific. Then
we could add generic code parsing the oftree flags into the port
specific struct and have SoC specific code which translates this struct
into the acual register settings.

> +
> +static struct imx6q_usbmisc *usbmisc;
> +
> +static int get_index(struct device *dev)
> +{
> +	int i, id = -1;
> +
> +	for (i = 0; i < USB_DEV_MAX; i ++) {
> +		if (usbmisc->usb_dev[i] == dev) {
> +			id = i;
> +			break;
> +		}
> +	}
> +
> +	if (id != -1)
> +		return id;
> +
> +	id = of_alias_get_id(dev->of_node, "usb");
> +	if (id < 0)
> +		dev_err(dev, "failed to get alias id, errno %d\n", id);
> +	return id;
> +}
> +
> +static int usbmisc_imx6q_init(struct device *usb_dev)
> +{
> +
> +	int id;
> +	u32 reg;
> +	struct device_node *usb_np;
> +	unsigned long flags;
> +
> +	id = get_index(usb_dev);
> +	if (id < 0)
> +		return id;
> +
> +	usb_np = usb_dev->of_node;
> +	if (of_find_property(usb_np, "disable-over-current", NULL)) {
> +		spin_lock_irqsave(&usbmisc->lock, flags);
> +		usbmisc->disable_oc |= BIT(id);
> +		reg = readl(usbmisc->base + id * 4);
> +		writel(reg | BM_OVER_CUR_DIS, usbmisc->base + id * 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;
> +	}

the clk devm ops do not disable the clock, you still have to do this in
your error path.

> +
> +	ret = usbmisc_set_ops(&imx6q_usbmisc_ops);
> +	if (ret)
> +		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,
> +	 },
> +};
> +
> +static int __init usbmisc_imx6q_drv_init(void)
> +{
> +	return platform_driver_register(&usbmisc_imx6q_driver);
> +}
> +subsys_initcall(usbmisc_imx6q_drv_init);
> +
> +static 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("i.MX6Q non-core usb register handling");
> +MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
> -- 
> 1.7.9.5
> 
> 
> 

-- 
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 v2 2/2] ARM: dts: imx6q-sabrelite: add usbmisc device

[Sascha Hauer]

On Wed, Jul 18, 2012 at 06:29:07PM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-sabrelite.dts |    4 ++++
>  arch/arm/boot/dts/imx6q.dtsi          |   19 +++++++++++++++----
>  arch/arm/mach-imx/clk-imx6q.c         |    1 +
>  3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index d42e851..c66ff9d 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -62,12 +62,16 @@
>  		aips-bus at 02100000 { /* AIPS2 */
>  			usb at 02184000 { /* USB OTG */
>  				vbus-supply = <&reg_usb_otg_vbus>;
> +				disable-over-current;
>  				status = "okay";
>  			};
>  
>  			usb at 02184200 { /* USB1 */
>  				status = "okay";
>  			};
> +			usbmisc at 02184800 {
> +				status = "okay";
> +			};

Do we need to have this disabled initially in the SoC dtsi?

Also, common style in this file is to add a blank line between nodes.
Please keep it.

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

[Richard Zhao]

On Wed, Jul 18, 2012 at 12:52:04PM +0200, Sascha Hauer wrote:
> On Wed, Jul 18, 2012 at 06:29:06PM +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>
> > ---
> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 +
> >  .../devicetree/bindings/usb/usbmisc-imx.txt        |   12 ++
> >  drivers/usb/chipidea/Makefile                      |    2 +-
> >  drivers/usb/chipidea/ci13xxx_imx.c                 |   23 +++
> >  drivers/usb/chipidea/usbmisc_imx6q.c               |  161 ++++++++++++++++++++
> >  5 files changed, 199 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> >  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..06105ce 100644
> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -8,6 +8,7 @@ Required properties:
> >  Optional properties:
> >  - fsl,usbphy: phandler of usb phy that connects to the only one port
> >  - vbus-supply: regulator for vbus
> > +- disable-over-current: disable over current detect
> >  
> >  Examples:
> >  usb at 02184000 { /* USB OTG */
> > @@ -15,4 +16,5 @@ usb at 02184000 { /* USB OTG */
> >  	reg = <0x02184000 0x200>;
> >  	interrupts = <0 43 0x04>;
> >  	fsl,usbphy = <&usbphy1>;
> > +	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..4fa500d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > @@ -0,0 +1,12 @@
> > +* Freescale i.MX non-core registers
> > +
> > +Required properties:
> > +- compatible: Should be one of below:
> > +	"fsl,imx6q-usbmisc" for imx6q
> > +- reg: Should contain registers location and length
> > +
> > +Examples:
> > +usbmisc at 02184800 {
> > +	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..e790c0e 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,25 @@ struct ci13xxx_imx_data {
> >  	struct regulator *reg_vbus;
> >  };
> >  
> > +static const struct usbmisc_ops *usbmisc_ops;
> > +
> > +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);
> > +
> >  static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
> >  	.name			= "ci13xxx_imx",
> >  	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> > @@ -120,6 +140,9 @@ 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);
> >  	}
> > +
> > +	usbmisc_ops->init(&pdev->dev);
> 
> usbmisc_ops can be NULL and also can return an error.
Yes. And I find it can not work when usbmisc is module.
> 
> > +
> >  	plat_ci = ci13xxx_add_device(&pdev->dev,
> >  				pdev->resource, pdev->num_resources,
> >  				&ci13xxx_imx_platdata);
> > diff --git a/drivers/usb/chipidea/usbmisc_imx6q.c b/drivers/usb/chipidea/usbmisc_imx6q.c
> > new file mode 100644
> > index 0000000..9f69a8c
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/usbmisc_imx6q.c
> > @@ -0,0 +1,161 @@
> > +/*
> > + * 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;
> > +	struct clk *clk;
> > +	struct device *usb_dev[USB_DEV_MAX];
> > +	spinlock_t lock;
> > +
> > +	int disable_oc:USB_DEV_MAX;
> > +};
> 
> Please add a per-port struct instead of adding multiple arrays into
> struct imx6q_usbmisc.
ok

> 
> Also, I think this per port struct shouldn't be imx6q specific. Then
> we could add generic code parsing the oftree flags into the port
> specific struct and have SoC specific code which translates this struct
> into the acual register settings.
hmm... I thought only ops is generic. The code is SoC specific, I doubt
do we really need to take the properties as generic?
> 
> > +
> > +static struct imx6q_usbmisc *usbmisc;
> > +
> > +static int get_index(struct device *dev)
> > +{
> > +	int i, id = -1;
> > +
> > +	for (i = 0; i < USB_DEV_MAX; i ++) {
> > +		if (usbmisc->usb_dev[i] == dev) {
> > +			id = i;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (id != -1)
> > +		return id;
> > +
> > +	id = of_alias_get_id(dev->of_node, "usb");
> > +	if (id < 0)
> > +		dev_err(dev, "failed to get alias id, errno %d\n", id);
> > +	return id;
> > +}
> > +
> > +static int usbmisc_imx6q_init(struct device *usb_dev)
> > +{
> > +
> > +	int id;
> > +	u32 reg;
> > +	struct device_node *usb_np;
> > +	unsigned long flags;
> > +
> > +	id = get_index(usb_dev);
> > +	if (id < 0)
> > +		return id;
> > +
> > +	usb_np = usb_dev->of_node;
> > +	if (of_find_property(usb_np, "disable-over-current", NULL)) {
> > +		spin_lock_irqsave(&usbmisc->lock, flags);
> > +		usbmisc->disable_oc |= BIT(id);
> > +		reg = readl(usbmisc->base + id * 4);
> > +		writel(reg | BM_OVER_CUR_DIS, usbmisc->base + id * 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;
> > +	}
> 
> the clk devm ops do not disable the clock, you still have to do this in
> your error path.
Thanks.

Richard
> 
> > +
> > +	ret = usbmisc_set_ops(&imx6q_usbmisc_ops);
> > +	if (ret)
> > +		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,
> > +	 },
> > +};
> > +
> > +static int __init usbmisc_imx6q_drv_init(void)
> > +{
> > +	return platform_driver_register(&usbmisc_imx6q_driver);
> > +}
> > +subsys_initcall(usbmisc_imx6q_drv_init);
> > +
> > +static 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("i.MX6Q non-core usb register handling");
> > +MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
> > -- 
> > 1.7.9.5
> > 
> > 
> > 
> 
> -- 
> 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 v2 2/2] ARM: dts: imx6q-sabrelite: add usbmisc device

[Richard Zhao]

On Wed, Jul 18, 2012 at 12:54:31PM +0200, Sascha Hauer wrote:
> On Wed, Jul 18, 2012 at 06:29:07PM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  arch/arm/boot/dts/imx6q-sabrelite.dts |    4 ++++
> >  arch/arm/boot/dts/imx6q.dtsi          |   19 +++++++++++++++----
> >  arch/arm/mach-imx/clk-imx6q.c         |    1 +
> >  3 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > index d42e851..c66ff9d 100644
> > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > @@ -62,12 +62,16 @@
> >  		aips-bus at 02100000 { /* AIPS2 */
> >  			usb at 02184000 { /* USB OTG */
> >  				vbus-supply = <&reg_usb_otg_vbus>;
> > +				disable-over-current;
> >  				status = "okay";
> >  			};
> >  
> >  			usb at 02184200 { /* USB1 */
> >  				status = "okay";
> >  			};
> > +			usbmisc at 02184800 {
> > +				status = "okay";
> > +			};
> 
> Do we need to have this disabled initially in the SoC dtsi?
It's in SoC, and don't affect anything outside of Soc. So I think
it can be enabled by default.
> 
> Also, common style in this file is to add a blank line between nodes.
> Please keep it.
yes.

Thanks
Richard
> 
> 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 v2 1/2] USB: chipidea: add imx usbmisc support

[Sascha Hauer]

On Wed, Jul 18, 2012 at 07:19:17PM +0800, Richard Zhao wrote:
> > > +
> > > +struct imx6q_usbmisc {
> > > +	void __iomem *base;
> > > +	struct clk *clk;
> > > +	struct device *usb_dev[USB_DEV_MAX];
> > > +	spinlock_t lock;
> > > +
> > > +	int disable_oc:USB_DEV_MAX;
> > > +};
> > 
> > Please add a per-port struct instead of adding multiple arrays into
> > struct imx6q_usbmisc.
> ok
> 
> > 
> > Also, I think this per port struct shouldn't be imx6q specific. Then
> > we could add generic code parsing the oftree flags into the port
> > specific struct and have SoC specific code which translates this struct
> > into the acual register settings.
> hmm... I thought only ops is generic. The code is SoC specific, I doubt
> do we really need to take the properties as generic?

I think it would be good. They mostly match across the different i.MX
SoCs.

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

[Richard Zhao]

[snip]
> > >  static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
> > >  	.name			= "ci13xxx_imx",
> > >  	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> > > @@ -120,6 +140,9 @@ 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);
> > >  	}
> > > +
> > > +	usbmisc_ops->init(&pdev->dev);
> > 
> > usbmisc_ops can be NULL and also can return an error.
> Yes. And I find it can not work when usbmisc is module.
It's hard to resolve module dependency. How about build all imx related
things into a single module? ci13xxx_imx module init will call
usbmisc_imx6q_drv_init, usbmisc_imx53_drv_init etc.

Thanks
Richard

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Marc Kleine-Budde]

On 07/18/2012 03:39 PM, Richard Zhao wrote:
> [snip]
>>>>  static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
>>>>  	.name			= "ci13xxx_imx",
>>>>  	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
>>>> @@ -120,6 +140,9 @@ 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);
>>>>  	}
>>>> +
>>>> +	usbmisc_ops->init(&pdev->dev);
>>>
>>> usbmisc_ops can be NULL and also can return an error.
>> Yes. And I find it can not work when usbmisc is module.
> It's hard to resolve module dependency. How about build all imx related
> things into a single module? ci13xxx_imx module init will call
> usbmisc_imx6q_drv_init, usbmisc_imx53_drv_init etc.

Why not call a single imx_usbmisc_init. The usbmisc can call the correct
function inside. The right init function can be selected via the
compatible in the device tree.

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

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Richard Zhao]

On Wed, Jul 18, 2012 at 03:43:00PM +0200, Marc Kleine-Budde wrote:
> On 07/18/2012 03:39 PM, Richard Zhao wrote:
> > [snip]
> >>>>  static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
> >>>>  	.name			= "ci13xxx_imx",
> >>>>  	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> >>>> @@ -120,6 +140,9 @@ 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);
> >>>>  	}
> >>>> +
> >>>> +	usbmisc_ops->init(&pdev->dev);
> >>>
> >>> usbmisc_ops can be NULL and also can return an error.
> >> Yes. And I find it can not work when usbmisc is module.
> > It's hard to resolve module dependency. How about build all imx related
> > things into a single module? ci13xxx_imx module init will call
> > usbmisc_imx6q_drv_init, usbmisc_imx53_drv_init etc.
> 
> Why not call a single imx_usbmisc_init. The usbmisc can call the correct
> function inside. The right init function can be selected via the
> compatible in the device tree.
Different SoC has different usbmisc driver and I have to register the
driver. A single imx_usbmisc_init shows nothing different. It doesn't
know what driver to register. Hope I got what you meant.

Thanks
Richard
> 
> 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 v2 2/2] ARM: dts: imx6q-sabrelite: add usbmisc device

[Shawn Guo]

On Wed, Jul 18, 2012 at 06:29:07PM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-sabrelite.dts |    4 ++++
>  arch/arm/boot/dts/imx6q.dtsi          |   19 +++++++++++++++----
>  arch/arm/mach-imx/clk-imx6q.c         |    1 +

It seems that the patch is meant to have dts changes only by reading
the subject.  So it might be good to get clk-imx6q.c out of here.

Shawn

[PATCH v2 2/2] ARM: dts: imx6q-sabrelite: add usbmisc device

[Richard Zhao]

On Wed, Jul 18, 2012 at 11:00:38PM +0800, Shawn Guo wrote:
> On Wed, Jul 18, 2012 at 06:29:07PM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  arch/arm/boot/dts/imx6q-sabrelite.dts |    4 ++++
> >  arch/arm/boot/dts/imx6q.dtsi          |   19 +++++++++++++++----
> >  arch/arm/mach-imx/clk-imx6q.c         |    1 +
> 
> It seems that the patch is meant to have dts changes only by reading
> the subject.  So it might be good to get clk-imx6q.c out of here.
Thanks. It'll be divided into two.

Richard
> 
> Shawn

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Felipe Balbi]

Hi,

On Wed, Jul 18, 2012 at 06:29:06PM +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>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 +
>  .../devicetree/bindings/usb/usbmisc-imx.txt        |   12 ++
>  drivers/usb/chipidea/Makefile                      |    2 +-
>  drivers/usb/chipidea/ci13xxx_imx.c                 |   23 +++
>  drivers/usb/chipidea/usbmisc_imx6q.c               |  161 ++++++++++++++++++++
>  5 files changed, 199 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
>  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..06105ce 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -8,6 +8,7 @@ Required properties:
>  Optional properties:
>  - fsl,usbphy: phandler of usb phy that connects to the only one port
>  - vbus-supply: regulator for vbus
> +- disable-over-current: disable over current detect
>  
>  Examples:
>  usb at 02184000 { /* USB OTG */
> @@ -15,4 +16,5 @@ usb at 02184000 { /* USB OTG */
>  	reg = <0x02184000 0x200>;
>  	interrupts = <0 43 0x04>;
>  	fsl,usbphy = <&usbphy1>;
> +	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..4fa500d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> @@ -0,0 +1,12 @@
> +* Freescale i.MX non-core registers
> +
> +Required properties:
> +- compatible: Should be one of below:
> +	"fsl,imx6q-usbmisc" for imx6q
> +- reg: Should contain registers location and length
> +
> +Examples:
> +usbmisc at 02184800 {
> +	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..e790c0e 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,25 @@ struct ci13xxx_imx_data {
>  	struct regulator *reg_vbus;
>  };
>  
> +static const struct usbmisc_ops *usbmisc_ops;
> +
> +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);
> +
>  static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
>  	.name			= "ci13xxx_imx",
>  	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> @@ -120,6 +140,9 @@ 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);
>  	}
> +
> +	usbmisc_ops->init(&pdev->dev);
> +
>  	plat_ci = ci13xxx_add_device(&pdev->dev,
>  				pdev->resource, pdev->num_resources,
>  				&ci13xxx_imx_platdata);
> diff --git a/drivers/usb/chipidea/usbmisc_imx6q.c b/drivers/usb/chipidea/usbmisc_imx6q.c
> new file mode 100644
> index 0000000..9f69a8c
> --- /dev/null
> +++ b/drivers/usb/chipidea/usbmisc_imx6q.c
> @@ -0,0 +1,161 @@
> +/*
> + * 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;
> +	struct clk *clk;
> +	struct device *usb_dev[USB_DEV_MAX];
> +	spinlock_t lock;
> +
> +	int disable_oc:USB_DEV_MAX;
> +};
> +
> +static struct imx6q_usbmisc *usbmisc;
> +
> +static int get_index(struct device *dev)
> +{
> +	int i, id = -1;
> +
> +	for (i = 0; i < USB_DEV_MAX; i ++) {
> +		if (usbmisc->usb_dev[i] == dev) {
> +			id = i;
> +			break;
> +		}
> +	}
> +
> +	if (id != -1)
> +		return id;
> +
> +	id = of_alias_get_id(dev->of_node, "usb");
> +	if (id < 0)
> +		dev_err(dev, "failed to get alias id, errno %d\n", id);
> +	return id;
> +}
> +
> +static int usbmisc_imx6q_init(struct device *usb_dev)
> +{
> +
> +	int id;
> +	u32 reg;
> +	struct device_node *usb_np;
> +	unsigned long flags;
> +
> +	id = get_index(usb_dev);
> +	if (id < 0)
> +		return id;
> +
> +	usb_np = usb_dev->of_node;
> +	if (of_find_property(usb_np, "disable-over-current", NULL)) {
> +		spin_lock_irqsave(&usbmisc->lock, flags);
> +		usbmisc->disable_oc |= BIT(id);
> +		reg = readl(usbmisc->base + id * 4);
> +		writel(reg | BM_OVER_CUR_DIS, usbmisc->base + id * 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)
> +		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,
> +	 },
> +};
> +
> +static int __init usbmisc_imx6q_drv_init(void)
> +{
> +	return platform_driver_register(&usbmisc_imx6q_driver);
> +}
> +subsys_initcall(usbmisc_imx6q_drv_init);
> +
> +static 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("i.MX6Q non-core usb register handling");
> +MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");

This patch looks like a recipe for disaster IMHO, but it's Alex's call.

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

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Richard Zhao]

> 
> This patch looks like a recipe for disaster IMHO, but it's Alex's call.
Well, if there's any better place to hold it, I'd like to move there.
Another place is arch/arm/mach-imx. Sascha/Shawn who are maintainers of
imx are also in cc list.

Thanks
Richard
> 
> -- 
> balbi

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Marc Kleine-Budde]

On 07/23/2012 11:27 AM, Richard Zhao wrote:
>> This patch looks like a recipe for disaster IMHO, but it's Alex's call.
> Well, if there's any better place to hold it, I'd like to move there.
> Another place is arch/arm/mach-imx. Sascha/Shawn who are maintainers of
> imx are also in cc list.

IMHO it's not about the place where the driver is located, it's about
the shortcomings Michael and Sascha mentioned.

regards, 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: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/b2a02636/attachment.sig>

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Felipe Balbi]

On Mon, Jul 23, 2012 at 11:30:18AM +0200, Marc Kleine-Budde wrote:
> On 07/23/2012 11:27 AM, Richard Zhao wrote:
> >> This patch looks like a recipe for disaster IMHO, but it's Alex's call.
> > Well, if there's any better place to hold it, I'd like to move there.
> > Another place is arch/arm/mach-imx. Sascha/Shawn who are maintainers of
> > imx are also in cc list.
> 
> IMHO it's not about the place where the driver is located, it's about
> the shortcomings Michael and Sascha mentioned.

exactly. It's not about where a file is placed or how it's called. It's
how you decided to implement it. This will prevent a platform with
multiple instances of the IP to be used, at least.

Also, if that set of registers are shared, you ought to have a dedicated
driver to handle mutual exclusion and so on. I don't know details about
the i.MX platform, that really smells funny by looking at it.

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

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Richard Zhao]

On Mon, Jul 23, 2012 at 01:51:55PM +0300, Felipe Balbi wrote:
> On Mon, Jul 23, 2012 at 11:30:18AM +0200, Marc Kleine-Budde wrote:
> > On 07/23/2012 11:27 AM, Richard Zhao wrote:
> > >> This patch looks like a recipe for disaster IMHO, but it's Alex's call.
> > > Well, if there's any better place to hold it, I'd like to move there.
> > > Another place is arch/arm/mach-imx. Sascha/Shawn who are maintainers of
> > > imx are also in cc list.
> > 
> > IMHO it's not about the place where the driver is located, it's about
> > the shortcomings Michael and Sascha mentioned.
Thanks, Marc.
> 
> exactly. It's not about where a file is placed or how it's called. It's
> how you decided to implement it. This will prevent a platform with
> multiple instances of the IP to be used, at least.
To me, usbmisc on imx6 is just a set of non-core registers which is
companion of Chipidea IP cores. All cores share the same usbmisc.
>From design perspective, ci13xxx_imx knows nothing about usbmisc. It
just call the ops in proper occasion. usbmisc driver wrappers all
SoC specific things.
> 
> Also, if that set of registers are shared, you ought to have a dedicated
> driver to handle mutual exclusion and so on.
Only usbmisc driver access the registers, but maybe called from
different Chipidea devices. So it has a spinlock to protect register
access.
> I don't know details about
> the i.MX platform, that really smells funny by looking at it.

Thanks
Richard
> 
> -- 
> balbi

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Felipe Balbi]

On Mon, Jul 23, 2012 at 07:13:52PM +0800, Richard Zhao wrote:
> On Mon, Jul 23, 2012 at 01:51:55PM +0300, Felipe Balbi wrote:
> > On Mon, Jul 23, 2012 at 11:30:18AM +0200, Marc Kleine-Budde wrote:
> > > On 07/23/2012 11:27 AM, Richard Zhao wrote:
> > > >> This patch looks like a recipe for disaster IMHO, but it's Alex's call.
> > > > Well, if there's any better place to hold it, I'd like to move there.
> > > > Another place is arch/arm/mach-imx. Sascha/Shawn who are maintainers of
> > > > imx are also in cc list.
> > > 
> > > IMHO it's not about the place where the driver is located, it's about
> > > the shortcomings Michael and Sascha mentioned.
> Thanks, Marc.
> > 
> > exactly. It's not about where a file is placed or how it's called. It's
> > how you decided to implement it. This will prevent a platform with
> > multiple instances of the IP to be used, at least.
> To me, usbmisc on imx6 is just a set of non-core registers which is
> companion of Chipidea IP cores. All cores share the same usbmisc.
> From design perspective, ci13xxx_imx knows nothing about usbmisc. It
> just call the ops in proper occasion. usbmisc driver wrappers all
> SoC specific things.

but if all you need is to call and initialization function, why don't
you just do it on probe() of that usbmisc stuff ? Is this usbmisc used
only for this USB IP ?

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

[PATCH v2 1/2] USB: chipidea: add imx usbmisc support

[Richard Zhao]

On Mon, Jul 23, 2012 at 02:21:06PM +0300, Felipe Balbi wrote:
> On Mon, Jul 23, 2012 at 07:13:52PM +0800, Richard Zhao wrote:
> > On Mon, Jul 23, 2012 at 01:51:55PM +0300, Felipe Balbi wrote:
> > > On Mon, Jul 23, 2012 at 11:30:18AM +0200, Marc Kleine-Budde wrote:
> > > > On 07/23/2012 11:27 AM, Richard Zhao wrote:
> > > > >> This patch looks like a recipe for disaster IMHO, but it's Alex's call.
> > > > > Well, if there's any better place to hold it, I'd like to move there.
> > > > > Another place is arch/arm/mach-imx. Sascha/Shawn who are maintainers of
> > > > > imx are also in cc list.
> > > > 
> > > > IMHO it's not about the place where the driver is located, it's about
> > > > the shortcomings Michael and Sascha mentioned.
> > Thanks, Marc.
> > > 
> > > exactly. It's not about where a file is placed or how it's called. It's
> > > how you decided to implement it. This will prevent a platform with
> > > multiple instances of the IP to be used, at least.
> > To me, usbmisc on imx6 is just a set of non-core registers which is
> > companion of Chipidea IP cores. All cores share the same usbmisc.
> > From design perspective, ci13xxx_imx knows nothing about usbmisc. It
> > just call the ops in proper occasion. usbmisc driver wrappers all
> > SoC specific things.
> 
> but if all you need is to call and initialization function, why don't
> you just do it on probe() of that usbmisc stuff ? Is this usbmisc used
> only for this USB IP ?
If it only need to init once, it may follow that way. But it also have
remote wakeup enable/disable bits, which in Freescale internal driver,
are set enabled or disabled at runtime.

Thanks
Richard
> 
> -- 
> balbi