Message ID | 1521817127-23061-1-git-send-email-jun.li@nxp.com |
---|---|
Headers | show |
Series | usb: typec: remove max_snk_mv/ma/mw | expand |
Hi, On 23-03-18 15:58, 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. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/tcpm.c | 127 +++++++++++++++++++++++++++++++++-------------- > 1 file changed, 90 insertions(+), 37 deletions(-) > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index 677d121..50a1979 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -254,6 +254,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; > These 3 variables are now no longer needed. > @@ -1772,40 +1775,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 +1844,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 +1859,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 +1889,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]" : ""); > } > > @@ -3599,6 +3630,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; > @@ -3644,6 +3688,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); > > And the code setting them thus also is no longer needed. Otherwise this looks good to me now. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 23-03-18 15:58, Li Jun wrote: > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a > variable PDO for sink config. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/fusb302/fusb302.c | 51 +++++++++++++++++++++++++++---------- > 1 file changed, 37 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c > index 7036171..db4d9cd 100644 > --- a/drivers/usb/typec/fusb302/fusb302.c > +++ b/drivers/usb/typec/fusb302/fusb302.c > @@ -120,6 +120,7 @@ struct fusb302_chip { > enum typec_cc_polarity cc_polarity; > enum typec_cc_status cc1; > enum typec_cc_status cc2; > + u32 snk_pdo[PDO_MAX_OBJECTS]; > > #ifdef CONFIG_DEBUG_FS > struct dentry *dentry; > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = { > static const struct tcpc_config fusb302_tcpc_config = { > .src_pdo = src_pdo, > .nr_src_pdo = ARRAY_SIZE(src_pdo), > - .snk_pdo = snk_pdo, > - .nr_snk_pdo = ARRAY_SIZE(snk_pdo), > - .max_snk_mv = 5000, > - .max_snk_ma = 3000, > - .max_snk_mw = 15000, > .operating_snk_mw = 2500, > .type = TYPEC_PORT_DRP, > .data = TYPEC_PORT_DRD, > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip) > return 0; > } > > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) > +{ > + struct device *dev = chip->dev; > + u32 mv, ma, mw, min_mv; > + unsigned int i; > + > + /* Copy the static snk pdo */ > + for (i = 0; i < ARRAY_SIZE(snk_pdo); i++) > + chip->snk_pdo[i] = snk_pdo[i]; The static snk PDO is constant and only contains: PDO_FIXED(5000, 400, PDO_FIXED_FLAGS), So you can just do: chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS); Here. > + > + if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) || > + device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) || > + device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw)) You can drop the reading of "fcs,max-sink-microwatt" here, it was only ever added because the tcpm code used to depend on max_mw being set, now that that is no longer the case, we don't need it. So this entire function can be simplified to: static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) { struct device *dev = chip->dev; u32 max_uv, max_ua; chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS); if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &max_uv) || device_property_read_u32(dev, "fcs,max-sink-microamp", &max_ua)) return 1; chip->snk_pdo[1] = PDO_VAR(5000, max_uv / 1000, max_ua / 1000); return 2; } > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client, > chip->tcpc_dev.config = &chip->tcpc_config; > mutex_init(&chip->lock); > > - if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v)) > - chip->tcpc_config.max_snk_mv = v / 1000; > - > - if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v)) > - chip->tcpc_config.max_snk_ma = v / 1000; > - > - if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v)) > - chip->tcpc_config.max_snk_mw = v / 1000; > - > if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v)) > chip->tcpc_config.operating_snk_mw = v / 1000; > > + /* Composite sink PDO */ > + chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip); > + chip->tcpc_config.snk_pdo = chip->snk_pdo; > + > /* > * Devicetree platforms should get extcon via phandle (not yet > * supported). On ACPI platforms, we get the name from a device prop. > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 23-03-18 15:58, Li Jun wrote: > This patch set is to remove max_snk_mv/ma/mw configs, as we should > define the sink capability by sink PDOs, the first patch update > the source PDO match policy by compare the voltage range between > source and sink PDOs no matter what type they are, the following > patchs remove those 3 variables from 2 existing users by adding > a variable PDO, then finial patch remove the max_snk_* from tcpm. > > Changes for v2: > - rebase the 1st patch to be based on commit 6f566af34628 > ("Revert "typec: tcpm: Only request matching pdos""). > - Convert the device properties passing max_snk_* to be a > variable sink pdo for fusb302. Thank you for the new version. I've replied with some comments to patches 1 and 2, the other 3 patches look good to me. Regards, Hans > > Li Jun (5): > usb: typec: tcpm: pdo matching optimization > usb: typec: fusb302: remove max_snk_* for sink config > dt-bindings: usb: fusb302: remove max-sink-* properties > usb: typec: wcove: remove max_snk_* for sink config > usb: typec: tcpm: remove max_snk_mv/ma/mw > > .../devicetree/bindings/usb/fcs,fusb302.txt | 6 - > drivers/usb/typec/fusb302/fusb302.c | 51 +++++--- > drivers/usb/typec/tcpm.c | 139 +++++++++++++-------- > drivers/usb/typec/typec_wcove.c | 4 +- > include/linux/usb/tcpm.h | 9 -- > 5 files changed, 128 insertions(+), 81 deletions(-) > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Li, On 2018-03-23 15:58, Li Jun wrote: > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a > variable PDO for sink config. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/typec/fusb302/fusb302.c | 51 +++++++++++++++++++++++++++---------- > 1 file changed, 37 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c > index 7036171..db4d9cd 100644 > --- a/drivers/usb/typec/fusb302/fusb302.c > +++ b/drivers/usb/typec/fusb302/fusb302.c > @@ -120,6 +120,7 @@ struct fusb302_chip { > enum typec_cc_polarity cc_polarity; > enum typec_cc_status cc1; > enum typec_cc_status cc2; > + u32 snk_pdo[PDO_MAX_OBJECTS]; > > #ifdef CONFIG_DEBUG_FS > struct dentry *dentry; > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = { > static const struct tcpc_config fusb302_tcpc_config = { > .src_pdo = src_pdo, > .nr_src_pdo = ARRAY_SIZE(src_pdo), > - .snk_pdo = snk_pdo, > - .nr_snk_pdo = ARRAY_SIZE(snk_pdo), > - .max_snk_mv = 5000, > - .max_snk_ma = 3000, > - .max_snk_mw = 15000, > .operating_snk_mw = 2500, > .type = TYPEC_PORT_DRP, > .data = TYPEC_PORT_DRD, > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip) > return 0; > } > > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) > +{ > + struct device *dev = chip->dev; > + u32 mv, ma, mw, min_mv; > + unsigned int i; > + > + /* Copy the static snk pdo */ > + for (i = 0; i < ARRAY_SIZE(snk_pdo); i++) > + chip->snk_pdo[i] = snk_pdo[i]; > + > + if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) || > + device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) || > + device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw)) > + return i; > + > + mv = mv / 1000; > + ma = ma / 1000; > + mw = mw / 1000; > + > + min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma; > + if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED)) You've got the parentheses wrong. Apart from that I don't like/understand why the PDO's should be fixed. Every product should be able to have its own settings, including the first PDO (e.g. it might need to specify a higher current and/or set the "Higher Capability" bit). I think this would be best solved the same way as in your TCPCI driver patch series [1] with a list freely specified by a property. [1] https://www.spinics.net/lists/linux-usb/msg167398.html > + min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1])); > + else > + min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1])); > + ma = min(ma, 1000 * mw / min_mv); > + > + /* Insert the new pdo to the end */ > + chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma); > + > + return i+1; > +} > + > static int fusb302_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client, > chip->tcpc_dev.config = &chip->tcpc_config; > mutex_init(&chip->lock); > > - if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v)) > - chip->tcpc_config.max_snk_mv = v / 1000; > - > - if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v)) > - chip->tcpc_config.max_snk_ma = v / 1000; > - > - if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v)) > - chip->tcpc_config.max_snk_mw = v / 1000; > - > if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v)) > chip->tcpc_config.operating_snk_mw = v / 1000; > > + /* Composite sink PDO */ > + chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip); > + chip->tcpc_config.snk_pdo = chip->snk_pdo; > + > /* > * Devicetree platforms should get extcon via phandle (not yet > * supported). On ACPI platforms, we get the name from a device prop. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 04-04-18 14:06, Mats Karrman wrote: > Hi Li, > > On 2018-03-23 15:58, Li Jun wrote: > >> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a >> variable PDO for sink config. >> >> Signed-off-by: Li Jun <jun.li@nxp.com> >> --- >> drivers/usb/typec/fusb302/fusb302.c | 51 +++++++++++++++++++++++++++---------- >> 1 file changed, 37 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c >> index 7036171..db4d9cd 100644 >> --- a/drivers/usb/typec/fusb302/fusb302.c >> +++ b/drivers/usb/typec/fusb302/fusb302.c >> @@ -120,6 +120,7 @@ struct fusb302_chip { >> enum typec_cc_polarity cc_polarity; >> enum typec_cc_status cc1; >> enum typec_cc_status cc2; >> + u32 snk_pdo[PDO_MAX_OBJECTS]; >> #ifdef CONFIG_DEBUG_FS >> struct dentry *dentry; >> @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = { >> static const struct tcpc_config fusb302_tcpc_config = { >> .src_pdo = src_pdo, >> .nr_src_pdo = ARRAY_SIZE(src_pdo), >> - .snk_pdo = snk_pdo, >> - .nr_snk_pdo = ARRAY_SIZE(snk_pdo), >> - .max_snk_mv = 5000, >> - .max_snk_ma = 3000, >> - .max_snk_mw = 15000, >> .operating_snk_mw = 2500, >> .type = TYPEC_PORT_DRP, >> .data = TYPEC_PORT_DRD, >> @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip) >> return 0; >> } >> +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) >> +{ >> + struct device *dev = chip->dev; >> + u32 mv, ma, mw, min_mv; >> + unsigned int i; >> + >> + /* Copy the static snk pdo */ >> + for (i = 0; i < ARRAY_SIZE(snk_pdo); i++) >> + chip->snk_pdo[i] = snk_pdo[i]; >> + >> + if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) || >> + device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) || >> + device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw)) >> + return i; >> + >> + mv = mv / 1000; >> + ma = ma / 1000; >> + mw = mw / 1000; >> + >> + min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma; >> + if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED)) > > You've got the parentheses wrong. > > Apart from that I don't like/understand why the PDO's should be fixed. > Every product should be able to have its own settings, including the first PDO (e.g. it > might need to specify a higher current and/or set the "Higher Capability" bit). > > I think this would be best solved the same way as in your TCPCI driver patch series [1] > with a list freely specified by a property. > > [1] https://www.spinics.net/lists/linux-usb/msg167398.html Hmm, interesting, for the x86 use-case that would require updating the properties for the fusb302 defined in: drivers/platform/x86/intel_cht_int33fe.c In tandem, which is easily doable. But what about other users of the fusb302 driver? Since this driver was added before I started using it for some x86 boards, I assume there are some non x86 users, so we should preserve compatibility for DTB files which don't define any sink PDOs in their properties, I guess we could fall to a default fixed 5V sink pdo for those. Regards, Hans > >> + min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1])); >> + else >> + min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1])); >> + ma = min(ma, 1000 * mw / min_mv); >> + >> + /* Insert the new pdo to the end */ >> + chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma); >> + >> + return i+1; >> +} >> + >> static int fusb302_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client, >> chip->tcpc_dev.config = &chip->tcpc_config; >> mutex_init(&chip->lock); >> - if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v)) >> - chip->tcpc_config.max_snk_mv = v / 1000; >> - >> - if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v)) >> - chip->tcpc_config.max_snk_ma = v / 1000; >> - >> - if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v)) >> - chip->tcpc_config.max_snk_mw = v / 1000; >> - >> if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v)) >> chip->tcpc_config.operating_snk_mw = v / 1000; >> + /* Composite sink PDO */ >> + chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip); >> + chip->tcpc_config.snk_pdo = chip->snk_pdo; >> + >> /* >> * Devicetree platforms should get extcon via phandle (not yet >> * supported). On ACPI platforms, we get the name from a device prop. >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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: Hans de Goede [mailto:hdegoede@redhat.com] > Sent: 2018年4月3日 23:17 > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; > mark.rutland@arm.com; heikki.krogerus@linux.intel.com > Cc: linux@roeck-us.net; rmfrfs@gmail.com; yueyao.zhu@gmail.com; > linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v2 1/5] usb: typec: tcpm: pdo matching optimization > > Hi, > > On 23-03-18 15:58, 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. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/typec/tcpm.c | 127 > +++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 90 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index > > 677d121..50a1979 100644 > > --- a/drivers/usb/typec/tcpm.c > > +++ b/drivers/usb/typec/tcpm.c > > @@ -254,6 +254,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; > > > > These 3 variables are now no longer needed. Yes, I will remove them. > > > @@ -1772,40 +1775,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 +1844,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 +1859,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 +1889,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]" : ""); > > } > > > > @@ -3599,6 +3630,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; > > @@ -3644,6 +3688,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); > > > > > > And the code setting them thus also is no longer needed. I will remove them. > > Otherwise this looks good to me now. Thanks for review. Jun > > Regards, > > Hans >
Hi > -----Original Message----- > From: Hans de Goede [mailto:hdegoede@redhat.com] > Sent: 2018年4月3日 23:26 > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; > mark.rutland@arm.com; heikki.krogerus@linux.intel.com > Cc: linux@roeck-us.net; rmfrfs@gmail.com; yueyao.zhu@gmail.com; > linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink > config > > Hi, > > On 23-03-18 15:58, Li Jun wrote: > > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a > > variable PDO for sink config. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/typec/fusb302/fusb302.c | 51 > +++++++++++++++++++++++++++---------- > > 1 file changed, 37 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/usb/typec/fusb302/fusb302.c > > b/drivers/usb/typec/fusb302/fusb302.c > > index 7036171..db4d9cd 100644 > > --- a/drivers/usb/typec/fusb302/fusb302.c > > +++ b/drivers/usb/typec/fusb302/fusb302.c > > @@ -120,6 +120,7 @@ struct fusb302_chip { > > enum typec_cc_polarity cc_polarity; > > enum typec_cc_status cc1; > > enum typec_cc_status cc2; > > + u32 snk_pdo[PDO_MAX_OBJECTS]; > > > > #ifdef CONFIG_DEBUG_FS > > struct dentry *dentry; > > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = { > > static const struct tcpc_config fusb302_tcpc_config = { > > .src_pdo = src_pdo, > > .nr_src_pdo = ARRAY_SIZE(src_pdo), > > - .snk_pdo = snk_pdo, > > - .nr_snk_pdo = ARRAY_SIZE(snk_pdo), > > - .max_snk_mv = 5000, > > - .max_snk_ma = 3000, > > - .max_snk_mw = 15000, > > .operating_snk_mw = 2500, > > .type = TYPEC_PORT_DRP, > > .data = TYPEC_PORT_DRD, > > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip) > > return 0; > > } > > > > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) > > +{ > > + struct device *dev = chip->dev; > > + u32 mv, ma, mw, min_mv; > > + unsigned int i; > > + > > + /* Copy the static snk pdo */ > > + for (i = 0; i < ARRAY_SIZE(snk_pdo); i++) > > + chip->snk_pdo[i] = snk_pdo[i]; > > The static snk PDO is constant and only contains: > PDO_FIXED(5000, 400, PDO_FIXED_FLAGS), > > So you can just do: > > chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS); > > Here. > > > + > > + if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) || > > + device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) || > > + device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw)) > > You can drop the reading of "fcs,max-sink-microwatt" here, it was only ever > added because the tcpm code used to depend on max_mw being set, now that > that is no longer the case, we don't need it. > > So this entire function can be simplified to: > > static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) { > struct device *dev = chip->dev; > u32 max_uv, max_ua; > > chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS); > > if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &max_uv) || > device_property_read_u32(dev, "fcs,max-sink-microamp", &max_ua)) > return 1; > > chip->snk_pdo[1] = PDO_VAR(5000, max_uv / 1000, max_ua / 1000); > return 2; > } OK, I will change in v3. Thanks Jun > > > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client, > > chip->tcpc_dev.config = &chip->tcpc_config; > > mutex_init(&chip->lock); > > > > - if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v)) > > - chip->tcpc_config.max_snk_mv = v / 1000; > > - > > - if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v)) > > - chip->tcpc_config.max_snk_ma = v / 1000; > > - > > - if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v)) > > - chip->tcpc_config.max_snk_mw = v / 1000; > > - > > if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v)) > > chip->tcpc_config.operating_snk_mw = v / 1000; > > > > + /* Composite sink PDO */ > > + chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip); > > + chip->tcpc_config.snk_pdo = chip->snk_pdo; > > + > > /* > > * Devicetree platforms should get extcon via phandle (not yet > > * supported). On ACPI platforms, we get the name from a device prop. > > > > Regards, > > Hans
Hi > -----Original Message----- > From: Mats Karrman [mailto:mats.dev.list@gmail.com] > Sent: 2018年4月4日 20:07 > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org; > mark.rutland@arm.com; heikki.krogerus@linux.intel.com; > hdegoede@redhat.com > Cc: linux@roeck-us.net; rmfrfs@gmail.com; yueyao.zhu@gmail.com; > linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink > config > > Hi Li, > > On 2018-03-23 15:58, Li Jun wrote: > > > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a > > variable PDO for sink config. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/typec/fusb302/fusb302.c | 51 > +++++++++++++++++++++++++++---------- > > 1 file changed, 37 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/usb/typec/fusb302/fusb302.c > > b/drivers/usb/typec/fusb302/fusb302.c > > index 7036171..db4d9cd 100644 > > --- a/drivers/usb/typec/fusb302/fusb302.c > > +++ b/drivers/usb/typec/fusb302/fusb302.c > > @@ -120,6 +120,7 @@ struct fusb302_chip { > > enum typec_cc_polarity cc_polarity; > > enum typec_cc_status cc1; > > enum typec_cc_status cc2; > > + u32 snk_pdo[PDO_MAX_OBJECTS]; > > > > #ifdef CONFIG_DEBUG_FS > > struct dentry *dentry; > > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = { > > static const struct tcpc_config fusb302_tcpc_config = { > > .src_pdo = src_pdo, > > .nr_src_pdo = ARRAY_SIZE(src_pdo), > > - .snk_pdo = snk_pdo, > > - .nr_snk_pdo = ARRAY_SIZE(snk_pdo), > > - .max_snk_mv = 5000, > > - .max_snk_ma = 3000, > > - .max_snk_mw = 15000, > > .operating_snk_mw = 2500, > > .type = TYPEC_PORT_DRP, > > .data = TYPEC_PORT_DRD, > > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip) > > return 0; > > } > > > > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) > > +{ > > + struct device *dev = chip->dev; > > + u32 mv, ma, mw, min_mv; > > + unsigned int i; > > + > > + /* Copy the static snk pdo */ > > + for (i = 0; i < ARRAY_SIZE(snk_pdo); i++) > > + chip->snk_pdo[i] = snk_pdo[i]; > > + > > + if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) || > > + device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) || > > + device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw)) > > + return i; > > + > > + mv = mv / 1000; > > + ma = ma / 1000; > > + mw = mw / 1000; > > + > > + min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma; > > + if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED)) > > You've got the parentheses wrong. Good catch. > > Apart from that I don't like/understand why the PDO's should be fixed. This is only for legacy setting(a fixed PDO) and properties(fcs,max-sink-*), to make it can still work after using the new PDO matching policy. > Every product should be able to have its own settings, including the first PDO (e.g. > it might need to specify a higher current and/or set the "Higher Capability" bit). > > I think this would be best solved the same way as in your TCPCI driver patch > series [1] with a list freely specified by a property. > > [1] > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.s > pinics.net%2Flists%2Flinux-usb%2Fmsg167398.html&data=02%7C01%7Cjun.li%40 > nxp.com%7C94bfdd71fa87479f54c808d59a24873a%7C686ea1d3bc2b4c6fa92cd99c > 5c301635%7C0%7C0%7C636584404043415663&sdata=RbOoMtJM9XtmvhHYNQ9a > nhPMn%2BJpdqwY3UagRPCgM9k%3D&reserved=0 Agreed, new code(dt) should use[1] to describe the power properties. Fusb302 driver just need pass a fwnode at init, tcpm will read and parse all the properties when register port. Thanks Jun > > > + min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1])); > > + else > > + min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1])); > > + ma = min(ma, 1000 * mw / min_mv); > > + > > + /* Insert the new pdo to the end */ > > + chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma); > > + > > + return i+1; > > +} > > + > > static int fusb302_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client, > > chip->tcpc_dev.config = &chip->tcpc_config; > > mutex_init(&chip->lock); > > > > - if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v)) > > - chip->tcpc_config.max_snk_mv = v / 1000; > > - > > - if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v)) > > - chip->tcpc_config.max_snk_ma = v / 1000; > > - > > - if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v)) > > - chip->tcpc_config.max_snk_mw = v / 1000; > > - > > if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v)) > > chip->tcpc_config.operating_snk_mw = v / 1000; > > > > + /* Composite sink PDO */ > > + chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip); > > + chip->tcpc_config.snk_pdo = chip->snk_pdo; > > + > > /* > > * Devicetree platforms should get extcon via phandle (not yet > > * supported). On ACPI platforms, we get the name from a device prop. > >