Introduce usb charger framework to deal with the usb gadget power negotation

Hi Baolin,

[auto build test ERROR on v4.9-rc8]
[cannot apply to balbi-usb/next usb/usb-testing battery/master next-20170220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20170220-173051
config: i386-randconfig-x015-201708 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All errors (new ones prefixed by >>):

In file included from drivers/usb/gadget/udc/core.c:28:0:
include/linux/usb/charger.h: In function 'usb_charger_get_state':

Quoted 1 line

Quoted 1 line

include/linux/usb/charger.h:151:9: error: 'USB_CHARGER_REMOVE' undeclared (first use in this function)

     return USB_CHARGER_REMOVE;

            ^~~~~~~~~~~~~~~~~~

include/linux/usb/charger.h:151:9: note: each undeclared identifier is reported only once for each function it appears in

vim +/USB_CHARGER_REMOVE +151 include/linux/usb/charger.h

28929615 Baolin Wang 2017-02-20  145    return UNKNOWN_TYPE;
28929615 Baolin Wang 2017-02-20  146  }
28929615 Baolin Wang 2017-02-20  147
28929615 Baolin Wang 2017-02-20  148  static inline enum usb_charger_state
28929615 Baolin Wang 2017-02-20  149  usb_charger_get_state(struct usb_charger *uchger)
28929615 Baolin Wang 2017-02-20  150  {
28929615 Baolin Wang 2017-02-20 @151    return USB_CHARGER_REMOVE;
28929615 Baolin Wang 2017-02-20  152  }
28929615 Baolin Wang 2017-02-20  153
28929615 Baolin Wang 2017-02-20  154  static inline int usb_charger_detect_type(struct usb_charger *uchger)

:::::: The code at line 151 was first introduced by commit
:::::: 289296154f05985dedc08273f04d5b738450b3c7 usb: gadget: Introduce the usb charger framework

:::::: TO: Baolin Wang <[email protected]>
:::::: CC: 0day robot <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as they should. Thus provide a standard framework for doing this in kernel.

Now introduce one user with wm831x_power to support and test the usb charger. Another user introduced to support charger detection by Jun Li:
https://www.spinics.net/lists/linux-usb/msg139425.html
Moreover there may be other potential users will use it in future.

1. Before v19 patchset we've fixed below issues in extcon subsystem and usb phy driver, now all were merged. (Thanks for Neil's suggestion)
(1) Have fixed the inconsistencies with USB connector types in extcon subsystem by following links:
https://lkml.org/lkml/2016/12/21/13
https://lkml.org/lkml/2016/12/21/15
https://lkml.org/lkml/2016/12/21/79
https://lkml.org/lkml/2017/1/3/13

(2) Instead of using 'set_power' callback in phy drivers, we will introduce USB charger to set PMIC current drawn from USB configuration, moreover some 'set_power' callbacks did not implement anything to set PMIC current, thus remove them by following links:
https://lkml.org/lkml/2017/1/18/436
https://lkml.org/lkml/2017/1/18/439
https://lkml.org/lkml/2017/1/18/438
Now only two phy drivers (phy-isp1301-omap.c and phy-gpio-vbus-usb.c) still used 'set_power' callback to set current, we can remove them in future. (I have no platform with enabling these two phy drivers, so I can not test them if I converted 'set_power' callback to USB charger.)

2. Some issues pointed by Neil Brown were sill kept in this v19 patchset, and I expalined each issue and may be need discuss again:
(1) Change all usb phys to register an extcon and to send appropriate notifications. Firstly, now only 3 USB phy drivers (phy-qcom-8x16-usb.c, phy-omap-otg.c and phy-msm-usb.c) had registered an extcon, mostly did not. I can not change all usb phys to register an extcon, since there are no extcon device to register for these different phy drivers.

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

(2) Change the notifier of usb_phy to be used consistently.
Now only 3 phy drivers (phy-generic.c, phy-ab8500-usb.c and phy-gpio-vbus-usb.c) used the notifier of usb_phy. phy-generic.c and phy-gpio-vbus-usb.c were used to send out the connect events, and phy-ab8500-usb.c also was used to send out the MUSB connect events. There are no phy drivers will notify 'vbus_draw' information by the notifier of usb_phy, which was used consistently now. Moreover it is difficult to change the notifier of usb_phy to be used only to communicate the 'vbus_draw' information, since we need to refactor and test these related phy drivers, power drivers or some mfd drivers, which is a huge workload.

(3) Still keep charger_type_show() API.
Firstly I think we should combine all charger related information into one place for users, which is convenient.

Secondly not only we get charger type from extcon, but also in some scenarios we can get charger type from USB controller driver, USB type-c driver, pmic driver, we should also need one place to export the charger type.

Quoted 32 lines · Baolin Wang

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as they should. Thus provide a standard framework for doing this in kernel.

Now introduce one user with wm831x_power to support and test the usb charger. Another user introduced to support charger detection by Jun Li:
https://www.spinics.net/lists/linux-usb/msg139425.html
Moreover there may be other potential users will use it in future.

1. Before v19 patchset we've fixed below issues in extcon subsystem and usb phy driver, now all were merged. (Thanks for Neil's suggestion)
(1) Have fixed the inconsistencies with USB connector types in extcon subsystem by following links:
https://lkml.org/lkml/2016/12/21/13
https://lkml.org/lkml/2016/12/21/15
https://lkml.org/lkml/2016/12/21/79
https://lkml.org/lkml/2017/1/3/13

(2) Instead of using 'set_power' callback in phy drivers, we will introduce USB charger to set PMIC current drawn from USB configuration, moreover some 'set_power' callbacks did not implement anything to set PMIC current, thus remove them by following links:
https://lkml.org/lkml/2017/1/18/436
https://lkml.org/lkml/2017/1/18/439
https://lkml.org/lkml/2017/1/18/438
Now only two phy drivers (phy-isp1301-omap.c and phy-gpio-vbus-usb.c) still used 'set_power' callback to set current, we can remove them in future. (I have no platform with enabling these two phy drivers, so I can not test them if I converted 'set_power' callback to USB charger.)

Quoted 7 lines

2. Some issues pointed by Neil Brown were sill kept in this v19 patchset, and I expalined each issue and may be need discuss again:
(1) Change all usb phys to register an extcon and to send appropriate notifications. Firstly, now only 3 USB phy drivers (phy-qcom-8x16-usb.c, phy-omap-otg.c and phy-msm-usb.c) had registered an extcon, mostly did not. I can not change all usb phys to register an extcon, since there are no extcon device to register for these different phy drivers.

You don't have to change every driver. You just need to make it easy and obvious how to change drivers in a consistent coherent way. For a start you would add a 'struct extcon_dev' to 'struct usb_phy', and possibly add or extend some 'static inline's in linux/usb/phy.h to send notification on that extcon (if it is non-NULL).
e.g. usb_phy_vbus_on() could send an extcon notification.

Then any phy driver which adds support for setting phy->extcon_dev appropriately, immediately gets the relevant notifications sent.

Quoted 6 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification.
Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together.
Somehow these three need to work together to determine what is plugged in to the external connection port. Somewhere there much an 'extcon' device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the extcon. It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that represents it.

Quoted 12 lines

