[PATCH 0/3] Kesytone II USB support

[PATCH 0/3] Kesytone II USB support

[WingMan Kwok]

Series adds USB host support for Keystone SOCs. Keystone SOCs uses dwc3
hardware IP implementation.  On Keystone II platforms, we use no-op phy
driver.

All three patches are posted together just for completeness. Only first patch
is expected to go via usb tree and other two via linux-keystone tree.

Patchset are tested on Keystone II EVM with USB2.0 and USB3.0 flash drives.

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>

WingMan Kwok (3):
  usb: dwc3: Add Keystone specific glue layer
  ARM: dts: keystone: Add usb devicetree bindings
  ARM: keystone: defconfig: enable USB host mode support

 .../devicetree/bindings/usb/keystone-usb.txt       |   43 ++++
 arch/arm/boot/dts/keystone.dtsi                    |   27 ++
 arch/arm/configs/keystone_defconfig                |   20 +-
 drivers/usb/dwc3/Kconfig                           |    7 +
 drivers/usb/dwc3/Makefile                          |    1 +
 drivers/usb/dwc3/dwc3-keystone.c                   |  272 ++++++++++++++++++++
 6 files changed, 369 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/usb/keystone-usb.txt
 create mode 100644 drivers/usb/dwc3/dwc3-keystone.c

-- 
1.7.9.5

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[WingMan Kwok]

Add Keystone platform specific glue layer to support
USB3 Host mode.

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
---
 drivers/usb/dwc3/Kconfig         |    7 +
 drivers/usb/dwc3/Makefile        |    1 +
 drivers/usb/dwc3/dwc3-keystone.c |  272 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+)
 create mode 100644 drivers/usb/dwc3/dwc3-keystone.c

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 70fc430..e2c730f 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -70,6 +70,13 @@ config USB_DWC3_PCI
 	  One such PCIe-based platform is Synopsys' PCIe HAPS model of
 	  this IP.
 
+config USB_DWC3_KEYSTONE
+	tristate "Texas Instruments Keystone2 Platforms"
+	default USB_DWC3
+	help
+	  Support of USB2/3 functionality in TI Keystone2 platforms.
+	  Say 'Y' or 'M' here if you have one such device
+
 comment "Debugging features"
 
 config USB_DWC3_DEBUG
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index dd17601..10ac3e7 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -32,3 +32,4 @@ endif
 obj-$(CONFIG_USB_DWC3_OMAP)		+= dwc3-omap.o
 obj-$(CONFIG_USB_DWC3_EXYNOS)		+= dwc3-exynos.o
 obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
