[PATCH 0/5] Enhancements to twl4030 phy to support better charging - V2

Hi Kishon,
 I wonder if you could queue the following for the next merge window.
 They allow the twl4030 phy to provide more information to the
 twl4030 battery charger.
 There are only minimal changes since the first version, particularly
 documentation has been improved.

Thanks,
NeilBrown


---

NeilBrown (5):
      usb: phy: twl4030: make runtime pm more reliable.
      usb: phy: twl4030: allow charger to see usb current draw limits.
      usb: phy: twl4030: add ABI documentation
      usb: phy: twl4030: add support for reading restore on ID pin.
      usb: phy: twl4030: test ID resistance to see if charger is present.


 .../ABI/testing/sysfs-platform-twl4030-usb         |   30 ++++
 drivers/phy/phy-twl4030-usb.c                      |  147 ++++++++++++++++++--
 2 files changed, 162 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-twl4030-usb

--
Signature

From: NeilBrown <redacted>

A construct like:

        if (pm_runtime_suspended(twl->dev))
               pm_runtime_get_sync(twl->dev);

is against the spirit of the runtime_pm interface as it
makes the internal refcounting useless.

In this case it is also racy, particularly as 'put_autosuspend'
is use to drop a reference.
When that happens a timer is started and the device is
runtime-suspended after the timeout.
If the above code runs in this window, the device will not be
found to be suspended so no pm_runtime reference is taken.
When the timer expires the device will be suspended, which is
against the intention of the code.

So be more direct is taking and dropping references.
If twl->linkstat is VBUS_VALID or ID_GROUND, then hold a
pm_runtime reference, otherwise don't.
Define "cable_present()" to test for this condition.

Tested-by: Tony Lindgren <redacted>
Signed-off-by: NeilBrown <redacted>
---
 drivers/phy/phy-twl4030-usb.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 8e87f54671f3..1a244f34b748 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c

@@ -144,6 +144,16 @@
 #define PMBR1				0x0D
 #define GPIO_USB_4PIN_ULPI_2430C	(3 << 0)
 
+/*
+ * If VBUS is valid or ID is ground, then we know a
+ * cable is present and we need to be runtime-enabled
+ */
+static inline bool cable_present(enum omap_musb_vbus_id_status stat)
+{
+	return stat == OMAP_MUSB_VBUS_VALID ||
+		stat == OMAP_MUSB_ID_GROUND;
+}
+
 struct twl4030_usb {
 	struct usb_phy		phy;
 	struct device		*dev;

@@ -536,8 +546,10 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 
 	mutex_lock(&twl->lock);
 	if (status >= 0 && status != twl->linkstat) {
+		status_changed =
+			cable_present(twl->linkstat) !=
+			cable_present(status);
 		twl->linkstat = status;
-		status_changed = true;
 	}
 	mutex_unlock(&twl->lock);
 

@@ -553,15 +565,11 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 		 * USB_LINK_VBUS state.  musb_hdrc won't care until it
 		 * starts to handle softconnect right.
 		 */
-		if ((status == OMAP_MUSB_VBUS_VALID) ||
-		    (status == OMAP_MUSB_ID_GROUND)) {
-			if (pm_runtime_suspended(twl->dev))
-				pm_runtime_get_sync(twl->dev);
+		if (cable_present(status)) {
+			pm_runtime_get_sync(twl->dev);
 		} else {
-			if (pm_runtime_active(twl->dev)) {
-				pm_runtime_mark_last_busy(twl->dev);
-				pm_runtime_put_autosuspend(twl->dev);
-			}
+			pm_runtime_mark_last_busy(twl->dev);
+			pm_runtime_put_autosuspend(twl->dev);
 		}
 		omap_musb_mailbox(status);
 	}

@@ -768,6 +776,9 @@ static int twl4030_usb_remove(struct platform_device *pdev)
 
 	/* disable complete OTG block */
 	twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB);
+
+	if (cable_present(twl->linkstat))
+		pm_runtime_put_noidle(twl->dev);
 	pm_runtime_mark_last_busy(twl->dev);
 	pm_runtime_put(twl->dev);
 

From: NeilBrown <redacted>

This driver device one local attribute: vbus.
Describe that in Documentation/ABI/testing/sysfs-platform/twl4030-usb.

Signed-off-by: NeilBrown <neil@brown.name>
---
 .../ABI/testing/sysfs-platform-twl4030-usb         |    8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-twl4030-usb

diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
new file mode 100644
index 000000000000..512c51be64ae
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb

@@ -0,0 +1,8 @@
+What: /sys/bus/platform/devices/*twl4030-usb/vbus
+Description:
+	Read-only status reporting if VBUS (approx 5V)
+	is being supplied by the USB bus.
+
+	Possible values: "on", "off".
+
+	Changes are notified via select/poll.

From: NeilBrown <redacted>

The twl4030 phy can measure, with low precision, the
resistance-to-ground of the ID pin.

Add a function to read the value, and export the result
via sysfs.

If the read fails, which it does sometimes, try again in 50msec.

Acked-by: Pavel Machek <redacted>
Signed-off-by: NeilBrown <redacted>
---
 .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
 drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
index 512c51be64ae..425d23676f8a 100644
--- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
+++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb

@@ -6,3 +6,25 @@ Description:
 	Possible values: "on", "off".
 
 	Changes are notified via select/poll.
+
+What: /sys/bus/platform/devices/*twl4030-usb/id
+Description:
+	Read-only report on measurement of USB-OTG ID pin.
+
+	The ID pin may be floating, grounded, or pulled to
+	ground by a resistor.
+
+	A very course grained reading of the resistance is
+	available.  The numbers given in kilo-ohms are roughly
+	the center-point of the detected range.
+
+	Possible values are:
+		ground
+		102k
+		200k
+		440k
+		floating
+		unknown
+
+	"unknown" indicates a problem with trying to detect
+	the resistance.

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 0fcef8717d5b..8dbf9d2a99d1 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c

@@ -384,6 +384,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 	}
 }
 
+enum twl4030_id_status {
+	TWL4030_GROUND,
+	TWL4030_102K,
+	TWL4030_200K,
+	TWL4030_440K,
+	TWL4030_FLOATING,
+	TWL4030_ID_UNKNOWN,
+};
+static char *twl4030_id_names[] = {
+	"ground",
+	"102k",
+	"200k",
+	"440k",
+	"floating",
+	"unknown"
+};
+
+enum twl4030_id_status twl4030_get_id(struct twl4030_usb *twl)
+{
+	int ret;
+
+	pm_runtime_get_sync(twl->dev);
+	if (twl->usb_mode == T2_USB_MODE_ULPI)
+		twl4030_i2c_access(twl, 1);
+	ret = twl4030_usb_read(twl, ULPI_OTG_CTRL);
+	if (ret < 0 || !(ret & ULPI_OTG_ID_PULLUP)) {
+		/* Need pull-up to read ID */
+		twl4030_usb_set_bits(twl, ULPI_OTG_CTRL,
+				     ULPI_OTG_ID_PULLUP);
+		mdelay(50);
+	}
+	ret = twl4030_usb_read(twl, ID_STATUS);
+	if (ret < 0 || (ret & 0x1f) == 0) {
+		mdelay(50);
+		ret = twl4030_usb_read(twl, ID_STATUS);
+	}
+
+	if (twl->usb_mode == T2_USB_MODE_ULPI)
+		twl4030_i2c_access(twl, 0);
+	pm_runtime_put_autosuspend(twl->dev);
+
+	if (ret < 0)
+		return TWL4030_ID_UNKNOWN;
+	ret = ffs(ret) - 1;
+	if (ret < TWL4030_GROUND || ret > TWL4030_FLOATING)
+		return TWL4030_ID_UNKNOWN;
+
+	return ret;
+}
+
 static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
 {
 	u8 pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);

@@ -541,6 +591,16 @@ static ssize_t twl4030_usb_vbus_show(struct device *dev,
 }
 static DEVICE_ATTR(vbus, 0444, twl4030_usb_vbus_show, NULL);
 
+static ssize_t twl4030_usb_id_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct twl4030_usb *twl = dev_get_drvdata(dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n",
+			 twl4030_id_names[twl4030_get_id(twl)]);
+}
+static DEVICE_ATTR(id, 0444, twl4030_usb_id_show, NULL);
+
 static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 {
 	struct twl4030_usb *twl = _twl;

@@ -729,6 +789,8 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, twl);
 	if (device_create_file(&pdev->dev, &dev_attr_vbus))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
+	if (device_create_file(&pdev->dev, &dev_attr_id))
+		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&twl->phy.notifier);
 

@@ -774,6 +836,7 @@ static int twl4030_usb_remove(struct platform_device *pdev)
 	pm_runtime_get_sync(twl->dev);
 	cancel_delayed_work(&twl->id_workaround_work);
 	device_remove_file(twl->dev, &dev_attr_vbus);
+	device_remove_file(twl->dev, &dev_attr_id);
 
 	/* set transceiver mode to power on defaults */
 	twl4030_usb_set_mode(twl, -1);

From: NeilBrown <redacted>

If an 'A' plug is inserted, ID should be pulled to ground.
If a 'B' plug, then ID should be floating.

If an Accessory Charger Adapter is inserted, then ID will
be neither grounded nor floating.  In this case tell the
USB subsystem that it is an A plug, and the battery
charging subsystem that it is a charger.

Fortunately, this will treat the Openmoko charger (and
other similar chargers) as a charger.

Acked-by: Pavel Machek <redacted>
Signed-off-by: NeilBrown <redacted>
---
 drivers/phy/phy-twl4030-usb.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 8dbf9d2a99d1..d81753bad1ca 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c

@@ -606,9 +606,31 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	struct twl4030_usb *twl = _twl;
 	enum omap_musb_vbus_id_status status;
 	bool status_changed = false;
+	bool found_charger = false;
 
 	status = twl4030_usb_linkstat(twl);
 
+	if (status == OMAP_MUSB_ID_GROUND ||
+	    status == OMAP_MUSB_VBUS_VALID) {
+		/* We should check the resistance on the ID pin.
+		 * If not a Ground or Floating, then this is
+		 * likely a charger
+		 */
+		enum twl4030_id_status sts = twl4030_get_id(twl);
+		if (sts > TWL4030_GROUND &&
+		    sts < TWL4030_FLOATING) {
+			/*
+			 * This might be a charger, or an
+			 * Accessory Charger Adapter.
+			 * In either case we can charge, and it
+			 * makes sense to tell the USB system
+			 * that we might be acting as a HOST.
+			 */
+			status = OMAP_MUSB_ID_GROUND;
+			found_charger = true;
+		}
+	}
+
 	mutex_lock(&twl->lock);
 	if (status >= 0 && status != twl->linkstat) {
 		status_changed =

@@ -633,6 +655,12 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 		}
 		omap_musb_mailbox(status);
 	}
