[PATCH/RFT v2 00/17] Add DT support for ohci-da8xx

[PATCH/RFT v2 00/17] Add DT support for ohci-da8xx

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

The purpose of this patch series is to add DT support and modernize
the ohci-da8xx glue driver without breaking the non-DT boot,
which is still used in unconverted davinci devices.

The first couple of patches, are the recent and ongoing efforts from
David Lechner to clean up davinci-mach code and the ochi-da8xx driver,
Im including these patches on this series for completeness sake.

Then I re-based an old patch by Manjunath Goudar[4] to be able to build
the driver as a module. We can now add this patch because we removed
references to sysconf registers by using David's patches.

Then I prepare the stage for a DT boot which does not depend on any
mach-arch/* callbacks by using a regulator to handle the vbus and
over current gpios. For this, i extended the fixed regulator driver to
register for an irq and send a regulator notification if an over curent
pin is specified.

The only dependency left for this series is the recently re-posted patch
by David to remove references to mach-code[1], which is approved and
should be picked up soon.

Note 1: Testing was done on a omap138-lcdk board, using DT, and non-DT boot,
and checking that in both cases the hub, usb mass storage and an input
device are correctly enumerated and working. The omap138-lcdk does not
have gpios to control vbus and get over current interrupt notifications.
If anyone has a da830-evm based board and could confirm that ohci is
correctly working, i would appreciate it.
(the OHCI option needs to be enabled in menuconfig)

Note 2: It seems that the davinci-gpio driver is broken for DT based boot
and any gpio > 32. (luckly none of the DT based boards use gpios yet)
The issue is discussed here [2]

Note 3: A git branch based on tag: next-20161021 with the dependencies
is available in my github here [3].

[1] usb: ohci-da8xx: Remove code that references mach
http://marc.info/?l=linux-kernel&m=147632744323084&w=4

[2] thread about invalid gpio in davinci
http://www.gossamer-threads.com/lists/linux/kernel/2550178

[3] Git branch:
https://github.com/axelhaslamx/linux-axel/commits/ohci-da8xx-dt-v2.1

[4] Separate ohci-da8xx driver patch
https://patches.linaro.org/patch/18234/

Changes from v1->v2
* Rebased and added patch to make ohci a separate driver
* Use a regulator instead of handling Gpios (David Lechner)
* Add an overcurrent mode to regulator framework
* Fixed regulator is able to register for and overcurrent irq
* Added patch by Alexandre to remove build warnings
* Moved global variables into private hcd structure.

Alexandre Bailon (1):
  ARM: davinci: da8xx: Fix some redefined symbol warnings

Axel Haslam (10):
  ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable
  ARM: davinci: hawk: add full constraints for ohci plat boot
  regulator: fixed: Add over current event
  USB: da8xx: use flag instead of bitmask for over current change
  USB: ochi-da8xx: Use a regulator for vbus/overcurrent
  USB: da8xx: use ohci priv data instead of globals
  ARM: davinci: register the usb20_phy clock on the DT file
  usb: host: ohci-da8xx: Add devicetree bindings documentation
  USB: ohci-da8xx: Allow probing from DT
  ARM: dts: da850: add usb device node

David Lechner (5):
  ARM: davinci: da8xx: add usb phy clocks
  ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.
  ARM: davinci: da8xx: Add USB PHY platform declaration
  ARM: DTS: da850: Add cfgchip syscon node
  ARM: DTS: da850: Add usb phy node

Manjunath Goudar (1):
  USB: OHCI: make ohci-da8xx a separate driver

 .../bindings/regulator/fixed-regulator.txt         |   4 +
 .../devicetree/bindings/usb/ohci-da8xx.txt         |  39 +++
 arch/arm/boot/dts/da850-lcdk.dts                   |   8 +
 arch/arm/boot/dts/da850.dtsi                       |  17 +
 arch/arm/mach-davinci/board-da830-evm.c            | 152 +++-----
 arch/arm/mach-davinci/board-da850-evm.c            |   4 +
 arch/arm/mach-davinci/board-mityomapl138.c         |   4 +
 arch/arm/mach-davinci/board-omapl138-hawk.c        | 122 ++-----
 arch/arm/mach-davinci/da8xx-dt.c                   |   2 +
 arch/arm/mach-davinci/devices-da8xx.c              |  28 ++
 arch/arm/mach-davinci/include/mach/da8xx.h         |   8 +-
 arch/arm/mach-davinci/usb-da8xx.c                  | 244 ++++++++++++-
 drivers/regulator/fixed.c                          |  64 ++++
 drivers/usb/host/Kconfig                           |   2 +-
 drivers/usb/host/Makefile                          |   1 +
 drivers/usb/host/ohci-da8xx.c                      | 382 +++++++++++----------
 drivers/usb/host/ohci-hcd.c                        |  18 -
 include/linux/platform_data/usb-davinci.h          |  43 ---
 include/linux/regulator/consumer.h                 |   5 +
 include/linux/regulator/fixed.h                    |   3 +
 20 files changed, 703 insertions(+), 447 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ohci-da8xx.txt

-- 
1.9.1

[PATCH/RFT v2 01/17] ARM: davinci: da8xx: add usb phy clocks

[ahaslam]

From: David Lechner <david@lechnology.com>

Up to this point, the USB phy clock configuration was handled manually in
the board files and in the usb drivers. This adds proper clocks so that
the usb drivers can use clk_get and clk_enable and not have to worry about
the details. Also, the related code is removed from the board files and
replaced with the new clock registration functions.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/mach-davinci/board-da830-evm.c     |  22 ++-
 arch/arm/mach-davinci/board-omapl138-hawk.c |  16 +-
 arch/arm/mach-davinci/include/mach/da8xx.h  |   3 +
 arch/arm/mach-davinci/usb-da8xx.c           | 219 +++++++++++++++++++++++++++-
 4 files changed, 239 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 3d8cf8c..605d444 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -115,18 +115,6 @@ static __init void da830_evm_usb_init(void)
 	 */
 	cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
 
-	/* USB2.0 PHY reference clock is 24 MHz */
-	cfgchip2 &= ~CFGCHIP2_REFFREQ;
-	cfgchip2 |=  CFGCHIP2_REFFREQ_24MHZ;
-
-	/*
-	 * Select internal reference clock for USB 2.0 PHY
-	 * and use it as a clock source for USB 1.1 PHY
-	 * (this is the default setting anyway).
-	 */
-	cfgchip2 &= ~CFGCHIP2_USB1PHYCLKMUX;
-	cfgchip2 |=  CFGCHIP2_USB2PHYCLKMUX;
-
 	/*
 	 * We have to override VBUS/ID signals when MUSB is configured into the
 	 * host-only mode -- ID pin will float if no cable is connected, so the
@@ -143,6 +131,16 @@ static __init void da830_evm_usb_init(void)
 	__raw_writel(cfgchip2, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
 
 	/* USB_REFCLKIN is not used. */
+	ret = da8xx_register_usb20_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 2.0 PHY CLK registration failed: %d\n",
+			__func__, ret);
+
+	ret = da8xx_register_usb11_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
+			__func__, ret);
+
 	ret = davinci_cfg_reg(DA830_USB0_DRVVBUS);
 	if (ret)
 		pr_warn("%s: USB 2.0 PinMux setup failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index ee62486..d4930b6 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -243,7 +243,6 @@ static irqreturn_t omapl138_hawk_usb_ocic_irq(int irq, void *dev_id)
 static __init void omapl138_hawk_usb_init(void)
 {
 	int ret;
-	u32 cfgchip2;
 
 	ret = davinci_cfg_reg_list(da850_hawk_usb11_pins);
 	if (ret) {
@@ -251,12 +250,15 @@ static __init void omapl138_hawk_usb_init(void)
 		return;
 	}
 
-	/* Setup the Ref. clock frequency for the HAWK at 24 MHz. */
-
-	cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
-	cfgchip2 &= ~CFGCHIP2_REFFREQ;
-	cfgchip2 |=  CFGCHIP2_REFFREQ_24MHZ;
-	__raw_writel(cfgchip2, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+	/* USB_REFCLKIN is not used. */
+	ret = da8xx_register_usb20_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 2.0 PHY CLK registration failed: %d\n",
+			__func__, ret);
+	ret = da8xx_register_usb11_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
+			__func__, ret);
 
 	ret = gpio_request_one(DA850_USB1_VBUS_PIN,
 			GPIOF_DIR_OUT, "USB1 VBUS");
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index f9f9713..c367530 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -88,6 +88,9 @@
 int da8xx_register_i2c(int instance, struct davinci_i2c_platform_data *pdata);
 int da8xx_register_spi_bus(int instance, unsigned num_chipselect);
 int da8xx_register_watchdog(void);
+int da8xx_register_usb_refclkin(int rate);
+int da8xx_register_usb20_phy_clk(bool use_usb_refclkin);
+int da8xx_register_usb11_phy_clk(bool use_usb_refclkin);
 int da8xx_register_usb20(unsigned mA, unsigned potpgt);
 int da8xx_register_usb11(struct da8xx_ohci_root_hub *pdata);
 int da8xx_register_emac(void);
diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index f141f51..c524d9e 100644
--- a/arch/arm/mach-davinci/usb-da8xx.c
+++ b/arch/arm/mach-davinci/usb-da8xx.c
@@ -1,20 +1,235 @@
 /*
  * DA8xx USB
  */
-#include <linux/dma-mapping.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/platform_data/usb-davinci.h>
 #include <linux/platform_device.h>
+#include <linux/mfd/da8xx-cfgchip.h>
 #include <linux/usb/musb.h>
 
+#include <mach/clock.h>
 #include <mach/common.h>
 #include <mach/cputype.h>
 #include <mach/da8xx.h>
-#include <mach/irqs.h>
+
+#include "clock.h"
 
 #define DA8XX_USB0_BASE		0x01e00000
 #define DA8XX_USB1_BASE		0x01e25000
 
+static struct clk usb_refclkin = {
+	.name		= "usb_refclkin",
+	.set_rate	= davinci_simple_set_rate,
+};
+
+static struct clk_lookup usb_refclkin_lookup =
+	CLK(NULL, "usb_refclkin", &usb_refclkin);
+
+/**
+ * da8xx_register_usb_refclkin - register USB_REFCLKIN clock
+ *
+ * @rate: The clock rate in Hz
+ *
+ * This clock is only needed if the board provides an external USB_REFCLKIN
+ * signal, in which case it will be used as the parent of usb20_phy_clk and/or
+ * usb11_phy_clk.
+ */
+int __init da8xx_register_usb_refclkin(int rate)
+{
+	int ret;
+
+	usb_refclkin.rate = rate;
+	ret = clk_register(&usb_refclkin);
+	if (ret)
+		return ret;
+
+	clkdev_add(&usb_refclkin_lookup);
+
+	return 0;
+}
+
+static void usb20_phy_clk_enable(struct clk *clk)
+{
+	u32 val;
+	u32 timeout = 500000; /* 500 msec */
+
+	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	/*
+	 * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
+	 * host may use the PLL clock without USB 2.0 OTG being used.
+	 */
+	val &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
+	val |= CFGCHIP2_PHY_PLLON;
+
+	writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	while (--timeout) {
+		val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+		if (val & CFGCHIP2_PHYCLKGD)
+			return;
+		udelay(1);
+	}
+
+	pr_err("Timeout waiting for USB 2.0 PHY clock good.\n");
+}
+
+static void usb20_phy_clk_disable(struct clk *clk)
+{
+	u32 val;
+
+	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+	val |= CFGCHIP2_PHYPWRDN;
+	writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+}
+
+static int usb20_phy_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	u32 val;
+
+	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	/* Set the mux depending on the parent clock. */
+	if (parent == &usb_refclkin) {
+		val &= ~CFGCHIP2_USB2PHYCLKMUX;
+	} else if (strcmp(parent->name, "pll0_aux_clk") == 0) {
+		val |= CFGCHIP2_USB2PHYCLKMUX;
+	} else {
+		pr_err("Bad parent on USB 2.0 PHY clock.\n");
+		return -EINVAL;
+	}
+
+	/* reference frequency also comes from parent clock */
+	val &= ~CFGCHIP2_REFFREQ_MASK;
+	switch (clk_get_rate(parent)) {
+	case 12000000:
+		val |= CFGCHIP2_REFFREQ_12MHZ;
+		break;
+	case 13000000:
+		val |= CFGCHIP2_REFFREQ_13MHZ;
+		break;
+	case 19200000:
+		val |= CFGCHIP2_REFFREQ_19_2MHZ;
+		break;
+	case 20000000:
+		val |= CFGCHIP2_REFFREQ_20MHZ;
+		break;
+	case 24000000:
+		val |= CFGCHIP2_REFFREQ_24MHZ;
+		break;
+	case 26000000:
+		val |= CFGCHIP2_REFFREQ_26MHZ;
+		break;
+	case 38400000:
+		val |= CFGCHIP2_REFFREQ_38_4MHZ;
+		break;
+	case 40000000:
+		val |= CFGCHIP2_REFFREQ_40MHZ;
+		break;
+	case 48000000:
+		val |= CFGCHIP2_REFFREQ_48MHZ;
+		break;
+	default:
+		pr_err("Bad parent clock rate on USB 2.0 PHY clock.\n");
+		return -EINVAL;
+	}
+
+	writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	return 0;
+}
+
+static struct clk usb20_phy_clk = {
+	.name		= "usb20_phy",
+	.clk_enable	= usb20_phy_clk_enable,
+	.clk_disable	= usb20_phy_clk_disable,
+	.set_parent	= usb20_phy_clk_set_parent,
+};
+
+static struct clk_lookup usb20_phy_clk_lookup =
+	CLK(NULL, "usb20_phy", &usb20_phy_clk);
+
+/**
+ * da8xx_register_usb20_phy_clk - register USB0PHYCLKMUX clock
+ *
+ * @use_usb_refclkin: Selects the parent clock - either "usb_refclkin" if true
+ *	or "pll0_aux" if false.
+ */
+int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin)
+{
+	struct clk *parent;
+	int ret = 0;
+
+	parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : "pll0_aux");
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	usb20_phy_clk.parent = parent;
+	ret = clk_register(&usb20_phy_clk);
+	if (!ret)
+		clkdev_add(&usb20_phy_clk_lookup);
+
+	clk_put(parent);
+
+	return ret;
+}
+
+static int usb11_phy_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	u32 val;
+
+	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	/* Set the USB 1.1 PHY clock mux based on the parent clock. */
+	if (parent == &usb20_phy_clk) {
+		val &= ~CFGCHIP2_USB1PHYCLKMUX;
+	} else if (parent == &usb_refclkin) {
+		val |= CFGCHIP2_USB1PHYCLKMUX;
+	} else {
+		pr_err("Bad parent on USB 1.1 PHY clock.\n");
+		return -EINVAL;
+	}
+
+	writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	return 0;
+}
+
+static struct clk usb11_phy_clk = {
+	.name		= "usb11_phy",
+	.set_parent	= usb11_phy_clk_set_parent,
+};
+
+static struct clk_lookup usb11_phy_clk_lookup =
+	CLK(NULL, "usb11_phy", &usb11_phy_clk);
+
+/**
+ * da8xx_register_usb11_phy_clk - register USB1PHYCLKMUX clock
+ *
+ * @use_usb_refclkin: Selects the parent clock - either "usb_refclkin" if true
+ *	or "usb20_phy" if false.
+ */
+int __init da8xx_register_usb11_phy_clk(bool use_usb_refclkin)
+{
+	struct clk *parent;
+	int ret = 0;
+
+	parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : "usb20_phy");
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	usb11_phy_clk.parent = parent;
+	ret = clk_register(&usb11_phy_clk);
+	if (!ret)
+		clkdev_add(&usb11_phy_clk_lookup);
+
+	clk_put(parent);
+
+	return ret;
+}
+
 #if IS_ENABLED(CONFIG_USB_MUSB_HDRC)
 
 static struct musb_hdrc_config musb_config = {
-- 
1.9.1

[PATCH/RFT v2 02/17] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>

The CFGCHIP registers are used by a number of devices, so using a syscon
device to share them. The first consumer of this will by the phy-da8xx-usb
driver.

Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
[Axel: minor fix: change id to -1]
Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 arch/arm/mach-davinci/board-da830-evm.c     |  4 ++++
 arch/arm/mach-davinci/board-da850-evm.c     |  4 ++++
 arch/arm/mach-davinci/board-mityomapl138.c  |  4 ++++
 arch/arm/mach-davinci/board-omapl138-hawk.c |  4 ++++
 arch/arm/mach-davinci/devices-da8xx.c       | 28 ++++++++++++++++++++++++++++
 arch/arm/mach-davinci/include/mach/da8xx.h  |  2 ++
 6 files changed, 46 insertions(+)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 605d444..3051cb6 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -586,6 +586,10 @@ static __init void da830_evm_init(void)
 	struct davinci_soc_info *soc_info = &davinci_soc_info;
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	ret = da830_register_gpio();
 	if (ret)
 		pr_warn("%s: GPIO init failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 8e4539f..ec5cb10 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1345,6 +1345,10 @@ static __init void da850_evm_init(void)
 {
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	ret = da850_register_gpio();
 	if (ret)
 		pr_warn("%s: GPIO init failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-mityomapl138.c b/arch/arm/mach-davinci/board-mityomapl138.c
index bc4e63f..1a6d430 100644
--- a/arch/arm/mach-davinci/board-mityomapl138.c
+++ b/arch/arm/mach-davinci/board-mityomapl138.c
@@ -514,6 +514,10 @@ static void __init mityomapl138_init(void)
 {
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	/* for now, no special EDMA channels are reserved */
 	ret = da850_register_edma(NULL);
 	if (ret)
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index d4930b6..8691a25 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -294,6 +294,10 @@ static __init void omapl138_hawk_init(void)
 {
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	ret = da850_register_gpio();
 	if (ret)
 		pr_warn("%s: GPIO init failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index add3771..31a99db 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -11,6 +11,7 @@
  * (at your option) any later version.
  */
 #include <linux/init.h>
+#include <linux/platform_data/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/dma-contiguous.h>
 #include <linux/serial_8250.h>
@@ -1089,3 +1090,30 @@ int __init da850_register_sata(unsigned long refclkpn)
 	return platform_device_register(&da850_sata_device);
 }
 #endif
+
+static struct syscon_platform_data da8xx_cfgchip_platform_data = {
+	.label	= "cfgchip",
+};
+
+static struct resource da8xx_cfgchip_resources[] = {
+	{
+		.start	= DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP0_REG,
+		.end	= DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP4_REG + 3,
+		.flags	= IORESOURCE_MEM,
+	},
+};
+
+static struct platform_device da8xx_cfgchip_device = {
+	.name	= "syscon",
+	.id	= -1,
+	.dev	= {
+		.platform_data	= &da8xx_cfgchip_platform_data,
+	},
+	.num_resources	= ARRAY_SIZE(da8xx_cfgchip_resources),
+	.resource	= da8xx_cfgchip_resources,
+};
+
+int __init da8xx_register_cfgchip(void)
+{
+	return platform_device_register(&da8xx_cfgchip_device);
+}
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index c367530..c32444b 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -61,6 +61,7 @@
 #define DA8XX_CFGCHIP1_REG	0x180
 #define DA8XX_CFGCHIP2_REG	0x184
 #define DA8XX_CFGCHIP3_REG	0x188
+#define DA8XX_CFGCHIP4_REG	0x18c
 
 #define DA8XX_SYSCFG1_BASE	(IO_PHYS + 0x22C000)
 #define DA8XX_SYSCFG1_VIRT(x)	(da8xx_syscfg1_base + (x))
@@ -116,6 +117,7 @@
 int da8xx_register_rproc(void);
 int da850_register_gpio(void);
 int da830_register_gpio(void);
+int da8xx_register_cfgchip(void);
 
 extern struct platform_device da8xx_serial_device[];
 extern struct emac_platform_data da8xx_emac_pdata;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 02/17] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[Sekhar Nori]

On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
> From: David Lechner <david@lechnology.com>
> 
> The CFGCHIP registers are used by a number of devices, so using a syscon
> device to share them. The first consumer of this will by the phy-da8xx-usb
> driver.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> [Axel: minor fix: change id to -1]

Can you please clarify this change? There could be other syscon devices
on the chip for other common registers. Why use the singular device-id?

> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

Thanks,
Sekhar

Re: [PATCH/RFT v2 02/17] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[Axel Haslam]

Hi Sekar,

On Tue, Oct 25, 2016 at 10:10 AM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> From: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>
>> The CFGCHIP registers are used by a number of devices, so using a syscon
>> device to share them. The first consumer of this will by the phy-da8xx-usb
>> driver.
>>
>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>> [Axel: minor fix: change id to -1]
>
> Can you please clarify this change? There could be other syscon devices
> on the chip for other common registers. Why use the singular device-id?
>

in the case of non DT boot, the phy driver is looking for "syscon" :

d_phy->regmap = syscon_regmap_lookup_by_pdevname("syscon");

if we register the syscon driver with id = 0, the actual name of the syscon
device will be "syscon.0" and the phy driver will fail to probe, because
the strncmp match in the syscon driver (syscon_match_pdevname)
will fail.

should i change the phy driver instead?


Regards,
Axel.



>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> Thanks,
> Sekhar
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 02/17] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[Sekhar Nori]

On Tuesday 25 October 2016 03:07 PM, Axel Haslam wrote:
> Hi Sekar,
> 
> On Tue, Oct 25, 2016 at 10:10 AM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
>> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>> From: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>>
>>> The CFGCHIP registers are used by a number of devices, so using a syscon
>>> device to share them. The first consumer of this will by the phy-da8xx-usb
>>> driver.
>>>
>>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>> [Axel: minor fix: change id to -1]
>>
>> Can you please clarify this change? There could be other syscon devices
>> on the chip for other common registers. Why use the singular device-id?
>>
> 
> in the case of non DT boot, the phy driver is looking for "syscon" :
> 
> d_phy->regmap = syscon_regmap_lookup_by_pdevname("syscon");
> 
> if we register the syscon driver with id = 0, the actual name of the syscon
> device will be "syscon.0" and the phy driver will fail to probe, because
> the strncmp match in the syscon driver (syscon_match_pdevname)
> will fail.
> 
> should i change the phy driver instead?

Yes, please. Forcing only one syscon region for the whole chip will be
too restrictive, I am pretty sure.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 02/17] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[David Lechner]

Hi Sekhar,

On 10/25/2016 05:17 AM, Sekhar Nori wrote:
> On Tuesday 25 October 2016 03:07 PM, Axel Haslam wrote:
>> Hi Sekar,
>>
>> On Tue, Oct 25, 2016 at 10:10 AM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
>>> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>>> From: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>>>
>>>> The CFGCHIP registers are used by a number of devices, so using a syscon
>>>> device to share them. The first consumer of this will by the phy-da8xx-usb
>>>> driver.
>>>>
>>>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>>> [Axel: minor fix: change id to -1]
>>>
>>> Can you please clarify this change? There could be other syscon devices
>>> on the chip for other common registers. Why use the singular device-id?
>>>
>>
>> in the case of non DT boot, the phy driver is looking for "syscon" :
>>
>> d_phy->regmap = syscon_regmap_lookup_by_pdevname("syscon");
>>
>> if we register the syscon driver with id = 0, the actual name of the syscon
>> device will be "syscon.0" and the phy driver will fail to probe, because
>> the strncmp match in the syscon driver (syscon_match_pdevname)
>> will fail.
>>
>> should i change the phy driver instead?
>
> Yes, please. Forcing only one syscon region for the whole chip will be
> too restrictive, I am pretty sure.
>
> Thanks,
> Sekhar
>

In the previous review, you requested that this be changed to -1 [1].

If we change it back to 0, it will also require reverting a patch to the 
phy driver that has already been merged[2].

[1]: http://www.gossamer-threads.com/lists/linux/kernel/2435807?page=last
[2]: http://www.gossamer-threads.com/lists/linux/kernel/2518804


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 02/17] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[Sekhar Nori]

On Tuesday 25 October 2016 09:23 PM, David Lechner wrote:
> Hi Sekhar,
> 
> On 10/25/2016 05:17 AM, Sekhar Nori wrote:
>> On Tuesday 25 October 2016 03:07 PM, Axel Haslam wrote:
>>> Hi Sekar,
>>>
>>> On Tue, Oct 25, 2016 at 10:10 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>>> On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
>>>>> From: David Lechner <david@lechnology.com>
>>>>>
>>>>> The CFGCHIP registers are used by a number of devices, so using a
>>>>> syscon
>>>>> device to share them. The first consumer of this will by the
>>>>> phy-da8xx-usb
>>>>> driver.
>>>>>
>>>>> Signed-off-by: David Lechner <david@lechnology.com>
>>>>> [Axel: minor fix: change id to -1]
>>>>
>>>> Can you please clarify this change? There could be other syscon devices
>>>> on the chip for other common registers. Why use the singular device-id?
>>>>
>>>
>>> in the case of non DT boot, the phy driver is looking for "syscon" :
>>>
>>> d_phy->regmap = syscon_regmap_lookup_by_pdevname("syscon");
>>>
>>> if we register the syscon driver with id = 0, the actual name of the
>>> syscon
>>> device will be "syscon.0" and the phy driver will fail to probe, because
>>> the strncmp match in the syscon driver (syscon_match_pdevname)
>>> will fail.
>>>
>>> should i change the phy driver instead?
>>
>> Yes, please. Forcing only one syscon region for the whole chip will be
>> too restrictive, I am pretty sure.
>>
>> Thanks,
>> Sekhar
>>
> 
> In the previous review, you requested that this be changed to -1 [1].
> 
> If we change it back to 0, it will also require reverting a patch to the
> phy driver that has already been merged[2].

Sigh. Sorry about going around in circles on this one. Lets go with what
you have. If and when there is a need for another syscon node, the
driver and platform code can be updated. At least we will know why the
change is being done at that time.

Thanks,
Sekhar

[PATCH/RFT v2 03/17] ARM: davinci: da8xx: Add USB PHY platform declaration

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>

There is now a proper phy driver for the DA8xx SoC USB PHY. This adds the
platform device declarations needed to use it.

Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
---
 arch/arm/mach-davinci/board-da830-evm.c     | 28 +++++-----------------------
 arch/arm/mach-davinci/board-omapl138-hawk.c |  5 +++++
 arch/arm/mach-davinci/include/mach/da8xx.h  |  1 +
 arch/arm/mach-davinci/usb-da8xx.c           | 11 +++++++++++
 4 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 3051cb6..c62766e 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -26,7 +26,6 @@
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/mtd-davinci-aemif.h>
 #include <linux/platform_data/spi-davinci.h>
-#include <linux/platform_data/usb-davinci.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -106,30 +105,8 @@ static irqreturn_t da830_evm_usb_ocic_irq(int irq, void *dev_id)
 
 static __init void da830_evm_usb_init(void)
 {
-	u32 cfgchip2;
 	int ret;
 
-	/*
-	 * Set up USB clock/mode in the CFGCHIP2 register.
-	 * FYI:  CFGCHIP2 is 0x0000ef00 initially.
-	 */
-	cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
-
-	/*
-	 * We have to override VBUS/ID signals when MUSB is configured into the
-	 * host-only mode -- ID pin will float if no cable is connected, so the
-	 * controller won't be able to drive VBUS thinking that it's a B-device.
-	 * Otherwise, we want to use the OTG mode and enable VBUS comparators.
-	 */
-	cfgchip2 &= ~CFGCHIP2_OTGMODE;
-#ifdef	CONFIG_USB_MUSB_HOST
-	cfgchip2 |=  CFGCHIP2_FORCE_HOST;
-#else
-	cfgchip2 |=  CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN;
-#endif
-
-	__raw_writel(cfgchip2, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
-
 	/* USB_REFCLKIN is not used. */
 	ret = da8xx_register_usb20_phy_clk(false);
 	if (ret)
@@ -141,6 +118,11 @@ static __init void da830_evm_usb_init(void)
 		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
 			__func__, ret);
 
+	ret = da8xx_register_usb_phy();
+	if (ret)
+		pr_warn("%s: USB PHY registration failed: %d\n",
+			__func__, ret);
+
 	ret = davinci_cfg_reg(DA830_USB0_DRVVBUS);
 	if (ret)
 		pr_warn("%s: USB 2.0 PinMux setup failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index 8691a25..c5cb8d9 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -260,6 +260,11 @@ static __init void omapl138_hawk_usb_init(void)
 		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
 			__func__, ret);
 
+	ret = da8xx_register_usb_phy();
+	if (ret)
+		pr_warn("%s: USB PHY registration failed: %d\n",
+			__func__, ret);
+
 	ret = gpio_request_one(DA850_USB1_VBUS_PIN,
 			GPIOF_DIR_OUT, "USB1 VBUS");
 	if (ret < 0) {
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index c32444b..38d932e 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -92,6 +92,7 @@
 int da8xx_register_usb_refclkin(int rate);
 int da8xx_register_usb20_phy_clk(bool use_usb_refclkin);
 int da8xx_register_usb11_phy_clk(bool use_usb_refclkin);
+int da8xx_register_usb_phy(void);
 int da8xx_register_usb20(unsigned mA, unsigned potpgt);
 int da8xx_register_usb11(struct da8xx_ohci_root_hub *pdata);
 int da8xx_register_emac(void);
diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index c524d9e..9e41a7f 100644
--- a/arch/arm/mach-davinci/usb-da8xx.c
+++ b/arch/arm/mach-davinci/usb-da8xx.c
@@ -7,6 +7,7 @@
 #include <linux/platform_data/usb-davinci.h>
 #include <linux/platform_device.h>
 #include <linux/mfd/da8xx-cfgchip.h>
+#include <linux/phy/phy.h>
 #include <linux/usb/musb.h>
 
 #include <mach/clock.h>
@@ -230,6 +231,16 @@ int __init da8xx_register_usb11_phy_clk(bool use_usb_refclkin)
 	return ret;
 }
 
+static struct platform_device da8xx_usb_phy = {
+	.name		= "da8xx-usb-phy",
+	.id		= 0,
+};
+
+int __init da8xx_register_usb_phy(void)
+{
+	return platform_device_register(&da8xx_usb_phy);
+}
+
 #if IS_ENABLED(CONFIG_USB_MUSB_HDRC)
 
 static struct musb_hdrc_config musb_config = {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 03/17] ARM: davinci: da8xx: Add USB PHY platform declaration

[Sekhar Nori]

On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
> +static struct platform_device da8xx_usb_phy = {
> +	.name		= "da8xx-usb-phy",
> +	.id		= 0,

There is a single phy control in the system for both 1.1 and 2.0 PHYs.
so this can be a singular device (id -1).

Thanks,
Sekhar

Re: [PATCH/RFT v2 03/17] ARM: davinci: da8xx: Add USB PHY platform declaration

[Axel Haslam]

On Tue, Oct 25, 2016 at 11:18 AM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> +static struct platform_device da8xx_usb_phy = {
>> +     .name           = "da8xx-usb-phy",
>> +     .id             = 0,
>
> There is a single phy control in the system for both 1.1 and 2.0 PHYs.
> so this can be a singular device (id -1).
>

Ok.

> Thanks,
> Sekhar
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 06/17] ARM: davinci: da8xx: Fix some redefined symbol warnings

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Some macro for DA8xx CFGCHIP are defined in usb-davinci.h,
but da8xx-cfgchip.h intend to replace them.
The usb-da8xx.c is using both headers, causing redefined symbol warnings.
Remove the old macros.

Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 include/linux/platform_data/usb-davinci.h | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/include/linux/platform_data/usb-davinci.h b/include/linux/platform_data/usb-davinci.h
index e0bc4ab..0926e99 100644
--- a/include/linux/platform_data/usb-davinci.h
+++ b/include/linux/platform_data/usb-davinci.h
@@ -11,29 +11,6 @@
 #ifndef __ASM_ARCH_USB_H
 #define __ASM_ARCH_USB_H
 
-/* DA8xx CFGCHIP2 (USB 2.0 PHY Control) register bits */
-#define CFGCHIP2_PHYCLKGD	(1 << 17)
-#define CFGCHIP2_VBUSSENSE	(1 << 16)
-#define CFGCHIP2_RESET		(1 << 15)
-#define CFGCHIP2_OTGMODE	(3 << 13)
-#define CFGCHIP2_NO_OVERRIDE	(0 << 13)
-#define CFGCHIP2_FORCE_HOST	(1 << 13)
-#define CFGCHIP2_FORCE_DEVICE 	(2 << 13)
-#define CFGCHIP2_FORCE_HOST_VBUS_LOW (3 << 13)
-#define CFGCHIP2_USB1PHYCLKMUX	(1 << 12)
-#define CFGCHIP2_USB2PHYCLKMUX	(1 << 11)
-#define CFGCHIP2_PHYPWRDN	(1 << 10)
-#define CFGCHIP2_OTGPWRDN	(1 << 9)
-#define CFGCHIP2_DATPOL 	(1 << 8)
-#define CFGCHIP2_USB1SUSPENDM	(1 << 7)
-#define CFGCHIP2_PHY_PLLON	(1 << 6)	/* override PLL suspend */
-#define CFGCHIP2_SESENDEN	(1 << 5)	/* Vsess_end comparator */
-#define CFGCHIP2_VBDTCTEN	(1 << 4)	/* Vbus comparator */
-#define CFGCHIP2_REFFREQ	(0xf << 0)
-#define CFGCHIP2_REFFREQ_12MHZ	(1 << 0)
-#define CFGCHIP2_REFFREQ_24MHZ	(2 << 0)
-#define CFGCHIP2_REFFREQ_48MHZ	(3 << 0)

Re: [PATCH/RFT v2 06/17] ARM: davinci: da8xx: Fix some redefined symbol warnings

[Sekhar Nori]

On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
> From: Alexandre Bailon <abailon@baylibre.com>
> 
> Some macro for DA8xx CFGCHIP are defined in usb-davinci.h,
> but da8xx-cfgchip.h intend to replace them.
> The usb-da8xx.c is using both headers, causing redefined symbol warnings.
> Remove the old macros.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

This is a v4.9-rc bug fix. Can you please post it as a separate patch
for Greg to pick up?

You can add:

Acked-by: Sekhar Nori <nsekhar@ti.com>

Thanks,
Sekhar

Re: [PATCH/RFT v2 06/17] ARM: davinci: da8xx: Fix some redefined symbol warnings

[Alexandre Bailon]

On 10/25/2016 12:03 PM, Sekhar Nori wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> From: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> Some macro for DA8xx CFGCHIP are defined in usb-davinci.h,
>> but da8xx-cfgchip.h intend to replace them.
>> The usb-da8xx.c is using both headers, causing redefined symbol warnings.
>> Remove the old macros.
>>
>> Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 
> This is a v4.9-rc bug fix. Can you please post it as a separate patch
> for Greg to pick up?
> 
Done
> You can add:
> 
> Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
Actually, I didn't add it because I had to make few changes to
submit as a separate patch. I hope I did it in right way.
> 
> Thanks,
> Sekhar
> 
Thanks,
Alexandre
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Some regulator supplies have an over-current pin that is
activated when the hw detects an over current condition.
When this happens, the hardware enters a current limited
mode.

Extend the fixed regulator driver with the ability
to handle irq's from the over-current pin and report
an over current event to the consumers via a regulator
notifier. Also, add device tree bindings to allow to
pass a gpio for over current monitoring.

Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 .../bindings/regulator/fixed-regulator.txt         |  4 ++
 drivers/regulator/fixed.c                          | 64 ++++++++++++++++++++++
 include/linux/regulator/consumer.h                 |  5 ++
 include/linux/regulator/fixed.h                    |  3 +
 4 files changed, 76 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
index 4fae41d..d20bf67 100644
--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -11,6 +11,8 @@ If this property is missing, the default assumed is Active low.
 - gpio-open-drain: GPIO is open drain type.
   If this property is missing then default assumption is false.
 -vin-supply: Input supply name.
+- oc-gpio: Input gpio that signals an over current condition
+- oc-active-high: The polarity of the over current pin is high
 
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
@@ -26,9 +28,11 @@ Example:
 		regulator-min-microvolt = <1800000>;
 		regulator-max-microvolt = <1800000>;
 		gpio = <&gpio1 16 0>;
+		oc-gpio = <&gpio1 18 0>;
 		startup-delay-us = <70000>;
 		enable-active-high;
 		regulator-boot-on;
 		gpio-open-drain;
+		oc-active-high;
 		vin-supply = <&parent_reg>;
 	};
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 988a747..e7964bb 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -30,10 +30,14 @@
 #include <linux/of_gpio.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/machine.h>
+#include <linux/interrupt.h>
 
 struct fixed_voltage_data {
 	struct regulator_desc desc;
 	struct regulator_dev *dev;
+	int oc_gpio;
+	unsigned has_oc_gpio:1;
+	unsigned oc_high:1;
 };
 
 
@@ -82,6 +86,14 @@ struct fixed_voltage_data {
 	if ((config->gpio < 0) && (config->gpio != -ENOENT))
 		return ERR_PTR(config->gpio);
 
+	config->oc_gpio = of_get_named_gpio(np, "oc-gpio", 0);
+	if (config->oc_gpio >= 0)
+		config->has_oc_gpio = true;
+	else if (config->oc_gpio != -ENOENT)
+		return ERR_PTR(config->oc_gpio);
+
+	config->oc_high = of_property_read_bool(np, "oc-active-high");
+
 	of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
 
 	config->enable_high = of_property_read_bool(np, "enable-active-high");
@@ -94,7 +106,34 @@ struct fixed_voltage_data {
 	return config;
 }
 
+static irqreturn_t reg_fixed_overcurrent_irq(int irq, void *data)
+{
+	struct fixed_voltage_data *drvdata = data;
+
+	regulator_notifier_call_chain(drvdata->dev,
+			REGULATOR_EVENT_OVER_CURRENT, NULL);
+
+	return IRQ_HANDLED;
+}
+
+static unsigned int reg_fixed_get_mode(struct regulator_dev *rdev)
+{
+	struct fixed_voltage_data *drvdata = rdev_get_drvdata(rdev);
+	unsigned int ret = REGULATOR_MODE_NORMAL;
+	int oc_value;
+
+	if (!drvdata->has_oc_gpio)
+		return ret;
+
+	oc_value = gpio_get_value(drvdata->oc_gpio);
+	if ((oc_value && drvdata->oc_high) || (!oc_value && !drvdata->oc_high))
+		ret = REGULATOR_MODE_OVERCURRENT;
+
+	return ret;
+}
+
 static struct regulator_ops fixed_voltage_ops = {
+	.get_mode = reg_fixed_get_mode,
 };
 
 static int reg_fixed_voltage_probe(struct platform_device *pdev)
@@ -175,6 +214,31 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	cfg.driver_data = drvdata;
 	cfg.of_node = pdev->dev.of_node;
 
+	if (config->has_oc_gpio && gpio_is_valid(config->oc_gpio)) {
+		ret = devm_gpio_request_one(&pdev->dev,
+					config->oc_gpio,
+					GPIOF_DIR_IN, "oc_gpio");
+		if (ret) {
+			pr_err("Failed to request gpio: %d\n", ret);
+			return ret;
+		}
+
+		ret = devm_request_threaded_irq(&pdev->dev,
+				gpio_to_irq(config->oc_gpio), NULL,
+				reg_fixed_overcurrent_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+				IRQF_ONESHOT,
+				"over_current", drvdata);
+		if (ret) {
+			pr_err("Failed to request irq: %d\n", ret);
+			return ret;
+		}
+
+		drvdata->oc_gpio = config->oc_gpio;
+		drvdata->oc_high = config->oc_high;
+		drvdata->has_oc_gpio = config->has_oc_gpio;
+	}
+
 	drvdata->dev = devm_regulator_register(&pdev->dev, &drvdata->desc,
 					       &cfg);
 	if (IS_ERR(drvdata->dev)) {
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 6921082..9269217 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -74,6 +74,10 @@
  *             the most noisy and may not be able to handle fast load
  *             switching.
  *
+ * OVERCURRENT Regulator has detected an overcurrent condition, and
+ *             may be limiting the supply output.
+ *
+ *
  * NOTE: Most regulators will only support a subset of these modes. Some
  * will only just support NORMAL.
  *
@@ -84,6 +88,7 @@
 #define REGULATOR_MODE_NORMAL			0x2
 #define REGULATOR_MODE_IDLE			0x4
 #define REGULATOR_MODE_STANDBY			0x8
+#define REGULATOR_MODE_OVERCURRENT		0x10
 
 /*
  * Regulator notifier events.
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index 48918be..79357be 100644
--- a/include/linux/regulator/fixed.h
+++ b/include/linux/regulator/fixed.h
@@ -50,10 +50,13 @@ struct fixed_voltage_config {
 	const char *input_supply;
 	int microvolts;
 	int gpio;
+	int oc_gpio;
 	unsigned startup_delay;
 	unsigned gpio_is_open_drain:1;
 	unsigned enable_high:1;
 	unsigned enabled_at_boot:1;
+	unsigned has_oc_gpio:1;
+	unsigned oc_high:1;
 	struct regulator_init_data *init_data;
 };
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Mon, Oct 24, 2016 at 06:46:26PM +0200, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 
> Some regulator supplies have an over-current pin that is
> activated when the hw detects an over current condition.
> When this happens, the hardware enters a current limited
> mode.

Please don't mix random enhancements like this into bigger system
specific RFC serieses, send them separately so they're easier to spot
and there's no confusion with dependencies and then reference them from
the system specific series when you post that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Axel Haslam]

Hi Mark,

On Mon, Oct 24, 2016 at 7:43 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 24, 2016 at 06:46:26PM +0200, ahaslam@baylibre.com wrote:
>> From: Axel Haslam <ahaslam@baylibre.com>
>>
>> Some regulator supplies have an over-current pin that is
>> activated when the hw detects an over current condition.
>> When this happens, the hardware enters a current limited
>> mode.
>
> Please don't mix random enhancements like this into bigger system
> specific RFC serieses, send them separately so they're easier to spot
> and there's no confusion with dependencies and then reference them from
> the system specific series when you post that.

Ok, sorry i had mixed feelings on how to post all of it.
 there are several dependencies on the series and i  kept
all together to give the context. Do you  want me to repost the regulator
changes seperatly? I can do that, but if you  don't agree with regulator
handling overcurrent, i will have to move the over current
gpio into the driver, and there is no point on re-posting that seperatly.

Regards
Axel

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Mon, Oct 24, 2016 at 06:46:26PM +0200, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:

> +		if (ret) {
> +			pr_err("Failed to request irq: %d\n", ret);

dev_err()

> +++ b/include/linux/regulator/consumer.h
> @@ -74,6 +74,10 @@
>   *             the most noisy and may not be able to handle fast load
>   *             switching.
>   *
> + * OVERCURRENT Regulator has detected an overcurrent condition, and
> + *             may be limiting the supply output.
> + *
> + *
>   * NOTE: Most regulators will only support a subset of these modes. Some
>   * will only just support NORMAL.
>   *
> @@ -84,6 +88,7 @@
>  #define REGULATOR_MODE_NORMAL			0x2
>  #define REGULATOR_MODE_IDLE			0x4
>  #define REGULATOR_MODE_STANDBY			0x8
> +#define REGULATOR_MODE_OVERCURRENT		0x10

This is adding a new core feature with a new mode and should have been
split out of the driver specific change with a spearate changelog.  Why
does it make sense to report this as a mode, we don't report other error
conditions as modes but instead use REGULATOR_STATUS_ with the
get_status() operation?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Axel Haslam]

On Mon, Oct 24, 2016 at 7:53 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Oct 24, 2016 at 06:46:26PM +0200, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>
>> +             if (ret) {
>> +                     pr_err("Failed to request irq: %d\n", ret);
>
> dev_err()
>
>> +++ b/include/linux/regulator/consumer.h
>> @@ -74,6 +74,10 @@
>>   *             the most noisy and may not be able to handle fast load
>>   *             switching.
>>   *
>> + * OVERCURRENT Regulator has detected an overcurrent condition, and
>> + *             may be limiting the supply output.
>> + *
>> + *
>>   * NOTE: Most regulators will only support a subset of these modes. Some
>>   * will only just support NORMAL.
>>   *
>> @@ -84,6 +88,7 @@
>>  #define REGULATOR_MODE_NORMAL                        0x2
>>  #define REGULATOR_MODE_IDLE                  0x4
>>  #define REGULATOR_MODE_STANDBY                       0x8
>> +#define REGULATOR_MODE_OVERCURRENT           0x10
>
> This is adding a new core feature with a new mode and should have been
> split out of the driver specific change with a spearate changelog.  Why

Ok, will do.

> does it make sense to report this as a mode, we don't report other error
> conditions as modes but instead use REGULATOR_STATUS_ with the
> get_status() operation?

I used mode, because when the regulator toggles the overcurrent
line, it means that it has entered a current limited mode, at least the
regulator im looking at. ill change to STATUS

Regards
Axel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On Mon, Oct 24, 2016 at 08:11:40PM +0200, Axel Haslam wrote:
> On Mon, Oct 24, 2016 at 7:53 PM, Mark Brown <broonie@kernel.org> wrote:

> > does it make sense to report this as a mode, we don't report other error
> > conditions as modes but instead use REGULATOR_STATUS_ with the
> > get_status() operation?

> I used mode, because when the regulator toggles the overcurrent
> line, it means that it has entered a current limited mode, at least the
> regulator im looking at. ill change to STATUS

That's not what regulator modes are - please look at the documentation
for the defines here.  They're about the quality of regulation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Axel Haslam]

Hi Mark,

On Mon, Oct 24, 2016 at 8:19 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 24, 2016 at 08:11:40PM +0200, Axel Haslam wrote:
>> On Mon, Oct 24, 2016 at 7:53 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > does it make sense to report this as a mode, we don't report other error
>> > conditions as modes but instead use REGULATOR_STATUS_ with the
>> > get_status() operation?
>
>> I used mode, because when the regulator toggles the overcurrent
>> line, it means that it has entered a current limited mode, at least the
>> regulator im looking at. ill change to STATUS
>
> That's not what regulator modes are - please look at the documentation
> for the defines here.  They're about the quality of regulation.

To be able to use regulator to handle the overcurrent pin, i need to be able
to somehow retrieve the over current pin state from the regulator driver.

As i was trying your suggestion, i remembered why i thought i should use
mode instead of status: Status seems to be for internal regulator driver use,
there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
in driver.h and not in consumer.h as  REGULATOR_MODE_*

it seems that status is only used to print sysfs info.

Would you be ok if i allow consumers to get the status via a new
"regulator_get_status" call?

Regards
Axel.

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

On Tue, Oct 25, 2016 at 02:55:48PM +0200, Axel Haslam wrote:

> To be able to use regulator to handle the overcurrent pin, i need to be able
> to somehow retrieve the over current pin state from the regulator driver.

What makes you say that, none of the existing users need this?  

> As i was trying your suggestion, i remembered why i thought i should use
> mode instead of status: Status seems to be for internal regulator driver use,
> there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
> in driver.h and not in consumer.h as  REGULATOR_MODE_*

> Would you be ok if i allow consumers to get the status via a new
> "regulator_get_status" call?

What would they do with this information that they can't do with the
existing error notification?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Axel Haslam]

On Tue, Oct 25, 2016 at 4:33 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 25, 2016 at 02:55:48PM +0200, Axel Haslam wrote:
>
>> To be able to use regulator to handle the overcurrent pin, i need to be able
>> to somehow retrieve the over current pin state from the regulator driver.
>
> What makes you say that, none of the existing users need this?
>
>> As i was trying your suggestion, i remembered why i thought i should use
>> mode instead of status: Status seems to be for internal regulator driver use,
>> there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
>> in driver.h and not in consumer.h as  REGULATOR_MODE_*
>
>> Would you be ok if i allow consumers to get the status via a new
>> "regulator_get_status" call?
>
> What would they do with this information that they can't do with the
> existing error notification?

the usb core relies in two flags that need too be set properly, one is the
over-current indicator RH_PS_POCI , and the other is the over current
indicator "change" (RH_PS_OCIC).

The idea was to use the notification to set the over current indicator
"change" flag,
which will happen for both rising and falling edges. And to use
get_status or get_mode
to set the over-current indicator flag which should reflect the actual
pin status.


-Axel.

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Axel Haslam]

On Tue, Oct 25, 2016 at 4:57 PM, Axel Haslam <ahaslam@baylibre.com> wrote:
> On Tue, Oct 25, 2016 at 4:33 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Oct 25, 2016 at 02:55:48PM +0200, Axel Haslam wrote:
>>
>>> To be able to use regulator to handle the overcurrent pin, i need to be able
>>> to somehow retrieve the over current pin state from the regulator driver.
>>
>> What makes you say that, none of the existing users need this?
>>
>>> As i was trying your suggestion, i remembered why i thought i should use
>>> mode instead of status: Status seems to be for internal regulator driver use,
>>> there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
>>> in driver.h and not in consumer.h as  REGULATOR_MODE_*
>>
>>> Would you be ok if i allow consumers to get the status via a new
>>> "regulator_get_status" call?
>>
>> What would they do with this information that they can't do with the
>> existing error notification?
>
> the usb core relies in two flags that need too be set properly, one is the
> over-current indicator RH_PS_POCI , and the other is the over current
> indicator "change" (RH_PS_OCIC).
>
> The idea was to use the notification to set the over current indicator
> "change" flag,
> which will happen for both rising and falling edges. And to use
> get_status or get_mode
> to set the over-current indicator flag which should reflect the actual
> pin status.
>

BTW, for the notification, i should have used a new event flag
something like: OVER_CURRENT_CHANGED and not just OVER_CURRENT


Regards
Axel

>
> -Axel.

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Rob Herring]

On Mon, Oct 24, 2016 at 06:46:26PM +0200, ahaslam@baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
> 
> Some regulator supplies have an over-current pin that is
> activated when the hw detects an over current condition.
> When this happens, the hardware enters a current limited
> mode.
> 
> Extend the fixed regulator driver with the ability
> to handle irq's from the over-current pin and report
> an over current event to the consumers via a regulator
> notifier. Also, add device tree bindings to allow to
> pass a gpio for over current monitoring.
> 
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
>  .../bindings/regulator/fixed-regulator.txt         |  4 ++
>  drivers/regulator/fixed.c                          | 64 ++++++++++++++++++++++
>  include/linux/regulator/consumer.h                 |  5 ++
>  include/linux/regulator/fixed.h                    |  3 +
>  4 files changed, 76 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> index 4fae41d..d20bf67 100644
> --- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> @@ -11,6 +11,8 @@ If this property is missing, the default assumed is Active low.
>  - gpio-open-drain: GPIO is open drain type.
>    If this property is missing then default assumption is false.
>  -vin-supply: Input supply name.
> +- oc-gpio: Input gpio that signals an over current condition

"-gpios" is the preferred form. So "oc-gpios".

> +- oc-active-high: The polarity of the over current pin is high

This should be specified in the gpio flags cell.

Rob

[PATCH/RFT v2 10/17] USB: da8xx: use flag instead of bitmask for over current change

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

The da8xx ohci has only one port connected. In hub_control we
check that the port is 1, or else we bail out early. There is no point
in passing as argument the port number, if we know, and check that it
will always be 1.

Simplify functions and callbacks, by removing the port parameter, and
converting the over current indicatior change form a mask to a flag.

Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 arch/arm/mach-davinci/board-da830-evm.c     |  8 ++++----
 arch/arm/mach-davinci/board-omapl138-hawk.c |  8 ++++----
 drivers/usb/host/ohci-da8xx.c               | 29 ++++++++++++++---------------
 include/linux/platform_data/usb-davinci.h   |  9 ++++-----
 4 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index b33fc6b..d12fcf5 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -50,18 +50,18 @@
 
 static da8xx_ocic_handler_t da830_evm_usb_ocic_handler;
 
-static int da830_evm_usb_set_power(unsigned port, int on)
+static int da830_evm_usb_set_power(int on)
 {
 	gpio_set_value(ON_BD_USB_DRV, on);
 	return 0;
 }
 
-static int da830_evm_usb_get_power(unsigned port)
+static int da830_evm_usb_get_power(void)
 {
 	return gpio_get_value(ON_BD_USB_DRV);
 }
 
-static int da830_evm_usb_get_oci(unsigned port)
+static int da830_evm_usb_get_oci(void)
 {
 	return !gpio_get_value(ON_BD_USB_OVC);
 }
@@ -100,7 +100,7 @@ static int da830_evm_usb_ocic_notify(da8xx_ocic_handler_t handler)
 
 static irqreturn_t da830_evm_usb_ocic_irq(int irq, void *dev_id)
 {
-	da830_evm_usb_ocic_handler(&da830_evm_usb11_pdata, 1);
+	da830_evm_usb_ocic_handler(&da830_evm_usb11_pdata);
 	return IRQ_HANDLED;
 }
 
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index c3ab7ea..1d31f45 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -189,18 +189,18 @@ static __init void omapl138_hawk_mmc_init(void)
 	-1
 };
 
-static int hawk_usb_set_power(unsigned port, int on)
+static int hawk_usb_set_power(int on)
 {
 	gpio_set_value(DA850_USB1_VBUS_PIN, on);
 	return 0;
 }
 
-static int hawk_usb_get_power(unsigned port)
+static int hawk_usb_get_power(void)
 {
 	return gpio_get_value(DA850_USB1_VBUS_PIN);
 }
 
-static int hawk_usb_get_oci(unsigned port)
+static int hawk_usb_get_oci(void)
 {
 	return !gpio_get_value(DA850_USB1_OC_PIN);
 }
@@ -237,7 +237,7 @@ static int hawk_usb_ocic_notify(da8xx_ocic_handler_t handler)
 
 static irqreturn_t omapl138_hawk_usb_ocic_irq(int irq, void *dev_id)
 {
-	hawk_usb_ocic_handler(&omapl138_hawk_usb11_pdata, 1);
+	hawk_usb_ocic_handler(&omapl138_hawk_usb11_pdata);
 	return IRQ_HANDLED;
 }
 
diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 3656d7c..e98066d 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -25,8 +25,8 @@
 static struct clk *usb11_clk;
 static struct phy *usb11_phy;
 
-/* Over-current indicator change bitmask */
-static volatile u16 ocic_mask;
+/* Over-current indicator change flag */
+static int ocic_flag;
 
 static int ohci_da8xx_enable(void)
 {
@@ -64,14 +64,13 @@ static void ohci_da8xx_disable(void)
 /*
  * Handle the port over-current indicator change.
  */
-static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub,
-				    unsigned port)
+static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub)
 {
-	ocic_mask |= 1 << port;
+	ocic_flag = 1;
 
 	/* Once over-current is detected, the port needs to be powered down */
-	if (hub->get_oci(port) > 0)
-		hub->set_power(port, 0);
+	if (hub->get_oci() > 0)
+		hub->set_power(0);
 }
 
 static int ohci_da8xx_init(struct usb_hcd *hcd)
@@ -147,8 +146,8 @@ static int ohci_da8xx_hub_status_data(struct usb_hcd *hcd, char *buf)
 {
 	int length		= ohci_hub_status_data(hcd, buf);
 
-	/* See if we have OCIC bit set on port 1 */
-	if (ocic_mask & (1 << 1)) {
+	/* See if we have OCIC flag set */
+	if (ocic_flag) {
 		dev_dbg(hcd->self.controller, "over-current indicator change "
 			"on port 1\n");
 
@@ -181,15 +180,15 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		temp = roothub_portstatus(hcd_to_ohci(hcd), wIndex - 1);
 
 		/* The port power status (PPS) bit defaults to 1 */
-		if (hub->get_power && hub->get_power(wIndex) == 0)
+		if (hub->get_power && hub->get_power() == 0)
 			temp &= ~RH_PS_PPS;
 
 		/* The port over-current indicator (POCI) bit is always 0 */
-		if (hub->get_oci && hub->get_oci(wIndex) > 0)
+		if (hub->get_oci && hub->get_oci() > 0)
 			temp |=  RH_PS_POCI;
 
 		/* The over-current indicator change (OCIC) bit is 0 too */
-		if (ocic_mask & (1 << wIndex))
+		if (ocic_flag)
 			temp |=  RH_PS_OCIC;
 
 		put_unaligned(cpu_to_le32(temp), (__le32 *)buf);
@@ -213,16 +212,16 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			if (!hub->set_power)
 				return -EPIPE;
 
-			return hub->set_power(wIndex, temp) ? -EPIPE : 0;
+			return hub->set_power(temp) ? -EPIPE : 0;
 		case USB_PORT_FEAT_C_OVER_CURRENT:
 			dev_dbg(dev, "%sPortFeature(%u): %s\n",
 				temp ? "Set" : "Clear", wIndex,
 				"C_OVER_CURRENT");
 
 			if (temp)
-				ocic_mask |= 1 << wIndex;
+				ocic_flag = 1;
 			else
-				ocic_mask &= ~(1 << wIndex);
+				ocic_flag = 0;
 			return 0;
 		}
 	}
diff --git a/include/linux/platform_data/usb-davinci.h b/include/linux/platform_data/usb-davinci.h
index 0926e99..3217fbe 100644
--- a/include/linux/platform_data/usb-davinci.h
+++ b/include/linux/platform_data/usb-davinci.h
@@ -13,17 +13,16 @@
 
 struct	da8xx_ohci_root_hub;
 
-typedef void (*da8xx_ocic_handler_t)(struct da8xx_ohci_root_hub *hub,
-				     unsigned port);
+typedef void (*da8xx_ocic_handler_t)(struct da8xx_ohci_root_hub *hub);
 
 /* Passed as the platform data to the OHCI driver */
 struct	da8xx_ohci_root_hub {
 	/* Switch the port power on/off */
-	int	(*set_power)(unsigned port, int on);
+	int	(*set_power)(int on);
 	/* Read the port power status */
-	int	(*get_power)(unsigned port);
+	int	(*get_power)(void);
 	/* Read the port over-current indicator */
-	int	(*get_oci)(unsigned port);
+	int	(*get_oci)(void);
 	/* Over-current indicator change notification (pass NULL to disable) */
 	int	(*ocic_notify)(da8xx_ocic_handler_t handler);
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Separate the Davinci OHCI host controller driver from ohci-hcd
host code so that it can be built as a separate driver module.
This work is part of enabling multi-platform kernels on ARM;
it would be nice to have in 3.11.

Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/usb/host/Kconfig      |   2 +-
 drivers/usb/host/Makefile     |   1 +
 drivers/usb/host/ohci-da8xx.c | 185 +++++++++++++++++-------------------------
 drivers/usb/host/ohci-hcd.c   |  18 ----
 4 files changed, 76 insertions(+), 130 deletions(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 83b6cec..642c6fe8 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -479,7 +479,7 @@ config USB_OHCI_HCD_OMAP3
 	  OMAP3 and later chips.
 
 config USB_OHCI_HCD_DAVINCI
-	bool "OHCI support for TI DaVinci DA8xx"
+	tristate "OHCI support for TI DaVinci DA8xx"
 	depends on ARCH_DAVINCI_DA8XX
 	depends on USB_OHCI_HCD=y
 	select PHY_DA8XX_USB
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 6ef785b..2644537 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_USB_OHCI_HCD_AT91)	+= ohci-at91.o
 obj-$(CONFIG_USB_OHCI_HCD_S3C2410)	+= ohci-s3c2410.o
 obj-$(CONFIG_USB_OHCI_HCD_LPC32XX)	+= ohci-nxp.o
 obj-$(CONFIG_USB_OHCI_HCD_PXA27X)	+= ohci-pxa27x.o
+obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
 
 obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
 obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index e98066d..5585d9e 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -11,16 +11,31 @@
  * kind, whether express or implied.
  */
 
+#include <linux/clk.h>
+#include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/clk.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_data/usb-davinci.h>
+#include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+#include <asm/unaligned.h>
 
-#ifndef CONFIG_ARCH_DAVINCI_DA8XX
-#error "This file is DA8xx bus glue.  Define CONFIG_ARCH_DAVINCI_DA8XX."
-#endif
+#include "ohci.h"
+
+#define DRIVER_DESC "OHCI DA8XX driver"
+
+static const char hcd_name[] = "ohci-da8xx";
+
+static struct hc_driver __read_mostly ohci_da8xx_hc_driver;
+
+static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
+			u16 wValue, u16 wIndex, char *buf, u16 wLength);
+static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
 
 static struct clk *usb11_clk;
 static struct phy *usb11_phy;
@@ -73,7 +88,7 @@ static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub)
 		hub->set_power(0);
 }
 
-static int ohci_da8xx_init(struct usb_hcd *hcd)
+static int ohci_da8xx_reset(struct usb_hcd *hcd)
 {
 	struct device *dev		= hcd->self.controller;
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
@@ -93,7 +108,7 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
 	 */
 	ohci->num_ports = 1;
 
-	result = ohci_init(ohci);
+	result = ohci_setup(hcd);
 	if (result < 0) {
 		ohci_da8xx_disable();
 		return result;
@@ -121,30 +136,12 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
 	return result;
 }
 
-static void ohci_da8xx_stop(struct usb_hcd *hcd)
-{
-	ohci_stop(hcd);
-	ohci_da8xx_disable();
-}
-
-static int ohci_da8xx_start(struct usb_hcd *hcd)
-{
-	struct ohci_hcd	*ohci		= hcd_to_ohci(hcd);
-	int result;
-
-	result = ohci_run(ohci);
-	if (result < 0)
-		ohci_da8xx_stop(hcd);
-
-	return result;
-}
-
 /*
  * Update the status data from the hub with the over-current indicator change.
  */
 static int ohci_da8xx_hub_status_data(struct usb_hcd *hcd, char *buf)
 {
-	int length		= ohci_hub_status_data(hcd, buf);
+	int length		= orig_ohci_hub_status_data(hcd, buf);
 
 	/* See if we have OCIC flag set */
 	if (ocic_flag) {
@@ -226,66 +223,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		}
 	}
 
-	return ohci_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
+	return orig_ohci_hub_control(hcd, typeReq, wValue,
+			wIndex, buf, wLength);
 }
 
-static const struct hc_driver ohci_da8xx_hc_driver = {
-	.description		= hcd_name,
-	.product_desc		= "DA8xx OHCI",
-	.hcd_priv_size		= sizeof(struct ohci_hcd),
-
-	/*
-	 * generic hardware linkage
-	 */
-	.irq			= ohci_irq,
-	.flags			= HCD_USB11 | HCD_MEMORY,
-
-	/*
-	 * basic lifecycle operations
-	 */
-	.reset			= ohci_da8xx_init,
-	.start			= ohci_da8xx_start,
-	.stop			= ohci_da8xx_stop,
-	.shutdown		= ohci_shutdown,
-
-	/*
-	 * managing i/o requests and associated device resources
-	 */
-	.urb_enqueue		= ohci_urb_enqueue,
-	.urb_dequeue		= ohci_urb_dequeue,
-	.endpoint_disable	= ohci_endpoint_disable,
-
-	/*
-	 * scheduling support
-	 */
-	.get_frame_number	= ohci_get_frame,
-
-	/*
-	 * root hub support
-	 */
-	.hub_status_data	= ohci_da8xx_hub_status_data,
-	.hub_control		= ohci_da8xx_hub_control,
-
-#ifdef	CONFIG_PM
-	.bus_suspend		= ohci_bus_suspend,
-	.bus_resume		= ohci_bus_resume,
-#endif
-	.start_port_reset	= ohci_start_port_reset,
-};
-
 /*-------------------------------------------------------------------------*/
 
-
-/**
- * usb_hcd_da8xx_probe - initialize DA8xx-based HCDs
- * Context: !in_interrupt()
- *
- * Allocates basic resources for this USB host controller, and
- * then invokes the start() method for the HCD associated with it
- * through the hotplug entry's driver_data.
- */
-static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
-			       struct platform_device *pdev)
+static int ohci_da8xx_probe(struct platform_device *pdev)
 {
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
 	struct usb_hcd	*hcd;
@@ -295,6 +239,11 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
 	if (hub == NULL)
 		return -ENODEV;
 
+	hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
+				dev_name(&pdev->dev));
+	if (!hcd)
+		return -ENOMEM;
+
 	usb11_clk = devm_clk_get(&pdev->dev, "usb11");
 	if (IS_ERR(usb11_clk)) {
 		if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
@@ -309,9 +258,6 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
 		return PTR_ERR(usb11_phy);
 	}
 
-	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
-	if (!hcd)
-		return -ENOMEM;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
@@ -323,13 +269,12 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
 	hcd->rsrc_start = mem->start;
 	hcd->rsrc_len = resource_size(mem);
 
-	ohci_hcd_init(hcd_to_ohci(hcd));
-
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		error = -ENODEV;
 		goto err;
 	}
+
 	error = usb_add_hcd(hcd, irq, 0);
 	if (error)
 		goto err;
@@ -348,35 +293,14 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
 	return error;
 }
 
-/**
- * usb_hcd_da8xx_remove - shutdown processing for DA8xx-based HCDs
- * @dev: USB Host Controller being removed
- * Context: !in_interrupt()
- *
- * Reverses the effect of usb_hcd_da8xx_probe(), first invoking
- * the HCD's stop() method.  It is always called from a thread
- * context, normally "rmmod", "apmd", or something similar.
- */
-static inline void
-usb_hcd_da8xx_remove(struct usb_hcd *hcd, struct platform_device *pdev)
+static int ohci_da8xx_remove(struct platform_device *pdev)
 {
+	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
 
 	hub->ocic_notify(NULL);
 	usb_remove_hcd(hcd);
 	usb_put_hcd(hcd);
-}
-
-static int ohci_hcd_da8xx_drv_probe(struct platform_device *dev)
-{
-	return usb_hcd_da8xx_probe(&ohci_da8xx_hc_driver, dev);
-}
-
-static int ohci_hcd_da8xx_drv_remove(struct platform_device *dev)
-{
-	struct usb_hcd	*hcd = platform_get_drvdata(dev);
-
-	usb_hcd_da8xx_remove(hcd, dev);
 
 	return 0;
 }
@@ -426,12 +350,16 @@ static int ohci_da8xx_resume(struct platform_device *dev)
 }
 #endif
 
+static const struct ohci_driver_overrides da8xx_overrides __initconst = {
+	.reset		= ohci_da8xx_reset
+};
+
 /*
  * Driver definition to register with platform structure.
  */
 static struct platform_driver ohci_hcd_da8xx_driver = {
-	.probe		= ohci_hcd_da8xx_drv_probe,
-	.remove		= ohci_hcd_da8xx_drv_remove,
+	.probe		= ohci_da8xx_probe,
+	.remove		= ohci_da8xx_remove,
 	.shutdown 	= usb_hcd_platform_shutdown,
 #ifdef	CONFIG_PM
 	.suspend	= ohci_da8xx_suspend,
@@ -442,4 +370,39 @@ static int ohci_da8xx_resume(struct platform_device *dev)
 	},
 };
 
+static int __init ohci_da8xx_init(void)
+{
+
+	if (usb_disabled())
+		return -ENODEV;
+
+	pr_info("%s: " DRIVER_DESC "\n", hcd_name);
+	ohci_init_driver(&ohci_da8xx_hc_driver, &da8xx_overrides);
+
+	/*
+	 * The Davinci da8xx HW has some unusual quirks, which require
+	 * da8xx-specific workarounds. We override certain hc_driver
+	 * functions here to achieve that. We explicitly do not enhance
+	 * ohci_driver_overrides to allow this more easily, since this
+	 * is an unusual case, and we don't want to encourage others to
+	 * override these functions by making it too easy.
+	 */
+
+	orig_ohci_hub_control = ohci_da8xx_hc_driver.hub_control;
+	orig_ohci_hub_status_data = ohci_da8xx_hc_driver.hub_status_data;
+
+	ohci_da8xx_hc_driver.hub_status_data     = ohci_da8xx_hub_status_data;
+	ohci_da8xx_hc_driver.hub_control         = ohci_da8xx_hub_control;
+
+	return platform_driver_register(&ohci_hcd_da8xx_driver);
+}
+module_init(ohci_da8xx_init);
+
+static void __exit ohci_da8xx_cleanup(void)
+{
+	platform_driver_unregister(&ohci_hcd_da8xx_driver);
+}
+module_exit(ohci_da8xx_cleanup);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:ohci");
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 1700908..8de174a 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -1219,11 +1219,6 @@ void ohci_init_driver(struct hc_driver *drv,
 #define SA1111_DRIVER		ohci_hcd_sa1111_driver
 #endif
 
-#ifdef CONFIG_USB_OHCI_HCD_DAVINCI
-#include "ohci-da8xx.c"
-#define DAVINCI_PLATFORM_DRIVER	ohci_hcd_da8xx_driver
-#endif
-
 #ifdef CONFIG_USB_OHCI_HCD_PPC_OF
 #include "ohci-ppc-of.c"
 #define OF_PLATFORM_DRIVER	ohci_hcd_ppc_of_driver
@@ -1303,19 +1298,9 @@ static int __init ohci_hcd_mod_init(void)
 		goto error_tmio;
 #endif
 
-#ifdef DAVINCI_PLATFORM_DRIVER
-	retval = platform_driver_register(&DAVINCI_PLATFORM_DRIVER);
-	if (retval < 0)
-		goto error_davinci;
-#endif

Re: [PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver

[David Lechner]

On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Separate the Davinci OHCI host controller driver from ohci-hcd
> host code so that it can be built as a separate driver module.
> This work is part of enabling multi-platform kernels on ARM;
> it would be nice to have in 3.11.

No need for comment about kernel 3.11.

>
> Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/usb/host/Kconfig      |   2 +-
>  drivers/usb/host/Makefile     |   1 +
>  drivers/usb/host/ohci-da8xx.c | 185 +++++++++++++++++-------------------------
>  drivers/usb/host/ohci-hcd.c   |  18 ----
>  4 files changed, 76 insertions(+), 130 deletions(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 83b6cec..642c6fe8 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -479,7 +479,7 @@ config USB_OHCI_HCD_OMAP3
>  	  OMAP3 and later chips.
>
>  config USB_OHCI_HCD_DAVINCI
> -	bool "OHCI support for TI DaVinci DA8xx"
> +	tristate "OHCI support for TI DaVinci DA8xx"
>  	depends on ARCH_DAVINCI_DA8XX
>  	depends on USB_OHCI_HCD=y

Need to drop the "=y" here, otherwise you still can't compile this as a 
module.

>  	select PHY_DA8XX_USB
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index 6ef785b..2644537 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_USB_OHCI_HCD_AT91)	+= ohci-at91.o
>  obj-$(CONFIG_USB_OHCI_HCD_S3C2410)	+= ohci-s3c2410.o
>  obj-$(CONFIG_USB_OHCI_HCD_LPC32XX)	+= ohci-nxp.o
>  obj-$(CONFIG_USB_OHCI_HCD_PXA27X)	+= ohci-pxa27x.o
> +obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
>
>  obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
>  obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
> index e98066d..5585d9e 100644
> --- a/drivers/usb/host/ohci-da8xx.c
> +++ b/drivers/usb/host/ohci-da8xx.c
> @@ -11,16 +11,31 @@
>   * kind, whether express or implied.
>   */
>
> +#include <linux/clk.h>
> +#include <linux/io.h>
>  #include <linux/interrupt.h>
>  #include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/clk.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_data/usb-davinci.h>
> +#include <linux/platform_device.h>

linux/platform_device.h is listed twice

> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +#include <asm/unaligned.h>
>
> -#ifndef CONFIG_ARCH_DAVINCI_DA8XX
> -#error "This file is DA8xx bus glue.  Define CONFIG_ARCH_DAVINCI_DA8XX."
> -#endif
> +#include "ohci.h"
> +
> +#define DRIVER_DESC "OHCI DA8XX driver"
> +
> +static const char hcd_name[] = "ohci-da8xx";

why static const char instead of #define? This is only used one time in 
a pr_info, so it seems kind of pointless anyway.

> +
> +static struct hc_driver __read_mostly ohci_da8xx_hc_driver;
> +
> +static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
> +			u16 wValue, u16 wIndex, char *buf, u16 wLength);
> +static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>
>  static struct clk *usb11_clk;
>  static struct phy *usb11_phy;
> @@ -73,7 +88,7 @@ static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub)
>  		hub->set_power(0);
>  }
>
> -static int ohci_da8xx_init(struct usb_hcd *hcd)
> +static int ohci_da8xx_reset(struct usb_hcd *hcd)
>  {
>  	struct device *dev		= hcd->self.controller;
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
> @@ -93,7 +108,7 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
>  	 */
>  	ohci->num_ports = 1;
>
> -	result = ohci_init(ohci);
> +	result = ohci_setup(hcd);
>  	if (result < 0) {
>  		ohci_da8xx_disable();
>  		return result;
> @@ -121,30 +136,12 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
>  	return result;
>  }
>
> -static void ohci_da8xx_stop(struct usb_hcd *hcd)
> -{
> -	ohci_stop(hcd);
> -	ohci_da8xx_disable();
> -}
> -
> -static int ohci_da8xx_start(struct usb_hcd *hcd)
> -{
> -	struct ohci_hcd	*ohci		= hcd_to_ohci(hcd);
> -	int result;
> -
> -	result = ohci_run(ohci);
> -	if (result < 0)
> -		ohci_da8xx_stop(hcd);
> -
> -	return result;
> -}
> -
>  /*
>   * Update the status data from the hub with the over-current indicator change.
>   */
>  static int ohci_da8xx_hub_status_data(struct usb_hcd *hcd, char *buf)
>  {
> -	int length		= ohci_hub_status_data(hcd, buf);
> +	int length		= orig_ohci_hub_status_data(hcd, buf);
>
>  	/* See if we have OCIC flag set */
>  	if (ocic_flag) {
> @@ -226,66 +223,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  		}
>  	}
>
> -	return ohci_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
> +	return orig_ohci_hub_control(hcd, typeReq, wValue,
> +			wIndex, buf, wLength);
>  }
>
> -static const struct hc_driver ohci_da8xx_hc_driver = {
> -	.description		= hcd_name,
> -	.product_desc		= "DA8xx OHCI",
> -	.hcd_priv_size		= sizeof(struct ohci_hcd),
> -
> -	/*
> -	 * generic hardware linkage
> -	 */
> -	.irq			= ohci_irq,
> -	.flags			= HCD_USB11 | HCD_MEMORY,
> -
> -	/*
> -	 * basic lifecycle operations
> -	 */
> -	.reset			= ohci_da8xx_init,
> -	.start			= ohci_da8xx_start,
> -	.stop			= ohci_da8xx_stop,
> -	.shutdown		= ohci_shutdown,
> -
> -	/*
> -	 * managing i/o requests and associated device resources
> -	 */
> -	.urb_enqueue		= ohci_urb_enqueue,
> -	.urb_dequeue		= ohci_urb_dequeue,
> -	.endpoint_disable	= ohci_endpoint_disable,
> -
> -	/*
> -	 * scheduling support
> -	 */
> -	.get_frame_number	= ohci_get_frame,
> -
> -	/*
> -	 * root hub support
> -	 */
> -	.hub_status_data	= ohci_da8xx_hub_status_data,
> -	.hub_control		= ohci_da8xx_hub_control,
> -
> -#ifdef	CONFIG_PM
> -	.bus_suspend		= ohci_bus_suspend,
> -	.bus_resume		= ohci_bus_resume,
> -#endif
> -	.start_port_reset	= ohci_start_port_reset,
> -};
> -
>  /*-------------------------------------------------------------------------*/
>
> -
> -/**
> - * usb_hcd_da8xx_probe - initialize DA8xx-based HCDs
> - * Context: !in_interrupt()
> - *
> - * Allocates basic resources for this USB host controller, and
> - * then invokes the start() method for the HCD associated with it
> - * through the hotplug entry's driver_data.
> - */
> -static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
> -			       struct platform_device *pdev)
> +static int ohci_da8xx_probe(struct platform_device *pdev)
>  {
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
>  	struct usb_hcd	*hcd;
> @@ -295,6 +239,11 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
>  	if (hub == NULL)
>  		return -ENODEV;
>
> +	hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
> +				dev_name(&pdev->dev));
> +	if (!hcd)
> +		return -ENOMEM;
> +

Won't this leak hdc if there is an error later?

>  	usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>  	if (IS_ERR(usb11_clk)) {
>  		if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
> @@ -309,9 +258,6 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
>  		return PTR_ERR(usb11_phy);
>  	}
>
> -	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> -	if (!hcd)
> -		return -ENOMEM;

Why does this need to be moved?

>
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
> @@ -323,13 +269,12 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
>  	hcd->rsrc_start = mem->start;
>  	hcd->rsrc_len = resource_size(mem);
>
> -	ohci_hcd_init(hcd_to_ohci(hcd));
> -
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		error = -ENODEV;
>  		goto err;
>  	}
> +
>  	error = usb_add_hcd(hcd, irq, 0);
>  	if (error)
>  		goto err;
> @@ -348,35 +293,14 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
>  	return error;
>  }
>
> -/**
> - * usb_hcd_da8xx_remove - shutdown processing for DA8xx-based HCDs
> - * @dev: USB Host Controller being removed
> - * Context: !in_interrupt()
> - *
> - * Reverses the effect of usb_hcd_da8xx_probe(), first invoking
> - * the HCD's stop() method.  It is always called from a thread
> - * context, normally "rmmod", "apmd", or something similar.
> - */
> -static inline void
> -usb_hcd_da8xx_remove(struct usb_hcd *hcd, struct platform_device *pdev)
> +static int ohci_da8xx_remove(struct platform_device *pdev)
>  {
> +	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
>
>  	hub->ocic_notify(NULL);
>  	usb_remove_hcd(hcd);
>  	usb_put_hcd(hcd);
> -}
> -
> -static int ohci_hcd_da8xx_drv_probe(struct platform_device *dev)
> -{
> -	return usb_hcd_da8xx_probe(&ohci_da8xx_hc_driver, dev);
> -}
> -
> -static int ohci_hcd_da8xx_drv_remove(struct platform_device *dev)
> -{
> -	struct usb_hcd	*hcd = platform_get_drvdata(dev);
> -
> -	usb_hcd_da8xx_remove(hcd, dev);
>
>  	return 0;
>  }
> @@ -426,12 +350,16 @@ static int ohci_da8xx_resume(struct platform_device *dev)
>  }
>  #endif
>
> +static const struct ohci_driver_overrides da8xx_overrides __initconst = {
> +	.reset		= ohci_da8xx_reset
> +};
> +
>  /*
>   * Driver definition to register with platform structure.
>   */
>  static struct platform_driver ohci_hcd_da8xx_driver = {
> -	.probe		= ohci_hcd_da8xx_drv_probe,
> -	.remove		= ohci_hcd_da8xx_drv_remove,
> +	.probe		= ohci_da8xx_probe,
> +	.remove		= ohci_da8xx_remove,
>  	.shutdown 	= usb_hcd_platform_shutdown,
>  #ifdef	CONFIG_PM
>  	.suspend	= ohci_da8xx_suspend,

It would probably be a good idea to change the driver name here. 
Currently it is "ohci". Although this would be better in a separate 
patch if the name has to be changed to match in other files as well.

> @@ -442,4 +370,39 @@ static int ohci_da8xx_resume(struct platform_device *dev)
>  	},
>  };
>
> +static int __init ohci_da8xx_init(void)
> +{
> +
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	pr_info("%s: " DRIVER_DESC "\n", hcd_name);
> +	ohci_init_driver(&ohci_da8xx_hc_driver, &da8xx_overrides);
> +
> +	/*
> +	 * The Davinci da8xx HW has some unusual quirks, which require
> +	 * da8xx-specific workarounds. We override certain hc_driver
> +	 * functions here to achieve that. We explicitly do not enhance
> +	 * ohci_driver_overrides to allow this more easily, since this
> +	 * is an unusual case, and we don't want to encourage others to
> +	 * override these functions by making it too easy.
> +	 */
> +
> +	orig_ohci_hub_control = ohci_da8xx_hc_driver.hub_control;
> +	orig_ohci_hub_status_data = ohci_da8xx_hc_driver.hub_status_data;
> +
> +	ohci_da8xx_hc_driver.hub_status_data     = ohci_da8xx_hub_status_data;
> +	ohci_da8xx_hc_driver.hub_control         = ohci_da8xx_hub_control;
> +
> +	return platform_driver_register(&ohci_hcd_da8xx_driver);
> +}
> +module_init(ohci_da8xx_init);
> +
> +static void __exit ohci_da8xx_cleanup(void)

ohci_da8xx_exit would be a better name

> +{
> +	platform_driver_unregister(&ohci_hcd_da8xx_driver);
> +}
> +module_exit(ohci_da8xx_cleanup);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
>  MODULE_ALIAS("platform:ohci");

this will need to be changed too if you change the driver name

> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index 1700908..8de174a 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -1219,11 +1219,6 @@ void ohci_init_driver(struct hc_driver *drv,
>  #define SA1111_DRIVER		ohci_hcd_sa1111_driver
>  #endif
>
> -#ifdef CONFIG_USB_OHCI_HCD_DAVINCI
> -#include "ohci-da8xx.c"
> -#define DAVINCI_PLATFORM_DRIVER	ohci_hcd_da8xx_driver
> -#endif
> -
>  #ifdef CONFIG_USB_OHCI_HCD_PPC_OF
>  #include "ohci-ppc-of.c"
>  #define OF_PLATFORM_DRIVER	ohci_hcd_ppc_of_driver
> @@ -1303,19 +1298,9 @@ static int __init ohci_hcd_mod_init(void)
>  		goto error_tmio;
>  #endif
>
> -#ifdef DAVINCI_PLATFORM_DRIVER
> -	retval = platform_driver_register(&DAVINCI_PLATFORM_DRIVER);
> -	if (retval < 0)
> -		goto error_davinci;
> -#endif
> -
>  	return retval;
>
>  	/* Error path */
> -#ifdef DAVINCI_PLATFORM_DRIVER
> -	platform_driver_unregister(&DAVINCI_PLATFORM_DRIVER);
> - error_davinci:
> -#endif
>  #ifdef TMIO_OHCI_DRIVER
>  	platform_driver_unregister(&TMIO_OHCI_DRIVER);
>   error_tmio:
> @@ -1351,9 +1336,6 @@ static int __init ohci_hcd_mod_init(void)
>
>  static void __exit ohci_hcd_mod_exit(void)
>  {
> -#ifdef DAVINCI_PLATFORM_DRIVER
> -	platform_driver_unregister(&DAVINCI_PLATFORM_DRIVER);
> -#endif
>  #ifdef TMIO_OHCI_DRIVER
>  	platform_driver_unregister(&TMIO_OHCI_DRIVER);
>  #endif
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver

[Axel Haslam]

On Tue, Oct 25, 2016 at 2:38 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>
>> From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> Separate the Davinci OHCI host controller driver from ohci-hcd
>> host code so that it can be built as a separate driver module.
>> This work is part of enabling multi-platform kernels on ARM;
>> it would be nice to have in 3.11.
>
>
> No need for comment about kernel 3.11.

yes, ok.

>
>>
>> Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/usb/host/Kconfig      |   2 +-
>>  drivers/usb/host/Makefile     |   1 +
>>  drivers/usb/host/ohci-da8xx.c | 185
>> +++++++++++++++++-------------------------
>>  drivers/usb/host/ohci-hcd.c   |  18 ----
>>  4 files changed, 76 insertions(+), 130 deletions(-)
>>
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index 83b6cec..642c6fe8 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -479,7 +479,7 @@ config USB_OHCI_HCD_OMAP3
>>           OMAP3 and later chips.
>>
>>  config USB_OHCI_HCD_DAVINCI
>> -       bool "OHCI support for TI DaVinci DA8xx"
>> +       tristate "OHCI support for TI DaVinci DA8xx"
>>         depends on ARCH_DAVINCI_DA8XX
>>         depends on USB_OHCI_HCD=y
>
>
> Need to drop the "=y" here, otherwise you still can't compile this as a
> module.

Im able to complile it as a module, but ok ill remove it.

>
>>         select PHY_DA8XX_USB
>> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
>> index 6ef785b..2644537 100644
>> --- a/drivers/usb/host/Makefile
>> +++ b/drivers/usb/host/Makefile
>> @@ -61,6 +61,7 @@ obj-$(CONFIG_USB_OHCI_HCD_AT91)       += ohci-at91.o
>>  obj-$(CONFIG_USB_OHCI_HCD_S3C2410)     += ohci-s3c2410.o
>>  obj-$(CONFIG_USB_OHCI_HCD_LPC32XX)     += ohci-nxp.o
>>  obj-$(CONFIG_USB_OHCI_HCD_PXA27X)      += ohci-pxa27x.o
>> +obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)     += ohci-da8xx.o
>>
>>  obj-$(CONFIG_USB_UHCI_HCD)     += uhci-hcd.o
>>  obj-$(CONFIG_USB_FHCI_HCD)     += fhci.o
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index e98066d..5585d9e 100644
>> --- a/drivers/usb/host/ohci-da8xx.c
>> +++ b/drivers/usb/host/ohci-da8xx.c
>> @@ -11,16 +11,31 @@
>>   * kind, whether express or implied.
>>   */
>>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/jiffies.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>>  #include <linux/platform_device.h>
>> -#include <linux/clk.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_data/usb-davinci.h>
>> +#include <linux/platform_device.h>
>
>
> linux/platform_device.h is listed twice
>
>> +#include <linux/usb.h>
>> +#include <linux/usb/hcd.h>
>> +#include <asm/unaligned.h>
>>
>> -#ifndef CONFIG_ARCH_DAVINCI_DA8XX
>> -#error "This file is DA8xx bus glue.  Define CONFIG_ARCH_DAVINCI_DA8XX."
>> -#endif
>> +#include "ohci.h"
>> +
>> +#define DRIVER_DESC "OHCI DA8XX driver"
>> +
>> +static const char hcd_name[] = "ohci-da8xx";
>
>
> why static const char instead of #define? This is only used one time in a
> pr_info, so it seems kind of pointless anyway.

Other drivers are using static const for the same variable.
i think static const is preferred over #define because #define doet give a type.
If you dont mind ill keep it static const.


>
>> +
>> +static struct hc_driver __read_mostly ohci_da8xx_hc_driver;
>> +
>> +static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
>> +                       u16 wValue, u16 wIndex, char *buf, u16 wLength);
>> +static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>>
>>  static struct clk *usb11_clk;
>>  static struct phy *usb11_phy;
>> @@ -73,7 +88,7 @@ static void ohci_da8xx_ocic_handler(struct
>> da8xx_ohci_root_hub *hub)
>>                 hub->set_power(0);
>>  }
>>
>> -static int ohci_da8xx_init(struct usb_hcd *hcd)
>> +static int ohci_da8xx_reset(struct usb_hcd *hcd)
>>  {
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> @@ -93,7 +108,7 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
>>          */
>>         ohci->num_ports = 1;
>>
>> -       result = ohci_init(ohci);
>> +       result = ohci_setup(hcd);
>>         if (result < 0) {
>>                 ohci_da8xx_disable();
>>                 return result;
>> @@ -121,30 +136,12 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
>>         return result;
>>  }
>>
>> -static void ohci_da8xx_stop(struct usb_hcd *hcd)
>> -{
>> -       ohci_stop(hcd);
>> -       ohci_da8xx_disable();
>> -}
>> -
>> -static int ohci_da8xx_start(struct usb_hcd *hcd)
>> -{
>> -       struct ohci_hcd *ohci           = hcd_to_ohci(hcd);
>> -       int result;
>> -
>> -       result = ohci_run(ohci);
>> -       if (result < 0)
>> -               ohci_da8xx_stop(hcd);
>> -
>> -       return result;
>> -}
>> -
>>  /*
>>   * Update the status data from the hub with the over-current indicator
>> change.
>>   */
>>  static int ohci_da8xx_hub_status_data(struct usb_hcd *hcd, char *buf)
>>  {
>> -       int length              = ohci_hub_status_data(hcd, buf);
>> +       int length              = orig_ohci_hub_status_data(hcd, buf);
>>
>>         /* See if we have OCIC flag set */
>>         if (ocic_flag) {
>> @@ -226,66 +223,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd
>> *hcd, u16 typeReq, u16 wValue,
>>                 }
>>         }
>>
>> -       return ohci_hub_control(hcd, typeReq, wValue, wIndex, buf,
>> wLength);
>> +       return orig_ohci_hub_control(hcd, typeReq, wValue,
>> +                       wIndex, buf, wLength);
>>  }
>>
>> -static const struct hc_driver ohci_da8xx_hc_driver = {
>> -       .description            = hcd_name,
>> -       .product_desc           = "DA8xx OHCI",
>> -       .hcd_priv_size          = sizeof(struct ohci_hcd),
>> -
>> -       /*
>> -        * generic hardware linkage
>> -        */
>> -       .irq                    = ohci_irq,
>> -       .flags                  = HCD_USB11 | HCD_MEMORY,
>> -
>> -       /*
>> -        * basic lifecycle operations
>> -        */
>> -       .reset                  = ohci_da8xx_init,
>> -       .start                  = ohci_da8xx_start,
>> -       .stop                   = ohci_da8xx_stop,
>> -       .shutdown               = ohci_shutdown,
>> -
>> -       /*
>> -        * managing i/o requests and associated device resources
>> -        */
>> -       .urb_enqueue            = ohci_urb_enqueue,
>> -       .urb_dequeue            = ohci_urb_dequeue,
>> -       .endpoint_disable       = ohci_endpoint_disable,
>> -
>> -       /*
>> -        * scheduling support
>> -        */
>> -       .get_frame_number       = ohci_get_frame,
>> -
>> -       /*
>> -        * root hub support
>> -        */
>> -       .hub_status_data        = ohci_da8xx_hub_status_data,
>> -       .hub_control            = ohci_da8xx_hub_control,
>> -
>> -#ifdef CONFIG_PM
>> -       .bus_suspend            = ohci_bus_suspend,
>> -       .bus_resume             = ohci_bus_resume,
>> -#endif
>> -       .start_port_reset       = ohci_start_port_reset,
>> -};
>> -
>>
>> /*-------------------------------------------------------------------------*/
>>
>> -
>> -/**
>> - * usb_hcd_da8xx_probe - initialize DA8xx-based HCDs
>> - * Context: !in_interrupt()
>> - *
>> - * Allocates basic resources for this USB host controller, and
>> - * then invokes the start() method for the HCD associated with it
>> - * through the hotplug entry's driver_data.
>> - */
>> -static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
>> -                              struct platform_device *pdev)
>> +static int ohci_da8xx_probe(struct platform_device *pdev)
>>  {
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(&pdev->dev);
>>         struct usb_hcd  *hcd;
>> @@ -295,6 +239,11 @@ static int usb_hcd_da8xx_probe(const struct hc_driver
>> *driver,
>>         if (hub == NULL)
>>                 return -ENODEV;
>>
>> +       hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
>> +                               dev_name(&pdev->dev));
>> +       if (!hcd)
>> +               return -ENOMEM;
>> +
>
>
> Won't this leak hdc if there is an error later?
>
>>         usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>>         if (IS_ERR(usb11_clk)) {
>>                 if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
>> @@ -309,9 +258,6 @@ static int usb_hcd_da8xx_probe(const struct hc_driver
>> *driver,
>>                 return PTR_ERR(usb11_phy);
>>         }
>>
>> -       hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>> -       if (!hcd)
>> -               return -ENOMEM;
>
>
> Why does this need to be moved?
>

it should not have moved this, will fix.


>>
>>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>         hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
>> @@ -323,13 +269,12 @@ static int usb_hcd_da8xx_probe(const struct
>> hc_driver *driver,
>>         hcd->rsrc_start = mem->start;
>>         hcd->rsrc_len = resource_size(mem);
>>
>> -       ohci_hcd_init(hcd_to_ohci(hcd));
>> -
>>         irq = platform_get_irq(pdev, 0);
>>         if (irq < 0) {
>>                 error = -ENODEV;
>>                 goto err;
>>         }
>> +
>>         error = usb_add_hcd(hcd, irq, 0);
>>         if (error)
>>                 goto err;
>> @@ -348,35 +293,14 @@ static int usb_hcd_da8xx_probe(const struct
>> hc_driver *driver,
>>         return error;
>>  }
>>
>> -/**
>> - * usb_hcd_da8xx_remove - shutdown processing for DA8xx-based HCDs
>> - * @dev: USB Host Controller being removed
>> - * Context: !in_interrupt()
>> - *
>> - * Reverses the effect of usb_hcd_da8xx_probe(), first invoking
>> - * the HCD's stop() method.  It is always called from a thread
>> - * context, normally "rmmod", "apmd", or something similar.
>> - */
>> -static inline void
>> -usb_hcd_da8xx_remove(struct usb_hcd *hcd, struct platform_device *pdev)
>> +static int ohci_da8xx_remove(struct platform_device *pdev)
>>  {
>> +       struct usb_hcd  *hcd = platform_get_drvdata(pdev);
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(&pdev->dev);
>>
>>         hub->ocic_notify(NULL);
>>         usb_remove_hcd(hcd);
>>         usb_put_hcd(hcd);
>> -}
>> -
>> -static int ohci_hcd_da8xx_drv_probe(struct platform_device *dev)
>> -{
>> -       return usb_hcd_da8xx_probe(&ohci_da8xx_hc_driver, dev);
>> -}
>> -
>> -static int ohci_hcd_da8xx_drv_remove(struct platform_device *dev)
>> -{
>> -       struct usb_hcd  *hcd = platform_get_drvdata(dev);
>> -
>> -       usb_hcd_da8xx_remove(hcd, dev);
>>
>>         return 0;
>>  }
>> @@ -426,12 +350,16 @@ static int ohci_da8xx_resume(struct platform_device
>> *dev)
>>  }
>>  #endif
>>
>> +static const struct ohci_driver_overrides da8xx_overrides __initconst = {
>> +       .reset          = ohci_da8xx_reset
>> +};
>> +
>>  /*
>>   * Driver definition to register with platform structure.
>>   */
>>  static struct platform_driver ohci_hcd_da8xx_driver = {
>> -       .probe          = ohci_hcd_da8xx_drv_probe,
>> -       .remove         = ohci_hcd_da8xx_drv_remove,
>> +       .probe          = ohci_da8xx_probe,
>> +       .remove         = ohci_da8xx_remove,
>>         .shutdown       = usb_hcd_platform_shutdown,
>>  #ifdef CONFIG_PM
>>         .suspend        = ohci_da8xx_suspend,
>
>
> It would probably be a good idea to change the driver name here. Currently
> it is "ohci". Although this would be better in a separate patch if the name
> has to be changed to match in other files as well.
>

ok, ill do a separate patch for that.

>> @@ -442,4 +370,39 @@ static int ohci_da8xx_resume(struct platform_device
>> *dev)
>>         },
>>  };
>>
>> +static int __init ohci_da8xx_init(void)
>> +{
>> +
>> +       if (usb_disabled())
>> +               return -ENODEV;
>> +
>> +       pr_info("%s: " DRIVER_DESC "\n", hcd_name);
>> +       ohci_init_driver(&ohci_da8xx_hc_driver, &da8xx_overrides);
>> +
>> +       /*
>> +        * The Davinci da8xx HW has some unusual quirks, which require
>> +        * da8xx-specific workarounds. We override certain hc_driver
>> +        * functions here to achieve that. We explicitly do not enhance
>> +        * ohci_driver_overrides to allow this more easily, since this
>> +        * is an unusual case, and we don't want to encourage others to
>> +        * override these functions by making it too easy.
>> +        */
>> +
>> +       orig_ohci_hub_control = ohci_da8xx_hc_driver.hub_control;
>> +       orig_ohci_hub_status_data = ohci_da8xx_hc_driver.hub_status_data;
>> +
>> +       ohci_da8xx_hc_driver.hub_status_data     =
>> ohci_da8xx_hub_status_data;
>> +       ohci_da8xx_hc_driver.hub_control         = ohci_da8xx_hub_control;
>> +
>> +       return platform_driver_register(&ohci_hcd_da8xx_driver);
>> +}
>> +module_init(ohci_da8xx_init);
>> +
>> +static void __exit ohci_da8xx_cleanup(void)
>
>
> ohci_da8xx_exit would be a better name
>

ok.

>> +{
>> +       platform_driver_unregister(&ohci_hcd_da8xx_driver);
>> +}
>> +module_exit(ohci_da8xx_cleanup);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_LICENSE("GPL");
>>  MODULE_ALIAS("platform:ohci");
>
>
> this will need to be changed too if you change the driver name
>
>> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
>> index 1700908..8de174a 100644
>> --- a/drivers/usb/host/ohci-hcd.c
>> +++ b/drivers/usb/host/ohci-hcd.c
>> @@ -1219,11 +1219,6 @@ void ohci_init_driver(struct hc_driver *drv,
>>  #define SA1111_DRIVER          ohci_hcd_sa1111_driver
>>  #endif
>>
>> -#ifdef CONFIG_USB_OHCI_HCD_DAVINCI
>> -#include "ohci-da8xx.c"
>> -#define DAVINCI_PLATFORM_DRIVER        ohci_hcd_da8xx_driver
>> -#endif
>> -
>>  #ifdef CONFIG_USB_OHCI_HCD_PPC_OF
>>  #include "ohci-ppc-of.c"
>>  #define OF_PLATFORM_DRIVER     ohci_hcd_ppc_of_driver
>> @@ -1303,19 +1298,9 @@ static int __init ohci_hcd_mod_init(void)
>>                 goto error_tmio;
>>  #endif
>>
>> -#ifdef DAVINCI_PLATFORM_DRIVER
>> -       retval = platform_driver_register(&DAVINCI_PLATFORM_DRIVER);
>> -       if (retval < 0)
>> -               goto error_davinci;
>> -#endif
>> -
>>         return retval;
>>
>>         /* Error path */
>> -#ifdef DAVINCI_PLATFORM_DRIVER
>> -       platform_driver_unregister(&DAVINCI_PLATFORM_DRIVER);
>> - error_davinci:
>> -#endif
>>  #ifdef TMIO_OHCI_DRIVER
>>         platform_driver_unregister(&TMIO_OHCI_DRIVER);
>>   error_tmio:
>> @@ -1351,9 +1336,6 @@ static int __init ohci_hcd_mod_init(void)
>>
>>  static void __exit ohci_hcd_mod_exit(void)
>>  {
>> -#ifdef DAVINCI_PLATFORM_DRIVER
>> -       platform_driver_unregister(&DAVINCI_PLATFORM_DRIVER);
>> -#endif
>>  #ifdef TMIO_OHCI_DRIVER
>>         platform_driver_unregister(&TMIO_OHCI_DRIVER);
>>  #endif
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver

[David Lechner]

On 10/25/2016 02:39 AM, Axel Haslam wrote:
> On Tue, Oct 25, 2016 at 2:38 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
>> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>>
>>> -#ifndef CONFIG_ARCH_DAVINCI_DA8XX
>>> -#error "This file is DA8xx bus glue.  Define CONFIG_ARCH_DAVINCI_DA8XX."
>>> -#endif
>>> +#include "ohci.h"
>>> +
>>> +#define DRIVER_DESC "OHCI DA8XX driver"
>>> +
>>> +static const char hcd_name[] = "ohci-da8xx";
>>
>>
>> why static const char instead of #define? This is only used one time in a
>> pr_info, so it seems kind of pointless anyway.
>
> Other drivers are using static const for the same variable.
> i think static const is preferred over #define because #define doet give a type.
> If you dont mind ill keep it static const.
>

If this string was used in this file more than one place, I would agree 
with you, but currently it is only used as the argument of a pr_info(). 
The string "ohci-da8xx" could just be included in the fmt string instead 
of using "%s".

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver

[Axel Haslam]

On Tue, Oct 25, 2016 at 6:12 PM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
> On 10/25/2016 02:39 AM, Axel Haslam wrote:
>>
>> On Tue, Oct 25, 2016 at 2:38 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>> wrote:
>>>
>>> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>>>
>>>>
>>>> -#ifndef CONFIG_ARCH_DAVINCI_DA8XX
>>>> -#error "This file is DA8xx bus glue.  Define
>>>> CONFIG_ARCH_DAVINCI_DA8XX."
>>>> -#endif
>>>> +#include "ohci.h"
>>>> +
>>>> +#define DRIVER_DESC "OHCI DA8XX driver"
>>>> +
>>>> +static const char hcd_name[] = "ohci-da8xx";
>>>
>>>
>>>
>>> why static const char instead of #define? This is only used one time in a
>>> pr_info, so it seems kind of pointless anyway.
>>
>>
>> Other drivers are using static const for the same variable.
>> i think static const is preferred over #define because #define doet give a
>> type.
>> If you dont mind ill keep it static const.
>>
>
> If this string was used in this file more than one place, I would agree with
> you, but currently it is only used as the argument of a pr_info(). The
> string "ohci-da8xx" could just be included in the fmt string instead of
> using "%s".

I think the purpose was to use it in the .name of the platform_driver
structure, too. only that not everybody is doing that, i looked at some bad
 examples :(

would you agree to keep it if we use it in .name too?

-Axel

>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver

[David Lechner]

On 10/25/2016 11:21 AM, Axel Haslam wrote:
> On Tue, Oct 25, 2016 at 6:12 PM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
>> On 10/25/2016 02:39 AM, Axel Haslam wrote:
>>>
>>> On Tue, Oct 25, 2016 at 2:38 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>> wrote:
>>>>
>>>> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>>>>
>>>>>
>>>>> -#ifndef CONFIG_ARCH_DAVINCI_DA8XX
>>>>> -#error "This file is DA8xx bus glue.  Define
>>>>> CONFIG_ARCH_DAVINCI_DA8XX."
>>>>> -#endif
>>>>> +#include "ohci.h"
>>>>> +
>>>>> +#define DRIVER_DESC "OHCI DA8XX driver"
>>>>> +
>>>>> +static const char hcd_name[] = "ohci-da8xx";
>>>>
>>>>
>>>>
>>>> why static const char instead of #define? This is only used one time in a
>>>> pr_info, so it seems kind of pointless anyway.
>>>
>>>
>>> Other drivers are using static const for the same variable.
>>> i think static const is preferred over #define because #define doet give a
>>> type.
>>> If you dont mind ill keep it static const.
>>>
>>
>> If this string was used in this file more than one place, I would agree with
>> you, but currently it is only used as the argument of a pr_info(). The
>> string "ohci-da8xx" could just be included in the fmt string instead of
>> using "%s".
>
> I think the purpose was to use it in the .name of the platform_driver
> structure, too. only that not everybody is doing that, i looked at some bad
>  examples :(
>
> would you agree to keep it if we use it in .name too?
>
> -Axel
>
>>

Yes, that will make more sense. ;-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Currently, the da8xx ohci driver uses a set of gpios and callbacks in
board files to handle vbus and overcurrent irqs form the power supply.
However, this does not play nice when moving to a DT based boot were
we wont have board files.

Instead of requesting and handling the gpio, use the regulator framework
to take care of enabling and disabling vbus power. This has the benefit
that we dont need to pass any more platform data to the driver:

These will be handled by the regulator framework:
set_power   ->  regulator_enable/regulator_disable
get_power   ->  regulator_is_enabled
get_oci     ->  regulator_get_mode
ocic_notify ->  regulator notification

We can keep the default potpgt and use the regulator start delay instead:
potpgt      -> regulator startup delay time

The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
(they should not have been decleared/reserved) so, just remove those
definitions from the hwk board file.

Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 arch/arm/mach-davinci/board-da830-evm.c     |  97 ++++++++----------------
 arch/arm/mach-davinci/board-omapl138-hawk.c |  96 +-----------------------
 arch/arm/mach-davinci/include/mach/da8xx.h  |   2 +-
 arch/arm/mach-davinci/usb-da8xx.c           |   3 +-
 drivers/usb/host/ohci-da8xx.c               | 111 ++++++++++++++++++----------
 include/linux/platform_data/usb-davinci.h   |  19 -----
 6 files changed, 105 insertions(+), 223 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index d12fcf5..d6f9f8a 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -27,6 +27,7 @@
 #include <linux/platform_data/mtd-davinci-aemif.h>
 #include <linux/platform_data/spi-davinci.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/fixed.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -48,61 +49,35 @@
 	-1
 };
 
-static da8xx_ocic_handler_t da830_evm_usb_ocic_handler;
+static struct regulator_consumer_supply usb_ohci_consumer_supply =
+	REGULATOR_SUPPLY("vbus", "ohci");
 
-static int da830_evm_usb_set_power(int on)
-{
-	gpio_set_value(ON_BD_USB_DRV, on);
-	return 0;
-}
-
-static int da830_evm_usb_get_power(void)
-{
-	return gpio_get_value(ON_BD_USB_DRV);
-}
-
-static int da830_evm_usb_get_oci(void)
-{
-	return !gpio_get_value(ON_BD_USB_OVC);
-}
-
-static irqreturn_t da830_evm_usb_ocic_irq(int, void *);
-
-static int da830_evm_usb_ocic_notify(da8xx_ocic_handler_t handler)
-{
-	int irq 	= gpio_to_irq(ON_BD_USB_OVC);
-	int error	= 0;
-
-	if (handler != NULL) {
-		da830_evm_usb_ocic_handler = handler;
-
-		error = request_irq(irq, da830_evm_usb_ocic_irq,
-				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-				    "OHCI over-current indicator", NULL);
-		if (error)
-			pr_err("%s: could not request IRQ to watch over-current indicator changes\n",
-			       __func__);
-	} else
-		free_irq(irq, NULL);
-
-	return error;
-}
-
-static struct da8xx_ohci_root_hub da830_evm_usb11_pdata = {
-	.set_power	= da830_evm_usb_set_power,
-	.get_power	= da830_evm_usb_get_power,
-	.get_oci	= da830_evm_usb_get_oci,
-	.ocic_notify	= da830_evm_usb_ocic_notify,
+static struct regulator_init_data usb_ohci_initdata = {
+	.consumer_supplies = &usb_ohci_consumer_supply,
+	.num_consumer_supplies = 1,
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
 
-	/* TPS2065 switch @ 5V */
-	.potpgt		= (3 + 1) / 2,	/* 3 ms max */
+static struct fixed_voltage_config usb_ohci_config = {
+	.supply_name		= "vbus",
+	.microvolts		= 5000000,
+	.gpio			= ON_BD_USB_DRV,
+	.oc_gpio		= ON_BD_USB_OVC,
+	.has_oc_gpio		= 1,
+	.enable_high		= 1,
+	.enabled_at_boot	= 0,
+	.init_data		= &usb_ohci_initdata,
 };
 
-static irqreturn_t da830_evm_usb_ocic_irq(int irq, void *dev_id)
-{
-	da830_evm_usb_ocic_handler(&da830_evm_usb11_pdata);
-	return IRQ_HANDLED;
-}
+static struct platform_device da8xx_usb11_regulator = {
+	.name	= "reg-fixed-voltage",
+	.id	= 0,
+	.dev	= {
+		.platform_data = &usb_ohci_config,
+	},
+};
 
 static __init void da830_evm_usb_init(void)
 {
@@ -144,23 +119,11 @@ static __init void da830_evm_usb_init(void)
 		return;
 	}
 
-	ret = gpio_request(ON_BD_USB_DRV, "ON_BD_USB_DRV");
-	if (ret) {
-		pr_err("%s: failed to request GPIO for USB 1.1 port power control: %d\n",
-		       __func__, ret);
-		return;
-	}
-	gpio_direction_output(ON_BD_USB_DRV, 0);
-
-	ret = gpio_request(ON_BD_USB_OVC, "ON_BD_USB_OVC");
-	if (ret) {
-		pr_err("%s: failed to request GPIO for USB 1.1 port over-current indicator: %d\n",
-		       __func__, ret);
-		return;
-	}
-	gpio_direction_input(ON_BD_USB_OVC);
+	ret = platform_device_register(&da8xx_usb11_regulator);
+	if (ret)
+		pr_warn("fail to add ohci regulator\n");
 
-	ret = da8xx_register_usb11(&da830_evm_usb11_pdata);
+	ret = da8xx_register_usb11();
 	if (ret)
 		pr_warn("%s: USB 1.1 registration failed: %d\n", __func__, ret);
 }
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index 1d31f45..2df34cb 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -28,9 +28,6 @@
 #define DA850_HAWK_MMCSD_CD_PIN		GPIO_TO_PIN(3, 12)
 #define DA850_HAWK_MMCSD_WP_PIN		GPIO_TO_PIN(3, 13)
 
-#define DA850_USB1_VBUS_PIN		GPIO_TO_PIN(2, 4)
-#define DA850_USB1_OC_PIN		GPIO_TO_PIN(6, 13)
-
 static short omapl138_hawk_mii_pins[] __initdata = {
 	DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
 	DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
@@ -181,76 +178,10 @@ static __init void omapl138_hawk_mmc_init(void)
 	gpio_free(DA850_HAWK_MMCSD_CD_PIN);
 }
 
-static irqreturn_t omapl138_hawk_usb_ocic_irq(int irq, void *dev_id);
-static da8xx_ocic_handler_t hawk_usb_ocic_handler;
-
-static const short da850_hawk_usb11_pins[] = {
-	DA850_GPIO2_4, DA850_GPIO6_13,
-	-1
-};
-
-static int hawk_usb_set_power(int on)
-{
-	gpio_set_value(DA850_USB1_VBUS_PIN, on);
-	return 0;
-}
-
-static int hawk_usb_get_power(void)
-{
-	return gpio_get_value(DA850_USB1_VBUS_PIN);
-}
-
-static int hawk_usb_get_oci(void)
-{
-	return !gpio_get_value(DA850_USB1_OC_PIN);
-}
-
-static int hawk_usb_ocic_notify(da8xx_ocic_handler_t handler)
-{
-	int irq         = gpio_to_irq(DA850_USB1_OC_PIN);
-	int error       = 0;
-
-	if (handler != NULL) {
-		hawk_usb_ocic_handler = handler;
-
-		error = request_irq(irq, omapl138_hawk_usb_ocic_irq,
-					IRQF_TRIGGER_RISING |
-					IRQF_TRIGGER_FALLING,
-					"OHCI over-current indicator", NULL);
-		if (error)
-			pr_err("%s: could not request IRQ to watch "
-				"over-current indicator changes\n", __func__);
-	} else {
-		free_irq(irq, NULL);
-	}
-	return error;
-}
-
-static struct da8xx_ohci_root_hub omapl138_hawk_usb11_pdata = {
-	.set_power      = hawk_usb_set_power,
-	.get_power      = hawk_usb_get_power,
-	.get_oci        = hawk_usb_get_oci,
-	.ocic_notify    = hawk_usb_ocic_notify,
-	/* TPS2087 switch @ 5V */
-	.potpgt         = (3 + 1) / 2,  /* 3 ms max */
-};
-
-static irqreturn_t omapl138_hawk_usb_ocic_irq(int irq, void *dev_id)
-{
-	hawk_usb_ocic_handler(&omapl138_hawk_usb11_pdata);
-	return IRQ_HANDLED;
-}
-
 static __init void omapl138_hawk_usb_init(void)
 {
 	int ret;
 
-	ret = davinci_cfg_reg_list(da850_hawk_usb11_pins);
-	if (ret) {
-		pr_warn("%s: USB 1.1 PinMux setup failed: %d\n", __func__, ret);
-		return;
-	}
-
 	/* USB_REFCLKIN is not used. */
 	ret = da8xx_register_usb20_phy_clk(false);
 	if (ret)
@@ -266,34 +197,11 @@ static __init void omapl138_hawk_usb_init(void)
 		pr_warn("%s: USB PHY registration failed: %d\n",
 			__func__, ret);
 
-	ret = gpio_request_one(DA850_USB1_VBUS_PIN,
-			GPIOF_DIR_OUT, "USB1 VBUS");
-	if (ret < 0) {
-		pr_err("%s: failed to request GPIO for USB 1.1 port "
-			"power control: %d\n", __func__, ret);
-		return;
-	}
-
-	ret = gpio_request_one(DA850_USB1_OC_PIN,
-			GPIOF_DIR_IN, "USB1 OC");
-	if (ret < 0) {
-		pr_err("%s: failed to request GPIO for USB 1.1 port "
-			"over-current indicator: %d\n", __func__, ret);
-		goto usb11_setup_oc_fail;
-	}
-
-	ret = da8xx_register_usb11(&omapl138_hawk_usb11_pdata);
-	if (ret) {
+	ret = da8xx_register_usb11();
+	if (ret)
 		pr_warn("%s: USB 1.1 registration failed: %d\n", __func__, ret);
-		goto usb11_setup_fail;
-	}
 
 	return;
-
-usb11_setup_fail:
-	gpio_free(DA850_USB1_OC_PIN);
-usb11_setup_oc_fail:
-	gpio_free(DA850_USB1_VBUS_PIN);
 }
 
 static __init void omapl138_hawk_init(void)
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index 38d932e..93d2db3 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -94,7 +94,7 @@
 int da8xx_register_usb11_phy_clk(bool use_usb_refclkin);
 int da8xx_register_usb_phy(void);
 int da8xx_register_usb20(unsigned mA, unsigned potpgt);
-int da8xx_register_usb11(struct da8xx_ohci_root_hub *pdata);
+int da8xx_register_usb11(void);
 int da8xx_register_emac(void);
 int da8xx_register_uio_pruss(void);
 int da8xx_register_lcdc(struct da8xx_lcdc_platform_data *pdata);
diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index 982e105..7a9af216 100644
--- a/arch/arm/mach-davinci/usb-da8xx.c
+++ b/arch/arm/mach-davinci/usb-da8xx.c
@@ -337,8 +337,7 @@ int __init da8xx_register_usb20(unsigned int mA, unsigned int potpgt)
 	.resource	= da8xx_usb11_resources,
 };
 
-int __init da8xx_register_usb11(struct da8xx_ohci_root_hub *pdata)
+int __init da8xx_register_usb11(void)
 {
-	da8xx_usb11_device.dev.platform_data = pdata;
 	return platform_device_register(&da8xx_usb11_device);
 }
diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 5585d9e..f4bda4d 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -39,6 +39,8 @@ static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
 
 static struct clk *usb11_clk;
 static struct phy *usb11_phy;
+static struct regulator *vbus_reg;
+struct notifier_block nb;
 
 /* Over-current indicator change flag */
 static int ocic_flag;
@@ -76,22 +78,57 @@ static void ohci_da8xx_disable(void)
 	clk_disable_unprepare(usb11_clk);
 }
 
-/*
- * Handle the port over-current indicator change.
- */
-static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub)
+static int ohci_da8xx_set_power(int on)
+{
+	int ret = 0;
+
+	if (!vbus_reg)
+		return 0;
+
+	if (on) {
+		ret = regulator_enable(vbus_reg);
+		if (ret)
+			pr_err("fail to enable regulator: %d\n", ret);
+	} else {
+		ret = regulator_disable(vbus_reg);
+		if (ret)
+			pr_err("fail to disable regulator: %d\n", ret);
+	}
+
+	return ret;
+}
+
+static int ohci_da8xx_get_power(void)
+{
+	if (!vbus_reg)
+		return 1;
+
+	return regulator_is_enabled(vbus_reg);
+}
+
+static int ohci_da8xx_get_oci(void)
+{
+	if (regulator_get_mode(vbus_reg) == REGULATOR_MODE_OVERCURRENT)
+		return 1;
+
+	return 0;
+}
+
+static int ohci_da8xx_regulator_event(struct notifier_block *nb,
+				unsigned long event, void *data)
 {
-	ocic_flag = 1;
+	if (event & REGULATOR_EVENT_OVER_CURRENT) {
+		ocic_flag = 1;
+		if (ohci_da8xx_get_oci())
+			ohci_da8xx_set_power(0);
+	}
 
-	/* Once over-current is detected, the port needs to be powered down */
-	if (hub->get_oci() > 0)
-		hub->set_power(0);
+	return 0;
 }
 
 static int ohci_da8xx_reset(struct usb_hcd *hcd)
 {
 	struct device *dev		= hcd->self.controller;
-	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
 	struct ohci_hcd	*ohci		= hcd_to_ohci(hcd);
 	int result;
 	u32 rh_a;
@@ -121,16 +158,14 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
 	 * the correct hub descriptor...
 	 */
 	rh_a = ohci_readl(ohci, &ohci->regs->roothub.a);
-	if (hub->set_power) {
+
+	if (vbus_reg) {
 		rh_a &= ~RH_A_NPS;
 		rh_a |=  RH_A_PSM;
-	}
-	if (hub->get_oci) {
 		rh_a &= ~RH_A_NOCP;
 		rh_a |=  RH_A_OCPM;
 	}
-	rh_a &= ~RH_A_POTPGT;
-	rh_a |= hub->potpgt << 24;
+
 	ohci_writel(ohci, rh_a, &ohci->regs->roothub.a);
 
 	return result;
@@ -163,7 +198,6 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				  u16 wIndex, char *buf, u16 wLength)
 {
 	struct device *dev		= hcd->self.controller;
-	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
 	int temp;
 
 	switch (typeReq) {
@@ -177,11 +211,11 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		temp = roothub_portstatus(hcd_to_ohci(hcd), wIndex - 1);
 
 		/* The port power status (PPS) bit defaults to 1 */
-		if (hub->get_power && hub->get_power() == 0)
+		if (ohci_da8xx_get_power() == 0)
 			temp &= ~RH_PS_PPS;
 
 		/* The port over-current indicator (POCI) bit is always 0 */
-		if (hub->get_oci && hub->get_oci() > 0)
+		if (ohci_da8xx_get_oci() > 0)
 			temp |=  RH_PS_POCI;
 
 		/* The over-current indicator change (OCIC) bit is 0 too */
@@ -206,19 +240,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			dev_dbg(dev, "%sPortFeature(%u): %s\n",
 				temp ? "Set" : "Clear", wIndex, "POWER");
 
-			if (!hub->set_power)
-				return -EPIPE;
-
-			return hub->set_power(temp) ? -EPIPE : 0;
+			return ohci_da8xx_set_power(temp) ? -EPIPE : 0;
 		case USB_PORT_FEAT_C_OVER_CURRENT:
 			dev_dbg(dev, "%sPortFeature(%u): %s\n",
 				temp ? "Set" : "Clear", wIndex,
 				"C_OVER_CURRENT");
 
-			if (temp)
-				ocic_flag = 1;
-			else
-				ocic_flag = 0;
+			ocic_flag = temp;
 			return 0;
 		}
 	}
@@ -231,14 +259,10 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 static int ohci_da8xx_probe(struct platform_device *pdev)
 {
-	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
 	struct usb_hcd	*hcd;
 	struct resource *mem;
 	int error, irq;
 
-	if (hub == NULL)
-		return -ENODEV;
-
 	hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
 				dev_name(&pdev->dev));
 	if (!hcd)
@@ -258,6 +282,22 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 		return PTR_ERR(usb11_phy);
 	}
 
+	vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
+	if (IS_ERR(vbus_reg)) {
+		if (PTR_ERR(vbus_reg) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Failed to get regulator.\n");
+		return PTR_ERR(vbus_reg);
+	}
+
+	if (vbus_reg) {
+		nb.notifier_call = ohci_da8xx_regulator_event;
+		error = devm_regulator_register_notifier(vbus_reg, &nb);
+		if (error) {
+			dev_err(&pdev->dev,
+				"Could not register regulator notifier\n");
+			return error;
+		}
+	}
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
@@ -281,13 +321,7 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 
 	device_wakeup_enable(hcd->self.controller);
 
-	if (hub->ocic_notify) {
-		error = hub->ocic_notify(ohci_da8xx_ocic_handler);
-		if (!error)
-			return 0;
-	}
-
-	usb_remove_hcd(hcd);
+	return 0;
 err:
 	usb_put_hcd(hcd);
 	return error;
@@ -295,10 +329,8 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 
 static int ohci_da8xx_remove(struct platform_device *pdev)
 {
-	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
-	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
+	struct usb_hcd  *hcd = platform_get_drvdata(pdev);
 
-	hub->ocic_notify(NULL);
 	usb_remove_hcd(hcd);
 	usb_put_hcd(hcd);
 
@@ -314,7 +346,6 @@ static int ohci_da8xx_suspend(struct platform_device *pdev,
 	bool		do_wakeup	= device_may_wakeup(&pdev->dev);
 	int		ret;
 
-
 	if (time_before(jiffies, ohci->next_statechange))
 		msleep(5);
 	ohci->next_statechange = jiffies;
diff --git a/include/linux/platform_data/usb-davinci.h b/include/linux/platform_data/usb-davinci.h
index 3217fbe..58f4be0 100644
--- a/include/linux/platform_data/usb-davinci.h
+++ b/include/linux/platform_data/usb-davinci.h
@@ -11,25 +11,6 @@
 #ifndef __ASM_ARCH_USB_H
 #define __ASM_ARCH_USB_H
 
-struct	da8xx_ohci_root_hub;
-
-typedef void (*da8xx_ocic_handler_t)(struct da8xx_ohci_root_hub *hub);
-
-/* Passed as the platform data to the OHCI driver */
-struct	da8xx_ohci_root_hub {
-	/* Switch the port power on/off */
-	int	(*set_power)(int on);
-	/* Read the port power status */
-	int	(*get_power)(void);
-	/* Read the port over-current indicator */
-	int	(*get_oci)(void);
-	/* Over-current indicator change notification (pass NULL to disable) */
-	int	(*ocic_notify)(da8xx_ocic_handler_t handler);
-
-	/* Time from power on to power good (in 2 ms units) */
-	u8	potpgt;
-};
-
 void davinci_setup_usb(unsigned mA, unsigned potpgt_ms);
 
 #endif	/* ifndef __ASM_ARCH_USB_H */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent

[David Lechner]

On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
> board files to handle vbus and overcurrent irqs form the power supply.
> However, this does not play nice when moving to a DT based boot were
> we wont have board files.
>
> Instead of requesting and handling the gpio, use the regulator framework
> to take care of enabling and disabling vbus power.
> This has the benefit
> that we dont need to pass any more platform data to the driver:
>
> These will be handled by the regulator framework:
> set_power   ->  regulator_enable/regulator_disable
> get_power   ->  regulator_is_enabled
> get_oci     ->  regulator_get_mode
> ocic_notify ->  regulator notification
>
> We can keep the default potpgt and use the regulator start delay instead:
> potpgt      -> regulator startup delay time
>
> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
> (they should not have been decleared/reserved) so, just remove those
> definitions from the hwk board file.
>
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---


How do you recover after an overcurrent event?

I have configured a fixed-regulator using device-tree, but similar to 
the configuration in the board files here. However, when I shorted out 
the VBUS and caused an overcurrent event, I see nothing in the kernel 
log saying that there was an overcurrent event and after I remove the 
short, the regulator is never turned back on.



> @@ -163,7 +198,6 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  				  u16 wIndex, char *buf, u16 wLength)
>  {
>  	struct device *dev		= hcd->self.controller;
> -	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);

nit: unnecessary whitespace change

>  	int temp;
>
>  	switch (typeReq) {

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent

[Axel Haslam]

On Tue, Oct 25, 2016 at 3:39 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>
>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
>> board files to handle vbus and overcurrent irqs form the power supply.
>> However, this does not play nice when moving to a DT based boot were
>> we wont have board files.
>>
>> Instead of requesting and handling the gpio, use the regulator framework
>> to take care of enabling and disabling vbus power.
>> This has the benefit
>> that we dont need to pass any more platform data to the driver:
>>
>> These will be handled by the regulator framework:
>> set_power   ->  regulator_enable/regulator_disable
>> get_power   ->  regulator_is_enabled
>> get_oci     ->  regulator_get_mode
>> ocic_notify ->  regulator notification
>>
>> We can keep the default potpgt and use the regulator start delay instead:
>> potpgt      -> regulator startup delay time
>>
>> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
>> (they should not have been decleared/reserved) so, just remove those
>> definitions from the hwk board file.
>>
>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>
>
>
> How do you recover after an overcurrent event?
>
> I have configured a fixed-regulator using device-tree, but similar to the
> configuration in the board files here. However, when I shorted out the VBUS
> and caused an overcurrent event, I see nothing in the kernel log saying that
> there was an overcurrent event and after I remove the short, the regulator
> is never turned back on.
>
>

You should have the patch to fix gpiolib, and you should declare the
over current gpio on the regulator as such:
(if the pin is enabled high you should add oc-active-high);

       vbus_fixed: fixed-regulator-vbus {
               compatible = "regulator-fixed";
               gpio = <&gpio 109 0>;
               oc-gpio = <&gpio 36 0>;
               regulator-boot-on;
               enable-active-high;
               regulator-name = "vbus";
               regulator-min-microvolt = <5000000>;
               regulator-max-microvolt = <5000000>;
       };


Question: Do you see that the over current gpio was requested
in debugfs/gpio? and, do you see the interrupt in /proc/interrupts?

If you unplug and plug in back the usb device it should work again.
also you can unbind and bind it should also start to work:
something like:

echo usb1 >/sys/bus/usb/drivers/usb/unbind
echo usb1 >/sys/bus/usb/drivers/usb/bind


>
>> @@ -163,7 +198,6 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd,
>> u16 typeReq, u16 wValue,
>>                                   u16 wIndex, char *buf, u16 wLength)
>>  {
>>         struct device *dev              = hcd->self.controller;
>> -       struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>
>
> nit: unnecessary whitespace change
>
>>         int temp;
>>
>>         switch (typeReq) {
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent

[David Lechner]

On 10/25/2016 03:24 AM, Axel Haslam wrote:
> On Tue, Oct 25, 2016 at 3:39 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
>> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>>
>>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>
>>> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
>>> board files to handle vbus and overcurrent irqs form the power supply.
>>> However, this does not play nice when moving to a DT based boot were
>>> we wont have board files.
>>>
>>> Instead of requesting and handling the gpio, use the regulator framework
>>> to take care of enabling and disabling vbus power.
>>> This has the benefit
>>> that we dont need to pass any more platform data to the driver:
>>>
>>> These will be handled by the regulator framework:
>>> set_power   ->  regulator_enable/regulator_disable
>>> get_power   ->  regulator_is_enabled
>>> get_oci     ->  regulator_get_mode
>>> ocic_notify ->  regulator notification
>>>
>>> We can keep the default potpgt and use the regulator start delay instead:
>>> potpgt      -> regulator startup delay time
>>>
>>> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
>>> (they should not have been decleared/reserved) so, just remove those
>>> definitions from the hwk board file.
>>>
>>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>> ---
>>
>>
>>
>> How do you recover after an overcurrent event?
>>
>> I have configured a fixed-regulator using device-tree, but similar to the
>> configuration in the board files here. However, when I shorted out the VBUS
>> and caused an overcurrent event, I see nothing in the kernel log saying that
>> there was an overcurrent event and after I remove the short, the regulator
>> is never turned back on.
>>
>>
>
> You should have the patch to fix gpiolib, and you should declare the
> over current gpio on the regulator as such:
> (if the pin is enabled high you should add oc-active-high);
>
>        vbus_fixed: fixed-regulator-vbus {
>                compatible = "regulator-fixed";
>                gpio = <&gpio 109 0>;
>                oc-gpio = <&gpio 36 0>;
>                regulator-boot-on;
>                enable-active-high;
>                regulator-name = "vbus";
>                regulator-min-microvolt = <5000000>;
>                regulator-max-microvolt = <5000000>;
>        };
>
>
> Question: Do you see that the over current gpio was requested
> in debugfs/gpio? and, do you see the interrupt in /proc/interrupts?
>
> If you unplug and plug in back the usb device it should work again.
> also you can unbind and bind it should also start to work:
> something like:
>
> echo usb1 >/sys/bus/usb/drivers/usb/unbind
> echo usb1 >/sys/bus/usb/drivers/usb/bind
>
>

I have added oc-active-high and I get different results, but it is still 
not quite right. When I short the VBUS, I can see that my overcurrent 
gpio changes state. However, the driver does not turn of the VBUS. When 
I remove the short, I get an overcurrent error in the kernel log. I 
would expect this when I create the short, not when I remove it. I also 
tried adding  GPIO_ACTIVE_LOW to the oc-gpio, but this did not change 
the behavior. In either case, the oc_gpio shows as high under normal 
conditions, so perhaps there is a problem with the gpio-davinci driver 
not picking up GPIO_ACTIVE_LOW from the device tree.


My regulator is basically the same. My device just uses different gpios.

	vbus_reg: vbus-reg {
		compatible = "regulator-fixed";
		pinctrl-names = "default";
		pinctrl-0 = <&usb11_pins>;
		gpio = <&gpio 101 GPIO_ACTIVE_LOW>;
		oc-gpio = <&gpio 99 0>;
		enable-active-high;
		oc-active-high;
		regulator-name = "vbus";
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
	}


It seems to me though that I should not have oc-active-high since under 
normal conditions, the oc_gpio is high and during an overcurrent event, 
the oc_gpio is low. Double-checking the behavior without oc-active-high, 
I see that the vbus gpio is turned off in response to the overcurrent 
event, but I don't get the overcurrent message in the kernel log. 
Perhaps this is because as soon as there is an overcurrent event the 
vbus turns off and the oc_gpio returns to normal before the usb driver 
has a chance to poll the overcurrent state?

Also, unplugging the device and plugging it back in does nothing. 
Unbinding and binding the driver does work, but that does not seem like 
a very nice way to have to recover from an overcurrent event.



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent

[Axel Haslam]

On Tue, Oct 25, 2016 at 6:53 PM, David Lechner <david@lechnology.com> wrote:
> On 10/25/2016 03:24 AM, Axel Haslam wrote:
>>
>> On Tue, Oct 25, 2016 at 3:39 AM, David Lechner <david@lechnology.com>
>> wrote:
>>>
>>> On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
>>>>
>>>>
>>>> From: Axel Haslam <ahaslam@baylibre.com>
>>>>
>>>> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
>>>> board files to handle vbus and overcurrent irqs form the power supply.
>>>> However, this does not play nice when moving to a DT based boot were
>>>> we wont have board files.
>>>>
>>>> Instead of requesting and handling the gpio, use the regulator framework
>>>> to take care of enabling and disabling vbus power.
>>>> This has the benefit
>>>> that we dont need to pass any more platform data to the driver:
>>>>
>>>> These will be handled by the regulator framework:
>>>> set_power   ->  regulator_enable/regulator_disable
>>>> get_power   ->  regulator_is_enabled
>>>> get_oci     ->  regulator_get_mode
>>>> ocic_notify ->  regulator notification
>>>>
>>>> We can keep the default potpgt and use the regulator start delay
>>>> instead:
>>>> potpgt      -> regulator startup delay time
>>>>
>>>> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
>>>> (they should not have been decleared/reserved) so, just remove those
>>>> definitions from the hwk board file.
>>>>
>>>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>>>> ---
>>>
>>>
>>>
>>>
>>> How do you recover after an overcurrent event?
>>>
>>> I have configured a fixed-regulator using device-tree, but similar to the
>>> configuration in the board files here. However, when I shorted out the
>>> VBUS
>>> and caused an overcurrent event, I see nothing in the kernel log saying
>>> that
>>> there was an overcurrent event and after I remove the short, the
>>> regulator
>>> is never turned back on.
>>>
>>>
>>
>> You should have the patch to fix gpiolib, and you should declare the
>> over current gpio on the regulator as such:
>> (if the pin is enabled high you should add oc-active-high);
>>
>>        vbus_fixed: fixed-regulator-vbus {
>>                compatible = "regulator-fixed";
>>                gpio = <&gpio 109 0>;
>>                oc-gpio = <&gpio 36 0>;
>>                regulator-boot-on;
>>                enable-active-high;
>>                regulator-name = "vbus";
>>                regulator-min-microvolt = <5000000>;
>>                regulator-max-microvolt = <5000000>;
>>        };
>>
>>
>> Question: Do you see that the over current gpio was requested
>> in debugfs/gpio? and, do you see the interrupt in /proc/interrupts?
>>
>> If you unplug and plug in back the usb device it should work again.
>> also you can unbind and bind it should also start to work:
>> something like:
>>
>> echo usb1 >/sys/bus/usb/drivers/usb/unbind
>> echo usb1 >/sys/bus/usb/drivers/usb/bind
>>
>>
>
> I have added oc-active-high and I get different results, but it is still not
> quite right. When I short the VBUS, I can see that my overcurrent gpio
> changes state. However, the driver does not turn of the VBUS. When I remove
> the short, I get an overcurrent error in the kernel log. I would expect this
> when I create the short, not when I remove it. I also tried adding
> GPIO_ACTIVE_LOW to the oc-gpio, but this did not change the behavior. In
> either case, the oc_gpio shows as high under normal conditions, so perhaps
> there is a problem with the gpio-davinci driver not picking up
> GPIO_ACTIVE_LOW from the device tree.
>
>
> My regulator is basically the same. My device just uses different gpios.
>
>         vbus_reg: vbus-reg {
>                 compatible = "regulator-fixed";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&usb11_pins>;
>                 gpio = <&gpio 101 GPIO_ACTIVE_LOW>;
>                 oc-gpio = <&gpio 99 0>;
>                 enable-active-high;
>                 oc-active-high;
>                 regulator-name = "vbus";
>                 regulator-min-microvolt = <5000000>;
>                 regulator-max-microvolt = <5000000>;
>         }
>
>
> It seems to me though that I should not have oc-active-high since under
> normal conditions, the oc_gpio is high and during an overcurrent event, the

in my board the over current gpio is active low too, i was just mentiontioning
to add that in case yours  was not.

> oc_gpio is low. Double-checking the behavior without oc-active-high, I see
> that the vbus gpio is turned off in response to the overcurrent event, but I
> don't get the overcurrent message in the kernel log. Perhaps this is because
> as soon as there is an overcurrent event the vbus turns off and the oc_gpio
> returns to normal before the usb driver has a chance to poll the overcurrent
> state?

Perhaps. i dont have a board that has overcurrent, or that i can
switch vbus off.
what is exactly the log you are refering to?

im wondering, was the behavior different before the patches?
it should be the same without the patches.

>
> Also, unplugging the device and plugging it back in does nothing. Unbinding
> and binding the driver does work, but that does not seem like a very nice
> way to have to recover from an overcurrent event.

im guessing that unplug and plug wont work as vbus is gone and
we cannot detect it anymore (contrary to my setup were i always have vbus)
sorry for suggesting that before.

we could recover vbus automatically form the notification,
(when the over-current condition disappears) but then im afraid we
might oscillate, even if we retry after some timeout. so perhaps
bind/unbind is the right thing to do, as it requires the user to correct
the problem. I dont know if there is a better solution.


>
>
>

Re: [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent

[Sekhar Nori]

On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 
> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
> board files to handle vbus and overcurrent irqs form the power supply.
> However, this does not play nice when moving to a DT based boot were
> we wont have board files.
> 
> Instead of requesting and handling the gpio, use the regulator framework
> to take care of enabling and disabling vbus power. This has the benefit
> that we dont need to pass any more platform data to the driver:
> 
> These will be handled by the regulator framework:
> set_power   ->  regulator_enable/regulator_disable
> get_power   ->  regulator_is_enabled
> get_oci     ->  regulator_get_mode
> ocic_notify ->  regulator notification
> 
> We can keep the default potpgt and use the regulator start delay instead:
> potpgt      -> regulator startup delay time
> 
> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
> (they should not have been decleared/reserved) so, just remove those
> definitions from the hwk board file.
> 
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  arch/arm/mach-davinci/board-da830-evm.c     |  97 ++++++++----------------
>  arch/arm/mach-davinci/board-omapl138-hawk.c |  96 +-----------------------
>  arch/arm/mach-davinci/include/mach/da8xx.h  |   2 +-
>  arch/arm/mach-davinci/usb-da8xx.c           |   3 +-
>  drivers/usb/host/ohci-da8xx.c               | 111 ++++++++++++++++++----------
>  include/linux/platform_data/usb-davinci.h   |  19 -----
>  6 files changed, 105 insertions(+), 223 deletions(-)

Can you separate out the driver enhancement from the platform
(mach-davinci) changes? They need to go through different trees.

Thanks,
Sekhar


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent

[Axel Haslam]

On Tue, Oct 25, 2016 at 12:43 PM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
>> board files to handle vbus and overcurrent irqs form the power supply.
>> However, this does not play nice when moving to a DT based boot were
>> we wont have board files.
>>
>> Instead of requesting and handling the gpio, use the regulator framework
>> to take care of enabling and disabling vbus power. This has the benefit
>> that we dont need to pass any more platform data to the driver:
>>
>> These will be handled by the regulator framework:
>> set_power   ->  regulator_enable/regulator_disable
>> get_power   ->  regulator_is_enabled
>> get_oci     ->  regulator_get_mode
>> ocic_notify ->  regulator notification
>>
>> We can keep the default potpgt and use the regulator start delay instead:
>> potpgt      -> regulator startup delay time
>>
>> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
>> (they should not have been decleared/reserved) so, just remove those
>> definitions from the hwk board file.
>>
>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>>  arch/arm/mach-davinci/board-da830-evm.c     |  97 ++++++++----------------
>>  arch/arm/mach-davinci/board-omapl138-hawk.c |  96 +-----------------------
>>  arch/arm/mach-davinci/include/mach/da8xx.h  |   2 +-
>>  arch/arm/mach-davinci/usb-da8xx.c           |   3 +-
>>  drivers/usb/host/ohci-da8xx.c               | 111 ++++++++++++++++++----------
>>  include/linux/platform_data/usb-davinci.h   |  19 -----
>>  6 files changed, 105 insertions(+), 223 deletions(-)
>
> Can you separate out the driver enhancement from the platform
> (mach-davinci) changes? They need to go through different trees.
>

Ok, i will do that,  (it might require intermediate code to have
the driver working on each patch)

> Thanks,
> Sekhar
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 13/17] USB: da8xx: use ohci priv data instead of globals

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Instead of global variables, use the extra_priv_size of the ohci driver
to add a reference to driver private data.

Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 drivers/usb/host/ohci-da8xx.c | 135 ++++++++++++++++++++++++------------------
 1 file changed, 79 insertions(+), 56 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index f4bda4d..bebc3f0 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -37,60 +37,66 @@ static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
 			u16 wValue, u16 wIndex, char *buf, u16 wLength);
 static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
 
-static struct clk *usb11_clk;
-static struct phy *usb11_phy;
-static struct regulator *vbus_reg;
-struct notifier_block nb;
-
-/* Over-current indicator change flag */
-static int ocic_flag;
+struct da8xx_ohci_hcd {
+	struct usb_hcd *hcd;
+	struct clk *usb11_clk;
+	struct phy *usb11_phy;
+	struct regulator *vbus_reg;
+	struct notifier_block nb;
+	int ocic_flag;
+};
+#define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
 
-static int ohci_da8xx_enable(void)
+static int ohci_da8xx_enable(struct usb_hcd *hcd)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	int ret;
 
-	ret = clk_prepare_enable(usb11_clk);
+	ret = clk_prepare_enable(da8xx_ohci->usb11_clk);
 	if (ret)
 		return ret;
 
-	ret = phy_init(usb11_phy);
+	ret = phy_init(da8xx_ohci->usb11_phy);
 	if (ret)
 		goto err_phy_init;
 
-	ret = phy_power_on(usb11_phy);
+	ret = phy_power_on(da8xx_ohci->usb11_phy);
 	if (ret)
 		goto err_phy_power_on;
 
 	return 0;
 
 err_phy_power_on:
-	phy_exit(usb11_phy);
+	phy_exit(da8xx_ohci->usb11_phy);
 err_phy_init:
-	clk_disable_unprepare(usb11_clk);
+	clk_disable_unprepare(da8xx_ohci->usb11_clk);
 
 	return ret;
 }
 
-static void ohci_da8xx_disable(void)
+static void ohci_da8xx_disable(struct usb_hcd *hcd)
 {
-	phy_power_off(usb11_phy);
-	phy_exit(usb11_phy);
-	clk_disable_unprepare(usb11_clk);
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
+
+	phy_power_off(da8xx_ohci->usb11_phy);
+	phy_exit(da8xx_ohci->usb11_phy);
+	clk_disable_unprepare(da8xx_ohci->usb11_clk);
 }
 
-static int ohci_da8xx_set_power(int on)
+static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	int ret = 0;
 
-	if (!vbus_reg)
+	if (!da8xx_ohci->vbus_reg)
 		return 0;
 
 	if (on) {
-		ret = regulator_enable(vbus_reg);
+		ret = regulator_enable(da8xx_ohci->vbus_reg);
 		if (ret)
 			pr_err("fail to enable regulator: %d\n", ret);
 	} else {
-		ret = regulator_disable(vbus_reg);
+		ret = regulator_disable(da8xx_ohci->vbus_reg);
 		if (ret)
 			pr_err("fail to disable regulator: %d\n", ret);
 	}
@@ -98,17 +104,22 @@ static int ohci_da8xx_set_power(int on)
 	return ret;
 }
 
-static int ohci_da8xx_get_power(void)
+static int ohci_da8xx_get_power(struct usb_hcd *hcd)
 {
-	if (!vbus_reg)
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
+
+	if (!da8xx_ohci->vbus_reg)
 		return 1;
 
-	return regulator_is_enabled(vbus_reg);
+	return regulator_is_enabled(da8xx_ohci->vbus_reg);
 }
 
-static int ohci_da8xx_get_oci(void)
+static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
 {
-	if (regulator_get_mode(vbus_reg) == REGULATOR_MODE_OVERCURRENT)
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
+
+	if (regulator_get_mode(da8xx_ohci->vbus_reg) ==
+			REGULATOR_MODE_OVERCURRENT)
 		return 1;
 
 	return 0;
@@ -117,10 +128,13 @@ static int ohci_da8xx_get_oci(void)
 static int ohci_da8xx_regulator_event(struct notifier_block *nb,
 				unsigned long event, void *data)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci =
+			container_of(nb, struct da8xx_ohci_hcd, nb);
+
 	if (event & REGULATOR_EVENT_OVER_CURRENT) {
-		ocic_flag = 1;
-		if (ohci_da8xx_get_oci())
-			ohci_da8xx_set_power(0);
+		da8xx_ohci->ocic_flag = 1;
+		if (ohci_da8xx_get_oci(da8xx_ohci->hcd))
+			ohci_da8xx_set_power(da8xx_ohci->hcd, 0);
 	}
 
 	return 0;
@@ -130,12 +144,13 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
 {
 	struct device *dev		= hcd->self.controller;
 	struct ohci_hcd	*ohci		= hcd_to_ohci(hcd);
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	int result;
 	u32 rh_a;
 
 	dev_dbg(dev, "starting USB controller\n");
 
-	result = ohci_da8xx_enable();
+	result = ohci_da8xx_enable(hcd);
 	if (result < 0)
 		return result;
 
@@ -147,7 +162,7 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
 
 	result = ohci_setup(hcd);
 	if (result < 0) {
-		ohci_da8xx_disable();
+		ohci_da8xx_disable(hcd);
 		return result;
 	}
 
@@ -159,7 +174,7 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
 	 */
 	rh_a = ohci_readl(ohci, &ohci->regs->roothub.a);
 
-	if (vbus_reg) {
+	if (da8xx_ohci->vbus_reg) {
 		rh_a &= ~RH_A_NPS;
 		rh_a |=  RH_A_PSM;
 		rh_a &= ~RH_A_NOCP;
@@ -176,10 +191,11 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
  */
 static int ohci_da8xx_hub_status_data(struct usb_hcd *hcd, char *buf)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	int length		= orig_ohci_hub_status_data(hcd, buf);
 
 	/* See if we have OCIC flag set */
-	if (ocic_flag) {
+	if (da8xx_ohci->ocic_flag) {
 		dev_dbg(hcd->self.controller, "over-current indicator change "
 			"on port 1\n");
 
@@ -197,6 +213,7 @@ static int ohci_da8xx_hub_status_data(struct usb_hcd *hcd, char *buf)
 static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				  u16 wIndex, char *buf, u16 wLength)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	struct device *dev		= hcd->self.controller;
 	int temp;
 
@@ -211,15 +228,15 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		temp = roothub_portstatus(hcd_to_ohci(hcd), wIndex - 1);
 
 		/* The port power status (PPS) bit defaults to 1 */
-		if (ohci_da8xx_get_power() == 0)
+		if (ohci_da8xx_get_power(hcd) == 0)
 			temp &= ~RH_PS_PPS;
 
 		/* The port over-current indicator (POCI) bit is always 0 */
-		if (ohci_da8xx_get_oci() > 0)
+		if (ohci_da8xx_get_oci(hcd) > 0)
 			temp |=  RH_PS_POCI;
 
 		/* The over-current indicator change (OCIC) bit is 0 too */
-		if (ocic_flag)
+		if (da8xx_ohci->ocic_flag)
 			temp |=  RH_PS_OCIC;
 
 		put_unaligned(cpu_to_le32(temp), (__le32 *)buf);
@@ -240,13 +257,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			dev_dbg(dev, "%sPortFeature(%u): %s\n",
 				temp ? "Set" : "Clear", wIndex, "POWER");
 
-			return ohci_da8xx_set_power(temp) ? -EPIPE : 0;
+			return ohci_da8xx_set_power(hcd, temp) ? -EPIPE : 0;
 		case USB_PORT_FEAT_C_OVER_CURRENT:
 			dev_dbg(dev, "%sPortFeature(%u): %s\n",
 				temp ? "Set" : "Clear", wIndex,
 				"C_OVER_CURRENT");
 
-			ocic_flag = temp;
+			da8xx_ohci->ocic_flag = temp;
 			return 0;
 		}
 	}
@@ -259,6 +276,7 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 static int ohci_da8xx_probe(struct platform_device *pdev)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci;
 	struct usb_hcd	*hcd;
 	struct resource *mem;
 	int error, irq;
@@ -268,30 +286,34 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 	if (!hcd)
 		return -ENOMEM;
 
-	usb11_clk = devm_clk_get(&pdev->dev, "usb11");
-	if (IS_ERR(usb11_clk)) {
-		if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
+	da8xx_ohci = to_da8xx_ohci(hcd);
+	da8xx_ohci->hcd = hcd;
+
+	da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
+	if (IS_ERR(da8xx_ohci->usb11_clk)) {
+		if (PTR_ERR(da8xx_ohci->usb11_clk) != -EPROBE_DEFER)
 			dev_err(&pdev->dev, "Failed to get clock.\n");
-		return PTR_ERR(usb11_clk);
+		return PTR_ERR(da8xx_ohci->usb11_clk);
 	}
 
-	usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
-	if (IS_ERR(usb11_phy)) {
-		if (PTR_ERR(usb11_phy) != -EPROBE_DEFER)
+	da8xx_ohci->usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
+	if (IS_ERR(da8xx_ohci->usb11_phy)) {
+		if (PTR_ERR(da8xx_ohci->usb11_phy) != -EPROBE_DEFER)
 			dev_err(&pdev->dev, "Failed to get phy.\n");
-		return PTR_ERR(usb11_phy);
+		return PTR_ERR(da8xx_ohci->usb11_phy);
 	}
 
-	vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
-	if (IS_ERR(vbus_reg)) {
-		if (PTR_ERR(vbus_reg) != -EPROBE_DEFER)
+	da8xx_ohci->vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
+	if (IS_ERR(da8xx_ohci->vbus_reg)) {
+		if (PTR_ERR(da8xx_ohci->vbus_reg) != -EPROBE_DEFER)
 			dev_err(&pdev->dev, "Failed to get regulator.\n");
-		return PTR_ERR(vbus_reg);
+		return PTR_ERR(da8xx_ohci->vbus_reg);
 	}
 
-	if (vbus_reg) {
-		nb.notifier_call = ohci_da8xx_regulator_event;
-		error = devm_regulator_register_notifier(vbus_reg, &nb);
+	if (da8xx_ohci->vbus_reg) {
+		da8xx_ohci->nb.notifier_call = ohci_da8xx_regulator_event;
+		error = devm_regulator_register_notifier(da8xx_ohci->vbus_reg,
+							 &da8xx_ohci->nb);
 		if (error) {
 			dev_err(&pdev->dev,
 				"Could not register regulator notifier\n");
@@ -354,7 +376,7 @@ static int ohci_da8xx_suspend(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	ohci_da8xx_disable();
+	ohci_da8xx_disable(hcd);
 	hcd->state = HC_STATE_SUSPENDED;
 
 	return ret;
@@ -370,7 +392,7 @@ static int ohci_da8xx_resume(struct platform_device *dev)
 		msleep(5);
 	ohci->next_statechange = jiffies;
 
-	ret = ohci_da8xx_enable();
+	ret = ohci_da8xx_enable(hcd);
 	if (ret)
 		return ret;
 
@@ -382,7 +404,8 @@ static int ohci_da8xx_resume(struct platform_device *dev)
 #endif
 
 static const struct ohci_driver_overrides da8xx_overrides __initconst = {
-	.reset		= ohci_da8xx_reset
+	.reset		= ohci_da8xx_reset,
+	.extra_priv_size = sizeof(struct da8xx_ohci_hcd),
 };
 
 /*
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 13/17] USB: da8xx: use ohci priv data instead of globals

[David Lechner]

On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> >  static const struct ohci_driver_overrides da8xx_overrides __initconst = {
> -	.reset		= ohci_da8xx_reset
> +	.reset		= ohci_da8xx_reset,
> +	.extra_priv_size = sizeof(struct da8xx_ohci_hcd),

nit: since you are changing both lines anyway, you might as well make 
the ='s line up.

>  };
>
>  /*
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 13/17] USB: da8xx: use ohci priv data instead of globals

[Axel Haslam]

On Tue, Oct 25, 2016 at 3:12 AM, David Lechner <david@lechnology.com> wrote:
> On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
>>
>> From: Axel Haslam <ahaslam@baylibre.com>
>> >  static const struct ohci_driver_overrides da8xx_overrides __initconst =
>> > {
>> -       .reset          = ohci_da8xx_reset
>> +       .reset          = ohci_da8xx_reset,
>> +       .extra_priv_size = sizeof(struct da8xx_ohci_hcd),
>
>
> nit: since you are changing both lines anyway, you might as well make the
> ='s line up.

ok, will do.

>
>>  };
>>
>>  /*
>>
>

[PATCH/RFT v2 14/17] ARM: davinci: register the usb20_phy clock on the DT file

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

The usb20_phy clock needs to be registered for the driver to be able
to get and enable a clock. Currently the usb phy clocks are registered
form board files, which will not be called during a device tree based
boot.

To be able to probe correctly usb form a device tree boot, register
the usb phy clocks form the DT specific init.

Unfourtunatly, davinci does not have proper clock support on device tree
yet, so by registering the clock form de DT specific file we are
forced to hardcode the parent clock, and cannot select refclkin as
parent for any of the phy clocks of the da850 family.

As none of the current da850 based boards currently in mainline use
refclkin as source. I guess we can live with this limitation until clocks
are correctly represented through CCF/device tree.

Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 arch/arm/mach-davinci/da8xx-dt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index c9f7e92..7947267 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -45,6 +45,8 @@
 
 static void __init da850_init_machine(void)
 {
+	da8xx_register_usb20_phy_clk(false);
+	da8xx_register_usb11_phy_clk(false);
 	of_platform_default_populate(NULL, da850_auxdata_lookup, NULL);
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 15/17] usb: host: ohci-da8xx: Add devicetree bindings documentation

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

This patch documents the device tree bindings required for
the ohci controller found in TI da8xx family of SoC's

Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 .../devicetree/bindings/usb/ohci-da8xx.txt         | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ohci-da8xx.txt

diff --git a/Documentation/devicetree/bindings/usb/ohci-da8xx.txt b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
new file mode 100644
index 0000000..4251c84
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
@@ -0,0 +1,39 @@
+DA8XX USB OHCI controller
+
+Required properties:
+
+ - compatible: Should be "ti,da830-ohci"
+ - reg:        Should contain one register range i.e. start and length
+ - interrupts: Description of the interrupt line
+ - phys:       Phandle for the PHY device
+ - phy-names:  Should be "usb-phy"
+
+Optional properties:
+ - vbus-supply: Regulator that controls vbus power
+
+Example for omap138-lck:
+
+vbus_fixed: fixed-regulator-vbus {
+        compatible = "regulator-fixed";
+        gpio = <&gpio 109 0>;
+        oc-gpio = <&gpio 36 0>;
+        regulator-boot-on;
+        enable-active-high;
+        regulator-name = "vbus";
+        regulator-min-microvolt = <5000000>;
+        regulator-max-microvolt = <5000000>;
+};
+
+usb_phy: usb-phy {
+        compatible = "ti,da830-usb-phy";
+        #phy-cells = <1>;
+        status = "disabled";
+};
+usb: usb@0225000 {
+        compatible = "ti,da830-ohci";
+        reg = <0x225000 0x1000>;
+        interrupts = <59>;
+        phys = <&usb_phy 1>;
+        phy-names = "usb-phy";
+        status = "disabled";
+};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 15/17] usb: host: ohci-da8xx: Add devicetree bindings documentation

[David Lechner]

On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> This patch documents the device tree bindings required for
> the ohci controller found in TI da8xx family of SoC's
>
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/ohci-da8xx.txt         | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/ohci-da8xx.txt b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
> new file mode 100644
> index 0000000..4251c84
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
> @@ -0,0 +1,39 @@
> +DA8XX USB OHCI controller
> +
> +Required properties:
> +
> + - compatible: Should be "ti,da830-ohci"
> + - reg:        Should contain one register range i.e. start and length
> + - interrupts: Description of the interrupt line
> + - phys:       Phandle for the PHY device
> + - phy-names:  Should be "usb-phy"
> +
> +Optional properties:
> + - vbus-supply: Regulator that controls vbus power


Isn't vbus-supply property required?

If it is really supposed to be optional, the ohci driver needs to use 
devm_regulator_get_optional() and handle the case when there is no 
regulator.

I don't see a problem with making it required though since one can just 
use a dummy supply if there is not a real one.

> +
> +Example for omap138-lck:
> +
> +vbus_fixed: fixed-regulator-vbus {
> +        compatible = "regulator-fixed";
> +        gpio = <&gpio 109 0>;
> +        oc-gpio = <&gpio 36 0>;
> +        regulator-boot-on;
> +        enable-active-high;
> +        regulator-name = "vbus";
> +        regulator-min-microvolt = <5000000>;
> +        regulator-max-microvolt = <5000000>;
> +};
> +
> +usb_phy: usb-phy {
> +        compatible = "ti,da830-usb-phy";
> +        #phy-cells = <1>;
> +        status = "disabled";

why disabled?

> +};
> +usb: usb@0225000 {
> +        compatible = "ti,da830-ohci";
> +        reg = <0x225000 0x1000>;
> +        interrupts = <59>;
> +        phys = <&usb_phy 1>;
> +        phy-names = "usb-phy";

missing vbus-supply property

> +        status = "disabled";

why disabled?

> +};
>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 15/17] usb: host: ohci-da8xx: Add devicetree bindings documentation

[Axel Haslam]

On Tue, Oct 25, 2016 at 3:02 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>
>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> This patch documents the device tree bindings required for
>> the ohci controller found in TI da8xx family of SoC's
>>
>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>>  .../devicetree/bindings/usb/ohci-da8xx.txt         | 39
>> ++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>> b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>> new file mode 100644
>> index 0000000..4251c84
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>> @@ -0,0 +1,39 @@
>> +DA8XX USB OHCI controller
>> +
>> +Required properties:
>> +
>> + - compatible: Should be "ti,da830-ohci"
>> + - reg:        Should contain one register range i.e. start and length
>> + - interrupts: Description of the interrupt line
>> + - phys:       Phandle for the PHY device
>> + - phy-names:  Should be "usb-phy"
>> +
>> +Optional properties:
>> + - vbus-supply: Regulator that controls vbus power
>
>
>
> Isn't vbus-supply property required?
>
> If it is really supposed to be optional, the ohci driver needs to use
> devm_regulator_get_optional() and handle the case when there is no
> regulator.
>
> I don't see a problem with making it required though since one can just use
> a dummy supply if there is not a real one.

The regulator framework will use a dummy regulator if none is provided.

>
>> +
>> +Example for omap138-lck:
>> +
>> +vbus_fixed: fixed-regulator-vbus {
>> +        compatible = "regulator-fixed";
>> +        gpio = <&gpio 109 0>;
>> +        oc-gpio = <&gpio 36 0>;
>> +        regulator-boot-on;
>> +        enable-active-high;
>> +        regulator-name = "vbus";
>> +        regulator-min-microvolt = <5000000>;
>> +        regulator-max-microvolt = <5000000>;
>> +};
>> +
>> +usb_phy: usb-phy {
>> +        compatible = "ti,da830-usb-phy";
>> +        #phy-cells = <1>;
>> +        status = "disabled";
>
>
> why disabled?
>

yes, i copied from the device tree im using, but
i should enable these for the example,
i will fix.

>> +};
>> +usb: usb@0225000 {
>> +        compatible = "ti,da830-ohci";
>> +        reg = <0x225000 0x1000>;
>> +        interrupts = <59>;
>> +        phys = <&usb_phy 1>;
>> +        phy-names = "usb-phy";
>
>
> missing vbus-supply property
>
>> +        status = "disabled";
>
>
> why disabled?
>
>> +};
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 16/17] USB: ohci-da8xx: Allow probing from DT

[ahaslam-rdvid1DuHRBWk0Htik3J/w]

From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

This adds the compatible string to the ohci driver
to be able to probe from DT

Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 drivers/usb/host/ohci-da8xx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index bebc3f0..1a8db25 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -273,6 +273,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 }
 
 /*-------------------------------------------------------------------------*/
+#ifdef CONFIG_OF
+static const struct of_device_id da8xx_ohci_ids[] = {
+	{ .compatible = "ti,da830-ohci" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, da8xx_ohci_ids);
+#endif
 
 static int ohci_da8xx_probe(struct platform_device *pdev)
 {
@@ -421,6 +428,7 @@ static int ohci_da8xx_resume(struct platform_device *dev)
 #endif
 	.driver		= {
 		.name	= "ohci",
+		.of_match_table = da8xx_ohci_ids,
 	},
 };
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 16/17] USB: ohci-da8xx: Allow probing from DT

[David Lechner]

On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> This adds the compatible string to the ohci driver
> to be able to probe from DT
>
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/usb/host/ohci-da8xx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
> index bebc3f0..1a8db25 100644
> --- a/drivers/usb/host/ohci-da8xx.c
> +++ b/drivers/usb/host/ohci-da8xx.c
> @@ -273,6 +273,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  }
>
>  /*-------------------------------------------------------------------------*/
> +#ifdef CONFIG_OF

#ifdef CONFIG_OF is probably not needed here...

> +static const struct of_device_id da8xx_ohci_ids[] = {
> +	{ .compatible = "ti,da830-ohci" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, da8xx_ohci_ids);
> +#endif
>
>  static int ohci_da8xx_probe(struct platform_device *pdev)
>  {
> @@ -421,6 +428,7 @@ static int ohci_da8xx_resume(struct platform_device *dev)
>  #endif
>  	.driver		= {
>  		.name	= "ohci",
> +		.of_match_table = da8xx_ohci_ids,

...otherwise, da8xx_ohci_ids will not be defined here

>  	},
>  };
>
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 16/17] USB: ohci-da8xx: Allow probing from DT

[Axel Haslam]

On Tue, Oct 25, 2016 at 2:53 AM, David Lechner <david@lechnology.com> wrote:
> On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
>>
>> From: Axel Haslam <ahaslam@baylibre.com>
>>
>> This adds the compatible string to the ohci driver
>> to be able to probe from DT
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>>  drivers/usb/host/ohci-da8xx.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index bebc3f0..1a8db25 100644
>> --- a/drivers/usb/host/ohci-da8xx.c
>> +++ b/drivers/usb/host/ohci-da8xx.c
>> @@ -273,6 +273,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd
>> *hcd, u16 typeReq, u16 wValue,
>>  }
>>
>>
>> /*-------------------------------------------------------------------------*/
>> +#ifdef CONFIG_OF
>
>
> #ifdef CONFIG_OF is probably not needed here...
>
>> +static const struct of_device_id da8xx_ohci_ids[] = {
>> +       { .compatible = "ti,da830-ohci" },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, da8xx_ohci_ids);
>> +#endif
>>
>>  static int ohci_da8xx_probe(struct platform_device *pdev)
>>  {
>> @@ -421,6 +428,7 @@ static int ohci_da8xx_resume(struct platform_device
>> *dev)
>>  #endif
>>         .driver         = {
>>                 .name   = "ohci",
>> +               .of_match_table = da8xx_ohci_ids,
>
>
> ...otherwise, da8xx_ohci_ids will not be defined here

this should be  .of_match_table = of_match_ptr(da8xx_ohci_ids),
will fix.

>
>>         },
>>  };
>>
>>
>

Re: [PATCH/RFT v2 00/17] Add DT support for ohci-da8xx

[Sekhar Nori]

Hi Axel,

On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 
> The purpose of this patch series is to add DT support and modernize
> the ohci-da8xx glue driver without breaking the non-DT boot,
> which is still used in unconverted davinci devices.

>From a mach-davinci perspective, there are some patches which seem to be
safe to apply and some which depend on corresponding driver changes to
get in.

In order to speed up the process of applying this series, can you split
the mach-davinci updates which are safe to apply into a separate series.

For DT patches, the bindings should be accepted. For other patches, they
should not be causing any regressions.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 04/17] ARM: DTS: da850: Add cfgchip syscon node

[ahaslam]

From: David Lechner <david@lechnology.com>

Add a syscon node for the SoC CFGCHIPn registers. This is needed for
the new usb phy driver.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/da850.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b9..6bbf20d 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -188,6 +188,10 @@
 			};
 
 		};
+		cfgchip: cfgchip@1417c {
+			compatible = "ti,da830-cfgchip", "syscon";
+			reg = <0x1417c 0x14>;
+		};
 		edma0: edma@0 {
 			compatible = "ti,edma3-tpcc";
 			/* eDMA3 CC0: 0x01c0 0000 - 0x01c0 7fff */
-- 
1.9.1

[PATCH/RFT v2 05/17] ARM: DTS: da850: Add usb phy node

[ahaslam]

From: David Lechner <david@lechnology.com>

Add a node for the new usb phy driver.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/da850.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 6bbf20d..33fcdce 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -376,6 +376,11 @@
 					>;
 			status = "disabled";
 		};
+		usb_phy: usb-phy {
+			compatible = "ti,da830-usb-phy";
+			#phy-cells = <1>;
+			status = "disabled";
+		};
 		gpio: gpio@226000 {
 			compatible = "ti,dm6441-gpio";
 			gpio-controller;
-- 
1.9.1

[PATCH/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

While probing ochi phy with usb20 phy as a parent clock for usb11_phy,
the usb20_phy clock enable would time out. This is because the usb20
module clock needs to enabled while trying to lock the usb20_phy PLL.

Call clk enable and get for the usb20 peripheral before trying to
enable the phy PLL.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/mach-davinci/usb-da8xx.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index 9e41a7f..982e105 100644
--- a/arch/arm/mach-davinci/usb-da8xx.c
+++ b/arch/arm/mach-davinci/usb-da8xx.c
@@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate)
 
 static void usb20_phy_clk_enable(struct clk *clk)
 {
+	struct clk *usb20_clk;
 	u32 val;
 	u32 timeout = 500000; /* 500 msec */
 
 	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
 
+	usb20_clk = clk_get(NULL, "usb20");
+	if (IS_ERR(usb20_clk)) {
+		pr_err("could not get usb20 clk\n");
+		return;
+	}
+
+	clk_prepare_enable(usb20_clk);
 	/*
 	 * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
 	 * host may use the PLL clock without USB 2.0 OTG being used.
@@ -70,11 +78,14 @@ static void usb20_phy_clk_enable(struct clk *clk)
 	while (--timeout) {
 		val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
 		if (val & CFGCHIP2_PHYCLKGD)
-			return;
+			goto done;
 		udelay(1);
 	}
 
 	pr_err("Timeout waiting for USB 2.0 PHY clock good.\n");
+done:
+	clk_disable_unprepare(usb20_clk);
+	clk_put(usb20_clk);
 }
 
 static void usb20_phy_clk_disable(struct clk *clk)
-- 
1.9.1

Re: [PATCH/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable

[David Lechner]

On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
>
> While probing ochi phy with usb20 phy as a parent clock for usb11_phy,
> the usb20_phy clock enable would time out. This is because the usb20
> module clock needs to enabled while trying to lock the usb20_phy PLL.
>
> Call clk enable and get for the usb20 peripheral before trying to
> enable the phy PLL.
>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---


This patch can be combined with "ARM: davinci: da8xx: add usb phy 
clocks" since that patch has not been merged yet.

If you like, I can resubmit my patches from this series along with the 
changes from this patch.

Re: [PATCH/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable

[Axel Haslam]

On Tue, Oct 25, 2016 at 4:53 AM, David Lechner <david@lechnology.com> wrote:
> On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
>>
>> From: Axel Haslam <ahaslam@baylibre.com>
>>
>> While probing ochi phy with usb20 phy as a parent clock for usb11_phy,
>> the usb20_phy clock enable would time out. This is because the usb20
>> module clock needs to enabled while trying to lock the usb20_phy PLL.
>>
>> Call clk enable and get for the usb20 peripheral before trying to
>> enable the phy PLL.
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>
>
>
> This patch can be combined with "ARM: davinci: da8xx: add usb phy clocks"
> since that patch has not been merged yet.

yes, agree, these should be merged.

>
> If you like, I can resubmit my patches from this series along with the
> changes from this patch.

Ok, if you can resubmit those patches with this change  included
i can reference them and start making the series shorter.
i will also submit in separate patches the regulator changes, as requested
by Mark.



Regards
Axel.

>

Re: [PATCH/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable

[Sekhar Nori]

On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
> diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
> index 9e41a7f..982e105 100644
> --- a/arch/arm/mach-davinci/usb-da8xx.c
> +++ b/arch/arm/mach-davinci/usb-da8xx.c
> @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate)
>  
>  static void usb20_phy_clk_enable(struct clk *clk)
>  {
> +	struct clk *usb20_clk;
>  	u32 val;
>  	u32 timeout = 500000; /* 500 msec */
>  
>  	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
>  
> +	usb20_clk = clk_get(NULL, "usb20");

We should not be using a NULL device pointer here. Can you pass the musb
device pointer available in the same file? Also, da850_clks[] in da850.c
needs to be fixed to add the matching device name.

> +	if (IS_ERR(usb20_clk)) {
> +		pr_err("could not get usb20 clk\n");
> +		return;
> +	}

Thanks,
Sekhar

Re: [PATCH/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable

[David Lechner]

On 10/25/2016 05:12 AM, Sekhar Nori wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
>> index 9e41a7f..982e105 100644
>> --- a/arch/arm/mach-davinci/usb-da8xx.c
>> +++ b/arch/arm/mach-davinci/usb-da8xx.c
>> @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate)
>>
>>  static void usb20_phy_clk_enable(struct clk *clk)
>>  {
>> +	struct clk *usb20_clk;
>>  	u32 val;
>>  	u32 timeout = 500000; /* 500 msec */
>>
>>  	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
>>
>> +	usb20_clk = clk_get(NULL, "usb20");
>
> We should not be using a NULL device pointer here. Can you pass the musb
> device pointer available in the same file? Also, da850_clks[] in da850.c
> needs to be fixed to add the matching device name.

This clock can be used for usb 1.1 PHY even when musb is not being used, 
so I don't think we can depend on having a musb device here.

Also, in a previous review, it was decided that the usb clocks should 
*not* be added to da850_clks[] [1]. Instead, they are dynamically 
registered elsewhere.


[1]: http://www.gossamer-threads.com/lists/linux/kernel/2396533

>
>> +	if (IS_ERR(usb20_clk)) {
>> +		pr_err("could not get usb20 clk\n");
>> +		return;
>> +	}
>
> Thanks,
> Sekhar
>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable

[Sekhar Nori]

On Tuesday 25 October 2016 09:35 PM, David Lechner wrote:
> On 10/25/2016 05:12 AM, Sekhar Nori wrote:
>> On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
>>> diff --git a/arch/arm/mach-davinci/usb-da8xx.c
>>> b/arch/arm/mach-davinci/usb-da8xx.c
>>> index 9e41a7f..982e105 100644
>>> --- a/arch/arm/mach-davinci/usb-da8xx.c
>>> +++ b/arch/arm/mach-davinci/usb-da8xx.c
>>> @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate)
>>>
>>>  static void usb20_phy_clk_enable(struct clk *clk)
>>>  {
>>> +    struct clk *usb20_clk;
>>>      u32 val;
>>>      u32 timeout = 500000; /* 500 msec */
>>>
>>>      val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
>>>
>>> +    usb20_clk = clk_get(NULL, "usb20");
>>
>> We should not be using a NULL device pointer here. Can you pass the musb
>> device pointer available in the same file? Also, da850_clks[] in da850.c
>> needs to be fixed to add the matching device name.
> 
> This clock can be used for usb 1.1 PHY even when musb is not being used,
> so I don't think we can depend on having a musb device here.

Replied to this against the same question in v6 patch series you posted.

> Also, in a previous review, it was decided that the usb clocks should
> *not* be added to da850_clks[] [1]. Instead, they are dynamically
> registered elsewhere.

Thats only the USB phy clocks since there is associated
enable()/disable()/set_parent() code which is better kept outside of
da850.c for readability. No other reason. Lookup for the USB 2.0 module
clock is already present in da850.c

Thanks,
Sekhar

[PATCH/RFT v2 08/17] ARM: davinci: hawk: add full constraints for ohci plat boot

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

The phy framework requests an optional "phy" regulator. If it does
not find one, it returns -EPROBE_DEFER. In the case of non-DT based boot
for the omap138-lcdk board, this would prevent the usb11 phy to probe
correctly and ohci would not enumerate.

By calling "regulator_has_full_constraints", An error would be returned
instead of DEFER for the "optional" regulator, and the probe of
the phy driver can continue normally without a regulator.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/mach-davinci/board-da830-evm.c     | 3 +++
 arch/arm/mach-davinci/board-omapl138-hawk.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index c62766e..b33fc6b 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -26,6 +26,7 @@
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/mtd-davinci-aemif.h>
 #include <linux/platform_data/spi-davinci.h>
+#include <linux/regulator/machine.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -631,6 +632,8 @@ static __init void da830_evm_init(void)
 	ret = da8xx_register_spi_bus(0, ARRAY_SIZE(da830evm_spi_info));
 	if (ret)
 		pr_warn("%s: spi 0 registration failed: %d\n", __func__, ret);
+
+	regulator_has_full_constraints();
 }
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index c5cb8d9..c3ab7ea 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -14,6 +14,7 @@
 #include <linux/console.h>
 #include <linux/gpio.h>
 #include <linux/platform_data/gpio-davinci.h>
+#include <linux/regulator/machine.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -328,6 +329,8 @@ static __init void omapl138_hawk_init(void)
 	if (ret)
 		pr_warn("%s: dsp/rproc registration failed: %d\n",
 			__func__, ret);
+
+	regulator_has_full_constraints();
 }
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
-- 
1.9.1

Re: [PATCH/RFT v2 08/17] ARM: davinci: hawk: add full constraints for ohci plat boot

[Sekhar Nori]

On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 
> The phy framework requests an optional "phy" regulator. If it does
> not find one, it returns -EPROBE_DEFER. In the case of non-DT based boot
> for the omap138-lcdk board, this would prevent the usb11 phy to probe
> correctly and ohci would not enumerate.
> 
> By calling "regulator_has_full_constraints", An error would be returned

nit: prefer regulator_has_full_constraints()

> instead of DEFER for the "optional" regulator, and the probe of
> the phy driver can continue normally without a regulator.
> 
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Looks good to me. Just drop the "hawk: from subject line since you also
touch da830 evm. I am not sure what "ohci plat boot" means. How about
the following:

"ARM: davinci: da8xx: fix OHCI PHY probe for non-DT boot"

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 08/17] ARM: davinci: hawk: add full constraints for ohci plat boot

[Axel Haslam]

On Tue, Oct 25, 2016 at 12:28 PM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> The phy framework requests an optional "phy" regulator. If it does
>> not find one, it returns -EPROBE_DEFER. In the case of non-DT based boot
>> for the omap138-lcdk board, this would prevent the usb11 phy to probe
>> correctly and ohci would not enumerate.
>>
>> By calling "regulator_has_full_constraints", An error would be returned
>
> nit: prefer regulator_has_full_constraints()
>
>> instead of DEFER for the "optional" regulator, and the probe of
>> the phy driver can continue normally without a regulator.
>>
>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> Looks good to me. Just drop the "hawk: from subject line since you also
> touch da830 evm. I am not sure what "ohci plat boot" means. How about
> the following:
>
> "ARM: davinci: da8xx: fix OHCI PHY probe for non-DT boot"
>

Will do.

Thanks
Axel.

> Thanks,
> Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFT v2 17/17] ARM: dts: da850: add usb device node

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

This adds the usb (ohci) device node for the da850 soc.
Also it enables it for the lcdk board

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
 arch/arm/boot/dts/da850.dtsi     | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index 7b8ab21..fa91339 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -86,6 +86,14 @@
 	};
 };
 
+&usb_phy {
+	status = "okay";
+};
+
+&usb {
+	status = "okay";
+};
+
 &serial2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&serial2_rxtx_pins>;
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 33fcdce..ec2cec3 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -381,6 +381,14 @@
 			#phy-cells = <1>;
 			status = "disabled";
 		};
+		usb: usb@0225000 {
+			compatible = "ti,da830-ohci";
+			reg = <0x225000 0x1000>;
+			interrupts = <59>;
+			phys = <&usb_phy 1>;
+			phy-names = "usb-phy";
+			status = "disabled";
+		};
 		gpio: gpio@226000 {
 			compatible = "ti,dm6441-gpio";
 			gpio-controller;
-- 
1.9.1

Re: [PATCH/RFT v2 17/17] ARM: dts: da850: add usb device node

[David Lechner]

On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> This adds the usb (ohci) device node for the da850 soc.
> Also it enables it for the lcdk board
>
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
>  arch/arm/boot/dts/da850.dtsi     | 8 ++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
> index 7b8ab21..fa91339 100644
> --- a/arch/arm/boot/dts/da850-lcdk.dts
> +++ b/arch/arm/boot/dts/da850-lcdk.dts
> @@ -86,6 +86,14 @@
>  	};
>  };
>
> +&usb_phy {
> +	status = "okay";
> +};
> +
> +&usb {
> +	status = "okay";

Don't you need to specify a regulator here using the vbus-supply property?

> +};
> +
>  &serial2 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&serial2_rxtx_pins>;
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 33fcdce..ec2cec3 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -381,6 +381,14 @@
>  			#phy-cells = <1>;
>  			status = "disabled";
>  		};
> +		usb: usb@0225000 {

Don't need the leading 0 on usb@225000

The alias (usb:) might need to be more specific since there is a second 
usb device that will be added later for musb. (The comments in the 
previous review only referred to the "usb" in "usb@", not the alias.)

> +			compatible = "ti,da830-ohci";
> +			reg = <0x225000 0x1000>;
> +			interrupts = <59>;
> +			phys = <&usb_phy 1>;
> +			phy-names = "usb-phy";
> +			status = "disabled";
> +		};
>  		gpio: gpio@226000 {
>  			compatible = "ti,dm6441-gpio";
>  			gpio-controller;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFT v2 17/17] ARM: dts: da850: add usb device node

[Axel Haslam]

On Tue, Oct 25, 2016 at 2:48 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>
>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> This adds the usb (ohci) device node for the da850 soc.
>> Also it enables it for the lcdk board
>>
>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
>>  arch/arm/boot/dts/da850.dtsi     | 8 ++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850-lcdk.dts
>> b/arch/arm/boot/dts/da850-lcdk.dts
>> index 7b8ab21..fa91339 100644
>> --- a/arch/arm/boot/dts/da850-lcdk.dts
>> +++ b/arch/arm/boot/dts/da850-lcdk.dts
>> @@ -86,6 +86,14 @@
>>         };
>>  };
>>
>> +&usb_phy {
>> +       status = "okay";
>> +};
>> +
>> +&usb {
>> +       status = "okay";
>
>
> Don't you need to specify a regulator here using the vbus-supply property?

its is not mandatory, the regulator framework is giving a dummy supply
on regulator_get if it does not find a regulator on device tree. That is
what i could understand from: _regulator_get in regulator/core.c

Also, in the case of platform based boot, if the board init calls
regulator_has_full_constraints which is anyways needed for the phy to probe
correctly it will also return a dummy regulator.


>
>> +};
>> +
>>  &serial2 {
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&serial2_rxtx_pins>;
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index 33fcdce..ec2cec3 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -381,6 +381,14 @@
>>                         #phy-cells = <1>;
>>                         status = "disabled";
>>                 };
>> +               usb: usb@0225000 {
>
>
> Don't need the leading 0 on usb@225000
>
> The alias (usb:) might need to be more specific since there is a second usb
> device that will be added later for musb. (The comments in the previous
> review only referred to the "usb" in "usb@", not the alias.)

ok.

>
>> +                       compatible = "ti,da830-ohci";
>> +                       reg = <0x225000 0x1000>;
>> +                       interrupts = <59>;
>> +                       phys = <&usb_phy 1>;
>> +                       phy-names = "usb-phy";
>> +                       status = "disabled";
>> +               };
>>                 gpio: gpio@226000 {
>>                         compatible = "ti,dm6441-gpio";
>>                         gpio-controller;
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html