[PATCH v3 0/2] Kesytone II USB host and PHY drivers

[PATCH v3 0/2] Kesytone II USB host and PHY drivers

[WingMan Kwok]

Here is the updated version of the series which addresses comments from
earlier version [1]. The lock in the isr is removed as per the discussion.

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.

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

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

WingMan Kwok (2):
  usb: dwc3: Add Keystone specific glue layer
  usb: phy: Add keystone usb phy driver

 drivers/usb/dwc3/Kconfig         |    7 ++
 drivers/usb/dwc3/Makefile        |    1 +
 drivers/usb/dwc3/dwc3-keystone.c |  205 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/phy/Kconfig          |   10 ++
 drivers/usb/phy/Makefile         |    1 +
 drivers/usb/phy/phy-keystone.c   |  142 ++++++++++++++++++++++++++
 6 files changed, 366 insertions(+)
 create mode 100644 drivers/usb/dwc3/dwc3-keystone.c
 create mode 100644 drivers/usb/phy/phy-keystone.c

[1] http://www.spinics.net/lists/linux-usb/msg98861.html
-- 
1.7.9.5

[PATCH v3 1/2] 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>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
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 |  205 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 213 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..33f71330b
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -0,0 +1,205 @@
+/**
+ * dwc3-keystone.c - Keystone Specific Glue layer
+ *
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: 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/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/of_platform.h>
+
+/* USBSS register offsets */
+#define USBSS_REVISION		0x0000
+#define USBSS_SYSCONFIG		0x0010
+#define USBSS_IRQ_EOI		0x0018
+#define USBSS_IRQSTATUS_RAW_0	0x0020
+#define USBSS_IRQSTATUS_0	0x0024
+#define USBSS_IRQENABLE_SET_0	0x0028
+#define USBSS_IRQENABLE_CLR_0	0x002c
+
+/* IRQ register bits */
+#define USBSS_IRQ_EOI_LINE(n)	BIT(n)
+#define USBSS_IRQ_EVENT_ST	BIT(0)
+#define USBSS_IRQ_COREIRQ_EN	BIT(0)
+#define USBSS_IRQ_COREIRQ_CLR	BIT(0)
+
+static u64 kdwc3_dma_mask;
+
+struct dwc3_keystone {
+	struct device			*dev;
+	struct clk			*clk;
+	void __iomem			*usbss;
+};
+
+static inline u32 kdwc3_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void kdwc3_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static void kdwc3_enable_irqs(struct dwc3_keystone *kdwc)
+{
+	u32 val;
+
+	val = kdwc3_readl(kdwc->usbss, USBSS_IRQENABLE_SET_0);
+	val = USBSS_IRQ_COREIRQ_EN;
+	kdwc3_writel(kdwc->usbss, USBSS_IRQENABLE_SET_0, val);
+}
+
+static void kdwc3_disable_irqs(struct dwc3_keystone *kdwc)
+{
+	u32 val;
+
+	val = kdwc3_readl(kdwc->usbss, USBSS_IRQENABLE_SET_0);
+	val &= ~USBSS_IRQ_COREIRQ_EN;
+	kdwc3_writel(kdwc->usbss, USBSS_IRQENABLE_SET_0, val);
+}
+
+static irqreturn_t dwc3_keystone_interrupt(int irq, void *_kdwc)
+{
+	struct dwc3_keystone	*kdwc = _kdwc;
+
+	kdwc3_writel(kdwc->usbss, USBSS_IRQENABLE_CLR_0, USBSS_IRQ_COREIRQ_CLR);
+	kdwc3_writel(kdwc->usbss, USBSS_IRQSTATUS_0, USBSS_IRQ_EVENT_ST);
+	kdwc3_writel(kdwc->usbss, USBSS_IRQENABLE_SET_0, USBSS_IRQ_COREIRQ_EN);
+	kdwc3_writel(kdwc->usbss, USBSS_IRQ_EOI, USBSS_IRQ_EOI_LINE(0));
+
+	return IRQ_HANDLED;
+}
+
+static int kdwc3_probe(struct platform_device *pdev)
+{
+	struct device		*dev = &pdev->dev;
+	struct device_node	*node = pdev->dev.of_node;
+	struct dwc3_keystone	*kdwc;
+	struct resource		*res;
+	int			error, irq;
+
+	kdwc = devm_kzalloc(dev, sizeof(*kdwc), GFP_KERNEL);
+	if (!kdwc)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, kdwc);
+
+	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))
+		return PTR_ERR(kdwc->usbss);
+
+	kdwc3_dma_mask = dma_get_mask(dev);
+	dev->dma_mask = &kdwc3_dma_mask;
+
+	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;
+	}
+
+	error = clk_prepare_enable(kdwc->clk);
+	if (error < 0) {
+		dev_dbg(kdwc->dev, "unable to enable usb clock, err %d\n",
+			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,
+			dev_name(dev), kdwc);
+	if (error) {
+		dev_err(dev, "failed to request IRQ #%d --> %d\n",
+				irq, error);
+		goto err_irq;
+	}
+
+	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:
+	clk_disable_unprepare(kdwc->clk);
+
+	return error;
+}
+
+static int kdwc3_remove_core(struct device *dev, void *c)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	platform_device_unregister(pdev);
+
+	return 0;
+}
+
+static int kdwc3_remove(struct platform_device *pdev)
+{
+	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
+
+	kdwc3_disable_irqs(kdwc);
+	clk_disable_unprepare(kdwc->clk);
+	device_for_each_child(&pdev->dev, NULL, kdwc3_remove_core);
+	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 v3 2/2] usb: phy: Add keystone usb phy driver