+obj-$(CONFIG_USB_DWC3_KEYSTONE)		+= dwc3-keystone.o
diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
new file mode 100644
index 0000000..a4c9cbc
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -0,0 +1,272 @@
+/**
+ * dwc3-keystone.c - Keystone Specific Glue layer
+ *
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors: WingMan Kwok <w-kwok2@ti.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/usb/usb_phy_gen_xceiv.h>
+
+
+#define BITS(n)			(BIT(n) - 1)
+#define BITFIELD(x, s, n)	(((x) & BITS(n)) << (s))
+#define MASK			0xffffffff
+#define PHY_REF_SSP_EN(x)	BITFIELD(x, 29, 1)
+
+static u64 kdwc3_dma_mask;
+
+struct kdwc3_phy_ctrl_regs {
+	u32	phy_utmi;
+	u32	phy_pipe;
+	u32	phy_param_ctrl_1;
+	u32	phy_param_ctrl_2;
+	u32	phy_clock;
+	u32	phy_pll;
+};
+
+struct kdwc3_irq_regs {
+	u32		revision;
+	u32		_rsvd0[3];
+	u32		sysconfig;
+	u32		_rsvd1[1];
+	u32		irq_eoi;
+	u32		_rsvd2[1];
+	struct {
+		u32	raw_status;
+		u32	status;
+		u32	enable_set;
+		u32	enable_clr;
+	} irqs[16];
+};
+
+struct dwc3_keystone {
+	spinlock_t				lock;
+	struct device				*dev;
+	struct clk				*clk;
+	int					irqn;
+
+	struct kdwc3_phy_ctrl_regs __iomem	*phy_ctrl;
+	struct kdwc3_irq_regs __iomem		*usbss;
+};
+
+static void kdwc3_enable_irqs(struct dwc3_keystone *kdwc)
+{
+	writel(1, &kdwc->usbss->irqs[kdwc->irqn].enable_set);
+}
+
+static void kdwc3_disable_irqs(struct dwc3_keystone *kdwc)
+{
+	writel(0, &kdwc->usbss->irqs[kdwc->irqn].enable_set);
+}
+
+static int kdwc3_enable(struct dwc3_keystone *kdwc)
+{
+	int error;
+	u32 val;
+
+	kdwc->clk = devm_clk_get(kdwc->dev, "usb");
+	if (IS_ERR_OR_NULL(kdwc->clk)) {
+		dev_err(kdwc->dev, "unable to get kdwc usb clock\n");
+		return -ENODEV;
+	}
+
+	val  = readl(&kdwc->phy_ctrl->phy_clock);
+	writel(val | PHY_REF_SSP_EN(1), &kdwc->phy_ctrl->phy_clock);
+	udelay(20);
+
+	error = clk_prepare_enable(kdwc->clk);
+	if (error < 0) {
+		dev_dbg(kdwc->dev, "unable to enable usb clock, err %d\n",
+			error);
+		writel(val, &kdwc->phy_ctrl->phy_clock);
+		return error;
+	}
+
+	/* soft reset usbss */
+	writel(1, &kdwc->usbss->sysconfig);
+	while (readl(&kdwc->usbss->sysconfig) & 1)
+		;
+
+	val = readl(&kdwc->usbss->revision);
+	dev_info(kdwc->dev, "usbss revision %x\n", val);
+
+	return 0;
+}
+
+static void kdwc3_disable(struct dwc3_keystone *kdwc)
+{
+	u32 val;
+
+	clk_disable_unprepare(kdwc->clk);
+
+	val  = readl(&kdwc->phy_ctrl->phy_clock);
+	val &= ~PHY_REF_SSP_EN(MASK);
+	writel(val, &kdwc->phy_ctrl->phy_clock);
+}
+
+static irqreturn_t dwc3_keystone_interrupt(int irq, void *_kdwc)
+{
+	struct dwc3_keystone	*kdwc = _kdwc;
+	int			irqn = kdwc->irqn;
+
+	spin_lock(&kdwc->lock);
+	writel(1, &kdwc->usbss->irqs[irqn].enable_clr);
+	writel(1, &kdwc->usbss->irqs[irqn].status);
+	writel(1, &kdwc->usbss->irqs[irqn].enable_set);
+	writel(BIT(irqn), &kdwc->usbss->irq_eoi);
+	spin_unlock(&kdwc->lock);
+
+	return IRQ_HANDLED;
+}
+
+static int kdwc3_probe(struct platform_device *pdev)
+{
+	struct device_node	*node = pdev->dev.of_node;
+	struct device		*dev = &pdev->dev;
+	struct dwc3_keystone	*kdwc;
+	struct resource		*res;
+	int			error, irq;
+
+	if (!node) {
+		dev_err(dev, "device node not found\n");
+		return -EINVAL;
+	}
+
+	kdwc = devm_kzalloc(dev, sizeof(*kdwc), GFP_KERNEL);
+	if (!kdwc)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, kdwc);
+
+	spin_lock_init(&kdwc->lock);
+	kdwc->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "missing usbss resource\n");
+		return -EINVAL;
+	}
+
+	kdwc->usbss = devm_ioremap_resource(dev, res);
+	if (IS_ERR(kdwc->usbss)) {
+		dev_err(dev, "ioremap failed on usbss region\n");
+		return PTR_ERR(kdwc->usbss);
+	}
+
+	dev_dbg(dev, "usbss control start=%08x size=%d mapped=%08x\n",
+		(u32)(res->start), (int)resource_size(res),
+		(u32)(kdwc->usbss));
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(dev, "missing usb phy resource\n");
+		return -EINVAL;
+	}
+
+	kdwc->phy_ctrl = devm_ioremap_resource(dev, res);
+	if (IS_ERR(kdwc->phy_ctrl)) {
+		dev_err(dev, "ioremap failed on usb phy region\n");
+		return PTR_ERR(kdwc->phy_ctrl);
+	}
+
+	dev_dbg(dev, "phy control start=%08x size=%d mapped=%08x\n",
+		(u32)(res->start), (int)resource_size(res),
+		(u32)(kdwc->phy_ctrl));
+
+	kdwc3_dma_mask = dma_get_mask(dev);
+	dev->dma_mask = &kdwc3_dma_mask;
+
+	error = kdwc3_enable(kdwc);
+	if (error)
+		return error;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "missing irq\n");
+		goto err_irq;
+	}
+
+	error = devm_request_irq(dev, irq, dwc3_keystone_interrupt, IRQF_SHARED,
+			"keystone-dwc3", kdwc);
+	if (error) {
+		dev_err(dev, "failed to request IRQ #%d --> %d\n",
+				irq, error);
+		goto err_irq;
+	}
+
+	kdwc->irqn = 0;
+
+	kdwc3_enable_irqs(kdwc);
+
+	error = of_platform_populate(node, NULL, NULL, dev);
+	if (error) {
+		dev_err(&pdev->dev, "failed to create dwc3 core\n");
+		goto err_core;
+	}
+
+	return 0;
+
+err_core:
+	kdwc3_disable_irqs(kdwc);
+err_irq:
+	kdwc3_disable(kdwc);
+
+	return error;
+}
+
+static int kdwc3_remove(struct platform_device *pdev)
+{
+	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
+
+	if (kdwc) {
+		kdwc3_disable_irqs(kdwc);
+		kdwc3_disable(kdwc);
+		platform_set_drvdata(pdev, NULL);
+	}
+	return 0;
+}
+
+static const struct of_device_id kdwc3_of_match[] = {
+	{ .compatible = "ti,keystone-dwc3", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, kdwc3_of_match);
+
+static struct platform_driver kdwc3_driver = {
+	.probe		= kdwc3_probe,
+	.remove		= kdwc3_remove,
+	.driver		= {
+		.name	= "keystone-dwc3",
+		.owner	        = THIS_MODULE,
+		.of_match_table	= kdwc3_of_match,
+	},
+};
+
+module_platform_driver(kdwc3_driver);
+
+MODULE_ALIAS("platform:keystone-dwc3");
+MODULE_AUTHOR("WingMan Kwok <w-kwok2@ti.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("DesignWare USB3 KEYSTONE Glue Layer");
-- 
1.7.9.5

[PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[WingMan Kwok]

Added device tree support for TI's Keystone USB driver and updated the
Documentation with device tree binding information.

On Keystone II platforms, we use no-op phy driver.

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
---
 .../devicetree/bindings/usb/keystone-usb.txt       |   43 ++++++++++++++++++++
 arch/arm/boot/dts/keystone.dtsi                    |   27 ++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/keystone-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/keystone-usb.txt b/Documentation/devicetree/bindings/usb/keystone-usb.txt
new file mode 100644
index 0000000..a67de8f
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/keystone-usb.txt
@@ -0,0 +1,43 @@
+TI Keystone Soc USB Controller
+
+DWC3 GLUE
+
+Required properties:
+ - compatible: should be "ti,keystone-dwc3".
+ - #address-cells, #size-cells : should be '1' if the device has sub-nodes
+   with 'reg' property.
+ - reg : Address and length of the register set for the device. First pair
+   is the USB subsystem specific register set.  Second pair is the
+   USB subsystem PHY control register set.
+ - interrupts : The irq number of this device that is used to interrupt the
+   MPU.
+ - ranges: allows valid 1:1 translation between child's address space and
+   parent's address space.
+ - clocks: Clock IDs array as required by the controller.
+ - clock-names: names of clocks correseponding to IDs in the clock property.
+
+Sub-nodes:
+The dwc3 core should be added as subnode to Keystone DWC3 glue.
+- dwc3 :
+   The binding details of dwc3 can be found in:
+   Documentation/devicetree/bindings/usb/dwc3.txt
+
+Example:
+	usb: usb at 2680000 {
+		compatible = "ti,keystone-dwc3";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x2680000 0x10000
+		       0x2620738 32>;
+		clocks = <&clkusb>;
+		clock-names = "usb";
+		interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
+		ranges;
+
+		dwc3 at 2690000 {
+			compatible = "synopsys,dwc3";
+			reg = <0x2690000 0x70000>;
+			interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
+			usb-phy = <&usb2_phy>, <&usb3_phy>;
+		};
+	};
diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index f6d6d9e..1e1049c 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -181,5 +181,32 @@
 			interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
 			clocks = <&clkspi>;
 		};
+
+		usb2_phy: usb2_phy {
+			compatible = "usb-nop-xceiv";
+		};
+
+		usb3_phy: usb3_phy {
+			compatible = "usb-nop-xceiv";
+		};
+
+		usb: usb at 2680000 {
+			compatible = "ti,keystone-dwc3";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x2680000 0x10000
+			       0x2620738 32>;
+			clocks = <&clkusb>;
+			clock-names = "usb";
+			interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
+			ranges;
+
+			dwc3 at 2690000 {
+				compatible = "synopsys,dwc3";
+				reg = <0x2690000 0x70000>;
+				interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
+				usb-phy = <&usb2_phy>, <&usb3_phy>;
+			};
+		};
 	};
 };
