| Message ID | 20181217024805.122897-1-kyletso@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tcpm: Extend the matching rules on PPS APDO selection | expand |
On 12/16/18 6:48 PM, Kyle Tso wrote: > Current matching rules ensure that the voltage range of selected Source > Capability is entirely within the range defined in one of the Sink > Capabilities. This is reasonable but not practical because Sink may not > support wide range of voltage when sinking power while Source could > advertise its capabilities in raletively wider range. For example, a > Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed Nominal Voltage) > will not be selected if the Sink requires 5V-12V@3A PPS power. However, > the Sink could work well if the requested voltage range in RDOs is > 5V-11V@3A. > > Currently accepted: > |--------- source -----| > |----------- sink ---------------| > > Currently not accepted: > |--------- source -----| > |----------- sink ---------------| > > |--------- source -----| > |----------- sink ---------------| > > |--------- source -----------------| > |------ sink -------| > > To improve the usability, change the matching rules to what listed > below: > a. The Source PDO is selectable if any portion of the voltage range > overlaps one of the Sink PDO's voltage range. > b. The maximum operational voltage will be the lower one between the > selected Source PDO and the matching Sink PDO. > c. The maximum power will be the maximum operational voltage times the > maximum current defined in the selected Source PDO > d. Select the Source PDO with the highest maximum power > > Signed-off-by: Kyle Tso <kyletso@google.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > --- > Changelog since v1: > - updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net> > --- > drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 3620efee2688..3001df7bd602 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > unsigned int i, j, max_mw = 0, max_mv = 0; > unsigned int min_src_mv, max_src_mv, src_ma, src_mw; > unsigned int min_snk_mv, max_snk_mv; > - u32 pdo; > + unsigned int max_op_mv; > + u32 pdo, src, snk; > unsigned int src_pdo = 0, snk_pdo = 0; > > /* > @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > continue; > } > > - if (max_src_mv <= max_snk_mv && > - min_src_mv >= min_snk_mv) { > + if (min_src_mv <= max_snk_mv && > + max_src_mv >= min_snk_mv) { > + max_op_mv = min(max_src_mv, max_snk_mv); > + src_mw = (max_op_mv * src_ma) / 1000; > /* Prefer higher voltages if available */ > if ((src_mw == max_mw && > - min_src_mv > max_mv) || > + max_op_mv > max_mv) || > src_mw > max_mw) { > src_pdo = i; > snk_pdo = j; > max_mw = src_mw; > - max_mv = max_src_mv; > + max_mv = max_op_mv; > } > } > } > @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > } > > if (src_pdo) { > - pdo = port->source_caps[src_pdo]; > - > - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); > - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); > - port->pps_data.max_curr = > - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); > + src = port->source_caps[src_pdo]; > + snk = port->snk_pdo[snk_pdo]; > + > + port->pps_data.min_volt = max(pdo_pps_apdo_min_voltage(src), > + pdo_pps_apdo_min_voltage(snk)); > + port->pps_data.max_volt = min(pdo_pps_apdo_max_voltage(src), > + pdo_pps_apdo_max_voltage(snk)); > + port->pps_data.max_curr = min_pps_apdo_current(src, snk); > port->pps_data.out_volt = > - min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt); > + min(port->pps_data.max_volt, port->pps_data.out_volt); > port->pps_data.op_curr = > min(port->pps_data.max_curr, port->pps_data.op_curr); > } >
On Mon, Dec 17, 2018 at 10:48:05AM +0800, Kyle Tso wrote: > Current matching rules ensure that the voltage range of selected Source > Capability is entirely within the range defined in one of the Sink > Capabilities. This is reasonable but not practical because Sink may not > support wide range of voltage when sinking power while Source could > advertise its capabilities in raletively wider range. For example, a > Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed Nominal Voltage) > will not be selected if the Sink requires 5V-12V@3A PPS power. However, > the Sink could work well if the requested voltage range in RDOs is > 5V-11V@3A. > > Currently accepted: > |--------- source -----| > |----------- sink ---------------| > > Currently not accepted: > |--------- source -----| > |----------- sink ---------------| > > |--------- source -----| > |----------- sink ---------------| > > |--------- source -----------------| > |------ sink -------| > > To improve the usability, change the matching rules to what listed > below: > a. The Source PDO is selectable if any portion of the voltage range > overlaps one of the Sink PDO's voltage range. > b. The maximum operational voltage will be the lower one between the > selected Source PDO and the matching Sink PDO. > c. The maximum power will be the maximum operational voltage times the > maximum current defined in the selected Source PDO > d. Select the Source PDO with the highest maximum power > > Signed-off-by: Kyle Tso <kyletso@google.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> In case you'll do one more version, I have a minor comment about the style below, but no need to resend only because of that. > --- > Changelog since v1: > - updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net> > --- > drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 3620efee2688..3001df7bd602 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > unsigned int i, j, max_mw = 0, max_mv = 0; > unsigned int min_src_mv, max_src_mv, src_ma, src_mw; > unsigned int min_snk_mv, max_snk_mv; > - u32 pdo; > + unsigned int max_op_mv; > + u32 pdo, src, snk; > unsigned int src_pdo = 0, snk_pdo = 0; > > /* > @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > continue; > } > > - if (max_src_mv <= max_snk_mv && > - min_src_mv >= min_snk_mv) { > + if (min_src_mv <= max_snk_mv && > + max_src_mv >= min_snk_mv) { > + max_op_mv = min(max_src_mv, max_snk_mv); > + src_mw = (max_op_mv * src_ma) / 1000; > /* Prefer higher voltages if available */ > if ((src_mw == max_mw && > - min_src_mv > max_mv) || > + max_op_mv > max_mv) || > src_mw > max_mw) { > src_pdo = i; > snk_pdo = j; > max_mw = src_mw; > - max_mv = max_src_mv; > + max_mv = max_op_mv; > } > } > } > @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > } > > if (src_pdo) { > - pdo = port->source_caps[src_pdo]; > - > - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); > - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); > - port->pps_data.max_curr = > - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); > + src = port->source_caps[src_pdo]; > + snk = port->snk_pdo[snk_pdo]; > + > + port->pps_data.min_volt = max(pdo_pps_apdo_min_voltage(src), > + pdo_pps_apdo_min_voltage(snk)); > + port->pps_data.max_volt = min(pdo_pps_apdo_max_voltage(src), > + pdo_pps_apdo_max_voltage(snk)); > + port->pps_data.max_curr = min_pps_apdo_current(src, snk); > port->pps_data.out_volt = > - min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt); > + min(port->pps_data.max_volt, port->pps_data.out_volt); While here, you could have aligned that like the above lines: port->pps_data.out_volt = min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt); > port->pps_data.op_curr = > min(port->pps_data.max_curr, port->pps_data.op_curr); > } > -- > 2.20.0.405.gbc1bbc6f85-goog thanks,
On 17 December 2018 02:48, Kyle Tso wrote: > Current matching rules ensure that the voltage range of selected Source > Capability is entirely within the range defined in one of the Sink Capabilities. This > is reasonable but not practical because Sink may not support wide range of > voltage when sinking power while Source could advertise its capabilities in > raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V relatively > Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V- > 12V@3A PPS power. However, the Sink could work well if the requested voltage > range in RDOs is 5V-11V@3A. > > Currently accepted: > |--------- source -----| > |----------- sink ---------------| > > Currently not accepted: > |--------- source -----| > |----------- sink ---------------| > > |--------- source -----| > |----------- sink ---------------| > > |--------- source -----------------| > |------ sink -------| > > To improve the usability, change the matching rules to what listed > below: > a. The Source PDO is selectable if any portion of the voltage range > overlaps one of the Sink PDO's voltage range. > b. The maximum operational voltage will be the lower one between the > selected Source PDO and the matching Sink PDO. > c. The maximum power will be the maximum operational voltage times the > maximum current defined in the selected Source PDO d. Select the Source PDO > with the highest maximum power > > Signed-off-by: Kyle Tso <kyletso@google.com> > > --- > Changelog since v1: > - updated the commit message as suggested by Guenter Roeck <linux@roeck- > us.net> > --- > drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 3620efee2688..3001df7bd602 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > tcpm_port *port) > unsigned int i, j, max_mw = 0, max_mv = 0; > unsigned int min_src_mv, max_src_mv, src_ma, src_mw; > unsigned int min_snk_mv, max_snk_mv; > - u32 pdo; > + unsigned int max_op_mv; > + u32 pdo, src, snk; > unsigned int src_pdo = 0, snk_pdo = 0; > > /* > @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > tcpm_port *port) > continue; > } > > - if (max_src_mv <= max_snk_mv && > - min_src_mv >= min_snk_mv) { > + if (min_src_mv <= max_snk_mv && > + max_src_mv >= min_snk_mv) { > + max_op_mv = min(max_src_mv, > max_snk_mv); > + src_mw = (max_op_mv * src_ma) / 1000; > /* Prefer higher voltages if available */ > if ((src_mw == max_mw && > - min_src_mv > max_mv) || > + max_op_mv > max_mv) || > src_mw > max_mw) { Sorry I didn't raise this before, but came to mind this morning when I was considering your updates again. What happens if the Source validly provides two PPS APDOs, for example: 3.3V - 11V, 3A (9V programmable) 3.3V - 16V, 3A (15V programmable) and the sink APDO is: 5V - 9V, 3A I think the code here will now select the higher range (15V programmable) as it gives a larger power output value, even if the sink can only support a voltage that's far smaller. I really don't think this is correct. *If* you are going to allow selection of PPS APDOs that provide a larger voltage range than the Sink can actually cope with, then I think you should at least select the lower of those advertised which fulfils the needs of the Sink. > src_pdo = i; > snk_pdo = j; > max_mw = src_mw; > - max_mv = max_src_mv; > + max_mv = max_op_mv; > } > } > } > @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > tcpm_port *port) > } > > if (src_pdo) { > - pdo = port->source_caps[src_pdo]; > - > - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); > - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); > - port->pps_data.max_curr = > - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); > + src = port->source_caps[src_pdo]; > + snk = port->snk_pdo[snk_pdo]; > + > + port->pps_data.min_volt = > max(pdo_pps_apdo_min_voltage(src), > + pdo_pps_apdo_min_voltage(snk)); > + port->pps_data.max_volt = > min(pdo_pps_apdo_max_voltage(src), > + pdo_pps_apdo_max_voltage(snk)); > + port->pps_data.max_curr = min_pps_apdo_current(src, snk); > port->pps_data.out_volt = > - min(pdo_pps_apdo_max_voltage(pdo), port- > >pps_data.out_volt); > + min(port->pps_data.max_volt, port- > >pps_data.out_volt); > port->pps_data.op_curr = > min(port->pps_data.max_curr, port->pps_data.op_curr); > } > -- > 2.20.0.405.gbc1bbc6f85-goog
On Mon, Dec 17, 2018 at 7:36 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Mon, Dec 17, 2018 at 10:48:05AM +0800, Kyle Tso wrote: > > Current matching rules ensure that the voltage range of selected Source > > Capability is entirely within the range defined in one of the Sink > > Capabilities. This is reasonable but not practical because Sink may not > > support wide range of voltage when sinking power while Source could > > advertise its capabilities in raletively wider range. For example, a > > Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed Nominal Voltage) > > will not be selected if the Sink requires 5V-12V@3A PPS power. However, > > the Sink could work well if the requested voltage range in RDOs is > > 5V-11V@3A. > > > > Currently accepted: > > |--------- source -----| > > |----------- sink ---------------| > > > > Currently not accepted: > > |--------- source -----| > > |----------- sink ---------------| > > > > |--------- source -----| > > |----------- sink ---------------| > > > > |--------- source -----------------| > > |------ sink -------| > > > > To improve the usability, change the matching rules to what listed > > below: > > a. The Source PDO is selectable if any portion of the voltage range > > overlaps one of the Sink PDO's voltage range. > > b. The maximum operational voltage will be the lower one between the > > selected Source PDO and the matching Sink PDO. > > c. The maximum power will be the maximum operational voltage times the > > maximum current defined in the selected Source PDO > > d. Select the Source PDO with the highest maximum power > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > In case you'll do one more version, I have a minor comment about the > style below, but no need to resend only because of that. > > > --- > > Changelog since v1: > > - updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net> > > --- > > drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 3620efee2688..3001df7bd602 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > > unsigned int i, j, max_mw = 0, max_mv = 0; > > unsigned int min_src_mv, max_src_mv, src_ma, src_mw; > > unsigned int min_snk_mv, max_snk_mv; > > - u32 pdo; > > + unsigned int max_op_mv; > > + u32 pdo, src, snk; > > unsigned int src_pdo = 0, snk_pdo = 0; > > > > /* > > @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > > continue; > > } > > > > - if (max_src_mv <= max_snk_mv && > > - min_src_mv >= min_snk_mv) { > > + if (min_src_mv <= max_snk_mv && > > + max_src_mv >= min_snk_mv) { > > + max_op_mv = min(max_src_mv, max_snk_mv); > > + src_mw = (max_op_mv * src_ma) / 1000; > > /* Prefer higher voltages if available */ > > if ((src_mw == max_mw && > > - min_src_mv > max_mv) || > > + max_op_mv > max_mv) || > > src_mw > max_mw) { > > src_pdo = i; > > snk_pdo = j; > > max_mw = src_mw; > > - max_mv = max_src_mv; > > + max_mv = max_op_mv; > > } > > } > > } > > @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) > > } > > > > if (src_pdo) { > > - pdo = port->source_caps[src_pdo]; > > - > > - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); > > - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); > > - port->pps_data.max_curr = > > - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); > > + src = port->source_caps[src_pdo]; > > + snk = port->snk_pdo[snk_pdo]; > > + > > + port->pps_data.min_volt = max(pdo_pps_apdo_min_voltage(src), > > + pdo_pps_apdo_min_voltage(snk)); > > + port->pps_data.max_volt = min(pdo_pps_apdo_max_voltage(src), > > + pdo_pps_apdo_max_voltage(snk)); > > + port->pps_data.max_curr = min_pps_apdo_current(src, snk); > > port->pps_data.out_volt = > > - min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt); > > + min(port->pps_data.max_volt, port->pps_data.out_volt); > > While here, you could have aligned that like the above lines: > > port->pps_data.out_volt = min(pdo_pps_apdo_max_voltage(pdo), > port->pps_data.out_volt); > Thanks Heikki Krogerus, I will fix this (and the lines below as well) in the next (should be V3) patch. > > port->pps_data.op_curr = > > min(port->pps_data.max_curr, port->pps_data.op_curr); > > } > > -- > > 2.20.0.405.gbc1bbc6f85-goog > > thanks, > > -- > heikki
On Mon, Dec 17, 2018 at 8:23 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> wrote: > > On 17 December 2018 02:48, Kyle Tso wrote: > > > Current matching rules ensure that the voltage range of selected Source > > Capability is entirely within the range defined in one of the Sink Capabilities. This > > is reasonable but not practical because Sink may not support wide range of > > voltage when sinking power while Source could advertise its capabilities in > > raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V > > relatively > noted! Thanks for the correction. I will fix this in the next patch. > > Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V- > > 12V@3A PPS power. However, the Sink could work well if the requested voltage > > range in RDOs is 5V-11V@3A. > > > > Currently accepted: > > |--------- source -----| > > |----------- sink ---------------| > > > > Currently not accepted: > > |--------- source -----| > > |----------- sink ---------------| > > > > |--------- source -----| > > |----------- sink ---------------| > > > > |--------- source -----------------| > > |------ sink -------| > > > > To improve the usability, change the matching rules to what listed > > below: > > a. The Source PDO is selectable if any portion of the voltage range > > overlaps one of the Sink PDO's voltage range. > > b. The maximum operational voltage will be the lower one between the > > selected Source PDO and the matching Sink PDO. > > c. The maximum power will be the maximum operational voltage times the > > maximum current defined in the selected Source PDO d. Select the Source PDO > > with the highest maximum power > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > > > --- > > Changelog since v1: > > - updated the commit message as suggested by Guenter Roeck <linux@roeck- > > us.net> > > --- > > drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 3620efee2688..3001df7bd602 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > > tcpm_port *port) > > unsigned int i, j, max_mw = 0, max_mv = 0; > > unsigned int min_src_mv, max_src_mv, src_ma, src_mw; > > unsigned int min_snk_mv, max_snk_mv; > > - u32 pdo; > > + unsigned int max_op_mv; > > + u32 pdo, src, snk; > > unsigned int src_pdo = 0, snk_pdo = 0; > > > > /* > > @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > > tcpm_port *port) > > continue; > > } > > > > - if (max_src_mv <= max_snk_mv && > > - min_src_mv >= min_snk_mv) { > > + if (min_src_mv <= max_snk_mv && > > + max_src_mv >= min_snk_mv) { > > + max_op_mv = min(max_src_mv, > > max_snk_mv); > > + src_mw = (max_op_mv * src_ma) / 1000; > > /* Prefer higher voltages if available */ > > if ((src_mw == max_mw && > > - min_src_mv > max_mv) || > > + max_op_mv > max_mv) || > > src_mw > max_mw) { > > Sorry I didn't raise this before, but came to mind this morning when I was > considering your updates again. What happens if the Source validly provides two > PPS APDOs, for example: > > 3.3V - 11V, 3A (9V programmable) > 3.3V - 16V, 3A (15V programmable) > > and the sink APDO is: > > 5V - 9V, 3A > > I think the code here will now select the higher range (15V programmable) as it > gives a larger power output value, even if the sink can only support a voltage > that's far smaller. I really don't think this is correct. *If* you are going to > allow selection of PPS APDOs that provide a larger voltage range than the Sink > can actually cope with, then I think you should at least select the lower of > those advertised which fulfils the needs of the Sink. Source: 3.3V - 11V, 3A (9V programmable) 3.3V - 16V, 3A (15V programmable) Sink 5V - 9V, 3A In this case, the Sink will select "3.3V - 11V, 3A (9V programmable)" because the "max_op_mv" when dealing with both Source Cap will be the same. See line#2269, "max_op_mv" will be limited by "max_snk_mv" which is 9V. And in line#2273, "max_op_mv" (of the second src_cap) fails the check because "max_mv", which is equal to the "max_op_mv" of the previous src_cap, is the same as it. 2267 if (min_src_mv <= max_snk_mv && 2268 max_src_mv >= min_snk_mv) { 2269 max_op_mv = min(max_src_mv, max_snk_mv); 2270 src_mw = (max_op_mv * src_ma) / 1000; 2271 /* Prefer higher voltages if available */ 2272 if ((src_mw == max_mw && 2273 max_op_mv > max_mv) || 2274 src_mw > max_mw) { 2275 src_pdo = i; 2276 snk_pdo = j; 2277 max_mw = src_mw; 2278 max_mv = max_op_mv; 2279 } 2280 } > > > src_pdo = i; > > snk_pdo = j; > > max_mw = src_mw; > > - max_mv = max_src_mv; > > + max_mv = max_op_mv; > > } > > } > > } > > @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > > tcpm_port *port) > > } > > > > if (src_pdo) { > > - pdo = port->source_caps[src_pdo]; > > - > > - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); > > - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); > > - port->pps_data.max_curr = > > - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); > > + src = port->source_caps[src_pdo]; > > + snk = port->snk_pdo[snk_pdo]; > > + > > + port->pps_data.min_volt = > > max(pdo_pps_apdo_min_voltage(src), > > + pdo_pps_apdo_min_voltage(snk)); > > + port->pps_data.max_volt = > > min(pdo_pps_apdo_max_voltage(src), > > + pdo_pps_apdo_max_voltage(snk)); > > + port->pps_data.max_curr = min_pps_apdo_current(src, snk); > > port->pps_data.out_volt = > > - min(pdo_pps_apdo_max_voltage(pdo), port- > > >pps_data.out_volt); > > + min(port->pps_data.max_volt, port- > > >pps_data.out_volt); > > port->pps_data.op_curr = > > min(port->pps_data.max_curr, port->pps_data.op_curr); > > } > > -- > > 2.20.0.405.gbc1bbc6f85-goog >
On Mon, Dec 17, 2018 at 8:45 PM Kyle Tso <kyletso@google.com> wrote: > > On Mon, Dec 17, 2018 at 8:23 PM Adam Thomson > <Adam.Thomson.Opensource@diasemi.com> wrote: > > > > On 17 December 2018 02:48, Kyle Tso wrote: > > > > > Current matching rules ensure that the voltage range of selected Source > > > Capability is entirely within the range defined in one of the Sink Capabilities. This > > > is reasonable but not practical because Sink may not support wide range of > > > voltage when sinking power while Source could advertise its capabilities in > > > raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V > > > > relatively > > > > noted! > Thanks for the correction. I will fix this in the next patch. > > > > Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V- > > > 12V@3A PPS power. However, the Sink could work well if the requested voltage > > > range in RDOs is 5V-11V@3A. > > > > > > Currently accepted: > > > |--------- source -----| > > > |----------- sink ---------------| > > > > > > Currently not accepted: > > > |--------- source -----| > > > |----------- sink ---------------| > > > > > > |--------- source -----| > > > |----------- sink ---------------| > > > > > > |--------- source -----------------| > > > |------ sink -------| > > > > > > To improve the usability, change the matching rules to what listed > > > below: > > > a. The Source PDO is selectable if any portion of the voltage range > > > overlaps one of the Sink PDO's voltage range. > > > b. The maximum operational voltage will be the lower one between the > > > selected Source PDO and the matching Sink PDO. > > > c. The maximum power will be the maximum operational voltage times the > > > maximum current defined in the selected Source PDO d. Select the Source PDO > > > with the highest maximum power > > > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > > > > > --- > > > Changelog since v1: > > > - updated the commit message as suggested by Guenter Roeck <linux@roeck- > > > us.net> > > > --- > > > drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ > > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > index 3620efee2688..3001df7bd602 100644 > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > > > tcpm_port *port) > > > unsigned int i, j, max_mw = 0, max_mv = 0; > > > unsigned int min_src_mv, max_src_mv, src_ma, src_mw; > > > unsigned int min_snk_mv, max_snk_mv; > > > - u32 pdo; > > > + unsigned int max_op_mv; > > > + u32 pdo, src, snk; > > > unsigned int src_pdo = 0, snk_pdo = 0; > > > > > > /* > > > @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > > > tcpm_port *port) > > > continue; > > > } > > > > > > - if (max_src_mv <= max_snk_mv && > > > - min_src_mv >= min_snk_mv) { > > > + if (min_src_mv <= max_snk_mv && > > > + max_src_mv >= min_snk_mv) { > > > + max_op_mv = min(max_src_mv, > > > max_snk_mv); > > > + src_mw = (max_op_mv * src_ma) / 1000; > > > /* Prefer higher voltages if available */ > > > if ((src_mw == max_mw && > > > - min_src_mv > max_mv) || > > > + max_op_mv > max_mv) || > > > src_mw > max_mw) { > > > > Sorry I didn't raise this before, but came to mind this morning when I was > > considering your updates again. What happens if the Source validly provides two > > PPS APDOs, for example: > > > > 3.3V - 11V, 3A (9V programmable) > > 3.3V - 16V, 3A (15V programmable) > > > > and the sink APDO is: > > > > 5V - 9V, 3A > > > > I think the code here will now select the higher range (15V programmable) as it > > gives a larger power output value, even if the sink can only support a voltage > > that's far smaller. I really don't think this is correct. *If* you are going to > > allow selection of PPS APDOs that provide a larger voltage range than the Sink > > can actually cope with, then I think you should at least select the lower of > > those advertised which fulfils the needs of the Sink. > > Source: > 3.3V - 11V, 3A (9V programmable) > 3.3V - 16V, 3A (15V programmable) > > Sink > 5V - 9V, 3A > > In this case, the Sink will select "3.3V - 11V, 3A (9V programmable)" > because the > "max_op_mv" when dealing with both Source Cap will be the same. > > See line#2269, "max_op_mv" will be limited by "max_snk_mv" which is 9V. > And in line#2273, "max_op_mv" (of the second src_cap) fails the check > because "max_mv", which is equal to the "max_op_mv" of the previous src_cap, > is the same as it. > Sorry, I should have explained more clearer here. "max_op_mv" and "src_mw" are both limited to "max_snk_mv" (9V) in this case. Therefore in the following "if" statement (line#2272 to line#2274), both "src_mw" and "max_op_mv" remain the same as those regarding to the previous src_cap. So the "if" statement will be "false". ==> select the previous src_cap > 2267 if (min_src_mv <= max_snk_mv && > 2268 max_src_mv >= min_snk_mv) { > 2269 max_op_mv = min(max_src_mv, > max_snk_mv); > 2270 src_mw = (max_op_mv * src_ma) / 1000; > 2271 /* Prefer higher voltages if > available */ > 2272 if ((src_mw == max_mw && > 2273 max_op_mv > max_mv) || > 2274 src_mw > max_mw) { > 2275 src_pdo = i; > 2276 snk_pdo = j; > 2277 max_mw = src_mw; > 2278 max_mv = max_op_mv; > 2279 } > 2280 } > > > > > > src_pdo = i; > > > snk_pdo = j; > > > max_mw = src_mw; > > > - max_mv = max_src_mv; > > > + max_mv = max_op_mv; > > > } > > > } > > > } > > > @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > > > tcpm_port *port) > > > } > > > > > > if (src_pdo) { > > > - pdo = port->source_caps[src_pdo]; > > > - > > > - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); > > > - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); > > > - port->pps_data.max_curr = > > > - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); > > > + src = port->source_caps[src_pdo]; > > > + snk = port->snk_pdo[snk_pdo]; > > > + > > > + port->pps_data.min_volt = > > > max(pdo_pps_apdo_min_voltage(src), > > > + pdo_pps_apdo_min_voltage(snk)); > > > + port->pps_data.max_volt = > > > min(pdo_pps_apdo_max_voltage(src), > > > + pdo_pps_apdo_max_voltage(snk)); > > > + port->pps_data.max_curr = min_pps_apdo_current(src, snk); > > > port->pps_data.out_volt = > > > - min(pdo_pps_apdo_max_voltage(pdo), port- > > > >pps_data.out_volt); > > > + min(port->pps_data.max_volt, port- > > > >pps_data.out_volt); > > > port->pps_data.op_curr = > > > min(port->pps_data.max_curr, port->pps_data.op_curr); > > > } > > > -- > > > 2.20.0.405.gbc1bbc6f85-goog > >
On 17 December 2018 12:45, Kyle Tso wrote: > On Mon, Dec 17, 2018 at 8:23 PM Adam Thomson > <Adam.Thomson.Opensource@diasemi.com> wrote: > > > > On 17 December 2018 02:48, Kyle Tso wrote: > > > > > Current matching rules ensure that the voltage range of selected > > > Source Capability is entirely within the range defined in one of the > > > Sink Capabilities. This is reasonable but not practical because Sink > > > may not support wide range of voltage when sinking power while > > > Source could advertise its capabilities in raletively wider range. > > > For example, a Source PDO advertising 3.3V-11V@3A (9V > > > > relatively > > > > noted! > Thanks for the correction. I will fix this in the next patch. > > > > Prog of Fixed Nominal Voltage) will not be selected if the Sink > > > requires 5V- 12V@3A PPS power. However, the Sink could work well if > > > the requested voltage range in RDOs is 5V-11V@3A. > > > > > > Currently accepted: > > > |--------- source -----| > > > |----------- sink ---------------| > > > > > > Currently not accepted: > > > |--------- source -----| > > > |----------- sink ---------------| > > > > > > |--------- source -----| > > > |----------- sink ---------------| > > > > > > |--------- source -----------------| > > > |------ sink -------| > > > > > > To improve the usability, change the matching rules to what listed > > > below: > > > a. The Source PDO is selectable if any portion of the voltage range > > > overlaps one of the Sink PDO's voltage range. > > > b. The maximum operational voltage will be the lower one between the > > > selected Source PDO and the matching Sink PDO. > > > c. The maximum power will be the maximum operational voltage times the > > > maximum current defined in the selected Source PDO d. Select the > > > Source PDO with the highest maximum power > > > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > > > > > --- > > > Changelog since v1: > > > - updated the commit message as suggested by Guenter Roeck > > > <linux@roeck- us.net> > > > --- > > > drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ > > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c > > > b/drivers/usb/typec/tcpm/tcpm.c index 3620efee2688..3001df7bd602 > > > 100644 > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > @@ -2213,7 +2213,8 @@ static unsigned int > > > tcpm_pd_select_pps_apdo(struct tcpm_port *port) > > > unsigned int i, j, max_mw = 0, max_mv = 0; > > > unsigned int min_src_mv, max_src_mv, src_ma, src_mw; > > > unsigned int min_snk_mv, max_snk_mv; > > > - u32 pdo; > > > + unsigned int max_op_mv; > > > + u32 pdo, src, snk; > > > unsigned int src_pdo = 0, snk_pdo = 0; > > > > > > /* > > > @@ -2263,16 +2264,18 @@ static unsigned int > > > tcpm_pd_select_pps_apdo(struct tcpm_port *port) > > > continue; > > > } > > > > > > - if (max_src_mv <= max_snk_mv && > > > - min_src_mv >= min_snk_mv) { > > > + if (min_src_mv <= max_snk_mv && > > > + max_src_mv >= min_snk_mv) { > > > + max_op_mv = min(max_src_mv, > > > max_snk_mv); > > > + src_mw = (max_op_mv * src_ma) > > > + / 1000; > > > /* Prefer higher voltages if available */ > > > if ((src_mw == max_mw && > > > - min_src_mv > max_mv) || > > > + max_op_mv > max_mv) || > > > src_mw > max_mw) { > > > > Sorry I didn't raise this before, but came to mind this morning when I > > was considering your updates again. What happens if the Source validly > > provides two PPS APDOs, for example: > > > > 3.3V - 11V, 3A (9V programmable) > > 3.3V - 16V, 3A (15V programmable) > > > > and the sink APDO is: > > > > 5V - 9V, 3A > > > > I think the code here will now select the higher range (15V > > programmable) as it gives a larger power output value, even if the > > sink can only support a voltage that's far smaller. I really don't > > think this is correct. *If* you are going to allow selection of PPS > > APDOs that provide a larger voltage range than the Sink can actually > > cope with, then I think you should at least select the lower of those advertised > which fulfils the needs of the Sink. > > Source: > 3.3V - 11V, 3A (9V programmable) > 3.3V - 16V, 3A (15V programmable) > > Sink > 5V - 9V, 3A > > In this case, the Sink will select "3.3V - 11V, 3A (9V programmable)" > because the > "max_op_mv" when dealing with both Source Cap will be the same. > > See line#2269, "max_op_mv" will be limited by "max_snk_mv" which is 9V. > And in line#2273, "max_op_mv" (of the second src_cap) fails the check because > "max_mv", which is equal to the "max_op_mv" of the previous src_cap, is the > same as it. Ok, yes I missed the re-calculation of src_mw on line 2270. Apologies. > > 2267 if (min_src_mv <= max_snk_mv && > 2268 max_src_mv >= min_snk_mv) { > 2269 max_op_mv = min(max_src_mv, > max_snk_mv); > 2270 src_mw = (max_op_mv * src_ma) / 1000; > 2271 /* Prefer higher voltages if > available */ > 2272 if ((src_mw == max_mw && > 2273 max_op_mv > max_mv) || > 2274 src_mw > max_mw) { > 2275 src_pdo = i; > 2276 snk_pdo = j; > 2277 max_mw = src_mw; > 2278 max_mv = max_op_mv; > 2279 } > 2280 } > > > > > > src_pdo = i; > > > snk_pdo = j; > > > max_mw = src_mw; > > > - max_mv = max_src_mv; > > > + max_mv = max_op_mv; > > > } > > > } > > > } > > > @@ -2285,14 +2288,16 @@ static unsigned int > > > tcpm_pd_select_pps_apdo(struct tcpm_port *port) > > > } > > > > > > if (src_pdo) { > > > - pdo = port->source_caps[src_pdo]; > > > - > > > - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); > > > - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); > > > - port->pps_data.max_curr = > > > - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); > > > + src = port->source_caps[src_pdo]; > > > + snk = port->snk_pdo[snk_pdo]; > > > + > > > + port->pps_data.min_volt = > > > max(pdo_pps_apdo_min_voltage(src), > > > + pdo_pps_apdo_min_voltage(snk)); > > > + port->pps_data.max_volt = > > > min(pdo_pps_apdo_max_voltage(src), > > > + pdo_pps_apdo_max_voltage(snk)); > > > + port->pps_data.max_curr = min_pps_apdo_current(src, > > > + snk); > > > port->pps_data.out_volt = > > > - min(pdo_pps_apdo_max_voltage(pdo), port- > > > >pps_data.out_volt); > > > + min(port->pps_data.max_volt, port- > > > >pps_data.out_volt); > > > port->pps_data.op_curr = > > > min(port->pps_data.max_curr, port->pps_data.op_curr); > > > } > > > -- > > > 2.20.0.405.gbc1bbc6f85-goog > >
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 3620efee2688..3001df7bd602 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) unsigned int i, j, max_mw = 0, max_mv = 0; unsigned int min_src_mv, max_src_mv, src_ma, src_mw; unsigned int min_snk_mv, max_snk_mv; - u32 pdo; + unsigned int max_op_mv; + u32 pdo, src, snk; unsigned int src_pdo = 0, snk_pdo = 0; /* @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) continue; } - if (max_src_mv <= max_snk_mv && - min_src_mv >= min_snk_mv) { + if (min_src_mv <= max_snk_mv && + max_src_mv >= min_snk_mv) { + max_op_mv = min(max_src_mv, max_snk_mv); + src_mw = (max_op_mv * src_ma) / 1000; /* Prefer higher voltages if available */ if ((src_mw == max_mw && - min_src_mv > max_mv) || + max_op_mv > max_mv) || src_mw > max_mw) { src_pdo = i; snk_pdo = j; max_mw = src_mw; - max_mv = max_src_mv; + max_mv = max_op_mv; } } } @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port) } if (src_pdo) { - pdo = port->source_caps[src_pdo]; - - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo); - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo); - port->pps_data.max_curr = - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]); + src = port->source_caps[src_pdo]; + snk = port->snk_pdo[snk_pdo]; + + port->pps_data.min_volt = max(pdo_pps_apdo_min_voltage(src), + pdo_pps_apdo_min_voltage(snk)); + port->pps_data.max_volt = min(pdo_pps_apdo_max_voltage(src), + pdo_pps_apdo_max_voltage(snk)); + port->pps_data.max_curr = min_pps_apdo_current(src, snk); port->pps_data.out_volt = - min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt); + min(port->pps_data.max_volt, port->pps_data.out_volt); port->pps_data.op_curr = min(port->pps_data.max_curr, port->pps_data.op_curr); }
Current matching rules ensure that the voltage range of selected Source Capability is entirely within the range defined in one of the Sink Capabilities. This is reasonable but not practical because Sink may not support wide range of voltage when sinking power while Source could advertise its capabilities in raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V-12V@3A PPS power. However, the Sink could work well if the requested voltage range in RDOs is 5V-11V@3A. Currently accepted: |--------- source -----| |----------- sink ---------------| Currently not accepted: |--------- source -----| |----------- sink ---------------| |--------- source -----| |----------- sink ---------------| |--------- source -----------------| |------ sink -------| To improve the usability, change the matching rules to what listed below: a. The Source PDO is selectable if any portion of the voltage range overlaps one of the Sink PDO's voltage range. b. The maximum operational voltage will be the lower one between the selected Source PDO and the matching Sink PDO. c. The maximum power will be the maximum operational voltage times the maximum current defined in the selected Source PDO d. Select the Source PDO with the highest maximum power Signed-off-by: Kyle Tso <kyletso@google.com> --- Changelog since v1: - updated the commit message as suggested by Guenter Roeck <linux@roeck-us.net> --- drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)