[WingMan Kwok]

Add Keystone platform USB PHY driver support. Current main purpose
of this driver is to enable the PHY reference clock gate on the
Keystone SoC. Otherwise it is a nop PHY.

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

diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index 08e2f39..c6792f43 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -40,6 +40,16 @@ config ISP1301_OMAP
 	  This driver can also be built as a module.  If so, the module
 	  will be called isp1301_omap.
 
+config KEYSTONE_USB_PHY
+	tristate "Keystone USB PHY Driver"
+	depends on ARCH_KEYSTONE
+	select USB_PHY
+	select NOP_USB_XCEIV
+	help
+	  Enable this to support Keystone USB phy. This driver provides
+	  interface to interact with USB 2.0 and USB 3.0 PHY that is part
+	  of the Keystone SOC.
+
 config MV_U3D_PHY
 	bool "Marvell USB 3.0 PHY controller Driver"
 	depends on CPU_MMP3
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index 022c1da..311b47b 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_USB_RCAR_PHY)		+= phy-rcar-usb.o
 obj-$(CONFIG_USB_RCAR_GEN2_PHY)		+= phy-rcar-gen2-usb.o
 obj-$(CONFIG_USB_ULPI)			+= phy-ulpi.o
 obj-$(CONFIG_USB_ULPI_VIEWPORT)		+= phy-ulpi-viewport.o
