Message-ID: <CAGZ6i=0ksqGJgewdXJ+zHd_SMfUpHqZd2whFbaN2uYt7fRNzWQ@mail.gmail.com>
Date:   Thu, 13 Dec 2018 19:04:54 +0800
From:   Kyle Tso <kyletso@...gle.com>
To:     Adam.Thomson.Opensource@...semi.com
Cc:     Guenter Roeck <linux@...ck-us.net>,
        heikki.krogerus@...ux.intel.com, gregkh@...uxfoundation.org,
        Badhri Jagan Sridharan <badhri@...gle.com>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection

On Wed, Dec 12, 2018 at 6:15 PM Adam Thomson
<Adam.Thomson.Opensource@...semi.com> wrote:
>
> On 12 December 2018 02:47, Kyle Tso wrote:
>
> > On Mon, Dec 10, 2018 at 7:36 PM Adam Thomson
> > <Adam.Thomson.Opensource@...semi.com> wrote:
> > >
> > > On 10 December 2018 09:01, Adam Thomson wrote:
> > >
> > > > On 06 December 2018 03:02, Kyle Tso wrote:
> > > >
> > > > > Current matching rules ensure that the voltage range of selected
> > > > > Source Capability is entirely within the range defined in one of
> > > > > the Sink Capabilities. This is reasonable but not practical
> > > > > because Sink may not support wide range of voltage when sinking
> > > > > power while Source could advertise its capabilities in raletively
> > > > > wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V
> > > > > Prog of Fixed Nominal Voltage) will not be selected if the Sink
> > > > > requires 5V- 12V@3A PPS power. However, the Sink could work well
> > > > > if the requested voltage range in
> > > > RDOs is 5V-11V@3A.
> > > >
> > > > Is there a real world example of a sink requiring the 5V - 12V
> > > > range? In that scenario could we not add an additional sink
> > > > capability which allows for this range to be supported, and the current
> > implementation should work just fine?
> > >
> > > Ok, I maybe should have waited until after my morning coffee to
> > > respond. So because the lower limit on the sink side, is higher than
> > > the advertised source's PPS minimum voltage it never gets selected?
> > > Personally I'd prefer to keep the upper limit checking as is as I
> > > think that's an additional safety benefit helping to prevent
> > > over-voltage scenarios. I think if a PPS APDO can supply up to 11V
> > > then the system should be capable of handling that voltage, otherwise
> > > it shouldn't be considered at all. The Source provides limits checking
> > > as well to make sure the Sink doesn't request a value above the maximum
> > voltage limit for that selected APDO.
> > >
> >
> > If the over-voltage occurs, it means:
> > 1. the adapter malfunctioned. or
> > 2. the code on the Sink accidentally requests a voltage level which is over the limit
> > of the Sink.
> >
> > For 1., it is difficult to predict the behaviors of a malfunctioned adapter. The over-
> > voltage event may happen even if the Sink doesn't select the APDO from this
> > broken adapter.
>
> Yes, I agree it's almost impossible to do anything from software to mitigate
> this which is why the HW design has to have protection for this.
>
> > For 2., it is difficult to predict the behaviors from the careless code as well.
>
> Yes, that's also true, but if it's coded with the intention not to select an
> option that's potentially higher than the system can handle then we're less
> likely to fall foul of over-voltage scenarios in my opinion. By selecting a
> PPS APDO with an upper threshold which falls within the board limits, assuming
> the code were to accidentally request something higher than the PPS APDO maximum
> then the Source should reject this. Just feels a little safer as we're talking
> about controlling an external power source. At the end of the day though the
> decision lies with the maintainers on this.
>

The implementation of PPS in TCPM doesn't account for the decision of
the content
in each RDO. It is nearly fully passive and receives the key values
(voltage/current) from
external codes through the power_supply framework. There are already some basic
checks in TCPM which reject invalid requests from the external codes.

pps_data.min_volt
pps_data.max_volt
pps_data.max_curr

These values are set in tcpm_pd_select_pps_apdo() and they are
restricted within the
board limits (and vice versa if the limitation is on the source side).
I think it is enough
to protect it from careless external codes.

> > > For the lower limit I'm more inclined to agree with allowing a higher
> > > minimum on the sink side as that's less of a safety/damage issue as I
> > understand it.
> > > FWIW, what is the real world scenario? What happens if voltage drops below
> > 5V?
> > >
> >
> > Some products (in Sink mode) have under-voltage protection (the lower bound
> > might be around 3.8V - 4V before the calculation of IR-drop) that will cause the
> > disconnection.
>
> Ok, so the system would just stop charging, correct? I guess the calling code
> to control the Source/adapter, via TCPM, wouldn't select a value below 4V in that
> scenario anyway?

Yes, that's why I do these changes in this patch. It's weird if the
Sink claims its Sink
Capabilities with a wider range of voltage than it really can support.
But actually
many adapters (in market) have wider capabilities than this kind of Sink port.

thanks,
Kyle

Powered by blists - more mailing lists