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
 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 
>> ---
>> 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.carpen...@oracle.com
>>
>> Changelog since v2:
>> - Rebase
>>
>> Changelog since v3:
>> - Refactored code fixed formatting issues as suggested
>>   by heikki.kroge...@linux.intel.com
>>
>> 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 

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 = 

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

[Heikki Krogerus]

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 
> ---
> 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.carpen...@oracle.com
> 
> Changelog since v2:
> - Rebase
> 
> Changelog since v3:
> - Refactored code fixed formatting issues as suggested
>   by heikki.kroge...@linux.intel.com
> 
> 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 


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.

>  {
> - 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])) {
> +   

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

[Badhri Jagan Sridharan]

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 
---
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.carpen...@oracle.com

Changelog since v2:
- Rebase

Changelog since v3:
- Refactored code fixed formatting issues as suggested
  by heikki.kroge...@linux.intel.com

Changelog since v4:
- Rebase

 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)
 {
-   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.
+