+obj-$(CONFIG_KEYSTONE_USB_PHY)		+= phy-keystone.o
diff --git a/drivers/usb/phy/phy-keystone.c b/drivers/usb/phy/phy-keystone.c
new file mode 100644
index 0000000..e025eb2
--- /dev/null
+++ b/drivers/usb/phy/phy-keystone.c
@@ -0,0 +1,142 @@
+/*
+ * phy-keystone - USB PHY, talking to dwc3 controller in Keystone.
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Author: WingMan Kwok <w-kwok2@ti.com>
+ *
+ * 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/usb/usb_phy_gen_xceiv.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include "phy-generic.h"
+
+/* USB PHY control register offsets */
+#define USB_PHY_CTL_UTMI		0x0000
+#define USB_PHY_CTL_PIPE		0x0004
+#define USB_PHY_CTL_PARAM_1		0x0008
+#define USB_PHY_CTL_PARAM_2		0x000c
+#define USB_PHY_CTL_CLOCK		0x0010
+#define USB_PHY_CTL_PLL			0x0014
+
+#define PHY_REF_SSP_EN			BIT(29)
+
+struct keystone_usbphy {
+	struct usb_phy_gen_xceiv	usb_phy_gen;
+	void __iomem			*phy_ctrl;
+};
+
+static inline u32 keystone_usbphy_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void keystone_usbphy_writel(void __iomem *base,
+					  u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static int keystone_usbphy_init(struct usb_phy *phy)
+{
+	struct keystone_usbphy *k_phy = dev_get_drvdata(phy->dev);
+	u32 val;
+
+	val  = keystone_usbphy_readl(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK);
+	keystone_usbphy_writel(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK,
+				val | PHY_REF_SSP_EN);
+	udelay(20);
+	return 0;
+}
+
+static void keystone_usbphy_shutdown(struct usb_phy *phy)
+{
+	struct keystone_usbphy *k_phy = dev_get_drvdata(phy->dev);
+	u32 val;
+
+	val  = keystone_usbphy_readl(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK);
+	keystone_usbphy_writel(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK,
+				val &= ~PHY_REF_SSP_EN);
+}
+
+static int keystone_usbphy_probe(struct platform_device *pdev)
+{
+	struct device		*dev = &pdev->dev;
+	struct keystone_usbphy	*k_phy;
+	struct resource		*res;
+	int ret;
+
+	k_phy = devm_kzalloc(dev, sizeof(*k_phy), GFP_KERNEL);
+	if (!k_phy)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "missing usb phy resource\n");
+		return -EINVAL;
+	}
+
+	k_phy->phy_ctrl = devm_ioremap_resource(dev, res);
+	if (IS_ERR(k_phy->phy_ctrl))
+		return PTR_ERR(k_phy->phy_ctrl);
+
+	ret = usb_phy_gen_create_phy(dev, &k_phy->usb_phy_gen,
+			USB_PHY_TYPE_USB2, 0, false);
+	if (ret)
+		return ret;
+
+	ret = usb_add_phy_dev(&k_phy->usb_phy_gen.phy);
+	if (ret)
+		return ret;
+	k_phy->usb_phy_gen.phy.init = keystone_usbphy_init;
+	k_phy->usb_phy_gen.phy.shutdown = keystone_usbphy_shutdown;
+
+	platform_set_drvdata(pdev, k_phy);
+
+	return 0;
+}
+
+static int keystone_usbphy_remove(struct platform_device *pdev)
+{
+	struct keystone_usbphy *k_phy = platform_get_drvdata(pdev);
+
+	usb_remove_phy(&k_phy->usb_phy_gen.phy);
+
+	return 0;
+}
+
+static const struct of_device_id keystone_usbphy_ids[] = {
+	{ .compatible = "ti,keystone-usbphy" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, keystone_usbphy_ids);
+
+static struct platform_driver keystone_usbphy_driver = {
+	.probe          = keystone_usbphy_probe,
+	.remove         = keystone_usbphy_remove,
+	.driver         = {
+		.name   = "keystone-usbphy",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_match_ptr(keystone_usbphy_ids),
+	},
+};
+
+module_platform_driver(keystone_usbphy_driver);
+
+MODULE_ALIAS("platform: keystone-usbphy");
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("Keystone USB phy driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

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

[Felipe Balbi]

Hi,

On Mon, Dec 09, 2013 at 05:17:03PM -0500, WingMan Kwok wrote:
> +static void kdwc3_enable_irqs(struct dwc3_keystone *kdwc)
> +{
> +	u32 val;
> +
> +	val = kdwc3_readl(kdwc->usbss, USBSS_IRQENABLE_SET_0);
> +	val = USBSS_IRQ_COREIRQ_EN;

this misses the | in |=. I can fix it up while applying, this time only.

> +static int kdwc3_probe(struct platform_device *pdev)
> +{
> +	struct device		*dev = &pdev->dev;
> +	struct device_node	*node = pdev->dev.of_node;
> +	struct dwc3_keystone	*kdwc;
> +	struct resource		*res;
> +	int			error, irq;
> +
> +	kdwc = devm_kzalloc(dev, sizeof(*kdwc), GFP_KERNEL);
> +	if (!kdwc)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, kdwc);
> +
> +	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))
> +		return PTR_ERR(kdwc->usbss);
> +
> +	kdwc3_dma_mask = dma_get_mask(dev);
> +	dev->dma_mask = &kdwc3_dma_mask;
> +
> +	kdwc->clk = devm_clk_get(kdwc->dev, "usb");
> +	if (IS_ERR_OR_NULL(kdwc->clk)) {

clk_get() will never return NULL. This time, I'll fix it while applying.

> +static int kdwc3_remove(struct platform_device *pdev)
> +{
> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
> +
> +	kdwc3_disable_irqs(kdwc);
> +	clk_disable_unprepare(kdwc->clk);

I hope the clock isn't shared between core and wrapper, otherwise you
could run into some troubles here. Can you confirm ?

-- 
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/20131209/50050cfb/attachment.sig>

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Felipe Balbi]

Hi,

On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
> +	ret = usb_add_phy_dev(&k_phy->usb_phy_gen.phy);
> +	if (ret)
> +		return ret;
> +	k_phy->usb_phy_gen.phy.init = keystone_usbphy_init;
> +	k_phy->usb_phy_gen.phy.shutdown = keystone_usbphy_shutdown;

this *must* be initialized before adding the PHY to the subsystem. So
these two lines must be moved before usb_add_phy_dev().

-- 
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/20131209/ba92a459/attachment.sig>

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Roger Quadros]

On 12/10/2013 03:47 AM, WingMan Kwok wrote:
> Add Keystone platform USB PHY driver support. Current main purpose
> of this driver is to enable the PHY reference clock gate on the
> Keystone SoC. Otherwise it is a nop PHY.
> 
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> ---
>  drivers/usb/phy/Kconfig        |   10 +++
>  drivers/usb/phy/Makefile       |    1 +
>  drivers/usb/phy/phy-keystone.c |  142 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+)
>  create mode 100644 drivers/usb/phy/phy-keystone.c
> 
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index 08e2f39..c6792f43 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -40,6 +40,16 @@ config ISP1301_OMAP
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called isp1301_omap.
>  
> +config KEYSTONE_USB_PHY
> +	tristate "Keystone USB PHY Driver"
> +	depends on ARCH_KEYSTONE
> +	select USB_PHY

