-----Original Message-----
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年3月14日 18:39
To: Jun Li <jun...@nxp.com>; gre...@linuxfoundation.org; li...@roeck-us.net;
heikki.kroge...@linux.intel.com; adam.thomson.opensou...@diasemi.com;
bad...@google.com
Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>
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:hdego...@redhat.com]
Sent: 2018年3月13日 19:36
To: Jun Li <jun...@nxp.com>; gre...@linuxfoundation.org;
li...@roeck-us.net; heikki.kroge...@linux.intel.com;
adam.thomson.opensou...@diasemi.com;
bad...@google.com
Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>
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 <hdego...@redhat.com>
CC: Guenter Roeck <li...@roeck-us.net>
CC: Heikki Krogerus <heikki.kroge...@linux.intel.com>
CC: Adam Thomson <adam.thomson.opensou...@diasemi.com>
CC: Badhri Jagan Sridharan <bad...@google.com>
Signed-off-by: Li Jun <jun...@nxp.com>
---
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 <bad...@google.com>
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.