(2) Change the notifier of usb_phy to be used consistently.
Now only 3 phy drivers (phy-generic.c, phy-ab8500-usb.c and phy-gpio-vbus-usb.c) used the notifier of usb_phy. phy-generic.c and phy-gpio-vbus-usb.c were used to send out the connect events, and phy-ab8500-usb.c also was used to send out the MUSB connect events. There are no phy drivers will notify 'vbus_draw' information by the notifier of usb_phy, which was used consistently now. Moreover it is difficult to change the notifier of usb_phy to be used only to communicate the 'vbus_draw' information, since we need to refactor and test these related phy drivers, power drivers or some mfd drivers, which is a huge workload.

You missed drivers/usb/musb/omap2430.c in you list, but that hardly matters.

phy-ab8500-usb.c appears to send vbus_draw information.

I understand your reluctance to change drivers that you cannot test. An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to declare the passing of NULL to be deprecated (so hopefully people won't use it in new code).
Then any notification callback that expects a current can just ignore calls where the pointer is NULL.

The one difficulty with this is drivers/usb/gadget/udc/pxa27x_udc.c It is the only driver which expects a 'gadget', and it doesn't really need to as it already knows the gadget.
The patch below fixes this.
With that in place, phy-generic and phy-gpio-vbus-usb can be changed to pass NULL. When we can safely use the notifier to pass vbus_draw information uniformly.

Quoted 4 lines

(3) Still keep charger_type_show() API.
Firstly I think we should combine all charger related information into one place for users, which is convenient.

convenience is very much a secondary issue.

Quoted 3 lines

Secondly not only we get charger type from extcon, but also in some scenarios we can get charger type from USB controller driver, USB type-c driver, pmic driver, we should also need one place to export the charger type.

As I have said, all of these sources of information should feed into the extcon.

There are ultimately two possible sources of information about the current available from the usb port.
One is the physical properties of the cable, such as resistance of ID, any short between D+ and D- etc. Being properties of the cable, they should be reported through the extcon.

The other is information gathered during the USB protocol handshake. For USB2, this is the requested current of the profile that the host activates. This should be reported though the USB gadget device.

I don't know how USB3 and/or type-C work but I would be surprised if they don't fit into the two cases above. If you think otherwise, please surprise me. I'm always keen to learn.

If the extcon reports the type of cable detected, and the gadget reports the result of any negotiation, then that is enough to determine the charger type. It doesn't need to be more convenient than that.

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Tuesday, March 07, 2017 5:39 PM
To: NeilBrown <[email protected]>
Cc: Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Jun Li <[email protected]>; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 7 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as they should. Thus provide a

standard framework for doing this in kernel.

Quoted 2 lines

Quoted 2 lines

Now introduce one user with wm831x_power to support and test the usb

charger.

Quoted 27 lines

Quoted 27 lines

Another user introduced to support charger detection by Jun Li:
https://www.spinics.net/lists/linux-usb/msg139425.html
Moreover there may be other potential users will use it in future.

1. Before v19 patchset we've fixed below issues in extcon subsystem and usb phy driver, now all were merged. (Thanks for Neil's suggestion)
(1) Have fixed the inconsistencies with USB connector types in extcon subsystem by following links:
https://lkml.org/lkml/2016/12/21/13
https://lkml.org/lkml/2016/12/21/15
https://lkml.org/lkml/2016/12/21/79
https://lkml.org/lkml/2017/1/3/13

(2) Instead of using 'set_power' callback in phy drivers, we will introduce USB charger to set PMIC current drawn from USB configuration, moreover some 'set_power' callbacks did not implement anything to set PMIC current, thus remove them by following links:
https://lkml.org/lkml/2017/1/18/436
https://lkml.org/lkml/2017/1/18/439
https://lkml.org/lkml/2017/1/18/438
Now only two phy drivers (phy-isp1301-omap.c and phy-gpio-vbus-usb.c) still used 'set_power' callback to set current, we can remove them in future. (I have no platform with enabling these two phy drivers, so I can not test them if I converted 'set_power' callback to USB charger.)

Quoted 3 lines

Quoted 3 lines

2. Some issues pointed by Neil Brown were sill kept in this v19 patchset, and I expalined each issue and may be need discuss again:
(1) Change all usb phys to register an extcon and to send appropriate

notifications.

Quoted 13 lines

Quoted 5 lines

Firstly, now only 3 USB phy drivers (phy-qcom-8x16-usb.c, phy-omap-otg.c and
phy-msm-usb.c) had registered an extcon, mostly did not. I can not change all usb phys to register an extcon, since there are no extcon device to register for these different phy drivers.

You don't have to change every driver. You just need to make it easy and obvious how to change drivers in a consistent coherent way. For a start you would add a 'struct extcon_dev' to 'struct usb_phy', and possibly add or extend some 'static inline's in linux/usb/phy.h to send notification on that extcon (if it is non-NULL).
e.g. usb_phy_vbus_on() could send an extcon notification.

Quoted 2 lines

Then any phy driver which adds support for setting phy->extcon_dev appropriately, immediately gets the relevant notifications sent.

OK. We can make these extcon related code into phy common part.

Quoted 7 lines

Quoted 6 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

Quoted 6 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification.
Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together.
Somehow these three need to work together to determine what is

plugged

Quoted 9 lines

in to the external connection port. Somewhere there much an 'extcon' device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the extcon. It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

Quoted 2 lines

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that represents it.

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by software?

Quoted 8 lines

Quoted 8 lines

(2) Change the notifier of usb_phy to be used consistently. Now only 3 phy drivers (phy-generic.c, phy-ab8500-usb.c and phy-gpio-vbus-usb.c) used the notifier of usb_phy. phy-generic.c and phy-gpio-vbus-usb.c were used to send out the connect events, and phy-ab8500-usb.c also was used to send out the MUSB connect events. There are no phy drivers will notify 'vbus_draw' information by the

notifier of usb_phy, which was used consistently now.

Quoted 7 lines

Quoted 4 lines

Moreover it is difficult to change the notifier of usb_phy to be used only to communicate the 'vbus_draw' information, since we need to refactor and test these related phy drivers, power drivers or some mfd drivers, which is a huge workload.

You missed drivers/usb/musb/omap2430.c in you list, but that hardly matters.

But it did not use the notifier of usb_phy.

Quoted 1 line

phy-ab8500-usb.c appears to send vbus_draw information.

Users will not use the vbus_draw information send from phy-ab8500-usb.c

Quoted 9 lines

