On Wed, Dec 12, 2018 at 6:15 PM Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx> wrote: > > On 12 December 2018 02:47, Kyle Tso wrote: > > > On Mon, Dec 10, 2018 at 7:36 PM Adam Thomson > > <Adam.Thomson.Opensource@xxxxxxxxxxx> 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
