Enhancements to twl4030 phy to support better charging - V2

From: NeilBrown <[email protected]>

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 <[email protected]>

Signed-off-by NeilBrown <[email protected]>

---
 drivers/phy/phy-twl4030-usb.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

drivers/phy/phy-twl4030-usb.c +0 -0

index 8e87f54671f3..1a244f34b748 100644

Review

Quoted 1 line

(quoted diff) +0 -0

Quoted 7 lines

index 8e87f54671f3..1a244f34b748 100644

drivers/phy/phy-twl4030-usb.c +0 -0

@@ -144,6 +144,16 @@
 #define PMBR1				0x0D
 #define GPIO_USB_4PIN_ULPI_2430C	(3 << 0)
 

drivers/phy/phy-twl4030-usb.c line 151

Quoted 5 lines

drivers/phy/phy-twl4030-usb.c +5 -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?

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.

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.

Hi,

Quoted 6 lines · NeilBrown

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.

Quoted 10 lines · On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I…

Hi,

Quoted 6 lines · NeilBrown

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.

Quoted 5 lines

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 NeilBrown,

Quoted 19 lines · NeilBrown

Quoted 10 lines · On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I…

Hi,

Quoted 6 lines · NeilBrown

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Hi NeilBrown,

Quoted 19 lines · NeilBrown

Quoted 10 lines · On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I <[email protected]> wrote:

Hi,

Quoted 6 lines · NeilBrown

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 37 lines · On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I…

Hi NeilBrown,

Quoted 19 lines · NeilBrown

Quoted 10 lines · On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I…

Hi,

Quoted 6 lines · NeilBrown

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

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.

+Extcon MAINTAINERS

Hi,

Quoted 49 lines · NeilBrown

Quoted 45 lines · On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I…

Hi NeilBrown,

Quoted 19 lines · NeilBrown

Quoted 10 lines · On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I <[email protected]> wrote:

Hi,

Quoted 6 lines · NeilBrown

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 12 lines

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).

Quoted 6 lines

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 4 lines

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 6 lines

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?

Quoted 60 lines · On Fri, 3 Apr 2015 19:08:22 +0530 Kishon Vijay Abraham I…

+Extcon MAINTAINERS

Hi,

Quoted 54 lines · NeilBrown

Quoted 37 lines · On Thu, 26 Mar 2015 05:29:42 +0530 Kishon Vijay Abraham I…

Hi NeilBrown,

Quoted 19 lines · NeilBrown

Quoted 10 lines · On Thu, 26 Mar 2015 02:46:32 +0530 Kishon Vijay Abraham I…

Hi,

Quoted 6 lines · NeilBrown

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 1 line

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 30 lines

Quoted 14 lines

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).

Quoted 6 lines

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 4 lines

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 9 lines

Quoted 6 lines

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.