NOP_USB_XCEIV selects USB_PHY so not necessary.

> +	select NOP_USB_XCEIV
> +	help
> +	  Enable this to support Keystone USB phy. This driver provides
> +	  interface to interact with USB 2.0 and USB 3.0 PHY that is part
> +	  of the Keystone SOC.
> +
>  config MV_U3D_PHY
>  	bool "Marvell USB 3.0 PHY controller Driver"
>  	depends on CPU_MMP3
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index 022c1da..311b47b 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_USB_RCAR_PHY)		+= phy-rcar-usb.o
>  obj-$(CONFIG_USB_RCAR_GEN2_PHY)		+= phy-rcar-gen2-usb.o
>  obj-$(CONFIG_USB_ULPI)			+= phy-ulpi.o
>  obj-$(CONFIG_USB_ULPI_VIEWPORT)		+= phy-ulpi-viewport.o
> +obj-$(CONFIG_KEYSTONE_USB_PHY)		+= phy-keystone.o

cheers,
-roger

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Felipe Balbi]

Hi again,

On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
> +static int keystone_usbphy_init(struct usb_phy *phy)
> +{
> +	struct keystone_usbphy *k_phy = dev_get_drvdata(phy->dev);
> +	u32 val;
> +
> +	val  = keystone_usbphy_readl(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK);
> +	keystone_usbphy_writel(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK,
> +				val | PHY_REF_SSP_EN);

you need to enable this device's clock to access its registers right ?

> +	udelay(20);

why the magic 20 usecs ? Where does that come from ? Empirically found
or is there a documentation reference ? At least add a comment there.

-- 
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/20131209/7850247a/attachment-0001.sig>

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

[Santosh Shilimkar]

On Monday 09 December 2013 09:51 PM, Balbi, Felipe wrote:
> Hi,
> 
> On Mon, Dec 09, 2013 at 05:17:03PM -0500, WingMan Kwok wrote:
>> +static void kdwc3_enable_irqs(struct dwc3_keystone *kdwc)
>> +{
>> +	u32 val;
>> +
>> +	val = kdwc3_readl(kdwc->usbss, USBSS_IRQENABLE_SET_0);
>> +	val = USBSS_IRQ_COREIRQ_EN;
> 
> this misses the | in |=. I can fix it up while applying, this time only.
> 
>> +static int kdwc3_probe(struct platform_device *pdev)
>> +{
>> +	struct device		*dev = &pdev->dev;
>> +	struct device_node	*node = pdev->dev.of_node;
>> +	struct dwc3_keystone	*kdwc;
>> +	struct resource		*res;
>> +	int			error, irq;
>> +
>> +	kdwc = devm_kzalloc(dev, sizeof(*kdwc), GFP_KERNEL);
>> +	if (!kdwc)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, kdwc);
>> +
>> +	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))
>> +		return PTR_ERR(kdwc->usbss);
>> +
>> +	kdwc3_dma_mask = dma_get_mask(dev);
>> +	dev->dma_mask = &kdwc3_dma_mask;
>> +
>> +	kdwc->clk = devm_clk_get(kdwc->dev, "usb");
>> +	if (IS_ERR_OR_NULL(kdwc->clk)) {
> 
> clk_get() will never return NULL. This time, I'll fix it while applying.
> 
>> +static int kdwc3_remove(struct platform_device *pdev)
>> +{
>> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
>> +
>> +	kdwc3_disable_irqs(kdwc);
>> +	clk_disable_unprepare(kdwc->clk);
> 
> I hope the clock isn't shared between core and wrapper, otherwise you
> could run into some troubles here. Can you confirm ?
> 
Yes. the clock isn't shared. Thanks for taking care of other parts.

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Santosh Shilimkar]

On Monday 09 December 2013 09:54 PM, Balbi, Felipe wrote:
> Hi,
> 
> On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
>> +	ret = usb_add_phy_dev(&k_phy->usb_phy_gen.phy);
>> +	if (ret)
>> +		return ret;
>> +	k_phy->usb_phy_gen.phy.init = keystone_usbphy_init;
>> +	k_phy->usb_phy_gen.phy.shutdown = keystone_usbphy_shutdown;
> 
> this *must* be initialized before adding the PHY to the subsystem. So
> these two lines must be moved before usb_add_phy_dev().
> 
Make sense. Probably its good idea to repost the $subject patch with
above as well as other delay related comment.

Regards,
Santosh

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Santosh Shilimkar]

On Monday 09 December 2013 11:47 PM, Felipe Balbi wrote:
> Hi again,
> 
> On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
>> +static int keystone_usbphy_init(struct usb_phy *phy)
>> +{
>> +	struct keystone_usbphy *k_phy = dev_get_drvdata(phy->dev);
>> +	u32 val;
>> +
>> +	val  = keystone_usbphy_readl(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK);
>> +	keystone_usbphy_writel(k_phy->phy_ctrl, USB_PHY_CTL_CLOCK,
>> +				val | PHY_REF_SSP_EN);
> 
> you need to enable this device's clock to access its registers right ?
> 
Nope.. This clock is always running for CFG block where the phy control
is residing.

>> +	udelay(20);
> 
> why the magic 20 usecs ? Where does that come from ? Empirically found
> or is there a documentation reference ? At least add a comment there.
> 
Above probably isn't needed either but good to check why was this added.
In refreshed patch, this can be either removed or a comment can be
added accordingly.

Thanks for spotting that.

Regards,
Santosh

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Kwok, WingMan]

> -----Original Message-----
> From: Shilimkar, Santosh
> Sent: Tuesday, December 10, 2013 10:15 AM
> To: Balbi, Felipe
> Cc: Kwok, WingMan; linux-usb at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; Greg Kroah-Hartman
> Subject: Re: [PATCH v3 2/2] usb: phy: Add keystone usb phy driver
> 
> On Monday 09 December 2013 09:54 PM, Balbi, Felipe wrote:
> > Hi,
> >
> > On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
> >> +	ret = usb_add_phy_dev(&k_phy->usb_phy_gen.phy);
> >> +	if (ret)
> >> +		return ret;
> >> +	k_phy->usb_phy_gen.phy.init = keystone_usbphy_init;
> >> +	k_phy->usb_phy_gen.phy.shutdown = keystone_usbphy_shutdown;
> >
> > this *must* be initialized before adding the PHY to the subsystem. So
> > these two lines must be moved before usb_add_phy_dev().
> >
> Make sense. Probably its good idea to repost the $subject patch with above
> as well as other delay related comment.