-- 
1.7.9.5

[PATCH 3/3] ARM: keystone: defconfig: enable USB host mode support

[WingMan Kwok]

Enable the USB host mode support on TI's Keystone platform.
It also enables the support of usb mass storage, FAT and Ext4
filesystems to test rootfs mount over an USB disk.

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
---
 arch/arm/configs/keystone_defconfig |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm/configs/keystone_defconfig b/arch/arm/configs/keystone_defconfig
index 9943e5d..686f612 100644
--- a/arch/arm/configs/keystone_defconfig
+++ b/arch/arm/configs/keystone_defconfig
@@ -115,6 +115,8 @@ CONFIG_MTD_UBI=y
 CONFIG_PROC_DEVICETREE=y
 CONFIG_BLK_DEV_LOOP=y
 CONFIG_EEPROM_AT24=y
+CONFIG_SCSI=y
+CONFIG_BLK_DEV_SD=y
 CONFIG_NETDEVICES=y
 CONFIG_SERIAL_8250=y
 CONFIG_SERIAL_8250_CONSOLE=y
@@ -129,10 +131,24 @@ CONFIG_SPI_DAVINCI=y
 CONFIG_SPI_SPIDEV=y
 # CONFIG_HWMON is not set
 CONFIG_WATCHDOG=y
-# CONFIG_USB_SUPPORT is not set
+CONFIG_USB=y
+CONFIG_USB_DEBUG=y
+CONFIG_USB_ANNOUNCE_NEW_DEVICES=y
+CONFIG_USB_MON=y
+CONFIG_USB_XHCI_HCD=y
+CONFIG_USB_STORAGE=y
+CONFIG_USB_DWC3=y
+CONFIG_USB_DWC3_DEBUG=y
+CONFIG_USB_DWC3_VERBOSE=y
+CONFIG_NOP_USB_XCEIV=y
 CONFIG_DMADEVICES=y
 CONFIG_COMMON_CLK_DEBUG=y
 CONFIG_MEMORY=y
+CONFIG_EXT4_FS=y
+CONFIG_EXT4_FS_POSIX_ACL=y
+CONFIG_MSDOS_FS=y
+CONFIG_VFAT_FS=y
+CONFIG_NTFS_FS=y
 CONFIG_TMPFS=y
 CONFIG_JFFS2_FS=y
 CONFIG_JFFS2_FS_WBUF_VERIFY=y
@@ -144,6 +160,8 @@ CONFIG_ROOT_NFS=y
 CONFIG_NFSD=y
 CONFIG_NFSD_V3=y
 CONFIG_NFSD_V3_ACL=y
+CONFIG_NLS_CODEPAGE_437=y
+CONFIG_NLS_ISO8859_1=y
 CONFIG_PRINTK_TIME=y
 CONFIG_DEBUG_SHIRQ=y
 CONFIG_DEBUG_INFO=y
-- 
1.7.9.5

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Felipe Balbi]

Hi,

On Mon, Nov 25, 2013 at 03:16:19PM -0500, WingMan Kwok wrote:
> diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
> new file mode 100644
> index 0000000..a4c9cbc
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-keystone.c
> @@ -0,0 +1,272 @@
> +/**
> + * dwc3-keystone.c - Keystone Specific Glue layer
> + *
> + * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * Authors: WingMan Kwok <w-kwok2@ti.com>

singular -> Author :-) there's only a single one.

> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/ioport.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/usb/usb_phy_gen_xceiv.h>
> +
> +
> +#define BITS(n)			(BIT(n) - 1)

looks like unnecessary obfuscation.

> +#define BITFIELD(x, s, n)	(((x) & BITS(n)) << (s))

likewise.

> +#define MASK			0xffffffff

huh ? You probably want a better name here.

> +#define PHY_REF_SSP_EN(x)	BITFIELD(x, 29, 1)

hmm, should be part of a PHY driver (drivers/phy/)

> +static u64 kdwc3_dma_mask;
> +
> +struct kdwc3_phy_ctrl_regs {
> +	u32	phy_utmi;
> +	u32	phy_pipe;
> +	u32	phy_param_ctrl_1;
> +	u32	phy_param_ctrl_2;
> +	u32	phy_clock;
> +	u32	phy_pll;
> +};

should be part of a PHY driver. Sorry, but I'll be very pedantic.

> +struct kdwc3_irq_regs {
> +	u32		revision;
> +	u32		_rsvd0[3];
> +	u32		sysconfig;
> +	u32		_rsvd1[1];
> +	u32		irq_eoi;
> +	u32		_rsvd2[1];
> +	struct {
> +		u32	raw_status;
> +		u32	status;
> +		u32	enable_set;
> +		u32	enable_clr;
> +	} irqs[16];
> +};

please use #define macros instead.

> +struct dwc3_keystone {
> +	spinlock_t				lock;
> +	struct device				*dev;
> +	struct clk				*clk;
> +	int					irqn;
> +
> +	struct kdwc3_phy_ctrl_regs __iomem	*phy_ctrl;

not part of this driver.

> +	struct kdwc3_irq_regs __iomem		*usbss;
> +};
> +
> +static void kdwc3_enable_irqs(struct dwc3_keystone *kdwc)
> +{
> +	writel(1, &kdwc->usbss->irqs[kdwc->irqn].enable_set);

I would like to have a define here, no magic constants.

> +}
> +
> +static void kdwc3_disable_irqs(struct dwc3_keystone *kdwc)
> +{
> +	writel(0, &kdwc->usbss->irqs[kdwc->irqn].enable_set);
> +}

you want these two functions to be more generic, along the lines of:

static void kdwc3_writel(void __iomem *base, u32 offset, u32 val)
{
	writel(val, base + offset);
}

static u32 kdwc3_readl(void __iomem *base, u32 offset)
{
	return readl(base + offset);
}

then you can add:

static void kdwc3_enable_irqs(struct dwc3_keystone *kdwc)
{
	u32 reg;

	reg = kdwc3_readl(kdwc->base, KEYSTONE_DWC3_IRQ);
	reg |= KEYSTONE_DWC3_IRQ_ENABLE;
	kdwc3_writel(kdwc->base, KEYSTONE_DWC3_IRQ, reg);
}

static void kdwc3_disable_irqs(struct dwc3_keystone *kdwc)
{
	u32 reg;

	reg = kdwc3_readl(kdwc->base, KEYSTONE_DWC3_IRQ);
	reg &= ~KEYSTONE_DWC3_IRQ_ENABLE;
	kdwc3_writel(kdwc->base, KEYSTONE_DWC3_IRQ, reg);
}

this will be much more "future-safe".

> +static int kdwc3_enable(struct dwc3_keystone *kdwc)
> +{
> +	int error;
> +	u32 val;
> +
> +	kdwc->clk = devm_clk_get(kdwc->dev, "usb");
> +	if (IS_ERR_OR_NULL(kdwc->clk)) {
> +		dev_err(kdwc->dev, "unable to get kdwc usb clock\n");
> +		return -ENODEV;
> +	}
> +
> +	val  = readl(&kdwc->phy_ctrl->phy_clock);

no PHY registers will ever be accessed from a glue layer ;-) This should
all be part of a phy driver (drivers/phy/phy-keystone.c) and dwc3.ko
will take care of the rest.

> +	writel(val | PHY_REF_SSP_EN(1), &kdwc->phy_ctrl->phy_clock);
> +	udelay(20);
> +
> +	error = clk_prepare_enable(kdwc->clk);
> +	if (error < 0) {
> +		dev_dbg(kdwc->dev, "unable to enable usb clock, err %d\n",
> +			error);
> +		writel(val, &kdwc->phy_ctrl->phy_clock);
> +		return error;
> +	}
> +
> +	/* soft reset usbss */
> +	writel(1, &kdwc->usbss->sysconfig);

shoul we access sysconfig registers from drivers ? How about using a
reset driver ? (drivers/reset/).

> +	while (readl(&kdwc->usbss->sysconfig) & 1)
> +		;

infinite loop ? Never, please add a timeout.

> +	val = readl(&kdwc->usbss->revision);
> +	dev_info(kdwc->dev, "usbss revision %x\n", val);

unnecessary dev_info(). You're not even decoding the revision
information.

> +	return 0;
> +}
> +
> +static void kdwc3_disable(struct dwc3_keystone *kdwc)
> +{
> +	u32 val;
> +
> +	clk_disable_unprepare(kdwc->clk);
> +
> +	val  = readl(&kdwc->phy_ctrl->phy_clock);
> +	val &= ~PHY_REF_SSP_EN(MASK);
> +	writel(val, &kdwc->phy_ctrl->phy_clock);

should not be done here.

> +}
> +
> +static irqreturn_t dwc3_keystone_interrupt(int irq, void *_kdwc)
> +{
> +	struct dwc3_keystone	*kdwc = _kdwc;
> +	int			irqn = kdwc->irqn;

why ? the irq argument should already be the correct IRQ number, right ?

> +	spin_lock(&kdwc->lock);
> +	writel(1, &kdwc->usbss->irqs[irqn].enable_clr);
> +	writel(1, &kdwc->usbss->irqs[irqn].status);
> +	writel(1, &kdwc->usbss->irqs[irqn].enable_set);

no magic constants...

> +	writel(BIT(irqn), &kdwc->usbss->irq_eoi);
> +	spin_unlock(&kdwc->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int kdwc3_probe(struct platform_device *pdev)
> +{
> +	struct device_node	*node = pdev->dev.of_node;
> +	struct device		*dev = &pdev->dev;
> +	struct dwc3_keystone	*kdwc;
> +	struct resource		*res;
> +	int			error, irq;
> +
> +	if (!node) {
> +		dev_err(dev, "device node not found\n");
> +		return -EINVAL;
> +	}

if this will only be used on DT-based boot, node *must* be true. If it's
not, we're dealing with a pretty critical bug, in which case we want a
Kernel Oops to happen so we catch that bug, please drop this check.

> +
> +	kdwc = devm_kzalloc(dev, sizeof(*kdwc), GFP_KERNEL);
> +	if (!kdwc)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, kdwc);
> +
> +	spin_lock_init(&kdwc->lock);
> +	kdwc->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "missing usbss resource\n");
> +		return -EINVAL;
> +	}
> +
> +	kdwc->usbss = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(kdwc->usbss)) {
> +		dev_err(dev, "ioremap failed on usbss region\n");

no need to print anything, devm_ioremap_resource() already prints
sensible messages.

> +		return PTR_ERR(kdwc->usbss);
> +	}
> +
> +	dev_dbg(dev, "usbss control start=%08x size=%d mapped=%08x\n",
> +		(u32)(res->start), (int)resource_size(res),
> +		(u32)(kdwc->usbss));

this is really unnecessary, if you really want to have it, move it to
dev_vdbg().

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(dev, "missing usb phy resource\n");
> +		return -EINVAL;
> +	}
> +
> +	kdwc->phy_ctrl = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(kdwc->phy_ctrl)) {
> +		dev_err(dev, "ioremap failed on usb phy region\n");
> +		return PTR_ERR(kdwc->phy_ctrl);
> +	}

second resource should be part of the PHY driver. This is even more
important considering you're using DT, we don't want to allow broken DTS
data to be accepted.

> +	dev_dbg(dev, "phy control start=%08x size=%d mapped=%08x\n",
> +		(u32)(res->start), (int)resource_size(res),
> +		(u32)(kdwc->phy_ctrl));

unnecessary.

> +	kdwc3_dma_mask = dma_get_mask(dev);
> +	dev->dma_mask = &kdwc3_dma_mask;
> +
> +	error = kdwc3_enable(kdwc);

