Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <2E89032DDAA8B9408CB92943514A0337014C1B0E02@SW-EX-MBX01.diasemi.com>
References: <20171116010156.25259-1-Badhri@google.com> <20171116010156.25259-2-Badhri@google.com>
 <2E89032DDAA8B9408CB92943514A0337014C1B0E02@SW-EX-MBX01.diasemi.com>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Mon, 27 Nov 2017 17:38:02 -0800
Message-ID: <CAPTae5JR0ZTFJLXrAYrpTnTEtGybDrjiTy8hAhsLAP+o18M_sw@mail.gmail.com>
Subject: Re: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos
To:     Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Cc:     "heikki.krogerus@linux.intel.com" <heikki.krogerus@linux.intel.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "dan.carpenter@oracle.com" <dan.carpenter@oracle.com>,
        "linux@roeck-us.net" <linux@roeck-us.net>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 23, 2017 at 3:10 AM, Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
> On 16 November 2017 01:02, 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=E2=80=99s 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.
>
> Hi Badhri,
>
> I have some queries on this change. At the moment the src/snk PDOs are ha=
rd
> coded in the relevant PD controller driver (e.g. fusb302.c), and the only=
 way
> to update these is to call the tcpm_update_source_capabilities() or
> tcpm_update_sink_capabilities() functions. However there are no real worl=
d

Good point ! But I dont see any code which calls this either. I believe
Guenter added this. Guenter do you know ?

> examples in the kernel where this happens. Where are these functions like=
ly to
> be called from, as wherever that is it will need a reference to the port?
> Actually should we be adding DT bindings to set supported src/snk PDOs fr=
om FW,
> if you're taking this approach to PDO selection?
>
> This patch also seemingly leaves 'max_snk_mv', 'max_snk_ma' and 'max_snk_=
mv'
> redundant, although those values can be configured from a PD controller d=
river
> (e.g. fusb302 actually supports DT bindings which allow these to be set t=
hrough
> FW). Now these DT bindings are basically made redundant by your change as=
 they
> have no impact. Are we expecting these to be used again in the future, or=
 should

Yes, I think  'max_snk_mv', 'max_snk_ma' and 'max_snk_mw' etc should be rem=
oved.
The problem here is that maintaining these values implies that tcpm is
not going to
request pdo based on the sink_caps that are published. All these
values can be derived from
the sink_pdo objects that were declared, hence, they are redundant,
I will update the patch to remove this.

> these be removed? FYI, as part of my PPS patch set I have been using them=
 as
> part of the PPS APDO selection criteria, based on TCPM code prior to your
> modifications, as for PPS we're interested in a wide range of voltages an=
d
> currents but want to stay within the platforms limits.

Arent you defining a new PDO type similar to PDO_FIXED, PDO_VARIABLE etc ?
If so values such as " 'max_snk_mv', 'max_snk_ma' and
'max_snk_mw' " should be part of the APDO object right ? So why would
you still need
 'max_snk_mv', 'max_snk_ma' and 'max_snk_mw'  ?

