> -----Original Message-----
> From: Hans de Goede [mailto:[email protected]]
> Sent: 2018年4月14日 0:54
> To: Guenter Roeck <[email protected]>; Jun Li <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH v4 1/5] usb: typec: tcpm: pdo matching optimization
>
> Hi,
>
> On 13-04-18 17:58, Guenter Roeck wrote:
> > On Fri, Apr 13, 2018 at 10:18:41PM +0800, Li Jun wrote:
> >> This patch is a combination of commit 57e6f0d7b804
> >> ("typec: tcpm: Only request matching pdos") and source pdo selection
> >> optimization based on it, instead of only compare between the same
> >> pdo type of sink and source, we should check source pdo voltage range
> >> is within the voltage range of one sink pdo.
> >>
> >> Reviewed-by: Hans de Goede <[email protected]>
> >> Signed-off-by: Li Jun <[email protected]>
> >> ---
> >> drivers/usb/typec/tcpm.c | 102
> ++++++++++++++++++++++++++++++-----------------
> >> 1 file changed, 65 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> >> index 677d121..c127339 100644
> >> --- a/drivers/usb/typec/tcpm.c
> >> +++ b/drivers/usb/typec/tcpm.c
> >> @@ -1772,40 +1772,65 @@ 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,
> >> + int *src_pdo)
> >> {
> >> - unsigned int i, max_mw = 0, max_mv = 0;
> >> + unsigned int i, j, max_src_mv = 0, min_src_mv = 0, max_mw = 0,
> >> + max_mv = 0, src_mw = 0, src_ma = 0, max_snk_mv = 0,
> >> + min_snk_mv = 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_FIXED) {
> >> + max_src_mv = pdo_fixed_voltage(pdo);
> >> + min_src_mv = max_src_mv;
> >> + } else {
> >> + max_src_mv = pdo_max_voltage(pdo);
> >> + min_src_mv = pdo_min_voltage(pdo);
> >> + }
> >>
> >> if (type == PDO_TYPE_BATT) {
> >> - mw = pdo_max_power(pdo);
> >> + src_mw = pdo_max_power(pdo);
> >> } else {
> >> - ma = min(pdo_max_current(pdo),
> >> - port->max_snk_ma);
> >> - mw = ma * mv / 1000;
> >> + src_ma = pdo_max_current(pdo);
> >> + src_mw = src_ma * min_src_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;
> >> + for (j = 0; j < port->nr_snk_pdo; j++) {
> >> + pdo = port->snk_pdo[j];
> >> +
> >> + if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> >> + min_snk_mv = pdo_fixed_voltage(pdo);
> >> + max_snk_mv = pdo_fixed_voltage(pdo);
> >> + } else {
> >> + min_snk_mv = pdo_min_voltage(pdo);
> >> + max_snk_mv = pdo_max_voltage(pdo);
> >> + }
> >> +
> >> + if (max_src_mv <= max_snk_mv &&
> >> + min_src_mv >= min_snk_mv) {
> >> + /* Prefer higher voltages if available */
> >> + if ((src_mw == max_mw && min_src_mv > max_mv) ||
> >> + src_mw > max_mw) {
> >> + *src_pdo = i;
> >> + *sink_pdo = j;
> >> + max_mw = src_mw;
> >> + max_mv = min_src_mv;
> >> + }
> >> + break;
> >> + }
> >> }
> >> +
> >
> > Nitpick, but that added empty line is uneceessary.
> >
> >> }
> >>
> >> return ret;
> >> @@ -1816,13 +1841,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 src_pdo_index, snk_pdo_index;
> >> + u32 pdo, matching_snk_pdo;
> >>
> >> - index = tcpm_pd_select_pdo(port);
> >> - if (index < 0)
> >> + if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
> >> return -EINVAL;
> >
> > This may save you a variable, but tcpm_pd_select_pdo() may at some
> > point return a different error. Either case, this may trigger a static
> > analyzer warning that you are overriding a return value.
> >
> > Besides, does this even work ? I don't see where the function would
> > return anything but -EINVAL.
>
> Good catch, this indeed looks like this was not tested ?
I had this patch of old version tested, with new version only
did rebase and remove unused variable, I assumed it should
work the same way, didn't realize I actually removed one line
code "ret = 0", sorry for this.
>
> Next time please always test if possible.
Sure.
>
> we need to set ret = 0 in tcpm_pd_select_pdo() when we assign a value to the
> src and sink pdos.
>
> Regards,
>
> Hans
>
>
>
>
> >
> >> - pdo = port->source_caps[index];
> >> +
> >> + pdo = port->source_caps[src_pdo_index];
> >> + matching_snk_pdo = port->snk_pdo[snk_pdo_index];
> >> type = pdo_type(pdo);
> >>
> >> if (type == PDO_TYPE_FIXED)
> >> @@ -1830,26 +1856,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);
> >> }
> >>
> >> tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s
> polarity=%d",
> >> @@ -1858,16 +1886,16 @@ static int tcpm_pd_build_request(struct
> tcpm_port *port, u32 *rdo)
> >> port->polarity);
> >>
> >> if (type == PDO_TYPE_BATT) {
> >> - *rdo = RDO_BATT(index + 1, mw, max_mw, flags);
> >> + *rdo = 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 = RDO_FIXED(index + 1, ma, max_ma, flags);
> >> + *rdo = 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]" : "");
> >> }
> >>
> >> --
> >> 2.7.4
> >>
