Hi > -----Original Message----- > From: Hans de Goede [mailto:[email protected]] > Sent: 2018年3月13日 19:36 > To: Jun Li <[email protected]>; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected] > Cc: [email protected]; dl-linux-imx <[email protected]> > Subject: Re: [RFC PATCH] usb: typec: tcpm: match source fixed pdo with sink > variable pdo > > Hi, > > On 13-03-18 01:02, Li Jun wrote: > > This patch tries to fix the problem describled on revert patch > > commit[1], suppose any supported voltage ranges of sink should be > > describled by variable pdo, so instead of revert the patch of only > > comparing source and sink pdo to select one source pdo, this patch > > adds the match between source fixed pdo and sink variable pdo. > > > > [1] > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww > > .spinics.net%2Flists%2Flinux-usb%2Fmsg166366.html&data=02%7C01%7Cju > n.l > > > i%40nxp.com%7C569be5e2496442f514bd08d588d69fc6%7C686ea1d3bc2b4 > c6fa92cd > > > 99c5c301635%7C0%7C0%7C636565377744465803&sdata=hUPib1nNtXArpZ > Pn0TfMdui > > ARnVPvc6CezTr0UxJFCI%3D&reserved=0 > > > > CC: Hans de Goede <[email protected]> > > CC: Guenter Roeck <[email protected]> > > CC: Heikki Krogerus <[email protected]> > > CC: Adam Thomson <[email protected]> > > CC: Badhri Jagan Sridharan <[email protected]> > > Signed-off-by: Li Jun <[email protected]> > > --- > > drivers/usb/typec/tcpm.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index > > ef3a60d..8a74a43 100644 > > --- a/drivers/usb/typec/tcpm.c > > +++ b/drivers/usb/typec/tcpm.c > > @@ -1815,6 +1815,37 @@ static int tcpm_pd_select_pdo(struct tcpm_port > *port, int *sink_pdo, > > break; > > } > > } > > + > > + /* > > + * If the source pdo has the same voltage with one > > + * fixed pdo, no need check variable pdo for it. > > + */ > > + if (j < port->nr_fixed) > > + continue; > > + > > + for (j = port->nr_fixed + > > + port->nr_batt; > > + j < port->nr_fixed + > > + port->nr_batt + > > + port->nr_var; > > + j++) { > > + if (pdo_fixed_voltage(pdo) >= > > + pdo_min_voltage(port->snk_pdo[j]) && > > + pdo_fixed_voltage(pdo) <= > > + pdo_max_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; > > + } > > + } > > + } > > } else if (type == PDO_TYPE_BATT) { > > for (j = port->nr_fixed; > > j < port->nr_fixed + > > > > First of all, this seems to be a fix on top of your previous, reverted patch. >
It's on top of the patch to be reverted by you. > Since your patch has been reverted, please send a new fixed patch replacing > it. > It's Badhri's patch, not mine. Author: Badhri Jagan Sridharan <[email protected]> Date: Wed Nov 15 17:01:56 2017 -0800 typec: tcpm: Only request matching pdos > As for the proposed fix, you are just fixing the fixed source, variable snk > cap > case here. What if things are the other way around, so fixed snk, variable > source? I think that may not work, as the sink defines itself only can work at one fixed voltage, a variable PDO can make sure the output voltage fixed? Below is the description of variable supply PDO from PD spec: "The Variable Supply (non-Battery) PDO exposes very poorly regulated Sources. The output voltage of a Variable Supply (non-Battery) shall remain within the absolute maximum output voltage and the absolute minimum output voltage exposed in the Variable Supply PDO" So I think a basic rule is the voltage range of selected source PDO should be within the voltage range of sink PDO, no matter what type they are: (max_src_mv <= max_snk_mv && min_src_mv >= min_snk_mv) With this condition meet, then we can select one source PDO with bigger mw or, the same mw but higher voltage. > > You really need to stop comparing fixed to fixed and variable to variable, > etc. Agree. > > Instead do something like this > > 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 { > /* FIXME should not use max_snk_ma here */ > ma = min(pdo_max_current(pdo), > port->max_snk_ma); > mw = ma * mv / 1000; > } > > for (j = 0; j < port->nr_snk_pdo; j++) { > if (pdo_type(port->snk_pdo[j]) == PDO_TYPE_FIXED) > max_snk_mv = > pdo_fixed_voltage(port->snk_pdo[j]); > /* FIXME also get ma / mw settings */ > } else { > max_snk_mv = pdo_max_voltage(port->snk_pdo[j]); > /* FIXME also get ma / mw settings */ > } > > /* Prefer higher voltages if available */ > if ((mw > max_mw || (mw == max_mw && mv > > max_mv)) && > mv <= max_snk_mv) { > ret = i; > max_mw = mw; > max_mv = mv; > } > } > } > > Note the above example code only has been adjusted to compare the voltages > from the snk pdo-s it needs to be fixed to also deal with ma/mw We are dealing sink ma/mw after target source PDO is decided, in tcpm_pd_build_request(), it's used to set cap mismatch etc. the code looks like below: 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 max_src_mv, min_src_mv, min_snk_mv, max_snk_mv, src_mw, src_ma; 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) { src_mw = pdo_max_power(pdo); } else { src_ma = pdo_max_current(pdo); src_mw = src_ma * min_src_mv / 1000; } for (j = 0; j < port->nr_snk_pdo; j++) { if (pdo_type(port->snk_pdo[j]) == PDO_TYPE_FIXED) { min_snk_mv = pdo_fixed_voltage(port->snk_pdo[j]); max_snk_mv = pdo_fixed_voltage(port->snk_pdo[j]); } else { min_snk_mv = pdo_min_voltage(port->snk_pdo[j]); max_snk_mv = pdo_max_voltage(port->snk_pdo[j]); } if (max_src_mv <= max_snk_mv && min_src_mv >= min_snk_mv) { /* Prefer higher voltages if available */ if (src_mw > max_mw || (src_mw == max_mw && min_src_mv > max_mv)) { target_src_pdo = i; target_snk_pdo = j; max_mw = src_mw; max_mv = min_src_mv; } } } } Thanks Jun > > Regards, > > Hans