I understand your reluctance to change drivers that you cannot test. An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to declare the passing of NULL to be deprecated (so hopefully people won't use it in new code).
Then any notification callback that expects a current can just ignore calls where the pointer is NULL.

I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.

Quoted 10 lines

The one difficulty with this is drivers/usb/gadget/udc/pxa27x_udc.c It is the only driver which expects a 'gadget', and it doesn't really need to as it already knows the gadget.
The patch below fixes this.
With that in place, phy-generic and phy-gpio-vbus-usb can be changed to pass NULL. When we can safely use the notifier to pass vbus_draw information uniformly.

Quoted 1 line

Quoted 9 lines

Quoted 3 lines

(3) Still keep charger_type_show() API.
Firstly I think we should combine all charger related information into one place for users, which is convenient.

convenience is very much a secondary issue.

Quoted 3 lines

Secondly not only we get charger type from extcon, but also in some scenarios we can get charger type from USB controller driver, USB type-c driver, pmic driver, we should also need one place to export the

charger type.

Quoted 14 lines

As I have said, all of these sources of information should feed into the extcon.

There are ultimately two possible sources of information about the current available from the usb port.
One is the physical properties of the cable, such as resistance of ID, any short between D+ and D- etc. Being properties of the cable, they should be reported through the extcon.

The other is information gathered during the USB protocol handshake. For USB2, this is the requested current of the profile that the host activates. This should be reported though the USB gadget device.

Quoted 7 lines

I don't know how USB3 and/or type-C work but I would be surprised if they don't fit into the two cases above. If you think otherwise, please surprise me. I'm always keen to learn.

If the extcon reports the type of cable detected, and the gadget reports the result of any negotiation, then that is enough to determine the charger type. It doesn't need to be more convenient than

that.

If we are all agree we did not need the USB charger, then we can add 'current' attribute of USB gadget device.
Thanks for your suggestion.

--
Baolin.wang
Best Regards

Hi,

Quoted 76 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Tuesday, March 07, 2017 5:39 PM
To: NeilBrown <[email protected]>
Cc: Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Jun Li <[email protected]>; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 7 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as they should. Thus provide a

standard framework for doing this in kernel.

Quoted 2 lines

Quoted 2 lines

Now introduce one user with wm831x_power to support and test the usb

charger.

Quoted 27 lines

Quoted 27 lines

Another user introduced to support charger detection by Jun Li:
https://www.spinics.net/lists/linux-usb/msg139425.html
Moreover there may be other potential users will use it in future.

1. Before v19 patchset we've fixed below issues in extcon subsystem and usb phy driver, now all were merged. (Thanks for Neil's suggestion)
(1) Have fixed the inconsistencies with USB connector types in extcon subsystem by following links:
https://lkml.org/lkml/2016/12/21/13
https://lkml.org/lkml/2016/12/21/15
https://lkml.org/lkml/2016/12/21/79
https://lkml.org/lkml/2017/1/3/13

(2) Instead of using 'set_power' callback in phy drivers, we will introduce USB charger to set PMIC current drawn from USB configuration, moreover some 'set_power' callbacks did not implement anything to set PMIC current, thus remove them by following links:
https://lkml.org/lkml/2017/1/18/436
https://lkml.org/lkml/2017/1/18/439
https://lkml.org/lkml/2017/1/18/438
Now only two phy drivers (phy-isp1301-omap.c and phy-gpio-vbus-usb.c) still used 'set_power' callback to set current, we can remove them in future. (I have no platform with enabling these two phy drivers, so I can not test them if I converted 'set_power' callback to USB charger.)

Quoted 3 lines

Quoted 3 lines

2. Some issues pointed by Neil Brown were sill kept in this v19 patchset, and I expalined each issue and may be need discuss again:
(1) Change all usb phys to register an extcon and to send appropriate

notifications.

Quoted 13 lines

Quoted 5 lines

Firstly, now only 3 USB phy drivers (phy-qcom-8x16-usb.c, phy-omap-otg.c and
phy-msm-usb.c) had registered an extcon, mostly did not. I can not change all usb phys to register an extcon, since there are no extcon device to register for these different phy drivers.

You don't have to change every driver. You just need to make it easy and obvious how to change drivers in a consistent coherent way. For a start you would add a 'struct extcon_dev' to 'struct usb_phy', and possibly add or extend some 'static inline's in linux/usb/phy.h to send notification on that extcon (if it is non-NULL).
e.g. usb_phy_vbus_on() could send an extcon notification.

Quoted 5 lines

Quoted 2 lines

Then any phy driver which adds support for setting phy->extcon_dev appropriately, immediately gets the relevant notifications sent.

OK. We can make these extcon related code into phy common part.

Will generic phy need add extcon as well?

Quoted 27 lines

Quoted 8 lines

Quoted 7 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

Quoted 6 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification.
Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together.
Somehow these three need to work together to determine what is

plugged

Quoted 11 lines

in to the external connection port. Somewhere there much an 'extcon' device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the extcon. It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that represents it.

Quoted 3 lines

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by software?

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework, right?

Quoted 87 lines

Quoted 8 lines

Quoted 8 lines

(2) Change the notifier of usb_phy to be used consistently. Now only 3 phy drivers (phy-generic.c, phy-ab8500-usb.c and phy-gpio-vbus-usb.c) used the notifier of usb_phy. phy-generic.c and phy-gpio-vbus-usb.c were used to send out the connect events, and phy-ab8500-usb.c also was used to send out the MUSB connect events. There are no phy drivers will notify 'vbus_draw' information by the

notifier of usb_phy, which was used consistently now.

Quoted 7 lines

Quoted 4 lines

Moreover it is difficult to change the notifier of usb_phy to be used only to communicate the 'vbus_draw' information, since we need to refactor and test these related phy drivers, power drivers or some mfd drivers, which is a huge workload.

You missed drivers/usb/musb/omap2430.c in you list, but that hardly matters.

But it did not use the notifier of usb_phy.

Quoted 1 line

phy-ab8500-usb.c appears to send vbus_draw information.

Users will not use the vbus_draw information send from phy-ab8500-usb.c

Quoted 9 lines

I understand your reluctance to change drivers that you cannot test. An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to declare the passing of NULL to be deprecated (so hopefully people won't use it in new code).
Then any notification callback that expects a current can just ignore calls where the pointer is NULL.

I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.

Quoted 10 lines

The one difficulty with this is drivers/usb/gadget/udc/pxa27x_udc.c It is the only driver which expects a 'gadget', and it doesn't really need to as it already knows the gadget.
The patch below fixes this.
With that in place, phy-generic and phy-gpio-vbus-usb can be changed to pass NULL. When we can safely use the notifier to pass vbus_draw information uniformly.

Quoted 1 line

Quoted 9 lines

Quoted 3 lines

(3) Still keep charger_type_show() API.
Firstly I think we should combine all charger related information into one place for users, which is convenient.

convenience is very much a secondary issue.

Quoted 3 lines

Secondly not only we get charger type from extcon, but also in some scenarios we can get charger type from USB controller driver, USB type-c driver, pmic driver, we should also need one place to export the

charger type.

Quoted 14 lines

As I have said, all of these sources of information should feed into the extcon.

There are ultimately two possible sources of information about the current available from the usb port.
One is the physical properties of the cable, such as resistance of ID, any short between D+ and D- etc. Being properties of the cable, they should be reported through the extcon.

The other is information gathered during the USB protocol handshake. For USB2, this is the requested current of the profile that the host activates. This should be reported though the USB gadget device.

Quoted 7 lines

I don't know how USB3 and/or type-C work but I would be surprised if they don't fit into the two cases above. If you think otherwise, please surprise me. I'm always keen to learn.

If the extcon reports the type of cable detected, and the gadget reports the result of any negotiation, then that is enough to determine the charger type. It doesn't need to be more convenient than

that.

If we are all agree we did not need the USB charger, then we can add 'current' attribute of USB gadget device.
Thanks for your suggestion.

--
Baolin.wang
Best Regards

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Thursday, March 09, 2017 2:11 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Hi,

Quoted 57 lines · Jun Li

Hi,

Quoted 55 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Tuesday, March 07, 2017 5:39 PM
To: NeilBrown <[email protected]>
Cc: Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Jun Li <[email protected]>; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 8 lines · NeilBrown

Quoted 6 lines · Baolin Wang

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as they should. Thus provide a

standard framework for doing this in kernel.

Quoted 3 lines

Quoted 3 lines

Now introduce one user with wm831x_power to support and test the usb

charger.

Quoted 19 lines

Quoted 19 lines

Another user introduced to support charger detection by Jun Li:
https://www.spinics.net/lists/linux-usb/msg139425.html
Moreover there may be other potential users will use it in future.

1. Before v19 patchset we've fixed below issues in extcon subsystem and usb phy driver, now all were merged. (Thanks for Neil's
suggestion)
(1) Have fixed the inconsistencies with USB connector types in extcon subsystem by following links:
https://lkml.org/lkml/2016/12/21/13
https://lkml.org/lkml/2016/12/21/15
https://lkml.org/lkml/2016/12/21/79
https://lkml.org/lkml/2017/1/3/13

