Hi > -----Original Message----- > From: Hans de Goede [mailto:hdegoede@xxxxxxxxxx] > Sent: 2018年3月14日 18:39 > To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; > heikki.krogerus@xxxxxxxxxxxxxxx; Adam.Thomson.Opensource@xxxxxxxxxxx; > Badhri@xxxxxxxxxx > Cc: linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: Re: [RFC PATCH] usb: typec: tcpm: match source fixed pdo with sink > variable pdo > > Hi, > > On 14-03-18 10:32, Jun Li wrote: > > Hi > >> -----Original Message----- > >> From: Hans de Goede [mailto:hdegoede@xxxxxxxxxx] > >> Sent: 2018年3月13日 19:36 > >> To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; > >> linux@xxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx; > >> Adam.Thomson.Opensource@xxxxxxxxxxx; > >> Badhri@xxxxxxxxxx > >> Cc: linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > >> 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%2Fww > >> w > >>> .spinics.net%2Flists%2Flinux-usb%2Fmsg166366.html&data=02%7C01%7 > Cju > >> n.l > >>> > >> > i%40nxp.com%7C569be5e2496442f514bd08d588d69fc6%7C686ea1d3bc2b4 > >> c6fa92cd > >>> > >> > 99c5c301635%7C0%7C0%7C636565377744465803&sdata=hUPib1nNtXArpZ > >> Pn0TfMdui > >>> ARnVPvc6CezTr0UxJFCI%3D&reserved=0 > >>> > >>> CC: Hans de Goede <hdegoede@xxxxxxxxxx> > >>> CC: Guenter Roeck <linux@xxxxxxxxxxxx> > >>> CC: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > >>> CC: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx> > >>> CC: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx> > >>> Signed-off-by: Li Jun <jun.li@xxxxxxx> > >>> --- > >>> 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 <badhri@xxxxxxxxxx> > > 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) > > Yes you are right, I did not realize that a variable source PDO meant that the > voltage is not stable. > > I expected that to mean that the sink could pick a voltage and the source > would then regulate at that voltage, but clearly I was wrong. > > > With this condition meet, then we can select one source PDO with > > bigger mw or, the same mw but higher voltage. > > Agreed. > > > > >> > >> 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; > > } > > } > > } > > } > > Looks good to me. Note if we merge something like this we need to make sure > to also merge patches at the same time converting users of the existing > port->max_snk_* values over to declare an (extra) PDO_TYPE_VAR in their > source_caps[] so that they do not regress. > thanks for your remind, I will check existing users and create an(extra) sink variable pdo according to their max_snk_* settings > And then finally after we've merged the proposed changes and converted all > users of > port->max_snk_* over to declaring an (extra) PDO_TYPE_VAR in their > port->source_caps[], > I think we can remove port->max_snk_*? > Yes, I think so. Thanks Jun > Regards, > > Hans > ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