I would drop this function and just add your clk_prepare() here, then
move clk_enable()/clk_disable() to ->runtime_resume/->runtime_suspend()
respectively. Then you could just call pm_runtime_get_sync() when you
need to access your registers and pm_runtime_put() when you want to drop
the clock reference.

this will even make PM implementation a lot easier for you going
forward.

> +	if (error)
> +		return error;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "missing irq\n");
> +		goto err_irq;
> +	}
> +
> +	error = devm_request_irq(dev, irq, dwc3_keystone_interrupt, IRQF_SHARED,
> +			"keystone-dwc3", kdwc);

dev_name(dev) ?? If you happen to have multiple instances of a dwc3
core, it'll give it better names.

> +	if (error) {
> +		dev_err(dev, "failed to request IRQ #%d --> %d\n",
> +				irq, error);
> +		goto err_irq;
> +	}
> +
> +	kdwc->irqn = 0;

wait, what ? Can you describe how this part works ? Why do you even need
this if it's alway zero ?

> +	kdwc3_enable_irqs(kdwc);
> +
> +	error = of_platform_populate(node, NULL, NULL, dev);
> +	if (error) {
> +		dev_err(&pdev->dev, "failed to create dwc3 core\n");
> +		goto err_core;
> +	}
> +
> +	return 0;
> +
> +err_core:
> +	kdwc3_disable_irqs(kdwc);
> +err_irq:
> +	kdwc3_disable(kdwc);
> +
> +	return error;
> +}
> +
> +static int kdwc3_remove(struct platform_device *pdev)
> +{
> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
> +
> +	if (kdwc) {

kdwc will never be true, you can drop this check.

> +		kdwc3_disable_irqs(kdwc);
> +		kdwc3_disable(kdwc);
> +		platform_set_drvdata(pdev, NULL);
> +	}
> +	return 0;
> +}
> +
> +static const struct of_device_id kdwc3_of_match[] = {
> +	{ .compatible = "ti,keystone-dwc3", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, kdwc3_of_match);
> +
> +static struct platform_driver kdwc3_driver = {
> +	.probe		= kdwc3_probe,
> +	.remove		= kdwc3_remove,
> +	.driver		= {
> +		.name	= "keystone-dwc3",
> +		.owner	        = THIS_MODULE,
> +		.of_match_table	= kdwc3_of_match,
> +	},
> +};
> +
> +module_platform_driver(kdwc3_driver);
> +
> +MODULE_ALIAS("platform:keystone-dwc3");
> +MODULE_AUTHOR("WingMan Kwok <w-kwok2@ti.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("DesignWare USB3 KEYSTONE Glue Layer");
> -- 
> 1.7.9.5
> 

-- 
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/20131125/355dad57/attachment.sig>

[PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[Felipe Balbi]

Hi,

On Mon, Nov 25, 2013 at 03:16:20PM -0500, WingMan Kwok wrote:
> Added device tree support for TI's Keystone USB driver and updated the
> Documentation with device tree binding information.
> 
> On Keystone II platforms, we use no-op phy driver.
> 
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> ---
>  .../devicetree/bindings/usb/keystone-usb.txt       |   43 ++++++++++++++++++++
>  arch/arm/boot/dts/keystone.dtsi                    |   27 ++++++++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/keystone-usb.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/keystone-usb.txt b/Documentation/devicetree/bindings/usb/keystone-usb.txt
> new file mode 100644
> index 0000000..a67de8f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/keystone-usb.txt
> @@ -0,0 +1,43 @@
> +TI Keystone Soc USB Controller
> +
> +DWC3 GLUE
> +
> +Required properties:
> + - compatible: should be "ti,keystone-dwc3".
> + - #address-cells, #size-cells : should be '1' if the device has sub-nodes
> +   with 'reg' property.
> + - reg : Address and length of the register set for the device. First pair
> +   is the USB subsystem specific register set.  Second pair is the
> +   USB subsystem PHY control register set.
> + - interrupts : The irq number of this device that is used to interrupt the
> +   MPU.
> + - ranges: allows valid 1:1 translation between child's address space and
> +   parent's address space.
> + - clocks: Clock IDs array as required by the controller.
> + - clock-names: names of clocks correseponding to IDs in the clock property.
> +
> +Sub-nodes:
> +The dwc3 core should be added as subnode to Keystone DWC3 glue.
> +- dwc3 :
> +   The binding details of dwc3 can be found in:
> +   Documentation/devicetree/bindings/usb/dwc3.txt
> +
> +Example:
> +	usb: usb at 2680000 {
> +		compatible = "ti,keystone-dwc3";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x2680000 0x10000
> +		       0x2620738 32>;
> +		clocks = <&clkusb>;
> +		clock-names = "usb";
> +		interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> +		ranges;
> +
> +		dwc3 at 2690000 {
> +			compatible = "synopsys,dwc3";
> +			reg = <0x2690000 0x70000>;
> +			interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> +			usb-phy = <&usb2_phy>, <&usb3_phy>;
> +		};
> +	};
> diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
> index f6d6d9e..1e1049c 100644
> --- a/arch/arm/boot/dts/keystone.dtsi
> +++ b/arch/arm/boot/dts/keystone.dtsi
> @@ -181,5 +181,32 @@
>  			interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
>  			clocks = <&clkspi>;
>  		};
> +
> +		usb2_phy: usb2_phy {
> +			compatible = "usb-nop-xceiv";
> +		};
> +
> +		usb3_phy: usb3_phy {
> +			compatible = "usb-nop-xceiv";
> +		};

you actually have some phy registers which need to be fiddled with. I'd
suggest implementing this the same way phy-am335x.c is implemented. It
still reuses most of phy-generic.c, but it has some hooks to implement
->init() and ->shutdown(), which seem to be the only methods you need.

BTW, some preliminary TRM coming my way would be cool, so I can better
understand how this HW behaves. A board would also go a long way, so I
could test this myself (we are part of the same company anyway).

cheers

-- 
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/20131125/34301f12/attachment.sig>

[PATCH 3/3] ARM: keystone: defconfig: enable USB host mode support

[Felipe Balbi]

Hi,

On Mon, Nov 25, 2013 at 03:16:21PM -0500, WingMan Kwok wrote:
> Enable the USB host mode support on TI's Keystone platform.
> It also enables the support of usb mass storage, FAT and Ext4
> filesystems to test rootfs mount over an USB disk.
> 
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>

no phy driver anywhere ? How will nop-xceiv ever probe if you don't
enable the driver for 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/20131125/f58f7ddf/attachment-0001.sig>

[PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[Santosh Shilimkar]

On Monday 25 November 2013 03:42 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 25, 2013 at 03:16:20PM -0500, WingMan Kwok wrote:
>> Added device tree support for TI's Keystone USB driver and updated the
>> Documentation with device tree binding information.
>>
>> On Keystone II platforms, we use no-op phy driver.
>>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
>> ---

[...]

> BTW, some preliminary TRM coming my way would be cool, so I can better
> understand how this HW behaves. A board would also go a long way, so I
> could test this myself (we are part of the same company anyway).
> 
TRM documentation is bit different. There is no single TRM which has all
the information but different chapters called user guides. For Keystone
USB, the user guide isn't ready yet but there is an internal version for
which we will send you a link. 

Boards are hard to get for now but we can see next year.

Regards,
Santosh

[PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[Felipe Balbi]

On Mon, Nov 25, 2013 at 04:04:47PM -0500, Santosh Shilimkar wrote:
> On Monday 25 November 2013 03:42 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Nov 25, 2013 at 03:16:20PM -0500, WingMan Kwok wrote:
> >> Added device tree support for TI's Keystone USB driver and updated the
> >> Documentation with device tree binding information.
> >>
> >> On Keystone II platforms, we use no-op phy driver.
> >>
> >> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> >> ---
> 
> [...]
> 
> > BTW, some preliminary TRM coming my way would be cool, so I can better
> > understand how this HW behaves. A board would also go a long way, so I
> > could test this myself (we are part of the same company anyway).
> > 
> TRM documentation is bit different. There is no single TRM which has all
> the information but different chapters called user guides. For Keystone
> USB, the user guide isn't ready yet but there is an internal version for
> which we will send you a link. 
> 
> Boards are hard to get for now but we can see next year.

fair enough, documentation already helps a lot though.

-- 
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/20131125/8c245baf/attachment.sig>

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Monday 25 November 2013 03:39 PM, Felipe Balbi wrote:
> Hi,
> 

[...]

> 
>> +	kdwc3_dma_mask = dma_get_mask(dev);
>> +	dev->dma_mask = &kdwc3_dma_mask;
>> +
>> +	error = kdwc3_enable(kdwc);
> 
> I would drop this function and just add your clk_prepare() here, then
> move clk_enable()/clk_disable() to ->runtime_resume/->runtime_suspend()
> respectively. Then you could just call pm_runtime_get_sync() when you
> need to access your registers and pm_runtime_put() when you want to drop
> the clock reference.
> 
> this will even make PM implementation a lot easier for you going
> forward.
> 
Just to make the PM runtime part clear, there are few issues with PM
core clock layer [1], hence drivers is using clock layer. Its trivial
to update the driver though once the issue is sorted out.

Meanwhile driver development continue to be with clock calls. 

Regards,
Santosh

[1] https://groups.google.com/forum/#!msg/linux.kernel/w5gFxFsIK-c/jYcnRET_EdAJ

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Felipe Balbi]

On Mon, Nov 25, 2013 at 07:49:01PM -0500, Santosh Shilimkar wrote:
> On Monday 25 November 2013 03:39 PM, Felipe Balbi wrote:
> > Hi,
> > 
> 
> [...]
> 
> > 
> >> +	kdwc3_dma_mask = dma_get_mask(dev);
> >> +	dev->dma_mask = &kdwc3_dma_mask;
> >> +
> >> +	error = kdwc3_enable(kdwc);
> > 
> > I would drop this function and just add your clk_prepare() here, then
> > move clk_enable()/clk_disable() to ->runtime_resume/->runtime_suspend()
> > respectively. Then you could just call pm_runtime_get_sync() when you
> > need to access your registers and pm_runtime_put() when you want to drop
> > the clock reference.
> > 
> > this will even make PM implementation a lot easier for you going
> > forward.
> > 
> Just to make the PM runtime part clear, there are few issues with PM
> core clock layer [1], hence drivers is using clock layer. Its trivial
> to update the driver though once the issue is sorted out.
> 
> Meanwhile driver development continue to be with clock calls. 

I don't mind having those clock calls, just suggested a different
placement for them.

-- 
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/20131126/d5fef4bc/attachment.sig>

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Tuesday 26 November 2013 12:16 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 07:49:01PM -0500, Santosh Shilimkar wrote:
>> On Monday 25 November 2013 03:39 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>
>> [...]
>>
>>>
>>>> +	kdwc3_dma_mask = dma_get_mask(dev);
>>>> +	dev->dma_mask = &kdwc3_dma_mask;
>>>> +
>>>> +	error = kdwc3_enable(kdwc);
>>>
>>> I would drop this function and just add your clk_prepare() here, then
>>> move clk_enable()/clk_disable() to ->runtime_resume/->runtime_suspend()
>>> respectively. Then you could just call pm_runtime_get_sync() when you
>>> need to access your registers and pm_runtime_put() when you want to drop
>>> the clock reference.
>>>
>>> this will even make PM implementation a lot easier for you going
>>> forward.
>>>
>> Just to make the PM runtime part clear, there are few issues with PM
>> core clock layer [1], hence drivers is using clock layer. Its trivial
>> to update the driver though once the issue is sorted out.
>>
>> Meanwhile driver development continue to be with clock calls. 
> 
> I don't mind having those clock calls, just suggested a different
> placement for them.
> 
I got that part. Just wanted to clear the runtime PM part.

Regards,
Santosh

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[George Cherian]

On 11/26/2013 1:46 AM, WingMan Kwok wrote:
> Add Keystone platform specific glue layer to support
> USB3 Host mode.
>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> ---
>   drivers/usb/dwc3/Kconfig         |    7 +
>   drivers/usb/dwc3/Makefile        |    1 +
>   drivers/usb/dwc3/dwc3-keystone.c |  272 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 280 insertions(+)
>   create mode 100644 drivers/usb/dwc3/dwc3-keystone.c

<snip>
> +	error = of_platform_populate(node, NULL, NULL, dev);
> +	if (error) {
> +		dev_err(&pdev->dev, "failed to create dwc3 core\n");
> +		goto err_core;
> +	}
> +
> +	return 0;
> +
> +err_core:
> +	kdwc3_disable_irqs(kdwc);
> +err_irq:
> +	kdwc3_disable(kdwc);
> +
> +	return error;
> +}
> +
> +static int kdwc3_remove(struct platform_device *pdev)
> +{
> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
> +
> +	if (kdwc) {
> +		kdwc3_disable_irqs(kdwc);
> +		kdwc3_disable(kdwc);
> +		platform_set_drvdata(pdev, NULL);
> +	}
> +	return 0;
> +}
> +

You need to unregister the child nodes in remove.
Also why can't the dwc3-omap driver be reused, Felipe??
I believe the TI wrapper for Keystone is similar to that of AM437x or 
OMAP5.
> +static const struct of_device_id kdwc3_of_match[] = {
> +	{ .compatible = "ti,keystone-dwc3", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, kdwc3_of_match);
> +
> +static struct platform_driver kdwc3_driver = {
> +	.probe		= kdwc3_probe,
> +	.remove		= kdwc3_remove,
> +	.driver		= {
> +		.name	= "keystone-dwc3",
> +		.owner	        = THIS_MODULE,
> +		.of_match_table	= kdwc3_of_match,
> +	},
> +};
> +
> +module_platform_driver(kdwc3_driver);
> +
> +MODULE_ALIAS("platform:keystone-dwc3");
> +MODULE_AUTHOR("WingMan Kwok <w-kwok2@ti.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("DesignWare USB3 KEYSTONE Glue Layer");


-- 
-George

[PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[George Cherian]

On 11/26/2013 1:46 AM, WingMan Kwok wrote:
> Added device tree support for TI's Keystone USB driver and updated the
> Documentation with device tree binding information.
>
> On Keystone II platforms, we use no-op phy driver.
>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> ---
>   .../devicetree/bindings/usb/keystone-usb.txt       |   43 ++++++++++++++++++++
>   arch/arm/boot/dts/keystone.dtsi                    |   27 ++++++++++++
>   2 files changed, 70 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/usb/keystone-usb.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/keystone-usb.txt b/Documentation/devicetree/bindings/usb/keystone-usb.txt
> new file mode 100644
> index 0000000..a67de8f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/keystone-usb.txt
> @@ -0,0 +1,43 @@
> +TI Keystone Soc USB Controller
> +
> +DWC3 GLUE
> +
> +Required properties:
> + - compatible: should be "ti,keystone-dwc3".
> + - #address-cells, #size-cells : should be '1' if the device has sub-nodes
> +   with 'reg' property.
> + - reg : Address and length of the register set for the device. First pair
> +   is the USB subsystem specific register set.  Second pair is the
> +   USB subsystem PHY control register set.
> + - interrupts : The irq number of this device that is used to interrupt the
> +   MPU.
> + - ranges: allows valid 1:1 translation between child's address space and
> +   parent's address space.
> + - clocks: Clock IDs array as required by the controller.
> + - clock-names: names of clocks correseponding to IDs in the clock property.
> +
> +Sub-nodes:
> +The dwc3 core should be added as subnode to Keystone DWC3 glue.
> +- dwc3 :
> +   The binding details of dwc3 can be found in:
> +   Documentation/devicetree/bindings/usb/dwc3.txt
> +
> +Example:
> +	usb: usb at 2680000 {
> +		compatible = "ti,keystone-dwc3";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x2680000 0x10000
> +		       0x2620738 32>;
> +		clocks = <&clkusb>;
> +		clock-names = "usb";
> +		interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> +		ranges;
> +
> +		dwc3 at 2690000 {
> +			compatible = "synopsys,dwc3";
> +			reg = <0x2690000 0x70000>;
> +			interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> +			usb-phy = <&usb2_phy>, <&usb3_phy>;
> +		};
> +	};
> diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
> index f6d6d9e..1e1049c 100644
> --- a/arch/arm/boot/dts/keystone.dtsi
> +++ b/arch/arm/boot/dts/keystone.dtsi
> @@ -181,5 +181,32 @@
>   			interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
>   			clocks = <&clkspi>;
>   		};
> +
> +		usb2_phy: usb2_phy {
> +			compatible = "usb-nop-xceiv";
> +		};
> +
> +		usb3_phy: usb3_phy {
> +			compatible = "usb-nop-xceiv";
> +		};
> +
> +		usb: usb at 2680000 {
> +			compatible = "ti,keystone-dwc3";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x2680000 0x10000
> +			       0x2620738 32>;
> +			clocks = <&clkusb>;
> +			clock-names = "usb";
> +			interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;

You don't have seperate interrrupt for wrapper and core?
Is it the same interrupt shared between XHCI,DWC3 and wrapper?

> +			ranges;
> +
> +			dwc3 at 2690000 {
> +				compatible = "synopsys,dwc3";
> +				reg = <0x2690000 0x70000>;
> +				interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> +				usb-phy = <&usb2_phy>, <&usb3_phy>;
> +			};
> +		};
>   	};
>   };


-- 
-George

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Felipe Balbi]

Hi,

On Wed, Nov 27, 2013 at 02:49:54PM +0530, George Cherian wrote:
> >+	error = of_platform_populate(node, NULL, NULL, dev);
> >+	if (error) {
> >+		dev_err(&pdev->dev, "failed to create dwc3 core\n");
> >+		goto err_core;
> >+	}
> >+
> >+	return 0;
> >+
> >+err_core:
> >+	kdwc3_disable_irqs(kdwc);
> >+err_irq:
> >+	kdwc3_disable(kdwc);
> >+
> >+	return error;
> >+}
> >+
> >+static int kdwc3_remove(struct platform_device *pdev)
> >+{
> >+	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
> >+
> >+	if (kdwc) {
> >+		kdwc3_disable_irqs(kdwc);
> >+		kdwc3_disable(kdwc);
> >+		platform_set_drvdata(pdev, NULL);
> >+	}
> >+	return 0;
> >+}
> >+
> 
> You need to unregister the child nodes in remove.
> Also why can't the dwc3-omap driver be reused, Felipe??
> I believe the TI wrapper for Keystone is similar to that of AM437x or
> OMAP5.