(2) Instead of using 'set_power' callback in phy drivers, we will introduce USB charger to set PMIC current drawn from USB configuration, moreover some 'set_power' callbacks did not implement anything to set PMIC current, thus remove them by

following links:

Quoted 31 lines

Quoted 31 lines

Quoted 10 lines

Quoted 10 lines

https://lkml.org/lkml/2017/1/18/436
https://lkml.org/lkml/2017/1/18/439
https://lkml.org/lkml/2017/1/18/438
Now only two phy drivers (phy-isp1301-omap.c and phy-gpio-vbus-usb.c) still used 'set_power' callback to set current, we can remove them in future. (I have no platform with enabling these two phy drivers, so I can not test them if I converted 'set_power' callback to USB
charger.)

Quoted 4 lines

Quoted 4 lines

2. Some issues pointed by Neil Brown were sill kept in this v19 patchset, and I expalined each issue and may be need discuss again:
(1) Change all usb phys to register an extcon and to send appropriate

notifications.

Quoted 15 lines

Quoted 5 lines

Firstly, now only 3 USB phy drivers (phy-qcom-8x16-usb.c, phy-omap-otg.c and
phy-msm-usb.c) had registered an extcon, mostly did not. I can not change all usb phys to register an extcon, since there are no extcon device to register for these different phy drivers.

You don't have to change every driver. You just need to make it easy and obvious how to change drivers in a consistent coherent way. For a start you would add a 'struct extcon_dev' to 'struct usb_phy', and possibly add or extend some 'static inline's in linux/usb/phy.h to send notification on that extcon (if it is non-NULL). e.g. usb_phy_vbus_on() could send an extcon notification.

Then any phy driver which adds support for setting phy->extcon_dev appropriately, immediately gets the relevant notifications sent.

Quoted 4 lines

Quoted 2 lines

OK. We can make these extcon related code into phy common part.

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 7 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together.
Somehow these three need to work together to determine what is

Quoted 6 lines

Quoted 6 lines

plugged

Quoted 5 lines

in to the external connection port. Somewhere there much an 'extcon' device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the

extcon.

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that represents it.

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

software?

Quoted 5 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework, right?

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by software) ?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(2) Change the notifier of usb_phy to be used consistently. Now only 3 phy drivers (phy-generic.c, phy-ab8500-usb.c and phy-gpio-vbus-usb.c) used the notifier of usb_phy. phy-generic.c and phy-gpio-vbus-usb.c were used to send out the connect events, and phy-ab8500-usb.c also was used to send out the MUSB connect

events.

Quoted 77 lines

Quoted 77 lines

Quoted 2 lines

Quoted 2 lines

There are no phy drivers will notify 'vbus_draw' information by the

notifier of usb_phy, which was used consistently now.

Quoted 7 lines

Quoted 4 lines

Moreover it is difficult to change the notifier of usb_phy to be used only to communicate the 'vbus_draw' information, since we need to refactor and test these related phy drivers, power drivers or some mfd drivers, which is a huge workload.

You missed drivers/usb/musb/omap2430.c in you list, but that hardly matters.

But it did not use the notifier of usb_phy.

Quoted 1 line

phy-ab8500-usb.c appears to send vbus_draw information.

Users will not use the vbus_draw information send from phy-ab8500-usb.c

Quoted 9 lines

I understand your reluctance to change drivers that you cannot test. An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to declare the passing of NULL to be deprecated (so hopefully people won't use it in new code).
Then any notification callback that expects a current can just ignore calls where the pointer is NULL.

I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.

Quoted 10 lines

The one difficulty with this is drivers/usb/gadget/udc/pxa27x_udc.c It is the only driver which expects a 'gadget', and it doesn't really need to as it already knows the gadget.
The patch below fixes this.
With that in place, phy-generic and phy-gpio-vbus-usb can be changed to pass NULL. When we can safely use the notifier to pass vbus_draw information uniformly.

Quoted 1 line

Quoted 10 lines

Quoted 3 lines

(3) Still keep charger_type_show() API.
Firstly I think we should combine all charger related information into one place for users, which is convenient.

convenience is very much a secondary issue.

Quoted 4 lines

Secondly not only we get charger type from extcon, but also in some scenarios we can get charger type from USB controller driver, USB type-c driver, pmic driver, we should also need one place to export the

charger type.

Quoted 14 lines

As I have said, all of these sources of information should feed into the extcon.

There are ultimately two possible sources of information about the current available from the usb port.
One is the physical properties of the cable, such as resistance of ID, any short between D+ and D- etc. Being properties of the cable, they should be reported through the extcon.

The other is information gathered during the USB protocol handshake. For USB2, this is the requested current of the profile that the host activates. This should be reported though the USB gadget device.

Quoted 8 lines

I don't know how USB3 and/or type-C work but I would be surprised if they don't fit into the two cases above. If you think otherwise, please surprise me. I'm always keen to learn.

If the extcon reports the type of cable detected, and the gadget reports the result of any negotiation, then that is enough to determine the charger type. It doesn't need to be more convenient than

that.

Quoted 7 lines

Quoted 7 lines

If we are all agree we did not need the USB charger, then we can add 'current' attribute of USB gadget device.
Thanks for your suggestion.

--
Baolin.wang
Best Regards

--
Baolin.wang
Best Regards

Quoted 117 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Thursday, March 09, 2017 2:11 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Hi,

Quoted 57 lines · Jun Li

Hi,

Quoted 55 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Tuesday, March 07, 2017 5:39 PM
To: NeilBrown <[email protected]>
Cc: Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Jun Li <[email protected]>; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 8 lines · NeilBrown

Quoted 6 lines · Baolin Wang

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as they should. Thus provide a

standard framework for doing this in kernel.

Quoted 3 lines

Quoted 3 lines

Now introduce one user with wm831x_power to support and test the usb

charger.

Quoted 19 lines

Quoted 19 lines

Another user introduced to support charger detection by Jun Li:
https://www.spinics.net/lists/linux-usb/msg139425.html
Moreover there may be other potential users will use it in future.

1. Before v19 patchset we've fixed below issues in extcon subsystem and usb phy driver, now all were merged. (Thanks for Neil's
suggestion)
(1) Have fixed the inconsistencies with USB connector types in extcon subsystem by following links:
https://lkml.org/lkml/2016/12/21/13
https://lkml.org/lkml/2016/12/21/15
https://lkml.org/lkml/2016/12/21/79
https://lkml.org/lkml/2017/1/3/13

