* [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo
@ 2018-03-13 0:02 Jun Li
0 siblings, 0 replies; 7+ messages in thread
From: Jun Li @ 2018-03-13 0:02 UTC (permalink / raw)
To: gregkh, hdegoede, linux, heikki.krogerus, Adam.Thomson.Opensource,
Badhri
Cc: linux-usb, linux-imx
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://www.spinics.net/lists/linux-usb/msg166366.html
CC: Hans de Goede <hdegoede@redhat.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Heikki Krogerus <heikki.krogerus@linux.intel.com>
CC: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
CC: Badhri Jagan Sridharan <Badhri@google.com>
Signed-off-by: Li Jun <jun.li@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 +
^ permalink raw reply related [flat|nested] 7+ messages in thread* [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo
@ 2018-03-13 11:36 Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2018-03-13 11:36 UTC (permalink / raw)
To: Li Jun, gregkh, linux, heikki.krogerus, Adam.Thomson.Opensource,
Badhri
Cc: linux-usb, linux-imx
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://www.spinics.net/lists/linux-usb/msg166366.html
>
> CC: Hans de Goede <hdegoede@redhat.com>
> CC: Guenter Roeck <linux@roeck-us.net>
> CC: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> CC: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> CC: Badhri Jagan Sridharan <Badhri@google.com>
> Signed-off-by: Li Jun <jun.li@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.
Since your patch has been reverted, please send a new fixed patch
replacing it.
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?
You really need to stop comparing fixed to fixed and variable
to variable, etc.
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
Regards,
Hans
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo
@ 2018-03-13 11:41 Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2018-03-13 11:41 UTC (permalink / raw)
To: Li Jun, gregkh, linux, heikki.krogerus, Adam.Thomson.Opensource,
Badhri
Cc: linux-usb, linux-imx
HI,
On 13-03-18 12:36, Hans de Goede wrote:
> 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://www.spinics.net/lists/linux-usb/msg166366.html
>>
>> CC: Hans de Goede <hdegoede@redhat.com>
>> CC: Guenter Roeck <linux@roeck-us.net>
>> CC: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> CC: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>> CC: Badhri Jagan Sridharan <Badhri@google.com>
>> Signed-off-by: Li Jun <jun.li@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.
>
> Since your patch has been reverted, please send a new fixed patch
> replacing it.
>
> 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?
>
> You really need to stop comparing fixed to fixed and variable
> to variable, etc.
>
> 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
Oh and for your use-case where the snk can only handle certain voltages you
also need to have a min_snk_mv, so basically 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)
min_snk_mv = pdo_fixed_voltage(port->snk_pdo[j]);
max_snk_mv = pdo_fixed_voltage(port->snk_pdo[j]);
/* FIXME also get ma / mw settings */
} else {
min_snk_mv = pdo_min_voltage(port->snk_pdo[j]);
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 >= min_snk_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
Regards,
Hans
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo
@ 2018-03-14 9:32 Jun Li
0 siblings, 0 replies; 7+ messages in thread
From: Jun Li @ 2018-03-14 9:32 UTC (permalink / raw)
To: Hans de Goede, gregkh@linuxfoundation.org, linux@roeck-us.net,
heikki.krogerus@linux.intel.com,
Adam.Thomson.Opensource@diasemi.com, Badhri@google.com
Cc: linux-usb@vger.kernel.org, dl-linux-imx
Hi
> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: 2018年3月13日 19:36
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; linux@roeck-us.net;
> heikki.krogerus@linux.intel.com; Adam.Thomson.Opensource@diasemi.com;
> Badhri@google.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@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%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 <hdegoede@redhat.com>
> > CC: Guenter Roeck <linux@roeck-us.net>
> > CC: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > CC: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > CC: Badhri Jagan Sridharan <Badhri@google.com>
> > Signed-off-by: Li Jun <jun.li@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 <badhri@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)
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo
@ 2018-03-14 10:38 Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2018-03-14 10:38 UTC (permalink / raw)
To: Jun Li, gregkh@linuxfoundation.org, linux@roeck-us.net,
heikki.krogerus@linux.intel.com,
Adam.Thomson.Opensource@diasemi.com, Badhri@google.com
Cc: linux-usb@vger.kernel.org, dl-linux-imx
Hi,
On 14-03-18 10:32, Jun Li wrote:
> Hi
>> -----Original Message-----
>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>> Sent: 2018年3月13日 19:36
>> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; linux@roeck-us.net;
>> heikki.krogerus@linux.intel.com; Adam.Thomson.Opensource@diasemi.com;
>> Badhri@google.com
>> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@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%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 <hdegoede@redhat.com>
>>> CC: Guenter Roeck <linux@roeck-us.net>
>>> CC: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> CC: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>>> CC: Badhri Jagan Sridharan <Badhri@google.com>
>>> Signed-off-by: Li Jun <jun.li@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 <badhri@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.
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 source_caps[],
I think we can remove port->max_snk_*?
Regards,
Hans
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo
@ 2018-03-19 9:43 Jun Li
0 siblings, 0 replies; 7+ messages in thread
From: Jun Li @ 2018-03-19 9:43 UTC (permalink / raw)
To: Hans de Goede, gregkh@linuxfoundation.org, linux@roeck-us.net,
heikki.krogerus@linux.intel.com,
Adam.Thomson.Opensource@diasemi.com, Badhri@google.com
Cc: linux-usb@vger.kernel.org, dl-linux-imx
Hi
> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: 2018年3月14日 18:39
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; linux@roeck-us.net;
> heikki.krogerus@linux.intel.com; Adam.Thomson.Opensource@diasemi.com;
> Badhri@google.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@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:hdegoede@redhat.com]
> >> Sent: 2018年3月13日 19:36
> >> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org;
> >> linux@roeck-us.net; heikki.krogerus@linux.intel.com;
> >> Adam.Thomson.Opensource@diasemi.com;
> >> Badhri@google.com
> >> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@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 <hdegoede@redhat.com>
> >>> CC: Guenter Roeck <linux@roeck-us.net>
> >>> CC: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>> CC: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> >>> CC: Badhri Jagan Sridharan <Badhri@google.com>
> >>> Signed-off-by: Li Jun <jun.li@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 <badhri@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.
>
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo
@ 2018-03-19 9:45 Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2018-03-19 9:45 UTC (permalink / raw)
To: Jun Li, gregkh@linuxfoundation.org, linux@roeck-us.net,
heikki.krogerus@linux.intel.com,
Adam.Thomson.Opensource@diasemi.com, Badhri@google.com
Cc: linux-usb@vger.kernel.org, dl-linux-imx
HI,
On 19-03-18 10:43, Jun Li wrote:
> Hi
>
>> -----Original Message-----
>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>> Sent: 2018年3月14日 18:39
>> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; linux@roeck-us.net;
>> heikki.krogerus@linux.intel.com; Adam.Thomson.Opensource@diasemi.com;
>> Badhri@google.com
>> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@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:hdegoede@redhat.com]
>>>> Sent: 2018年3月13日 19:36
>>>> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org;
>>>> linux@roeck-us.net; heikki.krogerus@linux.intel.com;
>>>> Adam.Thomson.Opensource@diasemi.com;
>>>> Badhri@google.com
>>>> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@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 <hdegoede@redhat.com>
>>>>> CC: Guenter Roeck <linux@roeck-us.net>
>>>>> CC: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>> CC: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>>>>> CC: Badhri Jagan Sridharan <Badhri@google.com>
>>>>> Signed-off-by: Li Jun <jun.li@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 <badhri@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.
>>
>
> thanks for your remind, I will check existing users and create an(extra) sink variable
> pdo according to their max_snk_* settings
Ok, I can test the fusb302 changes for this. Note that the fusb302 code reads
max_snk_* from device-properties, so you will need some code to dynamically
add a pdo base don those (and store the snk_pdo's in the driver-data struct
rather then having them be const).
Regards,
Hans
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-19 9:45 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-13 0:02 [RFC] usb: typec: tcpm: match source fixed pdo with sink variable pdo Jun Li
-- strict thread matches above, loose matches on Subject: below --
2018-03-13 11:36 Hans de Goede
2018-03-13 11:41 Hans de Goede
2018-03-14 9:32 Jun Li
2018-03-14 10:38 Hans de Goede
2018-03-19 9:43 Jun Li
2018-03-19 9:45 Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).