RE: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request matching pdos"
Hi,
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org
> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Heikki Krogerus
> Sent: 2018年3月6日 20:33
> To: Hans de Goede
> Cc: Greg Kroah-Hartman ; Guenter Roeck
> ; linux-usb@vger.kernel.org; Badhri Jagan Sridharan
>
> Subject: Re: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request
> matching pdos"
>
> On Tue, Mar 06, 2018 at 10:50:05AM +0100, Hans de Goede wrote:
> > Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is
> > causing a regression, before this commit e.g. the GPD win and GPD
> > pocket devices were charging at 9V 3A with a PD charger, now they are
> > instead slowly discharging at 5V 0.4A, as this commit causes the
> > ports max_snk_mv/ma/mw settings to be completely ignored.
> >
So max_snk_mv/ma/mw settings are back, I am dealing with PD properties
definition, so want to ask for long term, should we keep them, or use the
reverting patch way(only compare sink PDOs Vs source PDOs) but fix the
problem with it.
Thanks
Jun Li
> > Arguably the way to fix this would be to add a PDO_VAR() describing
> > the voltage range to the snk_caps of boards which can handle any
> > voltage in their range, but the "typec: tcpm: Only request matching
> > pdos" commit looks at the type of PDO advertised by the source/charger
> > and if that is fixed (as it typically is) only compairs against
> > PDO_FIXED entries in the snk_caps so supporting a range of voltage
> > would require adding a PDO_FIXED entry for *every possible* voltage to
> snk_caps.
> >
> > AFAICT there is no reason why a fixed source_cap cannot be matched
> > against a variable snk_cap, so at a minimum the commit should be
> > rewritten to support that.
> >
> > For now lets revert the "typec: tcpm: Only request matching pdos"
> > commit, fixing the regression.
> >
> > Cc: Badhri Jagan Sridharan
> > Signed-off-by: Hans de Goede
>
> You are correct. The patch should be rewritten.
>
> Acked-by: Heikki Krogerus
>
>
> Thanks,
>
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in the
> body
> of a message to majord...@vger.kernel.org More majordomo info at
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
> ernel.org%2Fmajordomo-info.html&data=02%7C01%7Cjun.li%40nxp.com%7C
> 6cd4e7a122244982de5e08d5835e618c%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C636559363813086942&sdata=nZw4ta%2Frro44iG%2FfH
> bhUrhOKZBdE16JvgSYUsb71qzg%3D&reserved=0
Re: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request matching pdos"
On Tue, Mar 06, 2018 at 10:50:05AM +0100, Hans de Goede wrote:
> Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is causing
> a regression, before this commit e.g. the GPD win and GPD pocket devices
> were charging at 9V 3A with a PD charger, now they are instead slowly
> discharging at 5V 0.4A, as this commit causes the ports max_snk_mv/ma/mw
> settings to be completely ignored.
>
> Arguably the way to fix this would be to add a PDO_VAR() describing the
> voltage range to the snk_caps of boards which can handle any voltage in
> their range, but the "typec: tcpm: Only request matching pdos" commit
> looks at the type of PDO advertised by the source/charger and if that
> is fixed (as it typically is) only compairs against PDO_FIXED entries
> in the snk_caps so supporting a range of voltage would require adding a
> PDO_FIXED entry for *every possible* voltage to snk_caps.
>
> AFAICT there is no reason why a fixed source_cap cannot be matched against
> a variable snk_cap, so at a minimum the commit should be rewritten to
> support that.
>
> For now lets revert the "typec: tcpm: Only request matching pdos" commit,
> fixing the regression.
>
> Cc: Badhri Jagan Sridharan
> Signed-off-by: Hans de Goede
You are correct. The patch should be rewritten.
Acked-by: Heikki Krogerus
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request matching pdos"
On 06 March 2018 09:50, Hans de Goede wrote:
> Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is causing
> a regression, before this commit e.g. the GPD win and GPD pocket devices
> were charging at 9V 3A with a PD charger, now they are instead slowly
> discharging at 5V 0.4A, as this commit causes the ports max_snk_mv/ma/mw
> settings to be completely ignored.
>
> Arguably the way to fix this would be to add a PDO_VAR() describing the
> voltage range to the snk_caps of boards which can handle any voltage in
> their range, but the "typec: tcpm: Only request matching pdos" commit
> looks at the type of PDO advertised by the source/charger and if that
> is fixed (as it typically is) only compairs against PDO_FIXED entries
> in the snk_caps so supporting a range of voltage would require adding a
> PDO_FIXED entry for *every possible* voltage to snk_caps.
>
> AFAICT there is no reason why a fixed source_cap cannot be matched against
> a variable snk_cap, so at a minimum the commit should be rewritten to
> support that.
>
> For now lets revert the "typec: tcpm: Only request matching pdos" commit,
> fixing the regression.
>
> Cc: Badhri Jagan Sridharan
> Signed-off-by: Hans de Goede
Thanks Hans. Sadly I alluded to this problem before the patch was pulled in but
this was seemingly missed:
https://lkml.org/lkml/2017/11/28/186
FWIW, Acked-by: Adam Thomson
> ---
> drivers/usb/typec/tcpm.c | 163
> ---
> 1 file changed, 42 insertions(+), 121 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index bfcaf6618a1f..735a6dd74c20 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -254,9 +254,6 @@ struct tcpm_port {
> unsigned int nr_src_pdo;
> u32 snk_pdo[PDO_MAX_OBJECTS];
> unsigned int nr_snk_pdo;
> - unsigned int nr_fixed; /* number of fixed sink PDOs */
> - unsigned int nr_var; /* number of variable sink PDOs */
> - unsigned int nr_batt; /* number of battery sink PDOs */
> u32 snk_vdo[VDO_MAX_OBJECTS];
> unsigned int nr_snk_vdo;
>
> @@ -1786,90 +1783,39 @@ static int tcpm_pd_check_request(struct tcpm_port
> *port)
> return 0;
> }
>
> -#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
> -#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
> -
> -static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
> - int *src_pdo)
> +static int tcpm_pd_select_pdo(struct tcpm_port *port)
> {
> - unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
> + unsigned int i, max_mw = 0, max_mv = 0;
> int ret = -EINVAL;
>
> /*
> - * Select the source PDO providing the most power which has a
> - * matchig sink cap.
> + * Select the source PDO providing the most power while staying within
> + * the board's voltage limits. Prefer PDO providing exp
>*/
> 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) {
> - for (j = 0; j < port->nr_fixed; j++) {
> - if (pdo_fixed_voltage(pdo) ==
> - pdo_fixed_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;
> - }
> - /* There could only be one fixed pdo
> - * at a specific voltage level.
> - * So breaking here.
> - */
> - break;
> - }
> - }
> - } else if (type == PDO_TYPE_BATT) {
> - for (j = port->nr_fixed;
> - j < port->nr_fixed +
> - port->nr_batt;
> - j++) {
> - if (pdo_min_voltage(pdo) >=
> - pdo_min_voltage(port->snk_pdo[j]) &&
> - pdo_max_voltage(pdo) <=
> - pdo_max_voltage(port->snk_pdo[j])) {
> -
Re: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request matching pdos"
On 03/06/2018 01:50 AM, Hans de Goede wrote:
Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is causing
a regression, before this commit e.g. the GPD win and GPD pocket devices
were charging at 9V 3A with a PD charger, now they are instead slowly
discharging at 5V 0.4A, as this commit causes the ports max_snk_mv/ma/mw
settings to be completely ignored.
Arguably the way to fix this would be to add a PDO_VAR() describing the
voltage range to the snk_caps of boards which can handle any voltage in
their range, but the "typec: tcpm: Only request matching pdos" commit
looks at the type of PDO advertised by the source/charger and if that
is fixed (as it typically is) only compairs against PDO_FIXED entries
in the snk_caps so supporting a range of voltage would require adding a
PDO_FIXED entry for *every possible* voltage to snk_caps.
AFAICT there is no reason why a fixed source_cap cannot be matched against
a variable snk_cap, so at a minimum the commit should be rewritten to
support that.
For now lets revert the "typec: tcpm: Only request matching pdos" commit,
fixing the regression.
Cc: Badhri Jagan Sridharan
Signed-off-by: Hans de Goede
Reviewed-by: Guenter Roeck
---
drivers/usb/typec/tcpm.c | 163 ---
1 file changed, 42 insertions(+), 121 deletions(-)
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index bfcaf6618a1f..735a6dd74c20 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -254,9 +254,6 @@ struct tcpm_port {
unsigned int nr_src_pdo;
u32 snk_pdo[PDO_MAX_OBJECTS];
unsigned int nr_snk_pdo;
- unsigned int nr_fixed; /* number of fixed sink PDOs */
- unsigned int nr_var; /* number of variable sink PDOs */
- unsigned int nr_batt; /* number of battery sink PDOs */
u32 snk_vdo[VDO_MAX_OBJECTS];
unsigned int nr_snk_vdo;
@@ -1786,90 +1783,39 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
return 0;
}
-#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
-#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
-
-static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
- int *src_pdo)
+static int tcpm_pd_select_pdo(struct tcpm_port *port)
{
- unsigned int i, j, max_mw = 0, max_mv = 0, mw = 0, mv = 0, ma = 0;
+ unsigned int i, max_mw = 0, max_mv = 0;
int ret = -EINVAL;
/*
-* Select the source PDO providing the most power which has a
-* matchig sink cap.
+* Select the source PDO providing the most power while staying within
+* the board's voltage limits. Prefer PDO providing exp
*/
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) {
- for (j = 0; j < port->nr_fixed; j++) {
- if (pdo_fixed_voltage(pdo) ==
- pdo_fixed_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;
- }
- /* There could only be one fixed pdo
-* at a specific voltage level.
-* So breaking here.
-*/
- break;
- }
- }
- } else if (type == PDO_TYPE_BATT) {
- for (j = port->nr_fixed;
-j < port->nr_fixed +
-port->nr_batt;
-j++) {
- if (pdo_min_voltage(pdo) >=
-pdo_min_voltage(port->snk_pdo[j]) &&
-pdo_max_voltage(pdo) <=
-pdo_max_voltage(port->snk_pdo[j])) {
- mw = min_power(pdo, port->snk_pdo[j]);
- mv = pdo_min_voltage(pdo);
- if (mw > max_mw ||
- (mw