Thanks.  I have updated my patch accordingly.  Please note that the same issue
exists on drivers/usb/phy/phy-am335x.c also.  If you want, I can send a fix for that.

Regards
WingMan

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Kwok, WingMan]

> -----Original Message-----
> From: Quadros, Roger
> Sent: Monday, December 09, 2013 10:09 PM
> To: Kwok, WingMan; linux-usb at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; Shilimkar, Santosh; Balbi, Felipe;
> Greg Kroah-Hartman
> Subject: Re: [PATCH v3 2/2] usb: phy: Add keystone usb phy driver
> 
> On 12/10/2013 03:47 AM, WingMan Kwok wrote:
> > Add Keystone platform USB PHY driver support. Current main purpose of
> > this driver is to enable the PHY reference clock gate on the Keystone
> > SoC. Otherwise it is a nop PHY.
> >
> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Cc: Felipe Balbi <balbi@ti.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> > ---
> >  drivers/usb/phy/Kconfig        |   10 +++
> >  drivers/usb/phy/Makefile       |    1 +
> >  drivers/usb/phy/phy-keystone.c |  142
> > ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 153 insertions(+)
> >  create mode 100644 drivers/usb/phy/phy-keystone.c
> >
> > diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig index
> > 08e2f39..c6792f43 100644
> > --- a/drivers/usb/phy/Kconfig
> > +++ b/drivers/usb/phy/Kconfig
> > @@ -40,6 +40,16 @@ config ISP1301_OMAP
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called isp1301_omap.
> >
> > +config KEYSTONE_USB_PHY
> > +	tristate "Keystone USB PHY Driver"
> > +	depends on ARCH_KEYSTONE
> > +	select USB_PHY
> 
> NOP_USB_XCEIV selects USB_PHY so not necessary.
> 

Yes I have fixed it and updated patch which I'll post.
" config AM335X_PHY_USB" needs to be fixed as well.

Regards
WingMan

[PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Felipe Balbi]

On Thu, Dec 12, 2013 at 11:20:54AM -0600, Kwok, WingMan wrote:
> > -----Original Message-----
> > From: Shilimkar, Santosh
> > Sent: Tuesday, December 10, 2013 10:15 AM
> > To: Balbi, Felipe
> > Cc: Kwok, WingMan; linux-usb at vger.kernel.org; linux-arm-
> > kernel at lists.infradead.org; Greg Kroah-Hartman
> > Subject: Re: [PATCH v3 2/2] usb: phy: Add keystone usb phy driver
> > 
> > On Monday 09 December 2013 09:54 PM, Balbi, Felipe wrote:
> > > Hi,
> > >
> > > On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
> > >> +	ret = usb_add_phy_dev(&k_phy->usb_phy_gen.phy);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +	k_phy->usb_phy_gen.phy.init = keystone_usbphy_init;
> > >> +	k_phy->usb_phy_gen.phy.shutdown = keystone_usbphy_shutdown;
> > >
> > > this *must* be initialized before adding the PHY to the subsystem. So
> > > these two lines must be moved before usb_add_phy_dev().
> > >
> > Make sense. Probably its good idea to repost the $subject patch with above
> > as well as other delay related comment.
> 
> Thanks.  I have updated my patch accordingly.  Please note that the same issue
> exists on drivers/usb/phy/phy-am335x.c also.  If you want, I can send a fix for that.

Will do, thanks for notifying me :-)

-- 
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/20131212/99d347fa/attachment.sig>

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

[Felipe Balbi]

Hi,