>
>>
>> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
>> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> ---
>> 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@oracle.com
>>
>> Changelog since v2:
>> - Rebase
>>
>> Changelog since v3:
>> - Refactored code fixed formatting issues as suggested
>>   by heikki.krogerus@linux.intel.com
>>
>> Changelog since v4:
>> - Rebase
>>
>> Changelog since v5:
>> - handling the sink pdo index as pointer argument in tcpm_pd_select_pdo
>> - ACK by heikki.krogerus@linux.intel.com
>>
>> Changelog since v6:
>> - Added Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>>  drivers/usb/typec/tcpm.c | 163 +++++++++++++++++++++++++++++++++++-----=
------
>> -
>>  1 file changed, 121 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>> index 8b637a4b474b..f4d563ee7690 100644
>> --- a/drivers/usb/typec/tcpm.c
>> +++ b/drivers/usb/typec/tcpm.c
>> @@ -252,6 +252,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;
>>
>> @@ -1767,39 +1770,90 @@ static int tcpm_pd_check_request(struct tcpm_por=
t
>> *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,
>> +                           int *src_pdo)
>>  {
>> -     unsigned int i, max_mw =3D 0, max_mv =3D 0;
>> +     unsigned int i, j, max_mw =3D 0, max_mv =3D 0, mw =3D 0, mv =3D 0,=
 ma =3D 0;
>>       int ret =3D -EINVAL;
>>
>>       /*
>> -      * Select the source PDO providing the most power while staying wi=
thin
>> -      * 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 =3D 0; i < port->nr_source_caps; i++) {
>>               u32 pdo =3D port->source_caps[i];
>>               enum pd_pdo_type type =3D pdo_type(pdo);
>> -             unsigned int mv, ma, mw;
>>
>> -             if (type =3D=3D PDO_TYPE_FIXED)
>> -                     mv =3D pdo_fixed_voltage(pdo);
>> -             else
>> -                     mv =3D pdo_min_voltage(pdo);
>> -
>> -             if (type =3D=3D PDO_TYPE_BATT) {
>> -                     mw =3D pdo_max_power(pdo);
>> -             } else {
>> -                     ma =3D min(pdo_max_current(pdo),
>> -                              port->max_snk_ma);
>> -                     mw =3D ma * mv / 1000;
>> -             }
>> -
>> -             /* Perfer higher voltages if available */
>> -             if ((mw > max_mw || (mw =3D=3D max_mw && mv > max_mv)) &&
>> -                 mv <=3D port->max_snk_mv) {
>> -                     ret =3D i;
>> -                     max_mw =3D mw;
>> -                     max_mv =3D mv;
>> +             if (type =3D=3D PDO_TYPE_FIXED) {
>> +                     for (j =3D 0; j < port->nr_fixed; j++) {
>> +                             if (pdo_fixed_voltage(pdo) =3D=3D
>> +                                 pdo_fixed_voltage(port->snk_pdo[j])) {
>> +                                     ma =3D min_current(pdo, port->snk_=
pdo[j]);
>> +                                     mv =3D pdo_fixed_voltage(pdo);
>> +                                     mw =3D ma * mv / 1000;
>> +                                     if (mw > max_mw ||
>> +                                         (mw =3D=3D max_mw && mv > max_=
mv)) {
>> +                                             ret =3D 0;
>> +                                             *src_pdo =3D i;
>> +                                             *sink_pdo =3D j;
>> +                                             max_mw =3D mw;
>> +                                             max_mv =3D mv;
>> +                                     }
>> +                                     /* There could only be one fixed p=
do
>> +                                      * at a specific voltage level.
>> +                                      * So breaking here.
>> +                                      */
>> +                                     break;
>> +                             }
>> +                     }
>> +             } else if (type =3D=3D PDO_TYPE_BATT) {
>> +                     for (j =3D port->nr_fixed;
>> +                          j < port->nr_fixed +
>> +                              port->nr_batt;
>> +                          j++) {
>> +                             if (pdo_min_voltage(pdo) >=3D
>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>> +                                  pdo_max_voltage(pdo) <=3D
>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>> +                                     mw =3D min_power(pdo, port->snk_pd=
o[j]);
>> +                                     mv =3D pdo_min_voltage(pdo);
>> +                                     if (mw > max_mw ||
>> +                                         (mw =3D=3D max_mw && mv > max_=
mv)) {
>> +                                             ret =3D 0;
>> +                                             *src_pdo =3D i;
>> +                                             *sink_pdo =3D j;
>> +                                             max_mw =3D mw;
>> +                                             max_mv =3D mv;
>> +                                     }
>> +                             }
>> +                     }
>> +             } else if (type =3D=3D PDO_TYPE_VAR) {
>> +                     for (j =3D port->nr_fixed +
>> +                              port->nr_batt;
>> +                          j < port->nr_fixed +
>> +                              port->nr_batt +
>> +                              port->nr_var;
>> +                          j++) {
>> +                             if (pdo_min_voltage(pdo) >=3D
>> +                                  pdo_min_voltage(port->snk_pdo[j]) &&
>> +                                  pdo_max_voltage(pdo) <=3D
>> +                                  pdo_max_voltage(port->snk_pdo[j])) {
>> +                                     ma =3D min_current(pdo, port->snk_=
pdo[j]);
>> +                                     mv =3D pdo_min_voltage(pdo);
>> +                                     mw =3D ma * mv / 1000;
>> +                                     if (mw > max_mw ||
>> +                                         (mw =3D=3D max_mw && mv > max_=
mv)) {
>> +                                             ret =3D 0;
>> +                                             *src_pdo =3D i;
>> +                                             *sink_pdo =3D j;
>> +                                             max_mw =3D mw;
>> +                                             max_mv =3D mv;
>> +                                     }
>> +                             }
>> +                     }
>>               }
>>       }
>>
>> @@ -1811,13 +1865,14 @@ static int tcpm_pd_build_request(struct tcpm_por=
t
>> *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 src_pdo_index, snk_pdo_index;
>> +     u32 pdo, matching_snk_pdo;
>>
>> -     index =3D tcpm_pd_select_pdo(port);
>> -     if (index < 0)
>> +     if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
>>               return -EINVAL;
>> -     pdo =3D port->source_caps[index];
>> +
>> +     pdo =3D port->source_caps[src_pdo_index];
>> +     matching_snk_pdo =3D port->snk_pdo[snk_pdo_index];
>>       type =3D pdo_type(pdo);
>>
>>       if (type =3D=3D PDO_TYPE_FIXED)
>> @@ -1825,26 +1880,28 @@ static int tcpm_pd_build_request(struct tcpm_por=
t
>> *port, u32 *rdo)
>>       else
>>               mv =3D 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 =3D=3D PDO_TYPE_BATT) {
>> -             mw =3D pdo_max_power(pdo);
>> -             ma =3D 1000 * min(mw, port->max_snk_mw) / mv;
>> +             mw =3D min_power(pdo, matching_snk_pdo);
>> +             ma =3D 1000 * mw / mv;
>>       } else {
>> -             ma =3D min(pdo_max_current(pdo),
>> -                      1000 * port->max_snk_mw / mv);
>> +             ma =3D min_current(pdo, matching_snk_pdo);
>> +             mw =3D ma * mv / 1000;
>>       }
>> -     ma =3D min(ma, port->max_snk_ma);
>>
>>       flags =3D RDO_USB_COMM | RDO_NO_SUSPEND;
>>
>>       /* Set mismatch bit if offered power is less than operating power =
*/
>> -     mw =3D ma * mv / 1000;
>>       max_ma =3D ma;
>>       max_mw =3D mw;
>>       if (mw < port->operating_snk_mw) {
>>               flags |=3D RDO_CAP_MISMATCH;
>> -             max_mw =3D port->operating_snk_mw;
>> -             max_ma =3D max_mw * 1000 / mv;
>> +             if (type =3D=3D PDO_TYPE_BATT &&
>> +                 (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo))=
)
>> +                     max_mw =3D pdo_max_power(matching_snk_pdo);
>> +             else if (pdo_max_current(matching_snk_pdo) >
>> +                      pdo_max_current(pdo))
>> +                     max_ma =3D pdo_max_current(matching_snk_pdo);
>>       }
>>
>>       tcpm_log(port, "cc=3D%d cc1=3D%d cc2=3D%d vbus=3D%d vconn=3D%s pol=
arity=3D%d",
>> @@ -1853,16 +1910,16 @@ static int tcpm_pd_build_request(struct tcpm_por=
t
>> *port, u32 *rdo)
>>                port->polarity);
>>
>>       if (type =3D=3D PDO_TYPE_BATT) {
>> -             *rdo =3D RDO_BATT(index + 1, mw, max_mw, flags);
>> +             *rdo =3D RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
>>
>>               tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
>> -                      index, mv, mw,
>> +                      src_pdo_index, mv, mw,
>>                        flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>>       } else {
>> -             *rdo =3D RDO_FIXED(index + 1, ma, max_ma, flags);
>> +             *rdo =3D RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
>>
>>               tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
>> -                      index, mv, ma,
>> +                      src_pdo_index, mv, ma,
>>                        flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>>       }
>>
>> @@ -3593,6 +3650,19 @@ int tcpm_update_sink_capabilities(struct tcpm_por=
t
>> *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 =3D 0;
>> +     int i;
>> +
>> +     for (i =3D 0; i < nr_pdo; i++) {
>> +             if (pdo_type(pdo[i]) =3D=3D type)
>> +                     count++;
>> +     }
>> +     return count;
>> +}
>> +
>>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_de=
v *tcpc)
>>  {
>>       struct tcpm_port *port;
>> @@ -3638,6 +3708,15 @@ struct tcpm_port *tcpm_register_port(struct devic=
e
>> *dev, struct tcpc_dev *tcpc)
>>                                         tcpc->config->nr_src_pdo);
>>       port->nr_snk_pdo =3D tcpm_copy_pdos(port->snk_pdo, tcpc->config-
>> >snk_pdo,
>>                                         tcpc->config->nr_snk_pdo);
>> +     port->nr_fixed =3D  nr_type_pdos(port->snk_pdo,
>> +                                    port->nr_snk_pdo,
>> +                                    PDO_TYPE_FIXED);
>> +     port->nr_var =3D nr_type_pdos(port->snk_pdo,
>> +                                 port->nr_snk_pdo,
>> +                                 PDO_TYPE_VAR);
>> +     port->nr_batt =3D nr_type_pdos(port->snk_pdo,
>> +                                  port->nr_snk_pdo,
>> +                                  PDO_TYPE_BATT);
>>       port->nr_snk_vdo =3D tcpm_copy_vdos(port->snk_vdo, tcpc->config-
>> >snk_vdo,
>>                                         tcpc->config->nr_snk_vdo);
>>
>> --
>> 2.15.0.448.gf294e3d99a-goog
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

From 1584854993908514143@xxx Thu Nov 23 11:11:42 +0000 2017
X-GM-THRID: 1584191010093958492
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread