twl4030-usb updates/fixes

From: David Brownell <[email protected]>

Simplify handling of VBUS and ID status and the USB_PRES interrupt. This seems to resolve the problems I previously saw on Beagle and Overo boards.

- Read the status directly, instead of trying to infer it from dodgey side effects of IRQ handling and trigger tweaking.

- Stop trying to arrange those dodgey side effects; just leave the IRQ enabled at all times.

- Check that status as part of driver probe, so the PHY can always be active by the time it's needed. (BUGFIX!)

- Re-sequence probe() to be less racey, and to avoid duplicating logic found elsewhere (notably irq handling and PHY activation).

- Make the irq_chip be the exclusive owner of PWR_EDR (finally).

- Get rid of needless work_struct ... in this irq handling thread, it's safe and cheap to call sysfs_notify() directly.

Stop enabling IRQs on the USB subchip. There's no IRQ handler for them, and since ALT_INT_REROUTE isn't being set they'll never arrive ... so there's no point to enabling them.

Also add some comments about other things such an otg_transciever driver should be doing, fixing minor omissions; and remove some unnecessary file inclusions.

Signed-off-by David Brownell <[email protected]>

---
NOTE: applies on top of the previous patches updating IRQ handling and creating drivers/mfd/twl4030-irq.c ...

 drivers/i2c/chips/twl4030-usb.c |  209 ++++++++++++++++++--------------------
 1 file changed, 104 insertions(+), 105 deletions(-)

drivers/i2c/chips/twl4030-usb.c +104 -106

@@ -26,16 +26,12 @@
 
 #include <linux/module.h>
 #include <linux/init.h>
-#include <linux/time.h>
 #include <linux/interrupt.h>
-#include <linux/irq.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/io.h>
-#include <linux/usb.h>
-#include <linux/usb/ch9.h>
-#include <linux/usb/gadget.h>
+#include <linux/delay.h>
 #include <linux/usb/otg.h>
 #include <linux/i2c/twl4030.h>
 #include <mach/usb.h>

@@ -237,10 +233,6 @@
 #define PMBR1				0x0D
 #define GPIO_USB_4PIN_ULPI_2430C	(3 << 0)
 
-/* In module TWL4030_MODULE_INT */
-#define REG_PWR_EDR1			0x05
-#define USB_PRES_FALLING		(1 << 4)
-#define USB_PRES_RISING			(1 << 5)
 
 /* bits in OTG_CTRL */
 #define	OTG_XCEIV_OUTPUTS \

@@ -256,8 +248,14 @@
 	OTG_CTRL_BITS)
 
 
+enum linkstat {
+	USB_LINK_UNKNOWN = 0,
+	USB_LINK_NONE,
+	USB_LINK_VBUS,
+	USB_LINK_ID,
+};
+
 struct twl4030_usb {
-	struct work_struct	irq_work;
 	struct otg_transceiver	otg;
 	struct device		*dev;
 

@@ -267,8 +265,8 @@ struct twl4030_usb {
 	/* pin configuration */
 	enum twl4030_usb_mode	usb_mode;
 
-	unsigned		vbus:1;
 	int			irq;
+	u8			linkstat;
 	u8			asleep;
 	bool			irq_enabled;
 };

@@ -326,21 +324,27 @@ static inline int twl4030_usb_write(stru
 	return ret;
 }
 
-static inline int twl4030_usb_read(struct twl4030_usb *twl, u8 address)
+static inline int twl4030_readb(struct twl4030_usb *twl, u8 module, u8 address)
 {
 	u8 data;
 	int ret = 0;
 
-	ret = twl4030_i2c_read_u8(TWL4030_MODULE_USB, &data, address);
+	ret = twl4030_i2c_read_u8(module, &data, address);
 	if (ret >= 0)
 		ret = data;
 	else
 		dev_warn(twl->dev,
-			"TWL4030:USB:Read[0x%x] Error %d\n", address, ret);
+			"TWL4030:readb[0x%x,0x%x] Error %d\n",
+					module, address, ret);
 
 	return ret;
 }
 
