Hi, +Dan and Guenter
I'm sorry for the late reply. These slipped under my radar. I do have a one more proposal below, and a few nits.. On Wed, Oct 18, 2017 at 01:22:48PM -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 <bad...@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.carpen...@oracle.com > > Changelog since v2: > - Rebase > > drivers/usb/typec/tcpm.c | 155 > ++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 120 insertions(+), 35 deletions(-) > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index e3f8b3da46b0..1b37333dc63b 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; > > @@ -1762,39 +1765,92 @@ static int tcpm_pd_check_request(struct tcpm_port > *port) > return 0; > } > > -static int tcpm_pd_select_pdo(struct tcpm_port *port) > +static void tcpm_pd_evaluate_pdo(struct tcpm_port *port, int src_pdo_index, > + int snk_pdo_index, int *max_mw, int *max_mv, > + int *selected_src_pdo, > + int *matching_snk_pdo) > { > - unsigned int i, max_mw = 0, max_mv = 0; > + unsigned int mv = 0, ma = 0, mw = 0; > + u32 src_pdo = port->source_caps[src_pdo_index]; > + u32 snk_pdo = port->snk_pdo[snk_pdo_index]; > + enum pd_pdo_type type = pdo_type(src_pdo); > + > + mv = (type == PDO_TYPE_FIXED) ? pdo_fixed_voltage(src_pdo) : > + pdo_min_voltage(src_pdo); > + > + if (type == PDO_TYPE_BATT) { > + mw = min(pdo_max_power(src_pdo), pdo_max_power(snk_pdo)); > + } else { > + ma = min(pdo_max_current(src_pdo), pdo_max_current(snk_pdo)); > + mw = ma * mv / 1000; > + } > + > + /* Perfer higher voltages if available */ > + if (mw > *max_mw || (mw == *max_mw && mv > *max_mv)) { > + *selected_src_pdo = src_pdo_index; > + *matching_snk_pdo = snk_pdo_index; > + *max_mw = mw; > + *max_mv = mv; > + } > +} > + > +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo) > +{ > + unsigned int i, j, max_mw = 0, max_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_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])) { > + tcpm_pd_evaluate_pdo(port, i, j, > + &max_mw, &max_mv, > + &ret, sink_pdo); > + /* There could only be one fixed pdo > + * at a specific voltage level. > + * So breaking here. > + */ > + break; > + } > + } > + } else if (type == PDO_TYPE_BATT) { > + for (j = port->nr_fixed; > + j < port->nr_fixed + > + port->nr_batt; > + j++) { > + if (pdo_min_voltage(pdo) >= > + pdo_min_voltage(port->snk_pdo[j]) && > + pdo_max_voltage(pdo) <= > + pdo_max_voltage(port->snk_pdo[j])) { > + tcpm_pd_evaluate_pdo(port, i, j, > + &max_mw, &max_mv, > + &ret, sink_pdo); > + } > + } > + } else if (type == PDO_TYPE_VAR) { > + for (j = port->nr_fixed + > + port->nr_batt; > + j < port->nr_fixed + > + port->nr_batt + > + port->nr_var; > + j++) { > + if (pdo_min_voltage(pdo) >= > + pdo_min_voltage(port->snk_pdo[j]) && > + pdo_max_voltage(pdo) <= > + pdo_max_voltage(port->snk_pdo[j])) { > + tcpm_pd_evaluate_pdo(port, i, j, > + &max_mw, &max_mv, > + &ret, sink_pdo); > + } > + } > } Wouldn't it make sense to call tcpm_pd_evaluate_pdo() here ones and run those nested loops there? You are already checking the type there in any case. > } > > @@ -1806,13 +1862,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 = tcpm_pd_select_pdo(port); > + index = tcpm_pd_select_pdo(port, &snk_pdo_index); > if (index < 0) > return -EINVAL; > pdo = port->source_caps[index]; > + matching_snk_pdo = port->snk_pdo[snk_pdo_index]; > type = pdo_type(pdo); > > if (type == PDO_TYPE_FIXED) > @@ -1820,26 +1877,32 @@ 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(pdo_max_power(pdo), pdo_max_power(matching_snk_pdo)); > + ma = 1000 * mw / mv; > } else { > ma = min(pdo_max_current(pdo), > - 1000 * port->max_snk_mw / mv); > + pdo_max_current(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) { > + if (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); > + } How about: 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); Though, that does not make it any more readable. Just a proposal. > } > > tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d", > @@ -3588,6 +3651,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) Please align that properly. > +{ > + int count = 0; > + int i; > + > + for (i = 0; i < nr_pdo; i++) { > + if (pdo_type(pdo[i]) == type) > + count++; > + } > + return count; > +} > + > struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev > *tcpc) > { > struct tcpm_port *port; > @@ -3633,6 +3709,15 @@ struct tcpm_port *tcpm_register_port(struct device > *dev, struct tcpc_dev *tcpc) > tcpc->config->nr_src_pdo); > port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo, > tcpc->config->nr_snk_pdo); > + port->nr_fixed = nr_type_pdos(port->snk_pdo, > + port->nr_snk_pdo, > + PDO_TYPE_FIXED); Ditto. > + port->nr_var = nr_type_pdos(port->snk_pdo, > + port->nr_snk_pdo, > + PDO_TYPE_VAR); Ditto. > + port->nr_batt = nr_type_pdos(port->snk_pdo, > + port->nr_snk_pdo, > + PDO_TYPE_BATT); Ditto. Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html