(2) Instead of using 'set_power' callback in phy drivers, we will introduce USB charger to set PMIC current drawn from USB configuration, moreover some 'set_power' callbacks did not implement anything to set PMIC current, thus remove them by

following links:

Quoted 34 lines

Quoted 33 lines

Quoted 10 lines

Quoted 10 lines

https://lkml.org/lkml/2017/1/18/436
https://lkml.org/lkml/2017/1/18/439
https://lkml.org/lkml/2017/1/18/438
Now only two phy drivers (phy-isp1301-omap.c and phy-gpio-vbus-usb.c) still used 'set_power' callback to set current, we can remove them in future. (I have no platform with enabling these two phy drivers, so I can not test them if I converted 'set_power' callback to USB
charger.)

Quoted 4 lines

Quoted 4 lines

2. Some issues pointed by Neil Brown were sill kept in this v19 patchset, and I expalined each issue and may be need discuss again:
(1) Change all usb phys to register an extcon and to send appropriate

notifications.

Quoted 13 lines

Quoted 5 lines

Firstly, now only 3 USB phy drivers (phy-qcom-8x16-usb.c, phy-omap-otg.c and
phy-msm-usb.c) had registered an extcon, mostly did not. I can not change all usb phys to register an extcon, since there are no extcon device to register for these different phy drivers.

You don't have to change every driver. You just need to make it easy and obvious how to change drivers in a consistent coherent way. For a start you would add a 'struct extcon_dev' to 'struct usb_phy', and possibly add or extend some 'static inline's in linux/usb/phy.h to send notification on that extcon (if it is non-NULL). e.g. usb_phy_vbus_on() could send an extcon notification.

Quoted 2 lines

Then any phy driver which adds support for setting phy->extcon_dev appropriately, immediately gets the relevant notifications sent.

OK. We can make these extcon related code into phy common part.

Quoted 5 lines

Quoted 1 line

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

I mean the common code need add 'struct extcon_dev' into both 'struct phy' and 'struct usb_phy', right? as some/new usb phy use that generic phy driver.

Quoted 37 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 7 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together.
Somehow these three need to work together to determine what is

Quoted 6 lines

Quoted 6 lines

plugged

Quoted 5 lines

in to the external connection port. Somewhere there much an 'extcon' device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the

extcon.

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that represents it.

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

software?

Quoted 5 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework, right?

Quoted 5 lines

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by software) ?

In my case, charger detection is done by controller driver and I need do charger type detection internally, and only pass the current draw info via phy which will send out, this seems ok for me, but I think it will be good if you or someone can show us an example user based on the design Neil suggested.
Will you work out that common code if this USB charger framework is not needed?

Quoted 98 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

(2) Change the notifier of usb_phy to be used consistently. Now only 3 phy drivers (phy-generic.c, phy-ab8500-usb.c and phy-gpio-vbus-usb.c) used the notifier of usb_phy. phy-generic.c and phy-gpio-vbus-usb.c were used to send out the connect events, and phy-ab8500-usb.c also was used to send out the MUSB connect

events.

Quoted 77 lines

Quoted 77 lines

Quoted 2 lines

Quoted 2 lines

There are no phy drivers will notify 'vbus_draw' information by the

notifier of usb_phy, which was used consistently now.

Quoted 7 lines

Quoted 4 lines

Moreover it is difficult to change the notifier of usb_phy to be used only to communicate the 'vbus_draw' information, since we need to refactor and test these related phy drivers, power drivers or some mfd drivers, which is a huge workload.

You missed drivers/usb/musb/omap2430.c in you list, but that hardly matters.

But it did not use the notifier of usb_phy.

Quoted 1 line

phy-ab8500-usb.c appears to send vbus_draw information.

Users will not use the vbus_draw information send from phy-ab8500-usb.c

Quoted 9 lines

I understand your reluctance to change drivers that you cannot test. An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to declare the passing of NULL to be deprecated (so hopefully people won't use it in new code).
Then any notification callback that expects a current can just ignore calls where the pointer is NULL.

I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.

Quoted 10 lines

The one difficulty with this is drivers/usb/gadget/udc/pxa27x_udc.c It is the only driver which expects a 'gadget', and it doesn't really need to as it already knows the gadget.
The patch below fixes this.
With that in place, phy-generic and phy-gpio-vbus-usb can be changed to pass NULL. When we can safely use the notifier to pass vbus_draw information uniformly.

Quoted 1 line

Quoted 10 lines

Quoted 3 lines

(3) Still keep charger_type_show() API.
Firstly I think we should combine all charger related information into one place for users, which is convenient.

convenience is very much a secondary issue.

Quoted 4 lines

Secondly not only we get charger type from extcon, but also in some scenarios we can get charger type from USB controller driver, USB type-c driver, pmic driver, we should also need one place to export the

charger type.

Quoted 14 lines

As I have said, all of these sources of information should feed into the extcon.

There are ultimately two possible sources of information about the current available from the usb port.
One is the physical properties of the cable, such as resistance of ID, any short between D+ and D- etc. Being properties of the cable, they should be reported through the extcon.

The other is information gathered during the USB protocol handshake. For USB2, this is the requested current of the profile that the host activates. This should be reported though the USB gadget device.

Quoted 8 lines

I don't know how USB3 and/or type-C work but I would be surprised if they don't fit into the two cases above. If you think otherwise, please surprise me. I'm always keen to learn.

If the extcon reports the type of cable detected, and the gadget reports the result of any negotiation, then that is enough to determine the charger type. It doesn't need to be more convenient than

that.

Quoted 7 lines

Quoted 7 lines

If we are all agree we did not need the USB charger, then we can add 'current' attribute of USB gadget device.
Thanks for your suggestion.

--
Baolin.wang
Best Regards

--
Baolin.wang
Best Regards

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Thursday, March 09, 2017 7:23 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 29 lines · Jun Li

Quoted 29 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Thursday, March 09, 2017 2:11 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Hi,

Quoted 2 lines · Jun Li

Hi,

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Tuesday, March 07, 2017 5:39 PM
To: NeilBrown <[email protected]>
Cc: Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David

Woodhouse

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

<[email protected]>; [email protected]; Jun Li <[email protected]>; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen

<[email protected]>;

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines · NeilBrown

Quoted 7 lines · Baolin Wang

Quoted 6 lines

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as

they should.

Quoted 54 lines

Quoted 54 lines

Quoted 21 lines

Quoted 21 lines

Quoted 1 line

Quoted 1 line

Thus provide a

standard framework for doing this in kernel.

Quoted 3 lines

Quoted 3 lines

Now introduce one user with wm831x_power to support and test the usb

charger.

Quoted 15 lines

Quoted 15 lines

Another user introduced to support charger detection by Jun Li:
https://www.spinics.net/lists/linux-usb/msg139425.html
Moreover there may be other potential users will use it in future.

1. Before v19 patchset we've fixed below issues in extcon subsystem and usb phy driver, now all were merged. (Thanks for Neil's
suggestion)
(1) Have fixed the inconsistencies with USB connector types in extcon subsystem by following links:
https://lkml.org/lkml/2016/12/21/13
https://lkml.org/lkml/2016/12/21/15
https://lkml.org/lkml/2016/12/21/79
https://lkml.org/lkml/2017/1/3/13

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