it is very similar indeed, if it can be easily re-use that glue, I'd
rather not add another.

-- 
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/20131127/eac509d5/attachment.sig>

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Wednesday 27 November 2013 12:41 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Nov 27, 2013 at 02:49:54PM +0530, George Cherian wrote:
>>> +	error = of_platform_populate(node, NULL, NULL, dev);
>>> +	if (error) {
>>> +		dev_err(&pdev->dev, "failed to create dwc3 core\n");
>>> +		goto err_core;
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +err_core:
>>> +	kdwc3_disable_irqs(kdwc);
>>> +err_irq:
>>> +	kdwc3_disable(kdwc);
>>> +
>>> +	return error;
>>> +}
>>> +
>>> +static int kdwc3_remove(struct platform_device *pdev)
>>> +{
>>> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
>>> +
>>> +	if (kdwc) {
>>> +		kdwc3_disable_irqs(kdwc);
>>> +		kdwc3_disable(kdwc);
>>> +		platform_set_drvdata(pdev, NULL);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>
>> You need to unregister the child nodes in remove.
>> Also why can't the dwc3-omap driver be reused, Felipe??
>> I believe the TI wrapper for Keystone is similar to that of AM437x or
>> OMAP5.
> 
> it is very similar indeed, if it can be easily re-use that glue, I'd
> rather not add another.
> 
Initial idea was actually to use same driver but the integration IP
is quite different on Keystone vs OMAP which lead to have a separate
glue layer. They look similar but there are differences in terms of phys,
interrupts, register space. And also non OTG support, runtime PM
vs clock etc for now.

After all the glue is really a very small code describes the integration
details thanks to nice abstracted dwc3 core layer. So as discussed
over irc, keeping the separate glue is preferred but do let us know
if you think otherwise.

Regards,
Santosh

[PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Felipe Balbi]

On Wed, Nov 27, 2013 at 01:32:04PM -0500, Santosh Shilimkar wrote:
> On Wednesday 27 November 2013 12:41 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Nov 27, 2013 at 02:49:54PM +0530, George Cherian wrote:
> >>> +	error = of_platform_populate(node, NULL, NULL, dev);
> >>> +	if (error) {
> >>> +		dev_err(&pdev->dev, "failed to create dwc3 core\n");
> >>> +		goto err_core;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err_core:
> >>> +	kdwc3_disable_irqs(kdwc);
> >>> +err_irq:
> >>> +	kdwc3_disable(kdwc);
> >>> +
> >>> +	return error;
> >>> +}
> >>> +
> >>> +static int kdwc3_remove(struct platform_device *pdev)
> >>> +{
> >>> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
> >>> +
> >>> +	if (kdwc) {
> >>> +		kdwc3_disable_irqs(kdwc);
> >>> +		kdwc3_disable(kdwc);
> >>> +		platform_set_drvdata(pdev, NULL);
> >>> +	}
> >>> +	return 0;
> >>> +}
> >>> +
> >>
> >> You need to unregister the child nodes in remove.
> >> Also why can't the dwc3-omap driver be reused, Felipe??
> >> I believe the TI wrapper for Keystone is similar to that of AM437x or
> >> OMAP5.
> > 
> > it is very similar indeed, if it can be easily re-use that glue, I'd
> > rather not add another.
> > 
> Initial idea was actually to use same driver but the integration IP
> is quite different on Keystone vs OMAP which lead to have a separate
> glue layer. They look similar but there are differences in terms of phys,
> interrupts, register space. And also non OTG support, runtime PM
> vs clock etc for now.

All of those can be easily taken care of:

a) PHY -> different phy driver
b) non-OTG -> there's a register on dwc3 which we can use
c) runtime vs clock -> we can add optional clock support on dwc3-omap.c

