Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support

[Guenter Roeck]

On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 24-04-17 15:12, Guenter Roeck wrote:
> >On 04/24/2017 04:02 AM, Heikki Krogerus wrote:
> >>+Guenter
> >>
> >>On Fri, Apr 21, 2017 at 09:51:50PM +0300, Andy Shevchenko wrote:
> >>>+Cc: Heikki.
> >>>
> >>>He might comment on this.
> >>
> >>Thanks Andy.
> >>
> >>>On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>>>Add support for USB TYPE-C cable detection on systems using a
> >>>>FUSB302 USB TYPE-C controller.
> >>>>
> >>>>Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>>>---
> >>>> drivers/extcon/Kconfig          |   8 +
> >>>> drivers/extcon/Makefile         |   1 +
> >>>> drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++
> >>>> 3 files changed, 791 insertions(+)
> >>>> create mode 100644 drivers/extcon/extcon-fusb302.c
> >>
> >>There is now the typec class in linux-next that really needs to be
> >>used with all USB Type-C PHYs like fusb302.
> >>
> >>Unless I'm mistaken, Guenter has also written a driver for fusb302. I
> >
> >Not me; it was Nathan.
> >
> >>have not seen it, but if I understood correctly, that driver will
> >>register itself with the upcoming tcpm (USB Type-C Port Manager) [1],
> >>which in practice would mean we can take properly advantage of the USB
> >>PD transceiver on fusb302.
> >>
> >>Guenter! Can you publish the fusb302 driver?
> >>
> >
> >Working on it.
> 
> Ok, I see this has landed in staging. So lets go with this driver
> then and not mine (I did not get around to the PD stuff yet, so
> that is fine).
> 
> Question, how is this supposed to interface with the rest of the
> kernel ? Specifically the commit message for the tcpm frameworks
> says:
> 
> "This driver only implements the state machine. Lower level drivers are
> responsible for
> - Reporting VBUS status and activating VBUS
> - Setting CC lines and providing CC line status
> - Setting line polarity
> - Activating and deactivating VCONN
> - Setting the current limit
> - Activating and deactivating PD message transfers
> - Sending and receiving PD messages"
> 
> But the FUSB302 cannot set the current limit...
> 

There is an underlying driver which I didn't pay enough attention to.

Badhri, Yueyao, can we publish the pd engine code as well ?
If that doesn't solve the problem, it might at least give a starting
point.

Thanks,
Guenter

> The device I'm working on:
> http://www.gpd.hk/gpdwin.asp
> 
> Has the following more or less relevant components:
> 
> -Intel Cherry Trail Z8700 SoC
> -Intel Cherry Trail Whiskey Cove PMIC (which is different from the Broxton Whiskey Cove PMIC),
>  this PMIC uses an external charger and fuel-gauge:
> -TI bq24292i charger
> -Maxim MAX17047 fuel-gauge
> -PI3USB30532 USB TYPE-C mux
> 
> Currently I only have the USB-C connector on the
> device working in USB-2 mode (I did not realize
> the USB-3 bits were wired up at first).
> 
> The device ships with a charger with an USB-A
> receptacle and an USB-2 only USB-A to USB-C
> cable.
> 
> The way I've hooked up USB-2 charging is like this:
> The Whiskey Cove PMIC has built in USB-2 BC detection, and
> I've written an extcon driver for this which reports one
> of the following cables depended on the BC detection:
> 
>         EXTCON_CHG_USB_SDP,
>         EXTCON_CHG_USB_CDP,
>         EXTCON_CHG_USB_DCP,
> 
> And I've modified the bq24290_charger driver to (optionally)
> receive an extcon name through device-properties and if
> it does, then set the input current based on the detected
> charger type (0.5 A for SDP, 1.5A for CDP, 2A for DCP).
> 
> Most of the patches for this are upstream. But once we
> hookup the FUSB302 driver we need to propagate the current
> limit it has negotiated to the bq24290_charger driver.
> 
> Which is part of the reason why I wrote the 5th patch of
> my original series, let me reply to Heikki's question about
> that one here as it is related:
> 
> On 24-04-17 16:25, Heikki Krogerus wrote:
> 
> > Doesn't this mean you have several extcon_dev registered for a
> > single port?
> 
> Yes this is unfortunate, but the primary consumer of extcon
> info is the kernel itself and we can teach it to look at the
> right one.
> 
> > I'm not completely sure how extcon framework is meant to
> > be used, but shouldn't a single extcon_dev represent all the types of
> > connectors a port is meant to support?
> 
> Ideally, yes. But here we've 2 chips / drivers connected
> to the same connector (more even, but 2 which provide extcon
> related info).
> 
> As explained above the device ships with an USB-2 charger +
> USB-A to USB-C cable, that cable correctly gets seen by the
> FUSB302 as a TYPEC_CC_RP_DEF cable, which means limiting input
> current to 0.5A, but as the FUSB302 datasheet says:
> 
> "The host software is expected to follow the USB Type-C
> specification for charging current priority based on
> feedback from the FUSB302 detection, external BC1.2 detection
> and any USB Power Delivery communication.
> 
> The FUSB302 does not integrate BC1.2 charger
> detection which is assumed available in the USB
> transceiver or USB charger in the system."
> 
> So when we see TYPEC_CC_RP_DEF we need to ask the
> Cherry Trail PMIC (in my case) to do BC detection
> and use the result of that, otherwise we end up
> setting input current limit way too low.
> 
> >> Since the Type-C controller info is incomplete and needs to be
> >> supplemented with the BC1.2 detection result in some cases, this
> >> commit makes the intel-cht-wc extcon driver monitor the extcon device
> >> registered by the Type-C controller and report that as our extcon state
> >> except when BC-1.2 detection should be used. This allows extcon
> >> consumers on these boards to keep monitoring only the intel-cht-wc extcon
> >> and then get complete extcon info from that, which combines the Type-C
> >> and BC-1.2 info.
> >
> > I don't really understand why does this need to be done in such a
> > complex manner? Both components should just report the result and be
> > done with it, and there is no need for any coupling between the two.
> > If there is a consumer for the power, the driver for it can decide on
> > its own the limits and maximum power from the two sources.
> 
> To me making the combination of the 2 sources the problem
> of the consumer seems just wrong, as you mentioned
> above there should really be only one extcon for the
> connector, likewise their should be only one definitive
> source on what the input current limit is.
> 
> TL;DR: We need some way to combine the current limit info
> from TYPE-C and USB-2 BC detection into a single source and
> to propagate that current to drivers which can actually set
> the current-limit.
> 
> Note I'm happy to use something else then extcon for this,
> but we do need some way to combine + propagate the
> current-limit.
> 
> Likewise we need a way to tell another driver to drive VBus
> on the USB-C connector. In my case this is again done by the
> bq24290_charger driver, which currently does this based on the
> combination of EXTCON_CHG_USB_* not being set on the extcon +
> EXTCON_USB_HOST being set.
> 
> One possible solution would be for the
> drivers/extcon/extcon-intel-cht-wc.c to somehow hook into
> the tpmc device and to get notifications of state changes,
> then it could reflect the info from the FUSB302 in its
> extcon info, not sure if that is the best design though.
> 
> What also seems to missing is for a way to hookup a
> mux driver to deal with upside-down plug-ins and
> alt-mode switching.
> 
> Regards,
> 
> Hans