Messages in this thread | | | Subject | Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support | From | Hans de Goede <> | Date | Fri, 12 May 2017 23:22:46 +0200 |
| |
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@redhat.com> 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@redhat.com> >>>> --- >>>> 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...
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
| |