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: <20171104185953.9532-2-Badhri@google.com>
References: <20171104185953.9532-1-Badhri@google.com> <20171104185953.9532-2-Badhri@google.com>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Sat, 4 Nov 2017 12:08:27 -0700
Message-ID: <CAPTae5LPxdDkVtrbiCQsoWHYayjzPtX0ZS1DbKQVmSjkHUP2Pw@mail.gmail.com>
Subject: Re: [PATCH 2/2 v4] typec: tcpm: Only request matching pdos
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Guenter Roeck <linux@roeck-us.net>
Cc:     USB <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Badhri Jagan Sridharan <Badhri@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Please disregard v4. Going to send another version again.

On Sat, Nov 4, 2017 at 11:59 AM, Badhri Jagan Sridharan
<badhri@google.com> 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 f=
ield.
> 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@google.com>
> ---
> 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 v2:
> - Refactored code fixed formatting issues as suggested
>   by heikki.krogerus@linux.intel.com
>
>  drivers/usb/typec/tcpm.c | 146 +++++++++++++++++++++++++++++++++++------=
------
>  1 file changed, 111 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index ecdad71a6159..44a080202a70 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 =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 w=
ithin
> -        * 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 i;
> +                                               *sink_pdo =3D j;
> +                                               max_mw =3D mw;
> +                                               max_mv =3D mv;
> +                                       }
> +                                       /* There could only be one fixed =
pdo
> +                                        * 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_p=
do[j]);
> +                                       mv =3D pdo_min_voltage(pdo);
> +                                       if (mw > max_mw ||
> +                                           (mw =3D=3D max_mw && mv > max=
_mv)) {
> +                                               ret =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 i;
> +                                               *sink_pdo =3D j;
> +                                               max_mw =3D mw;
> +                                               max_mv =3D 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 =3D tcpm_pd_select_pdo(port);
> +       index =3D tcpm_pd_select_pdo(port, &snk_pdo_index);
>         if (index < 0)
>                 return -EINVAL;
>         pdo =3D port->source_caps[index];
> +       matching_snk_pdo =3D port->snk_pdo[snk_pdo_index];
>         type =3D pdo_type(pdo);
>
>         if (type =3D=3D PDO_TYPE_FIXED)
> @@ -1819,26 +1870,29 @@ static int tcpm_pd_build_request(struct tcpm_port=
 *port, u32 *rdo)
>         else
>                 mv =3D pdo_min_voltage(pdo);
>
> -       /* Select maximum available current within the board's power limi=
t */
> +       /* 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(pdo_max_power(pdo), pdo_max_power(matching_snk=
_pdo));

Going to reuse the macro here in the next version.

> +               ma =3D 1000 * mw / mv;
>         } else {
>                 ma =3D min(pdo_max_current(pdo),
> -                        1000 * port->max_snk_mw / mv);
> +                        pdo_max_current(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);

Same here as well.

> +               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 po=
larity=3D%d",
> @@ -3587,6 +3641,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 =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_dev=
 *tcpc)
>  {
>         struct tcpm_port *port;
> @@ -3632,6 +3699,15 @@ struct tcpm_port *tcpm_register_port(struct device=
 *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.403.gc27cc4dac6-goog
>

From 1583163200779033107@xxx Sat Nov 04 19:01:23 +0000 2017
X-GM-THRID: 1581628275778143409
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread