am33x: multiple instances and dma support, v2

If the init / shutdown function of the phy moves out of dsps into the phy driver, then dsps needs to call the callbacks of the phy driver to ensure that this happens.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

The am335x PHY code is well hidden in multiple places. The glue layer uses the nop and does up/down in the background. This patch copies the power on / power off part out of dsps so it can be removed later in dsps. At this point I am not sure if it is better to write a new phy driver for am335x or just add the missing pieces to this one.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

On Mon, Jul 22, 2013 at 08:09:53PM +0200, Sebastian Andrzej Siewior wrote:
right, I replied to the wrong series, please add a patch renaming file and functions to phy-generic.c/usb_phy_gen_

or something similar.

Quoted 3 lines · * Felipe Balbi | 2013-07-25 17:33:30 [+0300]:

On Mon, Jul 22, 2013 at 08:09:53PM +0200, Sebastian Andrzej Siewior wrote:
right, I replied to the wrong series, please add a patch renaming file and functions to phy-generic.c/usb_phy_gen_

Sorry, I can't parse that.
You want to rename phy-nop.c to phy-generic.c and add the additional functions in usb_phy_gen.c?

This moves the two instances from the big node into two child nodes. The glue layer ontop does almost nothing.
There is one devices containing the (2) phy, (2) usb and later the dma engine. The usb device is the "glue device" which contains the musb device as a child. This is what we do ever since.
The new file musb_am335x is just here to prob the new bus and populate child devices.
There are a lot of changes to the dsps file as a result of the changes:
- musb_core_offset
This is gone. The device tree provides memory ressources information for the device there is no need to "fix" things
- instances
This is gone as well. If we have two instances then we have have two child enabled nodes in the device tree. For instance the SoC in beagle bone has two USB instances but only one has been wired up so there is no need to load and init the second instance since it won't be used.
- dsps_glue is now per glue device
In the past there was one of this structs but with an array of two and each instance accessed its variable depending on the platform device id.
- no unneeded copy of structs
I do not know why struct dsps_musb_wrapper is copied but it is not necessary. The same goes for musb_hdrc_platform_data which allocated on demand and then again by platform_device_add_data(). One copy is enough.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
  arch/arm/boot/dts/am335x-bone.dts  |  12 ++
  arch/arm/boot/dts/am335x-evm.dts   |  20 +++
  arch/arm/boot/dts/am335x-evmsk.dts |  12 ++
  arch/arm/boot/dts/am33xx.dtsi      |  89 +++++++++++--
  drivers/usb/musb/Kconfig           |   4 +
  drivers/usb/musb/Makefile          |   3 +
  drivers/usb/musb/musb_am335x.c     |  55 ++++++++
  drivers/usb/musb/musb_dsps.c       | 255 ++++++++++++++-----------------------
  8 files changed, 279 insertions(+), 171 deletions(-)

create mode 100644 drivers/usb/musb/musb_am335x.c

arch/arm/boot/dts/am335x-bone.dts +12 -0

index 444b4ed..2c2ac84 100644
@@ -120,6 +120,18 @@
  			status = "okay";
  		};
 
+		musb: usb@47400000 {
+			status = "okay";
+
+			phy@47401300 {
+				status = "okay";
+			};
+
+			usb@47401000 {
+				status = "okay";
+			};
+		};
+
  		i2c0: i2c@44e0b000 {
  			pinctrl-names = "default";
  			pinctrl-0 = <&i2c0_pins>;

arch/arm/boot/dts/am335x-evm.dts +4 -0

index 3aee1a4..a3a642a 100644
@@ -171,6 +171,26 @@
  			};
  		};
 
+		musb: usb@47400000 {
+			status = "okay";
+
+			phy@47401300 {

arch/arm/boot/dts/am335x-evm.dts +6 -0

+
+			usb@47401800 {
+				status = "okay";
+			};
+		};
+
  		i2c1: i2c@4802a000 {
  			pinctrl-names = "default";
  			pinctrl-0 = <&i2c1_pins>;

arch/arm/boot/dts/am335x-evmsk.dts +12 -0

index 0c8ad17..766c23a 100644
@@ -207,6 +207,18 @@
  			};
  		};
 
+		musb: usb@47400000 {
+			status = "okay";
+
+			phy@47401300 {
+				status = "okay";
+			};
+
+			usb@47401000 {
+				status = "okay";
+			};
+		};
+
  		epwmss2: epwmss@48304000 {
  			status = "okay";
 

arch/arm/boot/dts/am33xx.dtsi +75 -14

index 38b446b..81afb27 100644
@@ -26,6 +26,10 @@
  		serial5 = &uart5;
  		d_can0 = &dcan0;
  		d_can1 = &dcan1;
+		usb0 = &usb0;
+		usb1 = &usb1;
+		phy0 = &usb0_phy;
+		phy1 = &usb1_phy;
  	};
 
  	cpus {

@@ -333,21 +337,78 @@
  			status = "disabled";
  		};
 
-		usb@47400000 {
-			compatible = "ti,musb-am33xx";
-			reg = <0x47400000 0x1000	/* usbss */
-			       0x47401000 0x800		/* musb instance 0 */
-			       0x47401800 0x800>;	/* musb instance 1 */
-			interrupts = <17		/* usbss */
-				      18		/* musb instance 0 */
-				      19>;		/* musb instance 1 */
-			multipoint = <1>;
-			num-eps = <16>;
-			ram-bits = <12>;
-			port0-mode = <3>;
-			port1-mode = <3>;
-			power = <250>;
+		usb: usb@47400000 {
+			compatible = "ti,am33xx-usb";
+			reg = <0x47400000 0x1000>;
+			ranges;
+			#address-cells = <1>;
+			#size-cells = <1>;
  			ti,hwmods = "usb_otg_hs";
+			status = "disabled";
+
+			usb0_phy: phy@47401300 {
+				compatible = "ti,am335x-usb-phy";
+				reg = <0x44e10620 0x8
+					0x47401300 0x100>;
+				reg-names = "reset_mod", "phy";
+				status = "disabled";
+			};
+
+			usb0: usb@47401000 {
+				compatible = "ti,musb-am33xx";
+				ranges;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = <0x47401000 0x200>;
+				reg-names = "control";
+				status = "disabled";
+
+				musb0: usb@47401400 {
+					compatible = "mg,musbmhdrc";
+					reg = <0x47401400 0x400>;
+					reg-names = "mc";
+					interrupts = <18>;
+					interrupt-names = "mc";
+					multipoint = <1>;
+					num-eps = <16>;
+					ram-bits = <12>;
+					port-mode = <3>;
+					power = <250>;
+					phys = <&usb0_phy>;
+				};
+			};
+
+			usb1_phy: phy@47401b00 {
+				compatible = "ti,am335x-usb-phy";
+				reg = <0x44e10628 0x8
+					0x47401b00 0x100>;
+				reg-names = "reset_mod", "phy";
+				status = "disabled";
+			};
+
+			usb1: usb@47401800 {
+				compatible = "ti,musb-am33xx";
+				ranges;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = <0x47401800 0x200>;
+				reg-names = "control";
+				status = "disabled";
+
+				musb1: usb@47401c00 {
+					compatible = "mg,musbmhdrc";
+					reg = <0x47401c00 0x400>;
+					reg-names = "mc";
+					interrupts = <19>;
+					interrupt-names = "mc";
+					multipoint = <1>;
+					num-eps = <16>;
+					ram-bits = <12>;
+					port-mode = <3>;
+					power = <250>;
+					phys = <&usb1_phy>;
+				};
+			};
  		};
 
  		epwmss0: epwmss@48300000 {

drivers/usb/musb/Kconfig +4 -0

index 797e3fd..b7257ae 100644
@@ -83,6 +83,7 @@ config USB_MUSB_AM35X
 
  config USB_MUSB_DSPS
  	tristate "TI DSPS platforms"
+	select USB_MUSB_AM335X_CHILD
 
  config USB_MUSB_BLACKFIN
  	tristate "Blackfin"

@@ -93,6 +94,9 @@ config USB_MUSB_UX500
 
  endchoice
 
+config USB_MUSB_AM335X_CHILD
+	tristate
+
  choice
  	prompt 'MUSB DMA mode'
  	default MUSB_PIO_ONLY if ARCH_MULTIPLATFORM

drivers/usb/musb/Makefile +3 -0

index 2b82ed7..52f552c 100644
@@ -20,6 +20,9 @@ obj-$(CONFIG_USB_MUSB_DA8XX)			+= da8xx.o
  obj-$(CONFIG_USB_MUSB_BLACKFIN)			+= blackfin.o
  obj-$(CONFIG_USB_MUSB_UX500)			+= ux500.o
 
+
+obj-$(CONFIG_USB_MUSB_AM335X_CHILD)		+= musb_am335x.o
+
  # the kconfig must guarantee that only one of the
  # possible I/O schemes will be enabled at a time ...
  # PIO only, or DMA (several potential schemes).

drivers/usb/musb/musb_am335x.c new file +55 -0

new file mode 100644
index 0000000..41ac5b5
@@ -0,0 +1,55 @@
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+
+static int am335x_child_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
+}
+
+static int of_remove_populated_child(struct device *dev, void *d)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	of_device_unregister(pdev);
+	return 0;
+}
+
+static int am335x_child_remove(struct platform_device *pdev)
+{
+	device_for_each_child(&pdev->dev, NULL, of_remove_populated_child);
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
+static const struct of_device_id am335x_child_of_match[] = {
+	{ .compatible = "ti,am33xx-usb" },
+	{  },
+};
+MODULE_DEVICE_TABLE(of, am335x_child_of_match);
+
+static struct platform_driver am335x_child_driver = {
+	.probe		= am335x_child_probe,
+	.remove         = am335x_child_remove,
+	.driver         = {
+		.name   = "am335x-usb-childs",
+		.of_match_table	= of_match_ptr(am335x_child_of_match),
+	},
+};
+
+module_platform_driver(am335x_child_driver);
+MODULE_DESCRIPTION("AM33xx child devices");
+MODULE_LICENSE("GPL v2");

drivers/usb/musb/musb_dsps.c +98 -157

index 1440fac..37ebcbc 100644
@@ -43,6 +43,7 @@
  #include <linux/of.h>
  #include <linux/of_device.h>
  #include <linux/of_address.h>
+#include <linux/of_irq.h>
 
  #include "musb_core.h"
 

@@ -106,10 +107,7 @@ struct dsps_musb_wrapper {
  	/* bit positions for mode */
  	unsigned	iddig:5;
  	/* miscellaneous stuff */
-	u32		musb_core_offset;
  	u8		poll_seconds;
-	/* number of musb instances */
-	u8		instances;
  };
 
  /**

@@ -117,10 +115,10 @@ struct dsps_musb_wrapper {
   */
  struct dsps_glue {
  	struct device *dev;
-	struct platform_device *musb[2];	/* child musb pdev */
+	struct platform_device *musb;	/* child musb pdev */
  	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
-	struct timer_list timer[2];	/* otg_workaround timer */
-	unsigned long last_timer[2];    /* last timer data for each instance */
+	struct timer_list timer;	/* otg_workaround timer */
+	unsigned long last_timer;    /* last timer data for each instance */
  };
 
  /**

@@ -170,7 +168,6 @@ static void otg_timer(unsigned long _musb)
  	struct musb *musb = (void *)_musb;
  	void __iomem *mregs = musb->mregs;
  	struct device *dev = musb->controller;
-	struct platform_device *pdev = to_platform_device(dev);
  	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
  	const struct dsps_musb_wrapper *wrp = glue->wrp;
  	u8 devctl;

@@ -207,7 +204,7 @@ static void otg_timer(unsigned long _musb)
  	case OTG_STATE_B_IDLE:
  		devctl = dsps_readb(mregs, MUSB_DEVCTL);
  		if (devctl & MUSB_DEVCTL_BDEVICE)
-			mod_timer(&glue->timer[pdev->id],
+			mod_timer(&glue->timer,
  					jiffies + wrp->poll_seconds * HZ);
  		else
  			musb->xceiv->state = OTG_STATE_A_IDLE;

@@ -221,7 +218,6 @@ static void otg_timer(unsigned long _musb)
  static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
  {
  	struct device *dev = musb->controller;
-	struct platform_device *pdev = to_platform_device(dev);
  	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
 
  	if (timeout == 0)

@@ -232,23 +228,23 @@ static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
  				musb->xceiv->state == OTG_STATE_A_WAIT_BCON)) {
  		dev_dbg(musb->controller, "%s active, deleting timer\n",
  				usb_otg_state_string(musb->xceiv->state));
-		del_timer(&glue->timer[pdev->id]);
-		glue->last_timer[pdev->id] = jiffies;
+		del_timer(&glue->timer);
+		glue->last_timer = jiffies;
  		return;
  	}
 
-	if (time_after(glue->last_timer[pdev->id], timeout) &&
-				timer_pending(&glue->timer[pdev->id])) {
+	if (time_after(glue->last_timer, timeout) &&
+				timer_pending(&glue->timer)) {
  		dev_dbg(musb->controller,
  			"Longer idle timer already pending, ignoring...\n");
  		return;
  	}
-	glue->last_timer[pdev->id] = timeout;
+	glue->last_timer = timeout;
 
  	dev_dbg(musb->controller, "%s inactive, starting idle timer for %u ms\n",
  		usb_otg_state_string(musb->xceiv->state),
  			jiffies_to_msecs(timeout - jiffies));
-	mod_timer(&glue->timer[pdev->id], timeout);
+	mod_timer(&glue->timer, timeout);
  }
 
  static irqreturn_t dsps_interrupt(int irq, void *hci)

@@ -256,7 +252,6 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
  	struct musb  *musb = hci;
  	void __iomem *reg_base = musb->ctrl_base;
  	struct device *dev = musb->controller;
-	struct platform_device *pdev = to_platform_device(dev);
  	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
  	const struct dsps_musb_wrapper *wrp = glue->wrp;
  	unsigned long flags;

@@ -316,7 +311,7 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
  			 */
  			musb->int_usb &= ~MUSB_INTR_VBUSERROR;
  			musb->xceiv->state = OTG_STATE_A_WAIT_VFALL;
-			mod_timer(&glue->timer[pdev->id],
+			mod_timer(&glue->timer,
  					jiffies + wrp->poll_seconds * HZ);
  			WARNING("VBUS error workaround (delay coming)\n");
  		} else if (drvvbus) {

@@ -324,7 +319,7 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
  			MUSB_HST_MODE(musb);
  			musb->xceiv->otg->default_a = 1;
  			musb->xceiv->state = OTG_STATE_A_WAIT_VRISE;
-			del_timer(&glue->timer[pdev->id]);
+			del_timer(&glue->timer);
  		} else {
  			musb->is_active = 0;
  			MUSB_DEV_MODE(musb);

@@ -351,8 +346,7 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
 
  	/* Poll for ID change */
  	if (musb->xceiv->state == OTG_STATE_B_IDLE)
-		mod_timer(&glue->timer[pdev->id],
-			 jiffies + wrp->poll_seconds * HZ);
+		mod_timer(&glue->timer, jiffies + wrp->poll_seconds * HZ);
 
  	spin_unlock_irqrestore(&musb->lock, flags);
 

@@ -362,31 +356,34 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
  static int dsps_musb_init(struct musb *musb)
  {
  	struct device *dev = musb->controller;
-	struct platform_device *pdev = to_platform_device(dev);
  	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
+	struct platform_device *parent = to_platform_device(dev->parent);
  	const struct dsps_musb_wrapper *wrp = glue->wrp;
-	void __iomem *reg_base = musb->ctrl_base;
+	void __iomem *reg_base;
+	struct resource *r;
  	u32 rev, val;
-	int status;
 
-	/* mentor core register starts at offset of 0x400 from musb base */
-	musb->mregs += wrp->musb_core_offset;
+	r = platform_get_resource_byname(parent, IORESOURCE_MEM, "control");
+	if (!r)
+		return -EINVAL;
+
+	reg_base = devm_ioremap_resource(dev, r);
+	if (!musb->ctrl_base)
+		return -EINVAL;
+	musb->ctrl_base = reg_base;
 
  	/* NOP driver needs change if supporting dual instance */
-	musb->xceiv = devm_usb_get_phy_by_phandle(glue->dev, "phys", 0);
-	if (IS_ERR_OR_NULL(musb->xceiv))
-		return -EPROBE_DEFER;
+	musb->xceiv = devm_usb_get_phy_by_phandle(dev, "phys", 0);
+	if (IS_ERR(musb->xceiv))
+		return PTR_ERR(musb->xceiv);
 
  	/* Returns zero if e.g. not clocked */
  	rev = dsps_readl(reg_base, wrp->revision);
-	if (!rev) {
-		status = -ENODEV;
-		goto err0;
-	}
+	if (!rev)
+		return -ENODEV;
 
  	usb_phy_init(musb->xceiv);
-
-	setup_timer(&glue->timer[pdev->id], otg_timer, (unsigned long) musb);
+	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
 
  	/* Reset the musb */
  	dsps_writel(reg_base, wrp->control, (1 << wrp->reset));

@@ -402,17 +399,14 @@ static int dsps_musb_init(struct musb *musb)
  	dsps_writel(reg_base, wrp->eoi, 0);
 
  	return 0;
-err0:
-	return status;
  }
 
  static int dsps_musb_exit(struct musb *musb)
  {
  	struct device *dev = musb->controller;
-	struct platform_device *pdev = to_platform_device(dev);
  	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
 
-	del_timer_sync(&glue->timer[pdev->id]);
+	del_timer_sync(&glue->timer);
 
  	usb_phy_shutdown(musb->xceiv);
  	return 0;

@@ -430,106 +424,98 @@ static struct musb_platform_ops dsps_ops = {
 
  static u64 musb_dmamask = DMA_BIT_MASK(32);
 
-static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)
+static int get_int_prop(struct device_node *dn, const char *s)
  {
-	struct device *dev = glue->dev;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct musb_hdrc_platform_data  *pdata = dev->platform_data;
-	struct device_node *np = pdev->dev.of_node;
-	struct musb_hdrc_config	*config;
-	struct platform_device	*musb;
-	struct resource *res;
+	int ret;
+	u32 val;
+
+	ret = of_property_read_u32(dn, s, &val);
+	if (ret)
+		return 0;
+	return val;
+}
+
+static int dsps_create_musb_pdev(struct dsps_glue *glue,
+		struct platform_device *parent)
+{
+	struct musb_hdrc_platform_data pdata;
  	struct resource	resources[2];
-	char res_name[11];
+	struct device *dev = &parent->dev;
+	struct musb_hdrc_config	*config;
+	struct platform_device *musb;
+	struct device_node *dn = parent->dev.of_node;
+	struct device_node *child_node;
  	int ret;
 
-	/* first resource is for usbss, so start index from 1 */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, id + 1);
-	if (!res) {
-		dev_err(dev, "failed to get memory for instance %d\n", id);
-		ret = -ENODEV;
-		goto err0;
+	child_node = of_get_child_by_name(dn, "usb");
+	if (!child_node)
+		return -EINVAL;
+
+	memset(resources, 0, sizeof(resources));
+	ret = of_address_to_resource(child_node, 0, &resources[0]);
+	if (ret) {
+		dev_err(dev, "failed to get memory.\n");
+		return ret;
  	}
-	res->parent = NULL;
-	resources[0] = *res;
-
-	/* first resource is for usbss, so start index from 1 */
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, id + 1);
-	if (!res) {
-		dev_err(dev, "failed to get irq for instance %d\n", id);
-		ret = -ENODEV;
-		goto err0;
+
+	ret = of_irq_to_resource(child_node, 0, &resources[1]);
+	if (ret == 0) {
+		dev_err(dev, "failed to get irq.\n");
+		ret = -EINVAL;
+		return ret;
  	}
-	res->parent = NULL;
-	resources[1] = *res;
-	resources[1].name = "mc";
 
  	/* allocate the child platform device */
  	musb = platform_device_alloc("musb-hdrc", PLATFORM_DEVID_AUTO);
  	if (!musb) {
  		dev_err(dev, "failed to allocate musb device\n");
-		ret = -ENOMEM;
-		goto err0;
+		return -ENOMEM;
  	}
 
  	musb->dev.parent		= dev;
  	musb->dev.dma_mask		= &musb_dmamask;
  	musb->dev.coherent_dma_mask	= musb_dmamask;
+	musb->dev.of_node		= of_node_get(child_node);
 
-	glue->musb[id]			= musb;
+	glue->musb = musb;
 
-	ret = platform_device_add_resources(musb, resources, 2);
+	ret = platform_device_add_resources(musb, resources,
+			ARRAY_SIZE(resources));
  	if (ret) {
  		dev_err(dev, "failed to add resources\n");
-		goto err2;
+		goto err;
  	}
 
-	if (np) {
-		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
-		if (!pdata) {
-			dev_err(&pdev->dev,
-				"failed to allocate musb platform data\n");
-			ret = -ENOMEM;
-			goto err2;
-		}
-
-		config = devm_kzalloc(&pdev->dev, sizeof(*config), GFP_KERNEL);
-		if (!config) {
-			dev_err(&pdev->dev,
-				"failed to allocate musb hdrc config\n");
-			ret = -ENOMEM;
-			goto err2;
-		}
-
-		of_property_read_u32(np, "num-eps", (u32 *)&config->num_eps);
-		of_property_read_u32(np, "ram-bits", (u32 *)&config->ram_bits);
-		snprintf(res_name, sizeof(res_name), "port%d-mode", id);
-		of_property_read_u32(np, res_name, (u32 *)&pdata->mode);
-		of_property_read_u32(np, "power", (u32 *)&pdata->power);
-		config->multipoint = of_property_read_bool(np, "multipoint");
-
-		pdata->config		= config;
+	config = devm_kzalloc(&parent->dev, sizeof(*config), GFP_KERNEL);
+	if (!config) {
+		dev_err(dev, "failed to allocate musb hdrc config\n");
+		ret = -ENOMEM;
+		goto err;
  	}
+	pdata.config = config;
+	pdata.platform_ops = &dsps_ops;
 
-	pdata->platform_ops		= &dsps_ops;
+	config->num_eps = get_int_prop(child_node, "num-eps");
+	config->ram_bits = get_int_prop(child_node, "ram-bits");
+	pdata.mode = get_int_prop(child_node, "port-mode");
+	pdata.power = get_int_prop(child_node, "power");
+	config->multipoint = of_property_read_bool(child_node, "multipoint");
 
-	ret = platform_device_add_data(musb, pdata, sizeof(*pdata));
+	ret = platform_device_add_data(musb, &pdata, sizeof(pdata));
  	if (ret) {
  		dev_err(dev, "failed to add platform_data\n");
-		goto err2;
+		goto err;
  	}
 
  	ret = platform_device_add(musb);
  	if (ret) {
  		dev_err(dev, "failed to register musb device\n");
-		goto err2;
+		goto err;
  	}
-
  	return 0;
 
-err2:
+err:
  	platform_device_put(musb);
-err0:
  	return ret;
  }
 

@@ -538,14 +524,12 @@ static int dsps_probe(struct platform_device *pdev)
  	const struct of_device_id *match;
  	const struct dsps_musb_wrapper *wrp;
  	struct dsps_glue *glue;
-	struct resource *iomem;
-	int ret, i;
+	int ret;
 
  	match = of_match_node(musb_dsps_of_match, pdev->dev.of_node);
  	if (!match) {
  		dev_err(&pdev->dev, "fail to get matching of_match struct\n");
-		ret = -EINVAL;
-		goto err0;
+		return -EINVAL;
  	}
  	wrp = match->data;
 

@@ -553,29 +537,13 @@ static int dsps_probe(struct platform_device *pdev)
  	glue = kzalloc(sizeof(*glue), GFP_KERNEL);
  	if (!glue) {
  		dev_err(&pdev->dev, "unable to allocate glue memory\n");
-		ret = -ENOMEM;
-		goto err0;
-	}
-
-	/* get memory resource */
-	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iomem) {
-		dev_err(&pdev->dev, "failed to get usbss mem resourse\n");
-		ret = -ENODEV;
-		goto err1;
+		return -ENOMEM;
  	}
 
  	glue->dev = &pdev->dev;
+	glue->wrp = wrp;
 
-	glue->wrp = kmemdup(wrp, sizeof(*wrp), GFP_KERNEL);
-	if (!glue->wrp) {
-		dev_err(&pdev->dev, "failed to duplicate wrapper struct memory\n");
-		ret = -ENOMEM;
-		goto err1;
-	}
  	platform_set_drvdata(pdev, glue);
-
-	/* enable the usbss clocks */
  	pm_runtime_enable(&pdev->dev);
 
  	ret = pm_runtime_get_sync(&pdev->dev);

@@ -584,17 +552,9 @@ static int dsps_probe(struct platform_device *pdev)
  		goto err2;
  	}
 
-	/* create the child platform device for all instances of musb */
-	for (i = 0; i < wrp->instances ; i++) {
-		ret = dsps_create_musb_pdev(glue, i);
-		if (ret != 0) {
-			dev_err(&pdev->dev, "failed to create child pdev\n");
-			/* release resources of previously created instances */
-			for (i--; i >= 0 ; i--)
-				platform_device_unregister(glue->musb[i]);
-			goto err3;
-		}
-	}
+	ret = dsps_create_musb_pdev(glue, pdev);
+	if (ret)
+		goto err3;
 
  	return 0;
 

@@ -602,26 +562,19 @@ static int dsps_probe(struct platform_device *pdev)
  	pm_runtime_put(&pdev->dev);
  err2:
  	pm_runtime_disable(&pdev->dev);
-	kfree(glue->wrp);
-err1:
  	kfree(glue);
-err0:
  	return ret;
  }
+
  static int dsps_remove(struct platform_device *pdev)
  {
  	struct dsps_glue *glue = platform_get_drvdata(pdev);
-	const struct dsps_musb_wrapper *wrp = glue->wrp;
-	int i;
 
-	/* delete the child platform device */
-	for (i = 0; i < wrp->instances ; i++)
-		platform_device_unregister(glue->musb[i]);
+	platform_device_unregister(glue->musb);
 
  	/* disable usbss clocks */
  	pm_runtime_put(&pdev->dev);
  	pm_runtime_disable(&pdev->dev);
-	kfree(glue->wrp);
  	kfree(glue);
  	return 0;
  }

@@ -652,9 +605,7 @@ static const struct dsps_musb_wrapper ti81xx_driver_data = {
  	.rxep_shift		= 16,
  	.rxep_mask		= 0xfffe,
  	.rxep_bitmap		= (0xfffe << 16),
-	.musb_core_offset	= 0x400,
  	.poll_seconds		= 2,
-	.instances		= 1,
  };
 
  static const struct of_device_id musb_dsps_of_match[] = {

@@ -678,14 +629,4 @@ MODULE_AUTHOR("Ravi B <[email protected]>");
  MODULE_AUTHOR("Ajay Kumar Gupta <[email protected]>");
  MODULE_LICENSE("GPL v2");
 
-static int __init dsps_init(void)
-{
-	return platform_driver_register(&dsps_usbss_driver);
-}
-subsys_initcall(dsps_init);
-
-static void __exit dsps_exit(void)
-{
-	platform_driver_unregister(&dsps_usbss_driver);
-}
-module_exit(dsps_exit);
+module_platform_driver(dsps_usbss_driver);

Quoted 15 lines

arch/arm/boot/dts/am335x-evm.dts +6 -0

index 3aee1a4..a3a642a 100644
@@ -171,6 +171,26 @@
              };
          };
  +        musb: usb@47400000 {
+            status = "okay";
+
+            phy@47401300 {
+                status = "okay";
+            };
+

arch/arm/boot/dts/am335x-evm.dts line 187

Quoted 7 lines

arch/arm/boot/dts/am335x-evm.dts +7 -0

+            phy@47401b00 {
+                status = "okay";
+            };
+
+            usb@47401000 {
+                status = "no";
+            };

Any reason usb0 is disabled for am33xx evm? Just for testing?

This patch renames the type struct from ti81xx_driver_data to am33xx_driver_data since it is not used for ti81xx anymore. The EOI member is also removed since the am33xx SoC does not have such register. The interrupt is acknowledged by writting into the stat register.

Hi Sebastian,

Quoted 5 lines · Sebastian Andrzej Siewior

This patch renames the type struct from ti81xx_driver_data to am33xx_driver_data since it is not used for ti81xx anymore. The EOI member is also removed since the am33xx SoC does not have such register. The interrupt is acknowledged by writting into the stat register.

AM335X TRM Section 16.6.5 and 16.7.5 describes about EOI registers. Its at offset 0x24. Or is it that the interrupts are acknowledged even without writing to eoi register?

Quoted 10 lines · George Cherian

Hi Sebastian,

Quoted 5 lines · Sebastian Andrzej Siewior

This patch renames the type struct from ti81xx_driver_data to am33xx_driver_data since it is not used for ti81xx anymore. The EOI member is also removed since the am33xx SoC does not have such register. The interrupt is acknowledged by writting into the stat register.

Quoted 3 lines

AM335X TRM Section 16.6.5 and 16.7.5 describes about EOI registers. Its at offset 0x24. Or is it that the interrupts are acknowledged even without writing to eoi register?

I have here "Literature Number: SPRUH73H October 2011 ­ Revised April 2013" which calls itself "AM335x ARM® CortexTM-A8 Microprocessors (MPUs) Technical Reference Manual".

This document ends with 16.5 that means there is no chapter 16.6 or 16.7. The next one is 17.
16.5.2 and 16.5.3 describes the available registers of USB[01]_CTRL REGISTERS which is where the EOI register is accessed. I see here

20h USB0IRQMSTAT
28h USB0IRQSTATRAW0

so no 0x24 at least in my document.

Sebastian

Hi Sebastian,

Quoted 5 lines · Sebastian Andrzej Siewior

This patch renames the type struct from ti81xx_driver_data to am33xx_driver_data since it is not used for ti81xx anymore. The EOI member is also removed since the am33xx SoC does not have such register. The interrupt is acknowledged by writting into the stat register.

I guess the EOI register is removed from the TRM because AM33xx does not use it, there is no need to write to it to acknowledge. It does not hurt to write to it though since the register still exists, it just does nothing, I guess.

But I am not sure if it is a good idea to remove eoi from the musb_dsps driver. If the intension is to merge the support for other SoC, such as AM35xx, AM18xx, then EOI handling might be still needed. I just don't know how those devices use EOI.

Regards,
-Bin.

Hi Sebastian,

Quoted 5 lines · Sebastian Andrzej Siewior

This patch renames the type struct from ti81xx_driver_data to am33xx_driver_data since it is not used for ti81xx anymore. The EOI member is also removed since the am33xx SoC does not have such register. The interrupt is acknowledged by writting into the stat register.

I guess the EOI register is removed from the TRM because AM33xx does not use it, there is no need to write to it to acknowledge. It does not hurt to write to it though since the register still exists, it just does nothing, I guess.

But I am not sure if it is a good idea to remove eoi from the musb_dsps driver. If the intension is to merge the support for other SoC, such as AM35xx, AM18xx, then EOI handling might be still needed. I just don't know how those devices use EOI.

Regards,
-Bin.

Quoted 1 line · * Bin Liu | 2013-07-23 13:23:57 [-0500]:

Hi Sebastian,

Hi Liu,

Quoted 8 lines

Quoted 5 lines · Sebastian Andrzej Siewior

This patch renames the type struct from ti81xx_driver_data to am33xx_driver_data since it is not used for ti81xx anymore. The EOI member is also removed since the am33xx SoC does not have such register. The interrupt is acknowledged by writting into the stat register.

Quoted 4 lines

I guess the EOI register is removed from the TRM because AM33xx does not use it, there is no need to write to it to acknowledge. It does not hurt to write to it though since the register still exists, it just does nothing, I guess.

Is it really there or was it never there and it has been added to TRM by accident?

Quoted 5 lines

But I am not sure if it is a good idea to remove eoi from the musb_dsps driver. If the intension is to merge the support for other SoC, such as AM35xx, AM18xx, then EOI handling might be still needed. I just don't know how those devices use EOI.

If one of the architectures gets added which need an EOI then the offset can be 0 and the EOI will happen only if it is != 0.

Quoted 4 lines

Regards,
-Bin.

Sebastian

Sebastian,

Quoted 2 lines

Is it really there or was it never there and it has been added to TRM by accident?

The EOI register IS in the USB subsystem of AM33xx, but the SoC does not use it because it uses level triggering for USB interrupt.

Quoted 7 lines

Quoted 4 lines

But I am not sure if it is a good idea to remove eoi from the musb_dsps driver. If the intension is to merge the support for other SoC, such as AM35xx, AM18xx, then EOI handling might be still needed. I just don't know how those devices use EOI.

If one of the architectures gets added which need an EOI then the offset can be 0 and the EOI will happen only if it is != 0.

This patch cleaned up the use of EOI. Do you mean EOI handling will be added back with condition EOI offset != 0, when the support of new device which uses EIO is added?

Regards,
-Bin.

Quoted 1 line · Bin Liu

Sebastian,

Hi Bin,

Quoted 4 lines

Quoted 2 lines

Is it really there or was it never there and it has been added to TRM by accident?

The EOI register IS in the USB subsystem of AM33xx, but the SoC does not use it because it uses level triggering for USB interrupt.

I see.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

But I am not sure if it is a good idea to remove eoi from the musb_dsps driver. If the intension is to merge the support for other SoC, such as AM35xx, AM18xx, then EOI handling might be still needed. I just don't know how those devices use EOI.

Quoted 5 lines

Quoted 2 lines

If one of the architectures gets added which need an EOI then the offset can be 0 and the EOI will happen only if it is != 0.

This patch cleaned up the use of EOI. Do you mean EOI handling will be added back with condition EOI offset != 0, when the support of new device which uses EIO is added?

That is my intention.

Quoted 3 lines

Regards,
-Bin.

Sebastian

Sebastian,

Quoted 23 lines · Sebastian Andrzej Siewior

Quoted 1 line · Bin Liu

Sebastian,

Hi Bin,

Quoted 4 lines

Quoted 2 lines

Is it really there or was it never there and it has been added to TRM by accident?

The EOI register IS in the USB subsystem of AM33xx, but the SoC does not use it because it uses level triggering for USB interrupt.

I see.

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

But I am not sure if it is a good idea to remove eoi from the musb_dsps driver. If the intension is to merge the support for other SoC, such as AM35xx, AM18xx, then EOI handling might be still needed. I just don't know how those devices use EOI.

If one of the architectures gets added which need an EOI then the offset can be 0 and the EOI will happen only if it is != 0.

This patch cleaned up the use of EOI. Do you mean EOI handling will be added back with condition EOI offset != 0, when the support of new device which uses EIO is added?

Quoted 1 line

That is my intention.

Then should something like EOI cleanup be added into the commit message for better git log searching experience? I would think the EOI cleanup is more important then variable renaming in this patch. Or even better to separate the changes into two patches.

Quoted 1 line · Bin Liu

Sebastian,

Hi Bin,

Quoted 4 lines

Quoted 2 lines

Is it really there or was it never there and it has been added to TRM by accident?

The EOI register IS in the USB subsystem of AM33xx, but the SoC does not use it because it uses level triggering for USB interrupt.

I see.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

But I am not sure if it is a good idea to remove eoi from the musb_dsps driver. If the intension is to merge the support for other SoC, such as AM35xx, AM18xx, then EOI handling might be still needed. I just don't know how those devices use EOI.

Quoted 5 lines

Quoted 2 lines

If one of the architectures gets added which need an EOI then the offset can be 0 and the EOI will happen only if it is != 0.

This patch cleaned up the use of EOI. Do you mean EOI handling will be added back with condition EOI offset != 0, when the support of new device which uses EIO is added?

That is my intention.

Hi,

On Mon, Jul 22, 2013 at 1:10 PM, Sebastian Andrzej Siewior <[email protected] <mailto:[email protected]>> wrote:

This patch adds the MUSB_DEVCTL_SESSION back after it has been removed. If it is missing then the host session is not recognized. This bit is added initially added in musb_start() and removed after the first device disconnect.

AFAIK, after the device is disconnected, the OTG state machine will go back to B_IDLE/A_IDLE state. SESSION is not needed in this case.

In OTG mode, when no device is plugged, the ID pin is floating, you can never hold the host session in this case, even set the SESSION bit. The SESSION bit will be cleared by the controller after 100ms.

Regards,
-Bin.

Quoted 1 line · Bin Liu

Hi,

Hi,

Quoted 11 lines

On Mon, Jul 22, 2013 at 1:10 PM, Sebastian Andrzej Siewior <[email protected] <mailto:[email protected]>> wrote:

This patch adds the MUSB_DEVCTL_SESSION back after it has been removed. If it is missing then the host session is not recognized. This bit is added initially added in musb_start() and removed after the first device disconnect.

AFAIK, after the device is disconnected, the OTG state machine will go back to B_IDLE/A_IDLE state. SESSION is not needed in this case.

Okay.

Quoted 3 lines

In OTG mode, when no device is plugged, the ID pin is floating, you can never hold the host session in this case, even set the SESSION bit. The SESSION bit will be cleared by the controller after 100ms.

In my testing the bit remains set. How is the bit supposed to come back after I connect a host device?

Quoted 4 lines

Regards,
-Bin.

Sebastian

Hi Sebastian,

Quoted 2 lines

In my testing the bit remains set. How is the bit supposed to come back after I connect a host device?

The bit remains even when no device is plugged and ID ping is float?

what platform do you use to test it?

'a host device'? you meant a usb device? By the otg specs, the session will not automatically start. The user/app has to issue the command, either by SRP or HNP, or something else. In TI 3.2 kernel, there is workaround in otg_timer() to _toggle_ the SESSION bit to detect if ID pin is grounded, which means a USB device is connected.

Regards,
-Bin.

Hi Sebastian,

either by SRP or HNP, or something else. In TI 3.2 kernel, there is workaround in otg_timer() to _toggle_ the SESSION bit to detect if ID pin is grounded, which means a USB device is connected.

Regards,
-Bin.

Quoted 1 line · * Bin Liu | 2013-07-23 13:55:27 [-0500]:

Hi Sebastian,

Hi Bin,

Quoted 3 lines

either by SRP or HNP, or something else. In TI 3.2 kernel, there is workaround in otg_timer() to _toggle_ the SESSION bit to detect if ID pin is grounded, which means a USB device is connected.

I've been looking at
http://downloads.ti.com/sitara_linux/esd/AM335xSDK/latest/index_FDS.html

and I can't find what you are talking about. My MUSB_DEVCTL doesn't change after I connect a device and the SESSION bit is not set. Also I remain in a_idle state. Do you have maybe a hint for me? :)

Quoted 3 lines

Regards,
-Bin.

Sebastian

Sebastian,

Quoted 9 lines · Sebastian Andrzej Siewior

Quoted 1 line · * Bin Liu | 2013-07-23 13:55:27 [-0500]:

Hi Sebastian,

Hi Bin,

Quoted 3 lines

either by SRP or HNP, or something else. In TI 3.2 kernel, there is workaround in otg_timer() to _toggle_ the SESSION bit to detect if ID pin is grounded, which means a USB device is connected.

Quoted 6 lines

I've been looking at
http://downloads.ti.com/sitara_linux/esd/AM335xSDK/latest/index_FDS.html

and I can't find what you are talking about. My MUSB_DEVCTL doesn't change after I connect a device and the SESSION bit is not set. Also I remain in a_idle state. Do you have maybe a hint for me? :)

In TI 3.2 kernel, the otg_timer() toggles the SESSION bit in OTG_STATE_B_IDLE state. So whenever the ID pin is grounded by connecting a device, the controller will try go into host mode. But I have no idea why your board is in a_idle not b_idle. The toggle does not handle a_idle.

Do you use the AM335x GP EVM, not the smaller StarterKit EVM, right? the USB0 port on SK EVM is device only.

But anyway, I have not seen any use case which uses micro-AB receptacle and requires such dynamic role switching. So I kind of

Quoted 6 lines

In TI 3.2 kernel, the otg_timer() toggles the SESSION bit in OTG_STATE_B_IDLE state. So whenever the ID pin is grounded by connecting a device, the controller will try go into host mode. But I have no idea why your board is in a_idle not b_idle. The toggle does not handle a_idle.

Quoted 5 lines

Do you use the AM335x GP EVM, not the smaller StarterKit EVM, right? the USB0 port on SK EVM is device only.

But anyway, I have not seen any use case which uses micro-AB receptacle and requires such dynamic role switching. So I kind of

I meant the real customer use cases, not the EVMs ;)

On 07/26/2013 08:17 PM, Bin Liu wrote:

Hi Bin.

Quoted 12 lines

Quoted 11 lines

In TI 3.2 kernel, the otg_timer() toggles the SESSION bit in OTG_STATE_B_IDLE state. So whenever the ID pin is grounded by connecting a device, the controller will try go into host mode. But I have no idea why your board is in a_idle not b_idle. The toggle does not handle a_idle.

Do you use the AM335x GP EVM, not the smaller StarterKit EVM, right? the USB0 port on SK EVM is device only.

But anyway, I have not seen any use case which uses micro-AB receptacle and requires such dynamic role switching. So I kind of

Quoted 2 lines

I meant the real customer use cases, not the EVMs ;)

I am using the about the second port on am335x evm which is used as host (not OTG mode (3) but HOST (1)). If I start the timer OTG after disconnect I see that it gets back to A mode:

[   97.570852] musb-hdrc musb-hdrc.1.auto: Poll devctl 19 (a_wait_bcon)

[   99.570306] musb-hdrc musb-hdrc.1.auto: Poll devctl 98 (a_idle)
…
[  209.570259] musb-hdrc musb-hdrc.1.auto: Poll devctl 80 (a_idle)

and now the session bit is missing and never gets back so I can not plug any other device. I really have no idea what is missing here…

Sebastian

Quoted 1 line

[   97.570852] musb-hdrc musb-hdrc.1.auto: Poll devctl 19 (a_wait_bcon)

I don't think you are testing with the 3.2 kernel you referred from the ti.com link you mentioned above.
3.2 kernel should have no '.auto' at the end of the device name.

If it is 3.8 or later, it seems the driver does something wrong, SESSION bit should not be cleared after the device is unplugged.

The bit is removed in otg_timer() because the phy gets into OTG_STATE_A_WAIT_BCON.

Now, this is current HEAD + my stuff to get second port working. I downloaded v3.2 and browsed as I was looking for the workaround, never tested maybe I should…