(2) Instead of using 'set_power' callback in phy drivers, we will introduce USB charger to set PMIC current drawn from USB configuration, moreover some 'set_power' callbacks did not implement anything to set PMIC current, thus remove them by

following links:

Quoted 28 lines

Quoted 28 lines

Quoted 10 lines

Quoted 10 lines

https://lkml.org/lkml/2017/1/18/436
https://lkml.org/lkml/2017/1/18/439
https://lkml.org/lkml/2017/1/18/438
Now only two phy drivers (phy-isp1301-omap.c and phy-gpio-vbus-usb.c) still used 'set_power' callback to set current, we can remove them in future. (I have no platform with enabling these two phy drivers, so I can not test them if I converted 'set_power' callback to USB
charger.)

Quoted 4 lines

Quoted 4 lines

2. Some issues pointed by Neil Brown were sill kept in this v19 patchset, and I expalined each issue and may be need discuss again:
(1) Change all usb phys to register an extcon and to send appropriate

notifications.

Quoted 13 lines

Quoted 5 lines

Firstly, now only 3 USB phy drivers (phy-qcom-8x16-usb.c, phy-omap-otg.c and
phy-msm-usb.c) had registered an extcon, mostly did not. I can not change all usb phys to register an extcon, since there are no extcon device to register for these different phy drivers.

You don't have to change every driver. You just need to make it easy and obvious how to change drivers in a consistent coherent way. For a start you would add a 'struct extcon_dev' to 'struct usb_phy', and possibly add or extend some 'static inline's in linux/usb/phy.h to send notification on that extcon (if it is non-NULL). e.g. usb_phy_vbus_on() could send an extcon notification.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Then any phy driver which adds support for setting phy->extcon_dev appropriately, immediately gets the relevant

notifications sent.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

OK. We can make these extcon related code into phy common part.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

I mean the common code need add 'struct extcon_dev' into both 'struct phy' and 'struct usb_phy', right? as some/new usb phy use that generic phy

driver.

Ah, you remind me. Seems we need also add one 'struct extcon_dev' into 'struct phy'.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together. Somehow these three need to work together to determine what is

plugged

Quoted 1 line

in to the external connection port. Somewhere there much an

'extcon'

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the

Quoted 7 lines

Quoted 7 lines

extcon.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that

represents it.

Quoted 17 lines

Quoted 11 lines

Quoted 4 lines

Quoted 4 lines

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

software?

Quoted 5 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework, right?

Quoted 5 lines

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by software) ?

Quoted 4 lines

In my case, charger detection is done by controller driver and I need do charger type detection internally, and only pass the current draw info via phy which will send out, this seems ok for me, but I think it will be good if you or someone can show us an example user based on the

design Neil suggested.

Quoted 1 line

Will you work out that common code if this USB charger framework is not

needed?

I will add a 'struct extcon_dev*' in 'struct usb_phy' and struct phy“. Others are already ready if everyone has no complain about current design, except

my one concern. (I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.)

Quoted 67 lines

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

I mean the common code need add 'struct extcon_dev' into both 'struct phy' and 'struct usb_phy', right? as some/new usb phy use that generic phy

driver.

Ah, you remind me. Seems we need also add one 'struct extcon_dev' into 'struct phy'.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish the vbus detect.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together. Somehow these three need to work together to determine what is

plugged

Quoted 1 line

in to the external connection port. Somewhere there much an

'extcon'

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the

Quoted 7 lines

Quoted 7 lines

extcon.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that

represents it.

Quoted 17 lines

Quoted 11 lines

Quoted 4 lines

Quoted 4 lines

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

software?

Quoted 5 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework, right?

Quoted 5 lines

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by software) ?

Quoted 4 lines

In my case, charger detection is done by controller driver and I need do charger type detection internally, and only pass the current draw info via phy which will send out, this seems ok for me, but I think it will be good if you or someone can show us an example user based on the

design Neil suggested.

Quoted 1 line

Will you work out that common code if this USB charger framework is not

Quoted 4 lines

needed?

I will add a 'struct extcon_dev*' in 'struct usb_phy' and struct phy“. Others are already ready if everyone has no complain about current design, except

Only adding extcon_dev into usb_phy/phy and all others are ready? My understanding you will also do:
- We need find a central place to send the notification(phy common part).

- If the extcon_dev is directly added in usb_phy/phy, PMIC needs some API to findup it.

Quoted 5 lines

my one concern. (I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.)

Can we handle this in USB(so has super speed information) and just send out 900mA directly?

From Neil's suggestion, we only have one place to send out current

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, March 10, 2017 3:15 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 17 lines · Jun Li

Quoted 17 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

I mean the common code need add 'struct extcon_dev' into both 'struct phy' and 'struct usb_phy', right? as some/new usb phy use that generic phy

driver.

Ah, you remind me. Seems we need also add one 'struct extcon_dev' into 'struct phy'.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish

the vbus detect.

Quoted 28 lines

Quoted 28 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 8 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together. Somehow these three need to work together to determine what is

plugged

Quoted 1 line

in to the external connection port. Somewhere there much an

'extcon'

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the

Quoted 7 lines

Quoted 7 lines

extcon.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that

represents it.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

software?

Quoted 5 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework,

right?

Quoted 21 lines

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by software) ?

Quoted 5 lines

In my case, charger detection is done by controller driver and I need do charger type detection internally, and only pass the current draw info via phy which will send out, this seems ok for me, but I think it will be good if you or someone can show us an example user based on the

design Neil suggested.

Quoted 2 lines

Will you work out that common code if this USB charger framework is not

needed?

I will add a 'struct extcon_dev*' in 'struct usb_phy' and struct phy“. Others are already ready if everyone has no complain about current design, except

Quoted 3 lines

Only adding extcon_dev into usb_phy/phy and all others are ready? My understanding you will also do:
- We need find a central place to send the notification(phy common part).

That will include these implementation when adding extcon_dev.

Quoted 1 line

- If the extcon_dev is directly added in usb_phy/phy, PMIC needs some

API to findup it.

PMIC can find extcon device by phandle.

Quoted 8 lines

Quoted 7 lines

my one concern. (I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.)

Quoted 2 lines

Can we handle this in USB(so has super speed information) and just send out 900mA directly?

From Neil's suggestion, we only have one place to send out current information from usb phy, so I have this concern and doubt if we still need the USB charger framework.

--
Baolin.wang
Best Regards

Hi

Quoted 100 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, March 10, 2017 3:15 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 17 lines · Jun Li

Quoted 17 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

I mean the common code need add 'struct extcon_dev' into both 'struct phy' and 'struct usb_phy', right? as some/new usb phy use that generic phy

driver.

Ah, you remind me. Seems we need also add one 'struct extcon_dev' into 'struct phy'.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish

the vbus detect.

Quoted 28 lines

Quoted 28 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 8 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together. Somehow these three need to work together to determine what is

