From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752511AbdKWLK0 (ORCPT ); Thu, 23 Nov 2017 06:10:26 -0500 Received: from mail1.bemta6.messagelabs.com ([193.109.254.110]:37518 "EHLO mail1.bemta6.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751584AbdKWLKY (ORCPT ); Thu, 23 Nov 2017 06:10:24 -0500 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrGKsWRWlGSWpSXmKPExsUSt3Opse7cNWJ RBpvOGVjcnnqSyeL1v+ksFs2L17NZdK3eyWJxedccNotFy1qZLZ4sPMPkwO6xYFOpx7yTgR77 565h9/j49BaLx87vDewenzfJBbBFsWbmJeVXJLBmXOzexFSwsrbi6mnxBsYb1V2MXBxCAssYJ S4u3sraxcjJwSZgITH5xAM2kISIwBQmiZvzJ7GAJJgFqiX2nzrG3sXIwSEs4Cxx6J0SSFhEwE Vi/vZtTBC2kcSvtqXsIDaLgKrE8SuLGUFsXoEAiTsHO8FqhASSJOYvbmIFGcMpYC5x9L4ZSJh RQFbiS+NqZohN4hK3nswHK5cQEJBYsuc8M4QtKvHy8T9WCFteYvu33+wQtr3E63vvWEBGSgjo S/Q1FkOEDSVWTTvAAmGbS5zb/AWshFlAU2L9Ln2ITYoSU7ofskMcKShxcuYTlgmM4rOQHDELo WMWko5ZSDoWMLKsYlQvTi0qSy3SNdVLKspMzyjJTczM0TU0MNPLTS0uTkxPzUlMKtZLzs/dxA iMXQYg2ME4/bL/IUZJDiYlUd6zumJRQnxJ+SmVGYnFGfFFpTmpxYcYZTg4lCR4lVYD5QSLUtN TK9Iyc4BJBCYtwcGjJMIbDJLmLS5IzC3OTIdInWI05nhybd5fJo5nM183MAux5OXnpUqJ8zqB lAqAlGaU5sENgiW3S4yyUsK8jECnCfEUpBblZpagyr9iFOdgVBLmZQaZwpOZVwK37xXQKUxAp /w8LgxySkkiQkqqgbHm9TzriNgDC+7YS7Q5yW67LPo4ffbHvIbauXKl+fKp00UVT1/scrwz+0 a6o+osraTt0/tXhCw3nfz97aYNnUekLa5OZ7i36FPmPcdnr3n0D0eL/ThfqSSurZS7g+d9JV/ 1jysMnCeFU60sVCJ+3TTrau0QnWP/vidyOof5FrHC3fOfzjqzeqsSS3FGoqEWc1FxIgAgXZ9F aQMAAA== X-Env-Sender: Adam.Thomson.Opensource@diasemi.com X-Msg-Ref: server-10.tower-194.messagelabs.com!1511435421!114338780!1 X-Originating-IP: [94.185.165.51] X-StarScan-Received: X-StarScan-Version: 9.4.45; banners=-,-,- X-VirusChecked: Checked From: Adam Thomson To: Badhri Jagan Sridharan , "heikki.krogerus@linux.intel.com" , "gregkh@linuxfoundation.org" , "dan.carpenter@oracle.com" , "linux@roeck-us.net" CC: "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: RE: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos Thread-Topic: [PATCH 2/2 v7] typec: tcpm: Only request matching pdos Thread-Index: AQHTXnaaVwiVwD1IZUORgIX229NkPaMh0z+Q Date: Thu, 23 Nov 2017 11:10:18 +0000 Message-ID: <2E89032DDAA8B9408CB92943514A0337014C1B0E02@SW-EX-MBX01.diasemi.com> References: <20171116010156.25259-1-Badhri@google.com> <20171116010156.25259-2-Badhri@google.com> In-Reply-To: <20171116010156.25259-2-Badhri@google.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.20.24.43] x-kse-attachmentfiltering-interceptor-info: protection disabled x-kse-serverinfo: sw-ex-cashub01.diasemi.com, 9 x-kse-antivirus-interceptor-info: scan successful x-kse-antivirus-info: Clean, bases: 23/11/2017 09:49:00 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id vANBAaCe004903 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’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. Hi Badhri, I have some queries on this change. At the moment the src/snk PDOs are hard 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 world examples in the kernel where this happens. Where are these functions likely 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 from 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 driver (e.g. fusb302 actually supports DT bindings which allow these to be set through 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 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 and currents but want to stay within the platforms limits. > > Signed-off-by: Badhri Jagan Sridharan > Acked-by: Heikki Krogerus > Reviewed-by: Guenter Roeck > > --- > 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 > > 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_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_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 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])) { > + ma = min_current(pdo, port->snk_pdo[j]); > + mv = pdo_fixed_voltage(pdo); > + mw = ma * mv / 1000; > + if (mw > max_mw || > + (mw == max_mw && mv > max_mv)) { > + ret = 0; > + *src_pdo = i; > + *sink_pdo = j; > + max_mw = mw; > + max_mv = mv; > + } > + /* 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])) { > + mw = min_power(pdo, port->snk_pdo[j]); > + mv = pdo_min_voltage(pdo); > + if (mw > max_mw || > + (mw == max_mw && mv > max_mv)) { > + ret = 0; > + *src_pdo = i; > + *sink_pdo = j; > + max_mw = mw; > + max_mv = mv; > + } > + } > + } > + } 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])) { > + ma = min_current(pdo, port->snk_pdo[j]); > + mv = pdo_min_voltage(pdo); > + mw = ma * mv / 1000; > + if (mw > max_mw || > + (mw == max_mw && mv > max_mv)) { > + ret = 0; > + *src_pdo = i; > + *sink_pdo = j; > + max_mw = mw; > + max_mv = mv; > + } > + } > + } > } > } > > @@ -1811,13 +1865,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) > @@ -1825,26 +1880,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", > @@ -1853,16 +1910,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]" : ""); > } > > @@ -3593,6 +3650,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 = 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; > @@ -3638,6 +3708,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); > + port->nr_var = nr_type_pdos(port->snk_pdo, > + port->nr_snk_pdo, > + PDO_TYPE_VAR); > + port->nr_batt = nr_type_pdos(port->snk_pdo, > + port->nr_snk_pdo, > + PDO_TYPE_BATT); > port->nr_snk_vdo = 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