From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751470AbdKMANY (ORCPT ); Sun, 12 Nov 2017 19:13:24 -0500 Received: from mail-ot0-f195.google.com ([74.125.82.195]:54045 "EHLO mail-ot0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751043AbdKMANW (ORCPT ); Sun, 12 Nov 2017 19:13:22 -0500 X-Google-Smtp-Source: AGs4zMZEotDtHY8ObKJng1rSk5rsvLszZOdqGj2J7JzgkIAs2ECNpDD8gqBhNFHK7MR+bpUmGkSbaJBfNSzMBghCnhY= MIME-Version: 1.0 In-Reply-To: <20171107120733.GE18063@kuha.fi.intel.com> References: <20171104192213.14117-1-Badhri@google.com> <20171104192213.14117-2-Badhri@google.com> <20171107120733.GE18063@kuha.fi.intel.com> From: Badhri Jagan Sridharan Date: Sun, 12 Nov 2017 16:12:40 -0800 Message-ID: Subject: Re: [PATCH 2/2 v5] typec: tcpm: Only request matching pdos To: Heikki Krogerus Cc: Greg Kroah-Hartman , Dan Carpenter , Guenter Roeck , USB , LKML Content-Type: text/plain; charset="UTF-8" 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 quoted-printable to 8bit by nfs id vAD0DTSg022194 On Tue, Nov 7, 2017 at 4:07 AM, Heikki Krogerus wrote: > On Sat, Nov 04, 2017 at 12:22:13PM -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 >> --- >> 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 > > I'm fine with this, but in case you'll prepare one more version, I'll > put a few really minor nits below. I would prefer though that Guenter > gave his ACK to any patches touching tcpm.c or tcpci.c. > > But FWIW: > > Acked-by: Heikki Krogerus Thanks Heikki ! Addressing the minor nits as well. Guenter, Are you OK with this patch ? Regards, Badhri > > > Thanks, > >> drivers/usb/typec/tcpm.c | 147 +++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 111 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c >> index 8b30ab69ed79..9cd8fff12809 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) > > To me it's a bit confusing that your are returning the port's source > capability index as the return value, and handling the sink pdo index > as pointer argument. It would be more clear to just return > success/failure (0/-EINVAL), and handled both indexes as arguments to > the function. Addressing this in the next patch. > >> { >> - 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 = 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 = 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 = i; >> + *sink_pdo = j; >> + max_mw = mw; >> + max_mv = 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 = 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) >> @@ -1819,26 +1870,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); > > After thinking about this a bit more: > > if (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)) > if (type == PDO_TYPE_BATT) > max_mw = pdo_max_power(matching_snk_pdo); > else > max_ma = pdo_max_current(matching_snk_pdo); > Hmm.. Only BATT pdos have max_power field, VARIABLE and FIXED PDOs only have max_current field. Table 6-9 Fixed Supply PDO - Source : "B9…0 Maximum Current in 10mA units" Table 6-11 Variable Supply (non-Battery) PDO - Source : "B9…0 Maximum Current in 10mA units " Table 6-12 Battery Supply PDO - Source : " B9…0 Maximum Allowable Power in 250mW units" IMHO changing it this way is not accurate. Makes sense ? >> } >> >> tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d", >> @@ -3587,6 +3640,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; >> @@ -3632,6 +3698,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.403.gc27cc4dac6-goog > > -- > heikki