+	if (found_charger && twl->phy.last_event != USB_EVENT_CHARGER) {
+		twl->phy.last_event = USB_EVENT_CHARGER;
+		atomic_notifier_call_chain(&twl->phy.notifier,
+					   USB_EVENT_CHARGER,
+					   NULL);
+	}
 
 	/* don't schedule during sleep - irq works right then */
 	if (status == OMAP_MUSB_ID_GROUND && pm_runtime_active(twl->dev)) {

From: NeilBrown <redacted>

The charger needs to know when a USB gadget has been enumerated
and what the agreed maximum current was so that it can adjust
charging accordingly.

So define a "set_power()" function to record the permitted
draw, and pass a pointer to that when sending USB_EVENT_ENUMERATED
notification.

Signed-off-by: NeilBrown <redacted>
---
 drivers/phy/phy-twl4030-usb.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 1a244f34b748..0fcef8717d5b 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c

@@ -173,6 +173,11 @@ struct twl4030_usb {
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
 
+	/* Permitted vbus draw in mA - only meaningful after
+	 * USB_EVENT_ENUMERATED
+	 */
+	unsigned int		vbus_draw;
+
 	struct delayed_work	id_workaround_work;
 };
 

@@ -554,12 +559,7 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	mutex_unlock(&twl->lock);
 
 	if (status_changed) {
-		/* 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

@@ -631,6 +631,20 @@ static int twl4030_set_host(struct usb_otg *otg, struct usb_bus *host)
 	return 0;
 }
 
+static int twl4030_set_power(struct usb_phy *phy, unsigned mA)
+{
+	struct twl4030_usb *twl = phy_to_twl(phy);
+
+	if (twl->vbus_draw != mA) {
+		phy->last_event = USB_EVENT_ENUMERATED;
+		twl->vbus_draw = mA;
+		atomic_notifier_call_chain(&phy->notifier,
+					   USB_EVENT_ENUMERATED,
+					   &twl->vbus_draw);
+	}
+	return 0;
+}
+
 static const struct phy_ops ops = {
 	.init		= twl4030_phy_init,
 	.power_on	= twl4030_phy_power_on,

@@ -681,6 +695,7 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	twl->phy.label		= "twl4030";
 	twl->phy.otg		= otg;
 	twl->phy.type		= USB_PHY_TYPE_USB2;
+	twl->phy.set_power	= twl4030_set_power;
 
 	otg->usb_phy		= &twl->phy;
 	otg->set_host		= twl4030_set_host;

On Mon 2015-03-23 09:35:23, NeilBrown wrote:

Hi Kishon,
 I wonder if you could queue the following for the next merge window.
 They allow the twl4030 phy to provide more information to the
 twl4030 battery charger.
 There are only minimal changes since the first version, particularly
 documentation has been improved.

Patches 1-3 Acked-by: Pavel Machek [off-list ref]
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

Hi Kishon,
 I wonder if you could queue the following for the next merge window.
 They allow the twl4030 phy to provide more information to the
 twl4030 battery charger.
 There are only minimal changes since the first version, particularly
 documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

-Kishon

On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

quoted

Hi Kishon,
 I wonder if you could queue the following for the next merge window.
 They allow the twl4030 phy to provide more information to the
 twl4030 battery charger.
 There are only minimal changes since the first version, particularly
 documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

Hi,
 are you able to more specific at all?  What is the "USB PHY library"?
Where exactly is the "PHY framework"?

I know none of the history here and while I could try to guess I suspect
there is an even chance I would get wrong.
I'm happy to do the work but I want to be sure of what you are asking.

Thanks,
NeilBrown

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

From: NeilBrown <redacted>

A construct like:

        if (pm_runtime_suspended(twl->dev))
               pm_runtime_get_sync(twl->dev);

is against the spirit of the runtime_pm interface as it
makes the internal refcounting useless.

In this case it is also racy, particularly as 'put_autosuspend'
is use to drop a reference.
When that happens a timer is started and the device is
runtime-suspended after the timeout.
If the above code runs in this window, the device will not be
found to be suspended so no pm_runtime reference is taken.
When the timer expires the device will be suspended, which is
against the intention of the code.

So be more direct is taking and dropping references.
If twl->linkstat is VBUS_VALID or ID_GROUND, then hold a
pm_runtime reference, otherwise don't.
Define "cable_present()" to test for this condition.

minor comment below..

twl4030_cable_present?

-Kishon

Hi NeilBrown,

On Thursday 26 March 2015 02:52 AM, NeilBrown wrote:

On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

quoted

Hi Kishon,
 I wonder if you could queue the following for the next merge window.
 They allow the twl4030 phy to provide more information to the
 twl4030 battery charger.
 There are only minimal changes since the first version, particularly
 documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

Hi,
 are you able to more specific at all?  What is the "USB PHY library"?
Where exactly is the "PHY framework"?

There is a USB PHY library that exists in drivers/usb/phy/phy.c and there is
a Generic PHY framework that is present in drivers/phy/phy-core.c. twl4030
actually supports both the framework.

In your patch whatever uses struct usb_phy uses the old USB PHY library and
whatever uses struct phy uses the generic PHY framework. (Actually your patch
does not use the PHY framework at all). We want to deprecate using the USB PHY
library and make everyone use the generic PHY framework. Adding features
to a driver using the USB PHY library will make the transition to generic PHY
framework a bit more difficult.

Now all the features that is supported in the USB PHY library may not be
supported by the PHY framework. So we should start extending the PHY framework
instead of using the USB PHY library.

One think I noticed in your driver is using atomic notifier chain. IMO extcon
framework should be used in twl4030 USB driver to notify the controller driver
instead of using USB PHY notifier. For all other things we have to see if it
can be added in the PHY framework.

Thanks
Kishon

I know none of the history here and while I could try to guess I suspect
there is an even chance I would get wrong.
I'm happy to do the work but I want to be sure of what you are asking.

Thanks,
NeilBrown

On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

Hi NeilBrown,

On Thursday 26 March 2015 02:52 AM, NeilBrown wrote:

quoted

On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

quoted

Hi Kishon,
 I wonder if you could queue the following for the next merge window.
 They allow the twl4030 phy to provide more information to the
 twl4030 battery charger.
 There are only minimal changes since the first version, particularly
 documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

Hi,
 are you able to more specific at all?  What is the "USB PHY library"?
Where exactly is the "PHY framework"?

There is a USB PHY library that exists in drivers/usb/phy/phy.c and there is
a Generic PHY framework that is present in drivers/phy/phy-core.c. twl4030
actually supports both the framework.

In your patch whatever uses struct usb_phy uses the old USB PHY library and
whatever uses struct phy uses the generic PHY framework. (Actually your patch
does not use the PHY framework at all). We want to deprecate using the USB PHY
library and make everyone use the generic PHY framework. Adding features
to a driver using the USB PHY library will make the transition to generic PHY
framework a bit more difficult.

Now all the features that is supported in the USB PHY library may not be
supported by the PHY framework. So we should start extending the PHY framework
instead of using the USB PHY library.

One think I noticed in your driver is using atomic notifier chain. IMO extcon
framework should be used in twl4030 USB driver to notify the controller driver
instead of using USB PHY notifier. For all other things we have to see if it
can be added in the PHY framework.

Thanks a lot - exactly what I wanted.
I agree about extcon - I'll be very happy to make that work properly for twl.

I'll let you know when I have something for review.

Thanks,
NeilBrown

quoted

diff --git a/drivers/phy/phy-twl4030-usb.c

    b/drivers/phy/phy-twl4030-usb.c

quoted

index 8e87f54671f3..1a244f34b748 100644

--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c

@@ -144,6 +144,16 @@
 #define PMBR1				0x0D
 #define GPIO_USB_4PIN_ULPI_2430C	(3 << 0)
 
+/*
+ * If VBUS is valid or ID is ground, then we know a
+ * cable is present and we need to be runtime-enabled
+ */
+static inline bool cable_present(enum omap_musb_vbus_id_status stat)

twl4030_cable_present?

It is static function, no need for prefixes, they just make code
harder to read.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

Hi NeilBrown,

On Thursday 26 March 2015 02:52 AM, NeilBrown wrote:

quoted

On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

quoted

Hi Kishon,
 I wonder if you could queue the following for the next merge window.
 They allow the twl4030 phy to provide more information to the
 twl4030 battery charger.
 There are only minimal changes since the first version, particularly
 documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

Hi,
 are you able to more specific at all?  What is the "USB PHY library"?
Where exactly is the "PHY framework"?

There is a USB PHY library that exists in drivers/usb/phy/phy.c and there is
a Generic PHY framework that is present in drivers/phy/phy-core.c. twl4030
actually supports both the framework.

In your patch whatever uses struct usb_phy uses the old USB PHY library and
whatever uses struct phy uses the generic PHY framework. (Actually your patch
does not use the PHY framework at all). We want to deprecate using the USB PHY
library and make everyone use the generic PHY framework. Adding features
to a driver using the USB PHY library will make the transition to generic PHY
framework a bit more difficult.

Now all the features that is supported in the USB PHY library may not be
supported by the PHY framework. So we should start extending the PHY framework
instead of using the USB PHY library.

One think I noticed in your driver is using atomic notifier chain. IMO extcon
framework should be used in twl4030 USB driver to notify the controller driver
instead of using USB PHY notifier. For all other things we have to see if it
can be added in the PHY framework.

I've had a look at the code with these issues in mind, and there is one issue
that I'm not sure about.

In phy-twl4030-usb, the usb_phy is used to hold a reference to the
'struct otg', and for passing cable state changes to the notifier.

The former probably has to stay until musb can keep a reference to the otg,
separate form the usb_phy.  The latter can be changed to use extcon - to
some extent.  I actually have patches to do that from a couple of years back,
but I never proceeded with them.

The problem is that one thing that needs to be communicated to the charger is
the max current that was negotiated by a "Standard Downstream Port".
This could be 500mA from a powered hum, or much less from an unpowered hub.
(Currently the usb gadget code does negotiated between different
possibilities, but it could and hopefully will one day).

With the notifier chain there is an easy way to communicate the allowed
current once it is negotiated. e.g. ab8500_usb_set_power() does this.

'struct phy' has no equivalent of the 'set_power' callback  which 'struct
usb_phy' provides, and extcon has no mechanism (that I can see) for
communicating a number - just binary cable states.

Presumably a 'set_power' method could be added to 'struct phy' so the
usb-core can communicate the number to the phy, but it is not clear to me how
the 'phy' can communicate it to the charger.
The 'phy' could provide an API to request the current negotiated max current,
but there still needs to be a way to let the charger know that this has
changed.
That could in theory be done via extcon, by having a secondary
'USB_connected' cable type, but it isn't really a cable type and pretending
that it is seems wrong.

Any suggestions?

Thanks,
NeilBrown

+Extcon MAINTAINERS

Hi,

On Wednesday 01 April 2015 10:11 AM, NeilBrown wrote:

On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi NeilBrown,

On Thursday 26 March 2015 02:52 AM, NeilBrown wrote:

quoted

On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

quoted

Hi Kishon,
  I wonder if you could queue the following for the next merge window.
  They allow the twl4030 phy to provide more information to the
  twl4030 battery charger.
  There are only minimal changes since the first version, particularly
  documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

Hi,
  are you able to more specific at all?  What is the "USB PHY library"?
Where exactly is the "PHY framework"?

There is a USB PHY library that exists in drivers/usb/phy/phy.c and there is
a Generic PHY framework that is present in drivers/phy/phy-core.c. twl4030
actually supports both the framework.

In your patch whatever uses struct usb_phy uses the old USB PHY library and
whatever uses struct phy uses the generic PHY framework. (Actually your patch
does not use the PHY framework at all). We want to deprecate using the USB PHY
library and make everyone use the generic PHY framework. Adding features
to a driver using the USB PHY library will make the transition to generic PHY
framework a bit more difficult.

Now all the features that is supported in the USB PHY library may not be
supported by the PHY framework. So we should start extending the PHY framework
instead of using the USB PHY library.

One think I noticed in your driver is using atomic notifier chain. IMO extcon
framework should be used in twl4030 USB driver to notify the controller driver
instead of using USB PHY notifier. For all other things we have to see if it
can be added in the PHY framework.

I've had a look at the code with these issues in mind, and there is one issue
that I'm not sure about.

In phy-twl4030-usb, the usb_phy is used to hold a reference to the
'struct otg', and for passing cable state changes to the notifier.

right now we directly call omap_musb_mailbox no? we don't use notifiers right?

The former probably has to stay until musb can keep a reference to the otg,
separate form the usb_phy.  The latter can be changed to use extcon - to
some extent.  I actually have patches to do that from a couple of years back,
but I never proceeded with them.

The problem is that one thing that needs to be communicated to the charger is
the max current that was negotiated by a "Standard Downstream Port".
This could be 500mA from a powered hum, or much less from an unpowered hub.
(Currently the usb gadget code does negotiated between different
possibilities, but it could and hopefully will one day).

With the notifier chain there is an easy way to communicate the allowed
current once it is negotiated. e.g. ab8500_usb_set_power() does this.

'struct phy' has no equivalent of the 'set_power' callback  which 'struct
usb_phy' provides, and extcon has no mechanism (that I can see) for
communicating a number - just binary cable states.

Chanwoo Choi, Can this be modified so that we can communicate numbers like in
the case of EXTCON_CHARGE_DOWNSTREAM?

Presumably a 'set_power' method could be added to 'struct phy' so the
usb-core can communicate the number to the phy, but it is not clear to me how
the 'phy' can communicate it to the charger.

Should the PHY be involved in all this? We can make the gadget driver
directly communicate the value to the charger no?

The 'phy' could provide an API to request the current negotiated max current,
but there still needs to be a way to let the charger know that this has
changed.
That could in theory be done via extcon, by having a secondary
'USB_connected' cable type, but it isn't really a cable type and pretending
that it is seems wrong.

I think EXTCON_CHARGE_DOWNSTREAM was created for that purpose. Chanwoo?

Thanks
Kishon

On Fri, 3 Apr 2015 19:08:22 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

+Extcon MAINTAINERS

Hi,

On Wednesday 01 April 2015 10:11 AM, NeilBrown wrote:

quoted

On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi NeilBrown,

On Thursday 26 March 2015 02:52 AM, NeilBrown wrote:

quoted

On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

quoted

Hi Kishon,
  I wonder if you could queue the following for the next merge window.
  They allow the twl4030 phy to provide more information to the
  twl4030 battery charger.
  There are only minimal changes since the first version, particularly
  documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

Hi,
  are you able to more specific at all?  What is the "USB PHY library"?
Where exactly is the "PHY framework"?

There is a USB PHY library that exists in drivers/usb/phy/phy.c and there is
a Generic PHY framework that is present in drivers/phy/phy-core.c. twl4030
actually supports both the framework.

In your patch whatever uses struct usb_phy uses the old USB PHY library and
whatever uses struct phy uses the generic PHY framework. (Actually your patch
does not use the PHY framework at all). We want to deprecate using the USB PHY
library and make everyone use the generic PHY framework. Adding features
to a driver using the USB PHY library will make the transition to generic PHY
framework a bit more difficult.

Now all the features that is supported in the USB PHY library may not be
supported by the PHY framework. So we should start extending the PHY framework
instead of using the USB PHY library.

One think I noticed in your driver is using atomic notifier chain. IMO extcon
framework should be used in twl4030 USB driver to notify the controller driver
instead of using USB PHY notifier. For all other things we have to see if it
can be added in the PHY framework.

I've had a look at the code with these issues in mind, and there is one issue
that I'm not sure about.

In phy-twl4030-usb, the usb_phy is used to hold a reference to the
'struct otg', and for passing cable state changes to the notifier.

right now we directly call omap_musb_mailbox no? we don't use notifiers right?

Correct, and omap_musb_set_mailbox uses the notifier chain. 
phy-twl4030-usb does 
	ATOMIC_INIT_NOTIFIER_HEAD(&twl->phy.notifier);
That is the only place the current phy code interacts with the notifier chain.

quoted

The former probably has to stay until musb can keep a reference to the otg,
separate form the usb_phy.  The latter can be changed to use extcon - to
some extent.  I actually have patches to do that from a couple of years back,
but I never proceeded with them.

The problem is that one thing that needs to be communicated to the charger is
the max current that was negotiated by a "Standard Downstream Port".
This could be 500mA from a powered hum, or much less from an unpowered hub.
(Currently the usb gadget code does negotiated between different
possibilities, but it could and hopefully will one day).

With the notifier chain there is an easy way to communicate the allowed
current once it is negotiated. e.g. ab8500_usb_set_power() does this.

'struct phy' has no equivalent of the 'set_power' callback  which 'struct
usb_phy' provides, and extcon has no mechanism (that I can see) for
communicating a number - just binary cable states.

Chanwoo Choi, Can this be modified so that we can communicate numbers like in
the case of EXTCON_CHARGE_DOWNSTREAM?

quoted

Presumably a 'set_power' method could be added to 'struct phy' so the
usb-core can communicate the number to the phy, but it is not clear to me how
the 'phy' can communicate it to the charger.

Should the PHY be involved in all this? We can make the gadget driver
directly communicate the value to the charger no?

quoted

The 'phy' could provide an API to request the current negotiated max current,
but there still needs to be a way to let the charger know that this has
changed.
That could in theory be done via extcon, by having a secondary
'USB_connected' cable type, but it isn't really a cable type and pretending
that it is seems wrong.

I think EXTCON_CHARGE_DOWNSTREAM was created for that purpose. Chanwoo?

EXTCON_CHARGE_DOWNSTREAM is something quite different.

There are roughly three ways that the USB gadget can determine what sort of
thing has been plugged in to it and what current it can draw.

 - it can look at the resistance between the ID pin and GROUND.  This is a
   physical property of the cable and it makes a lot of sense of EXTCON
   to report different cables based on different resistances.

 - it can look at the voltage provided on different pins.  If it detects a
   certain voltage on D- when it asserts a voltage on D+, it can know
   that it is a Charging Downstream Port (EXTCON_CHARGE_DOWNSTREAM).  This
   might be a property of the cable (shorting D- to D+ can achieve this) or
   might be a property of the attached device.  It makes some sense for
   EXTCON to report cable type based on this sort of information.

 - it can wait for the connected host to initiate a USB session and select a
   particular profile.  That profile will include a "MaxPower" field.  When
   the host selects that profile, the gadget knows it is allowed to draw that
   much power ("current" really, measured in mA).

So EXTCON_CHARGE_DOWNSTREAM fits into the second category.  My question is
about the third category.
I need this "MaxPower" number to be communicated from the USB core to the
charger driver, presumably via the "phy" driver.

With "usb_phy", there is a ->set_power() callback to communicate from
usb-core to phy, and a notifier chain to communicate from phy to charger.
With "phy" there is nothing.

extcon _could_ be used to communicate the MaxPower, but I'm not sure it makes
sense.  It is a property of the power available on the cable, not a property
of the cable itself, or something that can be deduced from the properties of
the cable.

I guess I could get musb_gadget_vbus_draw(), or usb_phy_set_power(), to call
the notifier chain directly.  But if usb_phy is being deprecated, that isn't
a long-term solution.

Thanks,
NeilBrown

Hi,

On Saturday 04 April 2015 05:58 AM, NeilBrown wrote:

On Fri, 3 Apr 2015 19:08:22 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

+Extcon MAINTAINERS

Hi,

On Wednesday 01 April 2015 10:11 AM, NeilBrown wrote:

quoted

On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi NeilBrown,

On Thursday 26 March 2015 02:52 AM, NeilBrown wrote:

quoted

On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I [off-list ref]
wrote:

quoted

Hi,

On Monday 23 March 2015 04:05 AM, NeilBrown wrote:

quoted

Hi Kishon,
   I wonder if you could queue the following for the next merge window.
   They allow the twl4030 phy to provide more information to the
   twl4030 battery charger.
   There are only minimal changes since the first version, particularly
   documentation has been improved.

There are quite a few things in this series which use the USB PHY library
interface which is kindof deprecated. We should try and use the Generic PHY
library for all of them. It would also be better to add features to the
PHY framework if the we can't achieve something with the existing PHY
framework.

Hi,
   are you able to more specific at all?  What is the "USB PHY library"?
Where exactly is the "PHY framework"?

There is a USB PHY library that exists in drivers/usb/phy/phy.c and there is
a Generic PHY framework that is present in drivers/phy/phy-core.c. twl4030
actually supports both the framework.

In your patch whatever uses struct usb_phy uses the old USB PHY library and
whatever uses struct phy uses the generic PHY framework. (Actually your patch
does not use the PHY framework at all). We want to deprecate using the USB PHY
library and make everyone use the generic PHY framework. Adding features
to a driver using the USB PHY library will make the transition to generic PHY
framework a bit more difficult.

Now all the features that is supported in the USB PHY library may not be
supported by the PHY framework. So we should start extending the PHY framework
instead of using the USB PHY library.

One think I noticed in your driver is using atomic notifier chain. IMO extcon
framework should be used in twl4030 USB driver to notify the controller driver
instead of using USB PHY notifier. For all other things we have to see if it
can be added in the PHY framework.

I've had a look at the code with these issues in mind, and there is one issue
that I'm not sure about.

In phy-twl4030-usb, the usb_phy is used to hold a reference to the
'struct otg', and for passing cable state changes to the notifier.

right now we directly call omap_musb_mailbox no? we don't use notifiers right?

Correct, and omap_musb_set_mailbox uses the notifier chain.
phy-twl4030-usb does
	ATOMIC_INIT_NOTIFIER_HEAD(&twl->phy.notifier);
That is the only place the current phy code interacts with the notifier chain.

ah.. okay.

quoted

quoted

The former probably has to stay until musb can keep a reference to the otg,
separate form the usb_phy.  The latter can be changed to use extcon - to
some extent.  I actually have patches to do that from a couple of years back,
but I never proceeded with them.

The problem is that one thing that needs to be communicated to the charger is
the max current that was negotiated by a "Standard Downstream Port".
This could be 500mA from a powered hum, or much less from an unpowered hub.
(Currently the usb gadget code does negotiated between different
possibilities, but it could and hopefully will one day).

With the notifier chain there is an easy way to communicate the allowed
current once it is negotiated. e.g. ab8500_usb_set_power() does this.

'struct phy' has no equivalent of the 'set_power' callback  which 'struct
usb_phy' provides, and extcon has no mechanism (that I can see) for
communicating a number - just binary cable states.

Chanwoo Choi, Can this be modified so that we can communicate numbers like in
the case of EXTCON_CHARGE_DOWNSTREAM?

quoted

Presumably a 'set_power' method could be added to 'struct phy' so the
usb-core can communicate the number to the phy, but it is not clear to me how
the 'phy' can communicate it to the charger.

Should the PHY be involved in all this? We can make the gadget driver
directly communicate the value to the charger no?

quoted

The 'phy' could provide an API to request the current negotiated max current,
but there still needs to be a way to let the charger know that this has
changed.
That could in theory be done via extcon, by having a secondary
'USB_connected' cable type, but it isn't really a cable type and pretending
that it is seems wrong.

I think EXTCON_CHARGE_DOWNSTREAM was created for that purpose. Chanwoo?

EXTCON_CHARGE_DOWNSTREAM is something quite different.

There are roughly three ways that the USB gadget can determine what sort of
thing has been plugged in to it and what current it can draw.

  - it can look at the resistance between the ID pin and GROUND.  This is a
    physical property of the cable and it makes a lot of sense of EXTCON
    to report different cables based on different resistances.

  - it can look at the voltage provided on different pins.  If it detects a
    certain voltage on D- when it asserts a voltage on D+, it can know
    that it is a Charging Downstream Port (EXTCON_CHARGE_DOWNSTREAM).  This
    might be a property of the cable (shorting D- to D+ can achieve this) or
    might be a property of the attached device.  It makes some sense for
    EXTCON to report cable type based on this sort of information.

  - it can wait for the connected host to initiate a USB session and select a
    particular profile.  That profile will include a "MaxPower" field.  When
    the host selects that profile, the gadget knows it is allowed to draw that
    much power ("current" really, measured in mA).

Thanks for that explanation :-)

So EXTCON_CHARGE_DOWNSTREAM fits into the second category.  My question is
about the third category.
I need this "MaxPower" number to be communicated from the USB core to the
charger driver, presumably via the "phy" driver.

With "usb_phy", there is a ->set_power() callback to communicate from
usb-core to phy, and a notifier chain to communicate from phy to charger.
With "phy" there is nothing.

set_power sounds very specific to USB. Just thinking if we should make use of 
the regulator framework to set the current. With this the usb should create a 
dummy regulator and set the current and the charger can use the regulator.

Not sure if that makes sense though:-/

Thanks
Kishon