On Tue, Dec 10, 2013 at 10:11:36AM -0500, Santosh Shilimkar wrote:
> >> +static int kdwc3_remove(struct platform_device *pdev)
> >> +{
> >> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
> >> +
> >> +	kdwc3_disable_irqs(kdwc);
> >> +	clk_disable_unprepare(kdwc->clk);
> > 
> > I hope the clock isn't shared between core and wrapper, otherwise you
> > could run into some troubles here. Can you confirm ?
> > 
> Yes. the clock isn't shared. Thanks for taking care of other parts.

so clock for core is always running too ?

-- 
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/20131212/ec33f2df/attachment.sig>

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

[Santosh Shilimkar]

On Thursday 12 December 2013 12:48 PM, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Dec 10, 2013 at 10:11:36AM -0500, Santosh Shilimkar wrote:
>>>> +static int kdwc3_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
>>>> +
>>>> +	kdwc3_disable_irqs(kdwc);
>>>> +	clk_disable_unprepare(kdwc->clk);
>>>
>>> I hope the clock isn't shared between core and wrapper, otherwise you
>>> could run into some troubles here. Can you confirm ?
>>>
>> Yes. the clock isn't shared. Thanks for taking care of other parts.
> 
> so clock for core is always running too ?
> 
I take that back. The clock is actually common so we should disable
it after removing the  kdwc3_remove_core() as you suggested.

You won't see issue since the  kdwc3_remove_core() not doing
any register access but moving the clock disable after
the core remove is right thing to do.

Regards,
Santosh

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

[Felipe Balbi]

On Thu, Dec 12, 2013 at 02:36:01PM -0500, Santosh Shilimkar wrote:
> On Thursday 12 December 2013 12:48 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, Dec 10, 2013 at 10:11:36AM -0500, Santosh Shilimkar wrote:
> >>>> +static int kdwc3_remove(struct platform_device *pdev)
> >>>> +{
> >>>> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
> >>>> +
> >>>> +	kdwc3_disable_irqs(kdwc);
> >>>> +	clk_disable_unprepare(kdwc->clk);
> >>>
> >>> I hope the clock isn't shared between core and wrapper, otherwise you
> >>> could run into some troubles here. Can you confirm ?
> >>>
> >> Yes. the clock isn't shared. Thanks for taking care of other parts.
> > 
> > so clock for core is always running too ?
> > 
> I take that back. The clock is actually common so we should disable
> it after removing the  kdwc3_remove_core() as you suggested.
> 
> You won't see issue since the  kdwc3_remove_core() not doing
> any register access but moving the clock disable after
> the core remove is right thing to do.

the problem is not kdwc3_remove_core() accessing registers, but
dwc3_remove() _does_ access registers during remove. If you just
mopdrobe -r dwc3-keystone without removing dwc3.ko first, then
kdwc3_remove_core() will cause dwc3.ko to be removed (because of
platform_driver_unregister()) and, since clocks have already been
disabled, then we'd die :-)

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/20131212/fe9d8c4c/attachment.sig>

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

[Santosh Shilimkar]

On Thursday 12 December 2013 02:41 PM, Felipe Balbi wrote:
> On Thu, Dec 12, 2013 at 02:36:01PM -0500, Santosh Shilimkar wrote:
>> On Thursday 12 December 2013 12:48 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Tue, Dec 10, 2013 at 10:11:36AM -0500, Santosh Shilimkar wrote:
>>>>>> +static int kdwc3_remove(struct platform_device *pdev)
>>>>>> +{
>>>>>> +	struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
>>>>>> +
>>>>>> +	kdwc3_disable_irqs(kdwc);
>>>>>> +	clk_disable_unprepare(kdwc->clk);
>>>>>
>>>>> I hope the clock isn't shared between core and wrapper, otherwise you
>>>>> could run into some troubles here. Can you confirm ?
>>>>>
>>>> Yes. the clock isn't shared. Thanks for taking care of other parts.
>>>
>>> so clock for core is always running too ?
>>>
>> I take that back. The clock is actually common so we should disable
>> it after removing the  kdwc3_remove_core() as you suggested.
>>
>> You won't see issue since the  kdwc3_remove_core() not doing
>> any register access but moving the clock disable after
>> the core remove is right thing to do.
> 
> the problem is not kdwc3_remove_core() accessing registers, but
> dwc3_remove() _does_ access registers during remove. If you just
> mopdrobe -r dwc3-keystone without removing dwc3.ko first, then
> kdwc3_remove_core() will cause dwc3.ko to be removed (because of
> platform_driver_unregister()) and, since clocks have already been
> disabled, then we'd die :-)
> 
Oh yes, you are right. I see Wingman already posted the updated patch.

Regards,
Santosh