On v3.8 from git://git.ti.com/ti-linux-kernel/ti-linux-kernel.git I don't get the second port to work. I see

[    1.989101] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002
[    1.996361] usb usb1: New USB device strings: Mfr=3, Product=2,
SerialNumber=1
[    2.004060] usb usb1: Product: MUSB HDRC host driver
[    2.009363] usb usb1: Manufacturer: Linux 3.8.13+ musb-hcd
[    2.015183] usb usb1: SerialNumber: musb-hdrc.0.auto
[    2.033653] hub 1-0:1.0: USB hub found
[    2.037861] hub 1-0:1.0: 1 port detected

and nothing happens once I plug in the device.

Sebastian

Sebastian,

I started thinking when I tried this git.ti.com 3.8 kernel, I might have done 'devmem2 0x47401c60 b 0x81' to manually set the SESSION bit after plugged a device. It was months ago, I don't quite remember what I tested.

My build server is down this afternoon. Once it comes back next week, I will try 3.8 again, to see how I can help on this USB1 host mode issue. its devctl register should stay on 0x19 even nothing is connected.

Quoted 4 lines

My build server is down this afternoon. Once it comes back next week, I will try 3.8 again, to see how I can help on this USB1 host mode issue. its devctl register should stay on 0x19 even nothing is connected.

Your help is greatly appreciated. To hear what will happen :)

Sebastian

Sebastian,

I have not tested it yet, but I believe I found why host mode works on TI 3.2 kernel but not on mainline. Please look at Line 786 in 3.2 kernel musb_core.c [1].

773 if ((int_usb & MUSB_INTR_DISCONNECT) && !musb->ignore_disconnect) {
......
785 if (musb->a_wait_bcon != 0 &&
786 is_otg_enabled(musb))
787 musb_platform_try_idle(musb, jiffies
788 +
msecs_to_jiffies(musb->a_wait_bcon));

So when the device is unplugged, *_try_idle() is not called in host mode, then the SESSION bit will stay set. But in mainline kernel, *_try_idle() will be called regardless.

Please let me know your thoughts.

Regards,
-Bin.

So you say, am335x-evm is not able to run OTG mode and may only run in host mode and as a consequence it must not call musb_platform_try_idle() because it throws that one bit away and there is no way to bring it back?

However, if I look at the tree you reference, I see in arch/arm/mach-omap2/board-am335xevm.c:

| static struct omap_musb_board_data musb_board_data = {
|         .interface_type = MUSB_INTERFACE_ULPI,
|         .mode           = MUSB_OTG,
|         .power          = 500,
|         .instances      = 1,
| };

…
| static void __init am335x_evm_init(void)
…
| usb_musb_init(&musb_board_data);

and it creates a "ti81xx-usbss" device.

So it creates a musb device with mode MUSB_OTG no matter what.

I agree that without that try_idle part things keep working but it seems, that it is also called in the other tree.

Quoted 3 lines

Regards,
-Bin.

Sebastian

Sebastian,

I did not say AM335xEVM is not able to run OTG mode. The problem is in OTG mode the SESSION bit will be cleared once the device is unplugged, then there is no way the SESSION bit will come back in the current mainline kernel.

The TI 3.2 kernel works around this OTG issue by toggling the SESSION bit in b_idle handling in otg_timer(). The workaround makes most users happy but it causes VBUS voltage pulsing. That is why I said I don't know an ideal solution without a ID pin interrupt support.

I think you looked at a wrong file, maybe a wrong branch. Please check [1], which defines

2644          * mode[0:3] = USB0PORT's mode
2645          * mode[4:7] = USB1PORT's mode
2646          * AM335X beta EVM has USB0 in OTG mode and USB1 in host mode.
2647          */

2648 .mode = (MUSB_HOST << 4) | MUSB_OTG,

Quoted 3 lines

I agree that without that try_idle part things keep working but it seems, that it is also called in the other tree.

Then because AM335xEVM USB1 port does not call *try_idle() in this 3.2 kernel, its SESSION bit is always set.

But I don't have a clear idea how to solve this in the mainline kernel since is_otg_enabled() has been cleaned.

Regards,
-Bin.

Quoted 1 line · Bin Liu

Sebastian,

Hi Bin,

Quoted 4 lines

I did not say AM335xEVM is not able to run OTG mode. The problem is in OTG mode the SESSION bit will be cleared once the device is unplugged, then there is no way the SESSION bit will come back in the current mainline kernel.

Yes, you did.

Quoted 6 lines

The TI 3.2 kernel works around this OTG issue by toggling the SESSION bit in b_idle handling in otg_timer(). The workaround makes most users happy but it causes VBUS voltage pulsing. That is why I said I don't know an ideal solution without a ID pin interrupt support.

Now I understand more pieces of the puzzle. And the missing ID pin support is Am335x-evm specific since other am335x board may wire up that pin.

But you say there is no way that the phy or anything else could help to avoid that pulsing.

Quoted 8 lines

I think you looked at a wrong file, maybe a wrong branch. Please check [1], which defines

2644          * mode[0:3] = USB0PORT's mode
2645          * mode[4:7] = USB1PORT's mode
2646          * AM335X beta EVM has USB0 in OTG mode and USB1 in host mode.
2647          */

2648 .mode = (MUSB_HOST << 4) | MUSB_OTG,

Oh it is a long way. So looked into the wrong branch.

Quoted 7 lines

Quoted 3 lines

I agree that without that try_idle part things keep working but it seems, that it is also called in the other tree.

Then because AM335xEVM USB1 port does not call *try_idle() in this 3.2 kernel, its SESSION bit is always set.

Quoted 2 lines

But I don't have a clear idea how to solve this in the mainline kernel since is_otg_enabled() has been cleaned.

So now I think this is a board problem. What about switching port 1 from OTG to HOST only and avoiding kicking the timer in dsps_musb_try_idle() in such a case? It seems we only need to do this in OTG mode.

Quoted 3 lines

Regards,
-Bin.

Sebastian

This is what I observe:
On the first connect, the musb starts with DEVCTL.Session set. On disconnect, musb_core calls try_idle. That functions removes the Session bit signalize that the session is over (something that only in OTG is required). A new device, that is plugged, is no longer recognized. I've setup a timer and checked the DEVCTL register and I haven't seen a change in VBus and I saw the B-Device bit set. After setting the IDDIG into A mode and forcing the device to behave like I didn't see a change. Neither VBUS goes to 0b11 nor does a session start request come. The TI-v3.2 kernel they skip to call musb_platform_try_idle() in the OTG_STATE_A_WAIT_BCON state while not in OTG mode.
Since the second port hast a standard A plug the patch changes the port to run in host mode only, force the id-pin to host only and skip the timer which would remove DEVCTL.Session.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

drivers/usb/musb/musb_dsps.c +0 -0

index 4ffbaac..b0752fa 100644

Hello.

drivers/usb/musb/musb_dsps.c static int dsps_musb_init(struct musb *musb) · line 398

Quoted 3 lines

drivers/usb/musb/musb_dsps.c +3 -0

+    if (musb->port_mode == MUSB_PORT_MODE_HOST) {
+        val = USBMODE_IDDIG_A;
+        val |= USBMODE_ID_MUX_REG;

Why not do the above in one line and save on {} {}? It will look more
aesthetically pleasing IMHO.

Quoted 1 line · Sergei Shtylyov

Hello.

Hi Sergei,

Quoted 6 lines

drivers/usb/musb/musb_dsps.c static int dsps_musb_init(struct musb *musb) · line 398

Quoted 3 lines

drivers/usb/musb/musb_dsps.c +3 -0

+    if (musb->port_mode == MUSB_PORT_MODE_HOST) {
+        val = USBMODE_IDDIG_A;
+        val |= USBMODE_ID_MUX_REG;

Why not do the above in one line and save on {} {}? It will look more
aesthetically pleasing IMHO.

I'm going to redo this if it is agreed that we want to fix it that way.

Sebastian

Sebastian,

Quoted 4 lines

Quoted 4 lines

drivers/usb/musb/musb_dsps.c static int dsps_musb_init(struct musb *musb) · line 398

Quoted 3 lines

drivers/usb/musb/musb_dsps.c +3 -0

+    if (musb->port_mode == MUSB_PORT_MODE_HOST) {
+        val = USBMODE_IDDIG_A;
+        val |= USBMODE_ID_MUX_REG;

I am going to so some tests today for this. I don't object to use the mode register, but am wondering why we have to play with it for host-only mode. The ID pin is grounded for host-only mode, which should cause the same as setting mode register.

-Bin.

Quoted 1 line · Bin Liu

Sebastian,

Hi Bin,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

drivers/usb/musb/musb_dsps.c +3 -0

+    if (musb->port_mode == MUSB_PORT_MODE_HOST) {
+        val = USBMODE_IDDIG_A;
+        val |= USBMODE_ID_MUX_REG;

Quoted 4 lines

I am going to so some tests today for this. I don't object to use the mode register, but am wondering why we have to play with it for host-only mode. The ID pin is grounded for host-only mode, which should cause the same as setting mode register.

I've been looking at the wiki page and it did not mention the ID pin for the second port. If it is grounded then this piece can be removed

and the magic trick is just to skip the try_idle() call.

I haven't found anything saying that it is required to clear the session bin in host mode, only in OTG. And then, I would assume to

receive a session interrupt once we have the proper VBUS level which does not happen.

Quoted 2 lines

-Bin.

Sebastian

Sebastian,

Quoted 2 lines

I've been looking at the wiki page and it did not mention the ID pin for the second port. If it is grounded then this piece can be removed

I thought you have already tried that without setting the mode register the session bit cannot stay set.

I am not sure if anywhere mentioned about the ID pin, but ASAIK all the different boards using am335x have ID pin grounded for host port.

Quoted 1 line

and the magic trick is just to skip the try_idle() call.

Agreed.

Quoted 3 lines

I haven't found anything saying that it is required to clear the session bin in host mode, only in OTG. And then, I would assume to

Agreed.

Quoted 2 lines

receive a session interrupt once we have the proper VBUS level which does not happen.

The TI 3.2 kernel for am335x sets the session bit in musb_start() for host-only mode. Maybe we can do something similar in here? (I noticed mush_start() has gone in mainline, but have not got a chance to check the details...)

Quoted 1 line

Quoted 1 line

-Bin.

This was a misunderstanding then. Sorry. I understood that the bin has to be unset and then the controller set it once a device there.

Quoted 3 lines

I am not sure if anywhere mentioned about the ID pin, but ASAIK all the different boards using am335x have ID pin grounded for host port.

evm is the only I am aware of. The evm-sk and beagle bone have just one port. Beagle bone black is not mainline.

This is the case already. From musb_start()
…

        if (musb->port_mode != MUSB_PORT_MODE_HOST &&
             (devctl & MUSB_DEVCTL_VBUS) == MUSB_DEVCTL_VBUS) {
                 musb->is_active = 1;
         } else {
                 devctl |= MUSB_DEVCTL_SESSION;
         }

…

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

-Bin.

Sebastian

Sebastian,

Quoted 2 lines · Sebastian Andrzej Siewior

This was a misunderstanding then. Sorry. I understood that the bin has to be unset and then the controller set it once a device there.

You meant ID pin? I think it should be set all the time since the driver initialized for host-only mode, if it is unset, the controller has not way to know if a device is plugged or not.

Quoted 4 lines

Quoted 2 lines

I am not sure if anywhere mentioned about the ID pin, but ASAIK all the different boards using am335x have ID pin grounded for host port.

evm is the only I am aware of. The evm-sk and beagle bone have just one port. Beagle bone black is not mainline.

You meant the dts only supports one port for evm-sk and bone? The boards physically have two ports, usb0 is device only, usb1 is host only.

Quoted 9 lines

This is the case already. From musb_start()
…

        if (musb->port_mode != MUSB_PORT_MODE_HOST &&
             (devctl & MUSB_DEVCTL_VBUS) == MUSB_DEVCTL_VBUS) {
                 musb->is_active = 1;
         } else {
                 devctl |= MUSB_DEVCTL_SESSION;
         }

…

great! then the host port on gp evm should work now, right?

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

-Bin.

Where is my memory going? So now I have a beagle bone in front of me and I see a micro USB port a standard A connector. My memory was different.
The micro USB is the UART and standard is most likely the first musb instance. So this should run also in host mode and not in OTG mode. Hmmm. Let me double check this.

Sebastian

Sebastian,

Quoted 6 lines · Sebastian Andrzej Siewior

Where is my memory going? So now I have a beagle bone in front of me and I see a micro USB port a standard A connector. My memory was different.
The micro USB is the UART and standard is most likely the first musb instance. So this should run also in host mode and not in OTG mode. Hmmm. Let me double check this.

I don't know how dts instances both in mainline, but I would think if you create two musb instances, and load a gadget driver for this micro USB port, the USB host should enumerate it. The standard port should be the 2nd instance which has its ID pin grounded and work in host-only mode.

This is how they work in TI 3.2 kernel.

Quoted 1 line

Sebastian

So I tried to boot my beagle bone board to figure out if they use the first or the second port in host mode (but it doesn't boot). The tiny micro USB plug is 100% for UART and not wired up to MUSB.

Any comments on that patch? I would remove the ID pin changes and repost if you don't mind.

In USB RX path it is possible that the we receive less bytes than requested. Take the following example:
The driver for USB-to-UART submits an URB with 256 bytes in size and the dmaengine driver in turn programs a transfer of 256 bytes. The transfer is programmed and the dma engine waits for the data to arrive. Once data is sent on the UART the dma engine begins to move data. If there was only one data byte in the USB packet received then the DMA engine will only move one byte due to USB restrictions / rules. The real size of the transfer has to be notified to the user / caller so he set this to the caller.
This patch adds the transfered member to the dma_async_tx_descriptor where the caller can obtain the final size.

Cc Vinod Koul <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
  include/linux/dmaengine.h | 3 +++
  1 file changed, 3 insertions(+)

include/linux/dmaengine.h +2 -0

index cb286b1..c3a4635 100644
@@ -403,6 +403,8 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param);
   * @tx_submit: set the prepared descriptor(s) to be executed by the engine
   * @callback: routine to call after this operation is complete
   * @callback_param: general parameter to pass to the callback routine
+ * @transfered: number of bytes that were really transfered in case the channel
+ *	transfered less than requested.
   * ---async_tx api specific fields---
   * @next: at completion submit this descriptor
   * @parent: pointer to the next level up in the dependency chain

include/linux/dmaengine.h struct dma_async_tx_descriptor { · line 421

Quoted 5 lines

include/linux/dmaengine.h +1 -0

@@ -416,6 +418,7 @@ struct dma_async_tx_descriptor {
      dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx);
      dma_async_tx_callback callback;
      void *callback_param;
+    unsigned int transfered;

Correct grammar is "transferred".

WBR, Sergei

In USB RX path it is possible that the we receive less bytes than requested. Take the following example:
The driver for USB-to-UART submits an URB with 256 bytes in size and the dmaengine driver in turn programs a transfer of 256 bytes. The transfer

is programmed and the dma engine waits for the data to arrive. Once data is sent on the UART the dma engine begins to move data. If there was only one data byte in the USB packet received then the DMA engine will only move one byte due to USB restrictions / rules. The real size of the transfer has to be notified to the user / caller so he set this to the caller.
This patch adds the transfered member to the dma_async_tx_descriptor where the caller can obtain the final size.

I'm not sure that this will work. Once you've submitted the descriptor it is owned by the dmaengine driver and you are not allowed to dereference it anymore since the dmaengine driver might free the memory at any time. You should only reference the descriptor by the cookie returned by submit().

- Lars

Quoted 4 lines · Lars-Peter Clausen

I'm not sure that this will work. Once you've submitted the descriptor it is owned by the dmaengine driver and you are not allowed to dereference it anymore since the dmaengine driver might free the memory at any time. You should only reference the descriptor by the cookie returned by submit().

I see. But it can't be reused before calling the callback if it is going to call the callback, right?
So if this is a no-no, I'm left with an additional argument to the complete callback?

Quoted 4 lines

I see. But it can't be reused before calling the callback if it is going to call the callback, right?
So if this is a no-no, I'm left with an additional argument to the complete callback?

Hm, maybe using dmaengine_tx_status() and checking the residue field of the state struct. transferred is basically len - residue.

In USB RX path it is possible that the we receive less bytes than requested. Take the following example:
The driver for USB-to-UART submits an URB with 256 bytes in size and the dmaengine driver in turn programs a transfer of 256 bytes. The transfer is programmed and the dma engine waits for the data to arrive. Once data is sent on the UART the dma engine begins to move data. If there was only one data byte in the USB packet received then the DMA engine will only move one byte due to USB restrictions / rules. The real size of the transfer has to be notified to the user / caller so he set this to the caller.
This patch adds the transfered member to the dma_async_tx_descriptor where the caller can obtain the final size.

Cc Vinod Koul <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 include/linux/dmaengine.h | 3 +++
 1 file changed, 3 insertions(+)

include/linux/dmaengine.h +2 -0

index cb286b1..c3a4635 100644
@@ -403,6 +403,8 @@ typedef void (*dma_async_tx_callback)(void *dma_async_param);
  * @tx_submit: set the prepared descriptor(s) to be executed by the engine
  * @callback: routine to call after this operation is complete
  * @callback_param: general parameter to pass to the callback routine
+ * @transfered: number of bytes that were really transfered in case the channel
+ *	transfered less than requested.
  * ---async_tx api specific fields---
  * @next: at completion submit this descriptor
  * @parent: pointer to the next level up in the dependency chain

include/linux/dmaengine.h struct dma_async_tx_descriptor { · line 421

Quoted 5 lines

include/linux/dmaengine.h +1 -0

@@ -416,6 +418,7 @@ struct dma_async_tx_descriptor {
 	dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx);
 	dma_async_tx_callback callback;
 	void *callback_param;
+	unsigned int transfered;

I think this should be doable with residue as well. You can return DMA_SUCCESS for status and fill in what is NOT transfered indicating that txn completed but lesser number of bytes are valid

~Vinod