plugged

Quoted 1 line

in to the external connection port. Somewhere there much an

'extcon'

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection point. Each of the devices can get to the phy, and the phy can get to the

Quoted 7 lines

Quoted 7 lines

extcon.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on devicetree info).

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that

represents it.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

software?

Quoted 5 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework,

right?

Quoted 21 lines

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by software) ?

Quoted 5 lines

In my case, charger detection is done by controller driver and I need do charger type detection internally, and only pass the current draw info via phy which will send out, this seems ok for me, but I think it will be good if you or someone can show us an example user based on the

design Neil suggested.

Quoted 2 lines

Will you work out that common code if this USB charger framework is not

needed?

I will add a 'struct extcon_dev*' in 'struct usb_phy' and struct phy“. Others are already ready if everyone has no complain about current design, except

Quoted 6 lines

Quoted 3 lines

Only adding extcon_dev into usb_phy/phy and all others are ready? My understanding you will also do:
- We need find a central place to send the notification(phy common part).

That will include these implementation when adding extcon_dev.

OK, thanks.

Quoted 1 line

Quoted 1 line

- If the extcon_dev is directly added in usb_phy/phy, PMIC needs some

Quoted 3 lines

API to findup it.

PMIC can find extcon device by phandle.

extcon_dev(not a reference pointer) is directly added in usb_phy/phy, not via devicetree, how PMIC find it by phandle?

From my understanding, here we should add one pointer (extcon_dev *),

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

my one concern. (I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.)

Can we handle this in USB(so has super speed information) and just send out 900mA directly?

Quoted 3 lines

From Neil's suggestion, we only have one place to send out current information from usb phy, so I have this concern and doubt if we still need the USB charger framework.

So if put it in phy/usb_phy, this is a problem, that only one place should have the infor of both speed and usb state, how about put it at usb_gadget, then, e.g. send the notification in usb_gadget_vbus_connect()?

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, March 10, 2017 6:52 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 64 lines · Jun Li

Hi

Quoted 62 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, March 10, 2017 3:15 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 17 lines · Jun Li

Quoted 17 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

I mean the common code need add 'struct extcon_dev' into both 'struct phy' and 'struct usb_phy', right? as some/new usb phy use that generic phy

driver.

Ah, you remind me. Seems we need also add one 'struct extcon_dev' into 'struct phy'.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish

the vbus detect.

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together. Somehow these three need to work together to determine what is

plugged

Quoted 2 lines

in to the external connection port. Somewhere there much an

'extcon'

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection

point.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Each of the devices can get to the phy, and the phy can get to the

extcon.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on

devicetree info).

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that

represents it.

Quoted 11 lines

Quoted 11 lines

Quoted 4 lines

Quoted 4 lines

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

software?

Quoted 6 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework,

Quoted 7 lines

Quoted 7 lines

right?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by

software) ?

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

In my case, charger detection is done by controller driver and I need do charger type detection internally, and only pass the current draw info via phy which will send out, this seems ok for me, but I think it will be good if you or someone can show us an example user based on the

design Neil suggested.

Quoted 2 lines

Will you work out that common code if this USB charger framework is not

needed?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I will add a 'struct extcon_dev*' in 'struct usb_phy' and struct phy“. Others are already ready if everyone has no complain about current design, except

Only adding extcon_dev into usb_phy/phy and all others are ready? My understanding you will also do:
- We need find a central place to send the notification(phy common

part).

Quoted 10 lines

Quoted 3 lines

That will include these implementation when adding extcon_dev.

OK, thanks.

Quoted 4 lines

Quoted 2 lines

- If the extcon_dev is directly added in usb_phy/phy, PMIC needs some

API to findup it.

Quoted 4 lines

Quoted 1 line

PMIC can find extcon device by phandle.

extcon_dev(not a reference pointer) is directly added in usb_phy/phy, not via devicetree, how PMIC find it by phandle?

From my understanding, here we should add one pointer (extcon_dev *), since usb phy is not one external connect device.

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

Quoted 8 lines

Quoted 5 lines

my one concern. (I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.)

Can we handle this in USB(so has super speed information) and just send out 900mA directly?

From Neil's suggestion, we only have one place to send out current information from usb phy, so I have this concern and doubt if we still need the USB charger framework.

Quoted 3 lines

So if put it in phy/usb_phy, this is a problem, that only one place should have the infor of both speed and usb state, how about put it at usb_gadget, then, e.g. send the notification in

usb_gadget_vbus_connect()?

That is same what USB charger did, from this point, we need USB charger to send out vbus draw information according to speed and usb state. But I should listen to other guys suggestion. Peter and Felipe, what do you think?

--
Baolin.wang
Best Regards

Hi,

Quoted 145 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, March 10, 2017 6:52 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 64 lines · Jun Li

Hi

Quoted 62 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, March 10, 2017 3:15 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 17 lines · Jun Li

Quoted 17 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

Will generic phy need add extcon as well?

Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which will be common code.

I mean the common code need add 'struct extcon_dev' into both 'struct phy' and 'struct usb_phy', right? as some/new usb phy use that generic phy

driver.

Ah, you remind me. Seems we need also add one 'struct extcon_dev' into 'struct phy'.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Secondly, I also agreed with Peter's comments: Not only USB PHY to register an extcon, but also for the drivers which can detect USB charger type, it may be USB controller driver, USB type-c driver, pmic driver, and these drivers may not have an extcon device since the internal part can finish

the vbus detect.

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

Whichever part can detect vbus, the driver for that part must be able to find the extcon and trigger a notification. Maybe one part can detect VBUS, another can measure the resistance on ID and a third can work through the state machine to determine if D+ and D- are shorted together. Somehow these three need to work together to determine what is

plugged

Quoted 2 lines

in to the external connection port. Somewhere there much an

'extcon'

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

device which represents that port and which the three devices can find and can interact with.
I think it makes sense for the usb_phy to be the connection

point.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Each of the devices can get to the phy, and the phy can get to the

extcon.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

It doesn't matter very much if the usb phy driver creates the extcon, or if something else creates the extcon and the phy driver performs a lookup to find it (e.g. based on

devicetree info).

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

The point is that there is obviously an external physical connection, and so there should be an 'extcon' device that

represents it.

Quoted 11 lines

Quoted 11 lines

Quoted 4 lines

Quoted 4 lines

Peter & Jun, is it OK for you every phy has one extcon device to receive VBUS notification, especially for detecting the charger type by

software?

Quoted 6 lines

Quoted 1 line

My understanding is phy/usb_phy as the connection point, will send the notification to PMIC driver which actually control the charge current, also this will be done in your common framework,

Quoted 7 lines

Quoted 7 lines

right?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Not in USB charger framework. If we are all agree every usb_phy can register one extcon device, can get correct charger type and send out correct vbus_draw information, then we don't need USB charger framework as Neil suggested. So this will be okay for your case (especially for detecting the charger type by

software) ?

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

In my case, charger detection is done by controller driver and I need do charger type detection internally, and only pass the current draw info via phy which will send out, this seems ok for me, but I think it will be good if you or someone can show us an example user based on the

design Neil suggested.

Quoted 2 lines

Will you work out that common code if this USB charger framework is not

needed?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I will add a 'struct extcon_dev*' in 'struct usb_phy' and struct phy“. Others are already ready if everyone has no complain about current design, except

Only adding extcon_dev into usb_phy/phy and all others are ready? My understanding you will also do:
- We need find a central place to send the notification(phy common

part).

Quoted 10 lines

Quoted 3 lines

That will include these implementation when adding extcon_dev.

OK, thanks.

Quoted 4 lines

Quoted 2 lines

- If the extcon_dev is directly added in usb_phy/phy, PMIC needs some

API to findup it.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

PMIC can find extcon device by phandle.

extcon_dev(not a reference pointer) is directly added in usb_phy/phy, not via devicetree, how PMIC find it by phandle?

From my understanding, here we should add one pointer (extcon_dev *), since usb phy is not one external connect device.

Agreed.

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

my one concern. (I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.)

Quoted 6 lines

Quoted 2 lines

Can we handle this in USB(so has super speed information) and just send out 900mA directly?

From Neil's suggestion, we only have one place to send out current information from usb phy, so I have this concern and doubt if we still need the USB charger framework.

So if put it in phy/usb_phy, this is a problem, that only one place should have the infor of both speed and usb state, how about put it at usb_gadget, then, e.g. send the notification in

Quoted 5 lines

usb_gadget_vbus_connect()?

That is same what USB charger did, from this point, we need USB charger to send out vbus draw information according to speed and usb state. But I should listen to other guys suggestion. Peter and Felipe, what do you think?

So now the only to do work is to find a common place to send the notification out (based on gadget speed and sate).

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Monday, March 13, 2017 4:44 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <[email protected]>; [email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- [email protected]; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Quoted 3 lines

Quoted 3 lines

That is same what USB charger did, from this point, we need USB charger to send out vbus draw information according to speed and usb state. But I should listen to other guys suggestion. Peter and Felipe, what

do you think?

Quoted 3 lines

So now the only to do work is to find a common place to send the notification out (based on gadget speed and sate).

Yes.

--
Baolin.wang
Best Regards

Hi,

Quoted 20 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Monday, March 13, 2017 4:44 PM
To: Jun Li <[email protected]>
Cc: NeilBrown <neilb-IBi9RG/[email protected]>; Felipe Balbi <[email protected]>; Greg KH <[email protected]>; Sebastian Reichel <[email protected]>; Dmitry Eremin-Solenikov <[email protected]>; David Woodhouse <[email protected]>; [email protected]; Marek Szyprowski <[email protected]>; Ruslan Bilovol <[email protected]>; Peter Chen <[email protected]>; Alan Stern <stern-nwvwT67g6+6dFdvTe/[email protected]>; [email protected]; Yoshihiro Shimoda <[email protected]>; Lee Jones <[email protected]>; Mark Brown <[email protected]>; John Stultz <[email protected]>; Charles Keepax <ckeepax-yzvPICuk2AATkU/[email protected]>; patches-yzvPICuk2AATkU/[email protected]; Linux PM list <linux- [email protected]>; USB <[email protected]>; device- mainlining-cunTk1MwBs98uUxBSJOaYoYkZiVZrdSR2LY78lusg7I@public.gmane.org; LKML <[email protected]>
Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with
the usb gadget power negotation

... ...

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

That is same what USB charger did, from this point, we need USB charger to send out vbus draw information according to speed and usb state. But I should listen to other guys suggestion. Peter and Felipe, what

do you think?

Instead of waiting for maintainer's comments on this long discussion, I'd suggest you directly post the patches for the required change, so they can easily know what's the conclusion/solution looks like.

Quoted 9 lines · NeilBrown

I understand your reluctance to change drivers that you cannot test. An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to declare the passing of NULL to be deprecated (so hopefully people won't use it in new code).
Then any notification callback that expects a current can just ignore calls where the pointer is NULL.

I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.

Quoted 12 lines · Baolin Wang

Quoted 9 lines · NeilBrown

I understand your reluctance to change drivers that you cannot test. An alternative it do change all the
atomic_notifier_call_chain(.*notifier,
calls that don't pass a pointer to vbus_draw to pass NULL, and to declare the passing of NULL to be deprecated (so hopefully people won't use it in new code).
Then any notification callback that expects a current can just ignore calls where the pointer is NULL.

Quoted 3 lines

I am afraid if it is enough to send out vbus draw information from USB phy driver, for example you will miss super speed (900mA), which need get the speed information from gadget driver.

When the gadget driver determines that 900mA is available, it calls usb_phy_set_power() which calls the ->set_power() method on the usb_phy. The usb_phy the uses the notifier to inform all interested parties that 900mA is available.

 drivers/power/supply/wm831x_power.c |   63 +++
 drivers/usb/gadget/Kconfig          |    8 +
 drivers/usb/gadget/udc/Makefile     |    1 +
 drivers/usb/gadget/udc/charger.c    |  865 +++++++++++++++++++++++++++++++++++

 drivers/usb/gadget/udc/core.c       |   19 +-
 include/linux/usb/charger.h         |  176 +++++++
 include/linux/usb/gadget.h          |    3 +
 include/uapi/linux/usb/charger.h    |   31 ++
 8 files changed, 1165 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

On Mon, 20 Feb 2017, Baolin Wang wrote:

[...]

Quoted 4 lines

 drivers/power/supply/wm831x_power.c |   63 +++
 drivers/usb/gadget/Kconfig          |    8 +
 drivers/usb/gadget/udc/Makefile     |    1 +
 drivers/usb/gadget/udc/charger.c    |  865 +++++++++++++++++++++++++++++++++++

Quoted 8 lines

 drivers/usb/gadget/udc/core.c       |   19 +-
 include/linux/usb/charger.h         |  176 +++++++
 include/linux/usb/gadget.h          |    3 +
 include/uapi/linux/usb/charger.h    |   31 ++
 8 files changed, 1165 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

Why have you sent this set to me?

Hi,

Quoted 17 lines · Lee Jones

On Mon, 20 Feb 2017, Baolin Wang wrote:

[...]

Quoted 12 lines

 drivers/power/supply/wm831x_power.c |   63 +++
 drivers/usb/gadget/Kconfig          |    8 +
 drivers/usb/gadget/udc/Makefile     |    1 +
 drivers/usb/gadget/udc/charger.c    |  865 +++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c       |   19 +-
 include/linux/usb/charger.h         |  176 +++++++
 include/linux/usb/gadget.h          |    3 +
 include/uapi/linux/usb/charger.h    |   31 ++
 8 files changed, 1165 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

Quoted 1 line

Why have you sent this set to me?

Since you have one ack on patch 4, I think I need CC you. But If you are not interested in this patchset, I will not CC you next time. Sorry.

[PATCH v19 4/4] power: wm831x_power: Support USB charger current limit management

Integrate with the newly added USB charger interface to limit the current we draw from the USB input based on the input device configuration identified by the USB stack, allowing us to charge more quickly from high current inputs without drawing more current than specified from others.

Signed-off-by Mark Brown <[email protected]>

Signed-off-by Baolin Wang <[email protected]>

Acked-by Lee Jones <[email protected]>

Acked-by Charles Keepax <[email protected]>

Acked-by Peter Chen <[email protected]>

Acked-by Sebastian Reichel <[email protected]>

.......