Message-ID: <AM5PR04MB29614C707DE344E5A05FAB3889210@AM5PR04MB2961.eurprd04.prod.outlook.com>
Date:   Thu, 9 Mar 2017 01:50:39 +0000
From:   Jun Li <jun.li@....com>
To:     Baolin Wang <baolin.wang@...aro.org>, NeilBrown <neilb@...e.com>
CC:     Felipe Balbi <balbi@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Sebastian Reichel <sre@...nel.org>,
        Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        "robh@...nel.org" <robh@...nel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Ruslan Bilovol <ruslan.bilovol@...il.com>,
        "Peter Chen" <peter.chen@...escale.com>,
        Alan Stern <stern@...land.harvard.edu>,
        "grygorii.strashko@...com" <grygorii.strashko@...com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Lee Jones <lee.jones@...aro.org>,
        "Mark Brown" <broonie@...nel.org>,
        John Stultz <john.stultz@...aro.org>,
        "Charles Keepax" <ckeepax@...nsource.wolfsonmicro.com>,
        "patches@...nsource.wolfsonmicro.com" 
        <patches@...nsource.wolfsonmicro.com>,
        Linux PM list <linux-pm@...r.kernel.org>,
        USB <linux-usb@...r.kernel.org>,
        "device-mainlining@...ts.linuxfoundation.org" 
        <device-mainlining@...ts.linuxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v19 0/4] Introduce usb charger framework to deal with the
 usb gadget power negotation

Hi,

> -----Original Message-----
> From: Baolin Wang [mailto:baolin.wang@...aro.org]
> Sent: Tuesday, March 07, 2017 5:39 PM
> To: NeilBrown <neilb@...e.com>
> Cc: Felipe Balbi <balbi@...nel.org>; Greg KH <gregkh@...uxfoundation.org>;
> Sebastian Reichel <sre@...nel.org>; Dmitry Eremin-Solenikov
> <dbaryshkov@...il.com>; David Woodhouse <dwmw2@...radead.org>;
> robh@...nel.org; Jun Li <jun.li@....com>; Marek Szyprowski
> <m.szyprowski@...sung.com>; Ruslan Bilovol <ruslan.bilovol@...il.com>;
> Peter Chen <peter.chen@...escale.com>; Alan Stern
> <stern@...land.harvard.edu>; grygorii.strashko@...com; Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@...esas.com>; Lee Jones <lee.jones@...aro.org>;
> Mark Brown <broonie@...nel.org>; John Stultz <john.stultz@...aro.org>;
> Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>;
> patches@...nsource.wolfsonmicro.com; Linux PM list <linux-
> pm@...r.kernel.org>; USB <linux-usb@...r.kernel.org>; device-
> mainlining@...ts.linuxfoundation.org; LKML <linux-kernel@...r.kernel.org>
> Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with
> the usb gadget power negotation
> 
> On 3 March 2017 at 10:23, NeilBrown <neilb@...e.com> wrote:
> > On Mon, Feb 20 2017, Baolin Wang wrote:
> >
> >> 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.
> >
> > 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.
> 
> OK. We can make these extcon related code into phy common part.
> 	

Will generic phy need add extcon as well?

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

Li Jun 

> >
> >>
> >> (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.
> 
> But it did not use the notifier of usb_phy.
> 
> > phy-ab8500-usb.c appears to send vbus_draw information.
> 
> Users will not use the vbus_draw information send from phy-ab8500-usb.c
> 
> >
> > 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.
> 
> >
> > 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.
> >
> >>
> >> (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.
> >
> >> 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.
> 
> 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

Powered by blists - more mailing lists