what *really* prevents us from using the same, is register layout and
the fact that on OMAP, nobody wants drivers accessing SYS* registers (a
mistake IMO). That would be a lot more difficult to make generic.

BTW, OMAP isn't OTG either, it doesn't support the OTG specification
even though it can act in both roles. Same applies for keystone, if I'm
reading my docs correctly.

> After all the glue is really a very small code describes the integration
> details thanks to nice abstracted dwc3 core layer. So as discussed
> over irc, keeping the separate glue is preferred but do let us know
> if you think otherwise.

yeah, let's keep it separate. Keystone is, in general, an entire new SoC
architecture anyway.

-- 
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/20131127/2cf1b300/attachment.sig>

[PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[Santosh Shilimkar]

On Wednesday 27 November 2013 04:59 AM, George Cherian wrote:
> On 11/26/2013 1:46 AM, WingMan Kwok wrote:
>> Added device tree support for TI's Keystone USB driver and updated the
>> Documentation with device tree binding information.
>>
>> On Keystone II platforms, we use no-op phy driver.
>>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
>> ---
>>   .../devicetree/bindings/usb/keystone-usb.txt       |   43 ++++++++++++++++++++
>>   arch/arm/boot/dts/keystone.dtsi                    |   27 ++++++++++++
>>   2 files changed, 70 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/usb/keystone-usb.txt
>>
[...]

>> diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
>> index f6d6d9e..1e1049c 100644
>> --- a/arch/arm/boot/dts/keystone.dtsi
>> +++ b/arch/arm/boot/dts/keystone.dtsi
>> @@ -181,5 +181,32 @@
>>               interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
>>               clocks = <&clkspi>;
>>           };
>> +
>> +        usb2_phy: usb2_phy {
>> +            compatible = "usb-nop-xceiv";
>> +        };
>> +
>> +        usb3_phy: usb3_phy {
>> +            compatible = "usb-nop-xceiv";
>> +        };
>> +
>> +        usb: usb at 2680000 {
>> +            compatible = "ti,keystone-dwc3";
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            reg = <0x2680000 0x10000
>> +                   0x2620738 32>;
>> +            clocks = <&clkusb>;
>> +            clock-names = "usb";
>> +            interrupts = <GIC_SPI 393 IRQ_TYPE_EDGE_RISING>;
> 
> You don't have seperate interrrupt for wrapper and core?
> Is it the same interrupt shared between XHCI,DWC3 and wrapper?
>
You don't need actually two seperate interrupts.
DWC3 core actually registers IRQ for XHCI. And in OMAP case, there
is one more IRQ in wrapper. After checking with Felipe, it seems
the OMAP wrapper interrupt was more for debug purpose than any real
use.

On Keystone only one IRQ is used and the handling is managed
through IRQF_SHARED and that is also mainly because the IRQ
ack needs special write to EOI register unlike OMAP.

Regards,
Santosh