| Message ID | 1523629125-16332-2-git-send-email-jun.li@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <hdegoede@redhat.com> > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > 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. > - 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 > -- 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
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 <hdegoede@redhat.com> >> Signed-off-by: Li Jun <jun.li@nxp.com> >> --- >> 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 ? Next time please always test if possible. 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 >> -- 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
Hi > -----Original Message----- > From: Guenter Roeck [mailto:linux@roeck-us.net] > Sent: 2018年4月13日 23:58 > To: Jun Li <jun.li@nxp.com> > Cc: gregkh@linuxfoundation.org; robh+dt@kernel.org; > heikki.krogerus@linux.intel.com; hdegoede@redhat.com; rmfrfs@gmail.com; > yueyao.zhu@gmail.com; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v4 1/5] usb: typec: tcpm: pdo matching optimization > > 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 <hdegoede@redhat.com> > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > 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. Will remove it. > > > } > > > > 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. I will add a variable for this. > > Besides, does this even work ? I don't see where the function would return > anything but -EINVAL. Sorry, I had this patch tested, but forgot sync a change to this patch set, ret = 0 is missed. Thanks Jun > > > - 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 > >
> -----Original Message----- > From: Hans de Goede [mailto:hdegoede@redhat.com] > Sent: 2018年4月14日 0:54 > To: Guenter Roeck <linux@roeck-us.net>; Jun Li <jun.li@nxp.com> > Cc: gregkh@linuxfoundation.org; robh+dt@kernel.org; > heikki.krogerus@linux.intel.com; rmfrfs@gmail.com; yueyao.zhu@gmail.com; > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > 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 <hdegoede@redhat.com> > >> Signed-off-by: Li Jun <jun.li@nxp.com> > >> --- > >> 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 > >>
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; + } } + } 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; - 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]" : ""); }
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 <hdegoede@redhat.com> Signed-off-by: Li Jun <jun.li@nxp.com> --- drivers/usb/typec/tcpm.c | 102 ++++++++++++++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 37 deletions(-)