Re: [PATCH 2/2 v5] typec: tcpm: Only request matching pdos

[Badhri Jagan Sridharan]

On Tue, Nov 7, 2017 at 4:07 AM, Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> On Sat, Nov 04, 2017 at 12:22:13PM -0700, Badhri Jagan Sridharan wrote:
>> At present, TCPM code assumes that local device supports
>> variable/batt pdos and always selects the pdo with highest
>> possible power within the board limit. This assumption
>> might not hold good for all devices. To overcome this,
>> this patch makes TCPM only accept a source_pdo when there is
>> a matching sink pdo.
>>
>> For Fixed pdos: The voltage should match between the
>> incoming source_cap and the registered snk_pdo
>> For Variable/Batt pdos: The incoming source_cap voltage
>> range should fall within the registered snk_pdo's voltage
>> range.
>>
>> Also, when the cap_mismatch bit is set, the max_power/current
>> should be set to the max_current/power of the sink_pdo.
>> This is according to:
>>
>> "If the Capability Mismatch bit is set to one
>> The Maximum Operating Current/Power field may contain a value
>> larger than the maximum current/power offered in the Source
>> Capabilities message???s PDO as referenced by the Object position field.
>> This enables the Sink to indicate that it requires more current/power
>> than is being offered. If the Sink requires a different voltage this
>> will be indicated by its Sink Capabilities message.
>>
>> Signed-off-by: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx>
>> ---
>> Changelog since v1:
>> - Rebased the patch on top of drivers/usb/type/tcpm.c
>> - Added duplicate pdo check for variable/batt pdo.
>> - Fixed tabs as suggested by dan.carpenter@xxxxxxxxxx
>>
>> Changelog since v2:
>> - Rebase
>>
>> Changelog since v3:
>> - Refactored code fixed formatting issues as suggested
>>   by heikki.krogerus@xxxxxxxxxxxxxxx
>>
>> Changelog since v4:
>> - Rebase
>
> I'm fine with this, but in case you'll prepare one more version, I'll
> put a few really minor nits below. I would prefer though that Guenter
> gave his ACK to any patches touching tcpm.c or tcpci.c.
>
> But FWIW:
>
> Acked-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

Thanks Heikki ! Addressing the minor nits as well.

Guenter, Are you OK with this patch ?

Regards,
Badhri

>
>
> Thanks,
>
>>  drivers/usb/typec/tcpm.c | 147 +++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 111 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>> index 8b30ab69ed79..9cd8fff12809 100644
>> --- a/drivers/usb/typec/tcpm.c
>> +++ b/drivers/usb/typec/tcpm.c
>> @@ -261,6 +261,9 @@ struct tcpm_port {
>>       unsigned int nr_src_pdo;
>>       u32 snk_pdo[PDO_MAX_OBJECTS];
>>       unsigned int nr_snk_pdo;
>> +     unsigned int nr_fixed; /* number of fixed sink PDOs */
>> +     unsigned int nr_var; /* number of variable sink PDOs */
>> +     unsigned int nr_batt; /* number of battery sink PDOs */
>>       u32 snk_vdo[VDO_MAX_OBJECTS];
>>       unsigned int nr_snk_vdo;
>>
>> @@ -1761,39 +1764,86 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
>>       return 0;
>>  }
>>
>> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
>> +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
>> +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
>> +
>> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo)
>
> To me it's a bit confusing that your are returning the port's source
> capability index as the return value, and handling the sink pdo index
> as pointer argument. It would be more clear to just return
> success/failure (0/-EINVAL), and handled both indexes as arguments to
> the function.

Addressing this in the next patch.

