Message-ID: <5b09f8ad-be3c-b3c0-9e06-055cd4768700@redhat.com>
Date:   Fri, 12 May 2017 23:22:46 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Yueyao (Nathan) Zhu (yueyao"@google.com
Subject: Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support

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@...hat.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@...hat.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

Powered by blists - more mailing lists