+static inline int twl4030_usb_read(struct twl4030_usb *twl, u8 address)
+{
+	return twl4030_readb(twl, TWL4030_MODULE_USB, address);
+}
+
 /*-------------------------------------------------------------------------*/
 
 static inline int

@@ -357,6 +361,39 @@ twl4030_usb_clear_bits(struct twl4030_us
 
 /*-------------------------------------------------------------------------*/
 
+static enum linkstat twl4030_usb_linkstat(struct twl4030_usb *twl)
+{
+	int	status;
+	int	linkstat = USB_LINK_UNKNOWN;
+
+	/* STS_HW_CONDITIONS */
+	status = twl4030_readb(twl, TWL4030_MODULE_PM_MASTER, 0x0f);
+	if (status < 0)
+		dev_err(twl->dev, "USB link status err %d\n", status);
+	else if (status & BIT(7))
+		linkstat = USB_LINK_VBUS;
+	else if (status & BIT(2))
+		linkstat = USB_LINK_ID;
+	else
+		linkstat = USB_LINK_NONE;
+
+	dev_dbg(twl->dev, "HW_CONDITIONS 0x%02x/%d; link %d\n",
+			status, status, linkstat);
+
+	spin_lock_irq(&twl->lock);
+	twl->linkstat = linkstat;
+	if (linkstat == USB_LINK_ID) {
+		twl->otg.default_a = true;
+		twl->otg.state = OTG_STATE_A_IDLE;
+	} else {
+		twl->otg.default_a = false;
+		twl->otg.state = OTG_STATE_B_IDLE;
+	}
+	spin_unlock_irq(&twl->lock);
+
+	return linkstat;
+}
+
 static void twl4030_usb_set_mode(struct twl4030_usb *twl, int mode)
 {
 	twl->usb_mode = mode;

@@ -410,24 +447,6 @@ static void twl4030_i2c_access(struct tw
 	}
 }
 
-static void usb_irq_enable(struct twl4030_usb *twl, int trigger)
-{
-	set_irq_type(twl->irq, trigger);
-
-	if (!twl->irq_enabled) {
-		enable_irq(twl->irq);
-		twl->irq_enabled = true;
-	}
-}
-
-static void usb_irq_disable(struct twl4030_usb *twl)
-{
-	if (twl->irq_enabled) {
-		disable_irq(twl->irq);
-		twl->irq_enabled = false;
-	}
-}
-
 static void twl4030_phy_power(struct twl4030_usb *twl, int on)
 {
 	u8 pwr;

@@ -448,16 +467,9 @@ static void twl4030_phy_power(struct twl
 
 static void twl4030_phy_suspend(struct twl4030_usb *twl, int controller_off)
 {
-	if (controller_off)
-		usb_irq_disable(twl);
-
 	if (twl->asleep)
 		return;
 
-	if (!controller_off)
-		/* enable rising edge interrupt to detect cable attach */
-		usb_irq_enable(twl, IRQ_TYPE_EDGE_RISING);
-
 	twl4030_phy_power(twl, 0);
 	twl->asleep = 1;
 }

@@ -467,9 +479,6 @@ static void twl4030_phy_resume(struct tw
 	if (!twl->asleep)
 		return;
 
-	/* enable falling edge interrupt to detect cable detach */
-	usb_irq_enable(twl, IRQ_TYPE_EDGE_FALLING);
-
 	twl4030_phy_power(twl, 1);
 	twl4030_i2c_access(twl, 1);
 	twl4030_usb_set_mode(twl, twl->usb_mode);

@@ -514,25 +523,18 @@ static ssize_t twl4030_usb_vbus_show(str
 	int ret = -EINVAL;
 
 	spin_lock_irqsave(&twl->lock, flags);
-	ret = sprintf(buf, "%s\n", twl->vbus ? "on" : "off");
+	ret = sprintf(buf, "%s\n",
+			(twl->linkstat == USB_LINK_VBUS) ? "on" : "off");
 	spin_unlock_irqrestore(&twl->lock, flags);
 
 	return ret;
 }
 static DEVICE_ATTR(vbus, 0444, twl4030_usb_vbus_show, NULL);
 
-static void twl4030_usb_irq_work(struct work_struct *work)
-{
-	struct twl4030_usb *twl = container_of(work,
-			struct twl4030_usb, irq_work);
-
-	sysfs_notify(&twl->dev->kobj, NULL, "vbus");
-}
-
 static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 {
 	struct twl4030_usb *twl = _twl;
-	u8 val;
+	int status;
 
 #ifdef CONFIG_LOCKDEP
 	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which

@@ -542,27 +544,28 @@ static irqreturn_t twl4030_usb_irq(int i
 	local_irq_enable();
 #endif
 
-	/* FIXME stop accessing PWR_EDR1 ... if nothing else, we
-	 * know which edges we told the IRQ to trigger on.  And
-	 * there seem to be OTG_specific registers and irqs that
-	 * provide the right info without guessing like this:
-	 * USB_INT_STS, ID_STATUS, STS_HW_CONDITIONS, etc.
-	 */
+	status = twl4030_usb_linkstat(twl);
+	if (status != USB_LINK_UNKNOWN) {
 
-	/* action based on cable attach or detach */
-	WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT,
-				&val, REG_PWR_EDR1) < 0);
+		/* FIXME add a set_power() method so that B-devices can
+		 * configure the charger appropriately.  It's not always
+		 * correct to consume VBUS power, and how much current to
+		 * consume is a function of the USB configuration chosen
+		 * by the host.
+		 *
+		 * REVISIT usb_gadget_vbus_connect(...) as needed, ditto
+		 * its disconnect() sibling, when changing to/from the
+		 * USB_LINK_VBUS state.  musb_hdrc won't care until it
+		 * starts to handle softconnect right.
+		 */
+		twl4030charger_usb_en(status == USB_LINK_VBUS);
 
-	if (val & USB_PRES_RISING) {
-		twl4030_phy_resume(twl);
-		twl4030charger_usb_en(1);
-		twl->vbus = 1;
-	} else {
-		twl4030charger_usb_en(0);
-		twl->vbus = 0;
-		twl4030_phy_suspend(twl, 0);
+		if (status == USB_LINK_NONE)
+			twl4030_phy_suspend(twl, 0);
+		else
+			twl4030_phy_resume(twl);
 	}
-	schedule_work(&twl->irq_work);
+	sysfs_notify(&twl->dev->kobj, NULL, "vbus");
 
 	return IRQ_HANDLED;
 }

@@ -608,11 +611,6 @@ static int twl4030_set_peripheral(struct
 
 	twl->otg.state = OTG_STATE_B_IDLE;
 
-	twl4030_usb_set_bits(twl, USB_INT_EN_RISE,
-			USB_INT_SESSVALID | USB_INT_VBUSVALID);
-	twl4030_usb_set_bits(twl, USB_INT_EN_FALL,
-			USB_INT_SESSVALID | USB_INT_VBUSVALID);
-
 	return 0;
 }
 

@@ -639,8 +637,7 @@ static int twl4030_set_host(struct otg_t
 	twl4030_usb_set_bits(twl, TWL4030_OTG_CTRL,
 			TWL4030_OTG_CTRL_DMPULLDOWN
 				| TWL4030_OTG_CTRL_DPPULLDOWN);
-	twl4030_usb_set_bits(twl, USB_INT_EN_RISE, USB_INT_IDGND);
-	twl4030_usb_set_bits(twl, USB_INT_EN_FALL, USB_INT_IDGND);
+
 	twl4030_usb_set_bits(twl, FUNC_CTRL, FUNC_CTRL_SUSPENDM);
 	twl4030_usb_set_bits(twl, TWL4030_OTG_CTRL, TWL4030_OTG_CTRL_DRVVBUS);
 

@@ -651,8 +648,7 @@ static int __init twl4030_usb_probe(stru
 {
 	struct twl4030_usb_data *pdata = pdev->dev.platform_data;
 	struct twl4030_usb	*twl;
-	int status;
-	u8			vbus;
+	int			status;
 
 	twl = kzalloc(sizeof *twl, GFP_KERNEL);
 	if (!twl)

@@ -663,26 +659,38 @@ static int __init twl4030_usb_probe(stru
 		return -EINVAL;
 	}
 
-	WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT,
-				&vbus, REG_PWR_EDR1) < 0);
-	vbus &= USB_PRES_RISING;
-
 	twl->dev		= &pdev->dev;
 	twl->irq		= platform_get_irq(pdev, 0);
+	twl->otg.dev		= twl->dev;
+	twl->otg.label		= "twl4030";
 	twl->otg.set_host	= twl4030_set_host;
 	twl->otg.set_peripheral	= twl4030_set_peripheral;
 	twl->otg.set_suspend	= twl4030_set_suspend;
 	twl->usb_mode		= pdata->usb_mode;
-	twl->vbus		= vbus ? 1 : 0;
+	twl->asleep		= 1;
 
 	/* init spinlock for workqueue */
 	spin_lock_init(&twl->lock);
 
-	/* init irq workqueue before request_irq */
-	INIT_WORK(&twl->irq_work, twl4030_usb_irq_work);
+	twl4030_usb_ldo_init(twl);
+	otg_set_transceiver(&twl->otg);
+
+	platform_set_drvdata(pdev, twl);
+	if (device_create_file(&pdev->dev, &dev_attr_vbus))
+		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
+	/* Our job is to use irqs and status from the power module
+	 * to keep the transceiver disabled when nothing's connected.
+	 *
+	 * FIXME we actually shouldn't start enabling it until the
+	 * USB controller drivers have said they're ready, by calling
+	 * set_host() and/or set_peripheral() ... OTG_capable boards
+	 * need both handles, otherwise just one suffices.
+	 */
 	twl->irq_enabled = true;
-	status = request_irq(twl->irq, twl4030_usb_irq, 0, "twl4030_usb", twl);
+	status = request_irq(twl->irq, twl4030_usb_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"twl4030_usb", twl);
 	if (status < 0) {
 		dev_dbg(&pdev->dev, "can't get IRQ %d, err %d\n",
 			twl->irq, status);

@@ -690,25 +698,17 @@ static int __init twl4030_usb_probe(stru
 		return status;
 	}
 
-	twl4030_usb_ldo_init(twl);
-	twl4030_phy_power(twl, 1);
-	twl4030_i2c_access(twl, 1);
-	twl4030_usb_set_mode(twl, twl->usb_mode);
-
-	twl->asleep = 0;
-
-	if (twl->usb_mode == T2_USB_MODE_ULPI) {
-		twl4030_i2c_access(twl, 0);
-		twl4030_phy_suspend(twl, 0);
-	}
+	/* The IRQ handler just handles changes from the previous states
+	 * of the ID and VBUS pins ... in probe() we must initialize that
+	 * previous state.  The easy way:  fake an IRQ.
+	 *
+	 * REVISIT:  a real IRQ might have happened already, if PREEMPT is
+	 * enabled.  Else the IRQ may not yet be configured or enabled,
+	 * because of scheduling delays.
+	 */
+	twl4030_usb_irq(twl->irq, twl);
 
-	otg_set_transceiver(&twl->otg);
-	platform_set_drvdata(pdev, twl);
 	dev_info(&pdev->dev, "Initialized TWL4030 USB module\n");
-
-	if (device_create_file(&pdev->dev, &dev_attr_vbus))
-		dev_warn(&pdev->dev, "could not create sysfs file\n");
-
 	return 0;
 }
 

@@ -717,7 +717,6 @@ static int __exit twl4030_usb_remove(str
 	struct twl4030_usb *twl = platform_get_drvdata(pdev);
 	int val;
 
-	usb_irq_disable(twl);
 	free_irq(twl->irq, twl);
 	device_remove_file(twl->dev, &dev_attr_vbus);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pushing today, I guess that's it for the USB problems for now.