>
>>  {
>> -     unsigned int i, max_mw = 0, max_mv = 0;
>> +     unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
>>       int ret = -EINVAL;
>>
>>       /*
>> -      * Select the source PDO providing the most power while staying within
>> -      * the board's voltage limits. Prefer PDO providing exp
>> +      * Select the source PDO providing the most power which has a
>> +      * matchig sink cap.
>>        */
>>       for (i = 0; i < port->nr_source_caps; i++) {
>>               u32 pdo = port->source_caps[i];
>>               enum pd_pdo_type type = pdo_type(pdo);
>> -             unsigned int mv, ma, mw;
>>
>> -             if (type == PDO_TYPE_FIXED)
>> -                     mv = pdo_fixed_voltage(pdo);
>> -             else
>> -                     mv = pdo_min_voltage(pdo);
>> -
>> -             if (type == PDO_TYPE_BATT) {
>> -                     mw = pdo_max_power(pdo);
>> -             } else {
>> -                     ma = min(pdo_max_current(pdo),
>> -                              port->max_snk_ma);
>> -                     mw = ma * mv / 1000;
>> -             }
>> -
>> -             /* Perfer higher voltages if available */
>> -             if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
>> -                 mv <= port->max_snk_mv) {
>> -                     ret = i;
>> -                     max_mw = mw;
>> -                     max_mv = mv;
>> +             if (type == PDO_TYPE_FIXED) {
>> +                     for (j = 0; j < port->nr_fixed; j++) {
>> +                             if (pdo_fixed_voltage(pdo) ==
>> +                                 pdo_fixed_voltage(port->snk_pdo[j])) {
>> +                                     ma = min_current(pdo, port->snk_pdo[j]);
>> +                                     mv = pdo_fixed_voltage(pdo);
>> +                                     mw = ma * mv / 1000;
>> +                                     if (mw > max_mw ||
>> +                                         (mw == max_mw && mv > max_mv)) {
>> +                                             ret = i;
>> +                                             *sink_pdo = j;
>> +                                             max_mw = mw;
>> +                                             max_mv = mv;
>> +                                     }
>> +                                     /* There could only be one fixed pdo
>> +                                      * at a specific voltage level.
>> +                                      * So breaking here.
>> +                                      */
>> +                                     break;
>> +                             }
>> +                     }
>> +             } else if (type == PDO_TYPE_BATT) {
>> +                     for (j = port->nr_fixed;
>> +                          j < port->nr_fixed +
>> +                              port->nr_batt;
>> +                          j++) {
>> +                             if (pdo_min_voltage(pdo) >=
>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>> +                                  pdo_max_voltage(pdo) <=
>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>> +                                     mw = min_power(pdo, port->snk_pdo[j]);
>> +                                     mv = pdo_min_voltage(pdo);
>> +                                     if (mw > max_mw ||
>> +                                         (mw == max_mw && mv > max_mv)) {
>> +                                             ret = i;
>> +                                             *sink_pdo = j;
>> +                                             max_mw = mw;
>> +                                             max_mv = mv;
>> +                                     }
>> +                             }
>> +                     }
>> +             } else if (type == PDO_TYPE_VAR) {
>> +                     for (j = port->nr_fixed +
>> +                              port->nr_batt;
>> +                          j < port->nr_fixed +
>> +                              port->nr_batt +
>> +                              port->nr_var;
>> +                          j++) {
>> +                             if (pdo_min_voltage(pdo) >=
>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>> +                                  pdo_max_voltage(pdo) <=
>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>> +                                     ma = min_current(pdo, port->snk_pdo[j]);
>> +                                     mv = pdo_min_voltage(pdo);
>> +                                     mw = ma * mv / 1000;
>> +                                     if (mw > max_mw ||
>> +                                         (mw == max_mw && mv > max_mv)) {
>> +                                             ret = i;
>> +                                             *sink_pdo = j;
>> +                                             max_mw = mw;
>> +                                             max_mv = mv;
>> +                                     }
>> +                             }
>> +                     }
>>               }
>>       }
>>
>> @@ -1805,13 +1855,14 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>>       unsigned int mv, ma, mw, flags;
>>       unsigned int max_ma, max_mw;
>>       enum pd_pdo_type type;
>> -     int index;
>> -     u32 pdo;
>> +     int index, snk_pdo_index;
>> +     u32 pdo, matching_snk_pdo;
>>
>> -     index = tcpm_pd_select_pdo(port);
>> +     index = tcpm_pd_select_pdo(port, &snk_pdo_index);
>>       if (index < 0)
>>               return -EINVAL;
>>       pdo = port->source_caps[index];
>> +     matching_snk_pdo = port->snk_pdo[snk_pdo_index];
>>       type = pdo_type(pdo);
>>
>>       if (type == PDO_TYPE_FIXED)
>> @@ -1819,26 +1870,28 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>>       else
>>               mv = pdo_min_voltage(pdo);
>>
>> -     /* Select maximum available current within the board's power limit */
>> +     /* Select maximum available current within the sink pdo's limit */
>>       if (type == PDO_TYPE_BATT) {
>> -             mw = pdo_max_power(pdo);
>> -             ma = 1000 * min(mw, port->max_snk_mw) / mv;
>> +             mw = min_power(pdo, matching_snk_pdo);
>> +             ma = 1000 * mw / mv;
>>       } else {
>> -             ma = min(pdo_max_current(pdo),
>> -                      1000 * port->max_snk_mw / mv);
>> +             ma = min_current(pdo, matching_snk_pdo);
>> +             mw = ma * mv / 1000;
>>       }
>> -     ma = min(ma, port->max_snk_ma);
>>
>>       flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>>
>>       /* Set mismatch bit if offered power is less than operating power */
>> -     mw = ma * mv / 1000;
>>       max_ma = ma;
>>       max_mw = mw;
>>       if (mw < port->operating_snk_mw) {
>>               flags |= RDO_CAP_MISMATCH;
>> -             max_mw = port->operating_snk_mw;
>> -             max_ma = max_mw * 1000 / mv;
>> +             if (type == PDO_TYPE_BATT &&
>> +                 (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
>> +                     max_mw = pdo_max_power(matching_snk_pdo);
>> +             else if (pdo_max_current(matching_snk_pdo) >
>> +                      pdo_max_current(pdo))
>> +                     max_ma = pdo_max_current(matching_snk_pdo);
>
> After thinking about this a bit more:
>
>                 if (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo))
>                         if (type == PDO_TYPE_BATT)
>                                 max_mw = pdo_max_power(matching_snk_pdo);
>                         else
>                                 max_ma = pdo_max_current(matching_snk_pdo);
>

Hmm.. Only BATT pdos have max_power field, VARIABLE and FIXED PDOs
only have max_current field.
Table 6-9 Fixed Supply PDO - Source : "B9â0   Maximum Current in 10mA units"
Table 6-11 Variable Supply (non-Battery) PDO - Source :  "B9â0
Maximum Current in 10mA units "
Table 6-12 Battery Supply PDO - Source : " B9â0  Maximum Allowable
Power in 250mW units"

IMHO changing it this way is not accurate.
Makes sense ?

>>       }
>>
>>       tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
>> @@ -3587,6 +3640,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>>  }
>>  EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>>
>> +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
>> +                     enum pd_pdo_type type)
>> +{
>> +     int count = 0;
>> +     int i;
>> +
>> +     for (i = 0; i < nr_pdo; i++) {
>> +             if (pdo_type(pdo[i]) == type)
>> +                     count++;
>> +     }
>> +     return count;
>> +}
>> +
>>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>>  {
>>       struct tcpm_port *port;
>> @@ -3632,6 +3698,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>>                                         tcpc->config->nr_src_pdo);
>>       port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
>>                                         tcpc->config->nr_snk_pdo);
>> +     port->nr_fixed =  nr_type_pdos(port->snk_pdo,
>> +                                    port->nr_snk_pdo,
>> +                                    PDO_TYPE_FIXED);
>> +     port->nr_var = nr_type_pdos(port->snk_pdo,
>> +                                 port->nr_snk_pdo,
>> +                                 PDO_TYPE_VAR);
>> +     port->nr_batt = nr_type_pdos(port->snk_pdo,
>> +                                  port->nr_snk_pdo,
>> +                                  PDO_TYPE_BATT);
>>       port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
>>                                         tcpc->config->nr_snk_vdo);
>>
>> --
>> 2.15.0.403.gc27cc4dac6-goog
>
> --
> heikki