Hi,
(1) and (2) in the series addresses the problem that Adam Thomson
had pointed out in:
https://lore.kernel.org/linux-usb/PR3PR10MB4142450638A8E07A33E475B080689@PR3PR10MB4142.EURPRD10.PROD.OUTLOOK.COM/
(3) updates the power_supply_changed based on changes
made through (1) and (2)
(4) (5) (6) makes TCPM comply pSnkStby requirement for both fast
and slow charging loops. This was also previously sent as part
of https://lore.kernel.org/patchwork/patch/1283928/
Since the patches were dependent on each other sending them this
way.
Badhri Jagan Sridharan (6):
usb: typec: tcpm: Address incorrect values of tcpm psy for fixed
supply
usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply
usb: typec: tcpm: update power supply once partner accepts
usb: typec: tcpm: Honour pSnkStdby requirement during negotiation
usb: typec: tcpm: Allow slow charging loops to comply to pSnkStby
Documentation: connector: Add slow-charger-loop definition
.../bindings/connector/usb-connector.yaml | 7 +
drivers/usb/typec/tcpm/tcpm.c | 136 ++++++++++++------
include/linux/usb/pd.h | 2 +
3 files changed, 99 insertions(+), 46 deletions(-)
--
2.31.0.208.g409f899ff0-goog
tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
port->pps_data.max_volt, port->pps_data.max_curr even before
port partner accepts the requests. This leaves incorrect values
in current_limit and supply_voltage that get exported by
"tcpm-source-psy-". Solving this problem by caching the request
values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
req_op_curr. min_volt, max_volt, max_curr gets updated once the
partner accepts the request. current_limit, supply_voltage gets updated
once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
current_limit and supply_voltage is enforced.
Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
Signed-off-by: Badhri Jagan Sridharan <[email protected]>
---
drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
1 file changed, 49 insertions(+), 35 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 03eca5061132..d43774cc2ccf 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -269,11 +269,22 @@ struct pd_mode_data {
};
struct pd_pps_data {
+ /* Actual min voltage at the local port */
u32 min_volt;
+ /* Requested min voltage to the port partner */
+ u32 req_min_volt;
+ /* Actual max voltage at the local port */
u32 max_volt;
+ /* Requested max voltage to the port partner */
+ u32 req_max_volt;
+ /* Actual max current at the local port */
u32 max_curr;
- u32 out_volt;
- u32 op_curr;
+ /* Requested max current of the port partner */
+ u32 req_max_curr;
+ /* Requested output voltage to the port partner */
+ u32 req_out_volt;
+ /* Requested operating current to the port partner */
+ u32 req_op_curr;
bool supported;
bool active;
};
@@ -2498,8 +2509,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
break;
case SNK_NEGOTIATE_PPS_CAPABILITIES:
/* Revert data back from any requested PPS updates */
- port->pps_data.out_volt = port->supply_voltage;
- port->pps_data.op_curr = port->current_limit;
+ port->pps_data.req_out_volt = port->supply_voltage;
+ port->pps_data.req_op_curr = port->current_limit;
port->pps_status = (type == PD_CTRL_WAIT ?
-EAGAIN : -EOPNOTSUPP);
@@ -2548,8 +2559,11 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
break;
case SNK_NEGOTIATE_PPS_CAPABILITIES:
port->pps_data.active = true;
- port->req_supply_voltage = port->pps_data.out_volt;
- port->req_current_limit = port->pps_data.op_curr;
+ port->pps_data.min_volt = port->pps_data.req_min_volt;
+ port->pps_data.max_volt = port->pps_data.req_max_volt;
+ port->pps_data.max_curr = port->pps_data.req_max_curr;
+ port->req_supply_voltage = port->pps_data.req_out_volt;
+ port->req_current_limit = port->pps_data.req_op_curr;
tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
break;
case SOFT_RESET_SEND:
@@ -3108,16 +3122,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
src = port->source_caps[src_pdo];
snk = port->snk_pdo[snk_pdo];
- port->pps_data.min_volt = max(pdo_pps_apdo_min_voltage(src),
- pdo_pps_apdo_min_voltage(snk));
- port->pps_data.max_volt = min(pdo_pps_apdo_max_voltage(src),
- pdo_pps_apdo_max_voltage(snk));
- port->pps_data.max_curr = min_pps_apdo_current(src, snk);
- port->pps_data.out_volt = min(port->pps_data.max_volt,
- max(port->pps_data.min_volt,
- port->pps_data.out_volt));
- port->pps_data.op_curr = min(port->pps_data.max_curr,
- port->pps_data.op_curr);
+ port->pps_data.req_min_volt = max(pdo_pps_apdo_min_voltage(src),
+ pdo_pps_apdo_min_voltage(snk));
+ port->pps_data.req_max_volt = min(pdo_pps_apdo_max_voltage(src),
+ pdo_pps_apdo_max_voltage(snk));
+ port->pps_data.req_max_curr = min_pps_apdo_current(src, snk);
+ port->pps_data.req_out_volt = min(port->pps_data.max_volt,
+ max(port->pps_data.min_volt,
+ port->pps_data.req_out_volt));
+ port->pps_data.req_op_curr = min(port->pps_data.max_curr,
+ port->pps_data.req_op_curr);
power_supply_changed(port->psy);
}
@@ -3245,10 +3259,10 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
tcpm_log(port, "Invalid APDO selected!");
return -EINVAL;
}
- max_mv = port->pps_data.max_volt;
- max_ma = port->pps_data.max_curr;
- out_mv = port->pps_data.out_volt;
- op_ma = port->pps_data.op_curr;
+ max_mv = port->pps_data.req_max_volt;
+ max_ma = port->pps_data.req_max_curr;
+ out_mv = port->pps_data.req_out_volt;
+ op_ma = port->pps_data.req_op_curr;
break;
default:
tcpm_log(port, "Invalid PDO selected!");
@@ -3295,8 +3309,8 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
tcpm_log(port, "Requesting APDO %d: %u mV, %u mA",
src_pdo_index, out_mv, op_ma);
- port->pps_data.op_curr = op_ma;
- port->pps_data.out_volt = out_mv;
+ port->pps_data.req_op_curr = op_ma;
+ port->pps_data.req_out_volt = out_mv;
return 0;
}
@@ -5429,7 +5443,7 @@ static int tcpm_try_role(struct typec_port *p, int role)
return ret;
}
-static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
+static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 req_op_curr)
{
unsigned int target_mw;
int ret;
@@ -5447,12 +5461,12 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
goto port_unlock;
}
- if (op_curr > port->pps_data.max_curr) {
+ if (req_op_curr > port->pps_data.max_curr) {
ret = -EINVAL;
goto port_unlock;
}
- target_mw = (op_curr * port->pps_data.out_volt) / 1000;
+ target_mw = (req_op_curr * port->supply_voltage) / 1000;
if (target_mw < port->operating_snk_mw) {
ret = -EINVAL;
goto port_unlock;
@@ -5466,10 +5480,10 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
}
/* Round down operating current to align with PPS valid steps */
- op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP);
+ req_op_curr = req_op_curr - (req_op_curr % RDO_PROG_CURR_MA_STEP);
reinit_completion(&port->pps_complete);
- port->pps_data.op_curr = op_curr;
+ port->pps_data.req_op_curr = req_op_curr;
port->pps_status = 0;
port->pps_pending = true;
mutex_unlock(&port->lock);
@@ -5490,7 +5504,7 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
return ret;
}
-static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
+static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 req_out_volt)
{
unsigned int target_mw;
int ret;
@@ -5508,13 +5522,13 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
goto port_unlock;
}
- if (out_volt < port->pps_data.min_volt ||
- out_volt > port->pps_data.max_volt) {
+ if (req_out_volt < port->pps_data.min_volt ||
+ req_out_volt > port->pps_data.max_volt) {
ret = -EINVAL;
goto port_unlock;
}
- target_mw = (port->pps_data.op_curr * out_volt) / 1000;
+ target_mw = (port->current_limit * req_out_volt) / 1000;
if (target_mw < port->operating_snk_mw) {
ret = -EINVAL;
goto port_unlock;
@@ -5528,10 +5542,10 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
}
/* Round down output voltage to align with PPS valid steps */
- out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP);
+ req_out_volt = req_out_volt - (req_out_volt % RDO_PROG_VOLT_MV_STEP);
reinit_completion(&port->pps_complete);
- port->pps_data.out_volt = out_volt;
+ port->pps_data.req_out_volt = req_out_volt;
port->pps_status = 0;
port->pps_pending = true;
mutex_unlock(&port->lock);
@@ -5589,8 +5603,8 @@ static int tcpm_pps_activate(struct tcpm_port *port, bool activate)
/* Trigger PPS request or move back to standard PDO contract */
if (activate) {
- port->pps_data.out_volt = port->supply_voltage;
- port->pps_data.op_curr = port->current_limit;
+ port->pps_data.req_out_volt = port->supply_voltage;
+ port->pps_data.req_op_curr = port->current_limit;
}
mutex_unlock(&port->lock);
--
2.31.0.208.g409f899ff0-goog
power_supply_changed needs to be called to notify clients
after the partner accepts the requested values for the pps
case.
Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
Signed-off-by: Badhri Jagan Sridharan <[email protected]>
---
drivers/usb/typec/tcpm/tcpm.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index d43774cc2ccf..7708b01009cb 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -2564,6 +2564,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
port->pps_data.max_curr = port->pps_data.req_max_curr;
port->req_supply_voltage = port->pps_data.req_out_volt;
port->req_current_limit = port->pps_data.req_op_curr;
+ power_supply_changed(port->psy);
tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
break;
case SOFT_RESET_SEND:
@@ -3132,7 +3133,6 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
port->pps_data.req_out_volt));
port->pps_data.req_op_curr = min(port->pps_data.max_curr,
port->pps_data.req_op_curr);
- power_supply_changed(port->psy);
}
return src_pdo;
@@ -3557,8 +3557,6 @@ static void tcpm_reset_port(struct tcpm_port *port)
port->sink_cap_done = false;
if (port->tcpc->enable_frs)
port->tcpc->enable_frs(port->tcpc, false);
-
- power_supply_changed(port->psy);
}
static void tcpm_detach(struct tcpm_port *port)
--
2.31.0.208.g409f899ff0-goog
On Mon, Apr 05, 2021 at 06:36:39PM -0700, Badhri Jagan Sridharan wrote:
> tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
> port->pps_data.max_volt, port->pps_data.max_curr even before
> port partner accepts the requests. This leaves incorrect values
> in current_limit and supply_voltage that get exported by
> "tcpm-source-psy-". Solving this problem by caching the request
> values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
> req_op_curr. min_volt, max_volt, max_curr gets updated once the
> partner accepts the request. current_limit, supply_voltage gets updated
> once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
> current_limit and supply_voltage is enforced.
>
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through power_supply")
> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> ---
> drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
> 1 file changed, 49 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 03eca5061132..d43774cc2ccf 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -269,11 +269,22 @@ struct pd_mode_data {
> };
>
> struct pd_pps_data {
> + /* Actual min voltage at the local port */
> u32 min_volt;
> + /* Requested min voltage to the port partner */
> + u32 req_min_volt;
> + /* Actual max voltage at the local port */
> u32 max_volt;
> + /* Requested max voltage to the port partner */
> + u32 req_max_volt;
> + /* Actual max current at the local port */
> u32 max_curr;
> - u32 out_volt;
> - u32 op_curr;
> + /* Requested max current of the port partner */
> + u32 req_max_curr;
> + /* Requested output voltage to the port partner */
> + u32 req_out_volt;
> + /* Requested operating current to the port partner */
> + u32 req_op_curr;
Shouldn't you just document this all properly in a kerneldoc header
right above the structure?
thanks,
greg k-h
On 06 April 2021 02:37, Badhri Jagan Sridharan wrote:
> tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
> port->pps_data.max_volt, port->pps_data.max_curr even before
> port partner accepts the requests. This leaves incorrect values
> in current_limit and supply_voltage that get exported by
> "tcpm-source-psy-". Solving this problem by caching the request
> values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
> req_op_curr. min_volt, max_volt, max_curr gets updated once the
> partner accepts the request. current_limit, supply_voltage gets updated
> once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
> current_limit and supply_voltage is enforced.
>
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> power_supply")
> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> ---
Reviewed-by: Adam Thomson <[email protected]>
> drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
> 1 file changed, 49 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 03eca5061132..d43774cc2ccf 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -269,11 +269,22 @@ struct pd_mode_data {
> };
>
> struct pd_pps_data {
> + /* Actual min voltage at the local port */
> u32 min_volt;
> + /* Requested min voltage to the port partner */
> + u32 req_min_volt;
> + /* Actual max voltage at the local port */
> u32 max_volt;
> + /* Requested max voltage to the port partner */
> + u32 req_max_volt;
> + /* Actual max current at the local port */
> u32 max_curr;
> - u32 out_volt;
> - u32 op_curr;
> + /* Requested max current of the port partner */
> + u32 req_max_curr;
> + /* Requested output voltage to the port partner */
> + u32 req_out_volt;
> + /* Requested operating current to the port partner */
> + u32 req_op_curr;
> bool supported;
> bool active;
> };
> @@ -2498,8 +2509,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> break;
> case SNK_NEGOTIATE_PPS_CAPABILITIES:
> /* Revert data back from any requested PPS updates */
> - port->pps_data.out_volt = port->supply_voltage;
> - port->pps_data.op_curr = port->current_limit;
> + port->pps_data.req_out_volt = port->supply_voltage;
> + port->pps_data.req_op_curr = port->current_limit;
> port->pps_status = (type == PD_CTRL_WAIT ?
> -EAGAIN : -EOPNOTSUPP);
>
> @@ -2548,8 +2559,11 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> break;
> case SNK_NEGOTIATE_PPS_CAPABILITIES:
> port->pps_data.active = true;
> - port->req_supply_voltage = port->pps_data.out_volt;
> - port->req_current_limit = port->pps_data.op_curr;
> + port->pps_data.min_volt = port-
> >pps_data.req_min_volt;
> + port->pps_data.max_volt = port-
> >pps_data.req_max_volt;
> + port->pps_data.max_curr = port-
> >pps_data.req_max_curr;
> + port->req_supply_voltage = port-
> >pps_data.req_out_volt;
> + port->req_current_limit = port->pps_data.req_op_curr;
> tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> break;
> case SOFT_RESET_SEND:
> @@ -3108,16 +3122,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> src = port->source_caps[src_pdo];
> snk = port->snk_pdo[snk_pdo];
>
> - port->pps_data.min_volt =
> max(pdo_pps_apdo_min_voltage(src),
> - pdo_pps_apdo_min_voltage(snk));
> - port->pps_data.max_volt =
> min(pdo_pps_apdo_max_voltage(src),
> - pdo_pps_apdo_max_voltage(snk));
> - port->pps_data.max_curr = min_pps_apdo_current(src, snk);
> - port->pps_data.out_volt = min(port->pps_data.max_volt,
> - max(port->pps_data.min_volt,
> - port->pps_data.out_volt));
> - port->pps_data.op_curr = min(port->pps_data.max_curr,
> - port->pps_data.op_curr);
> + port->pps_data.req_min_volt =
> max(pdo_pps_apdo_min_voltage(src),
> +
> pdo_pps_apdo_min_voltage(snk));
> + port->pps_data.req_max_volt =
> min(pdo_pps_apdo_max_voltage(src),
> +
> pdo_pps_apdo_max_voltage(snk));
> + port->pps_data.req_max_curr = min_pps_apdo_current(src,
> snk);
> + port->pps_data.req_out_volt = min(port->pps_data.max_volt,
> + max(port->pps_data.min_volt,
> + port-
> >pps_data.req_out_volt));
> + port->pps_data.req_op_curr = min(port->pps_data.max_curr,
> + port->pps_data.req_op_curr);
> power_supply_changed(port->psy);
> }
>
> @@ -3245,10 +3259,10 @@ static int tcpm_pd_build_pps_request(struct
> tcpm_port *port, u32 *rdo)
> tcpm_log(port, "Invalid APDO selected!");
> return -EINVAL;
> }
> - max_mv = port->pps_data.max_volt;
> - max_ma = port->pps_data.max_curr;
> - out_mv = port->pps_data.out_volt;
> - op_ma = port->pps_data.op_curr;
> + max_mv = port->pps_data.req_max_volt;
> + max_ma = port->pps_data.req_max_curr;
> + out_mv = port->pps_data.req_out_volt;
> + op_ma = port->pps_data.req_op_curr;
> break;
> default:
> tcpm_log(port, "Invalid PDO selected!");
> @@ -3295,8 +3309,8 @@ static int tcpm_pd_build_pps_request(struct tcpm_port
> *port, u32 *rdo)
> tcpm_log(port, "Requesting APDO %d: %u mV, %u mA",
> src_pdo_index, out_mv, op_ma);
>
> - port->pps_data.op_curr = op_ma;
> - port->pps_data.out_volt = out_mv;
> + port->pps_data.req_op_curr = op_ma;
> + port->pps_data.req_out_volt = out_mv;
>
> return 0;
> }
> @@ -5429,7 +5443,7 @@ static int tcpm_try_role(struct typec_port *p, int role)
> return ret;
> }
>
> -static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 req_op_curr)
> {
> unsigned int target_mw;
> int ret;
> @@ -5447,12 +5461,12 @@ static int tcpm_pps_set_op_curr(struct tcpm_port
> *port, u16 op_curr)
> goto port_unlock;
> }
>
> - if (op_curr > port->pps_data.max_curr) {
> + if (req_op_curr > port->pps_data.max_curr) {
> ret = -EINVAL;
> goto port_unlock;
> }
>
> - target_mw = (op_curr * port->pps_data.out_volt) / 1000;
> + target_mw = (req_op_curr * port->supply_voltage) / 1000;
> if (target_mw < port->operating_snk_mw) {
> ret = -EINVAL;
> goto port_unlock;
> @@ -5466,10 +5480,10 @@ static int tcpm_pps_set_op_curr(struct tcpm_port
> *port, u16 op_curr)
> }
>
> /* Round down operating current to align with PPS valid steps */
> - op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP);
> + req_op_curr = req_op_curr - (req_op_curr %
> RDO_PROG_CURR_MA_STEP);
>
> reinit_completion(&port->pps_complete);
> - port->pps_data.op_curr = op_curr;
> + port->pps_data.req_op_curr = req_op_curr;
> port->pps_status = 0;
> port->pps_pending = true;
> mutex_unlock(&port->lock);
> @@ -5490,7 +5504,7 @@ static int tcpm_pps_set_op_curr(struct tcpm_port
> *port, u16 op_curr)
> return ret;
> }
>
> -static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
> +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 req_out_volt)
> {
> unsigned int target_mw;
> int ret;
> @@ -5508,13 +5522,13 @@ static int tcpm_pps_set_out_volt(struct tcpm_port
> *port, u16 out_volt)
> goto port_unlock;
> }
>
> - if (out_volt < port->pps_data.min_volt ||
> - out_volt > port->pps_data.max_volt) {
> + if (req_out_volt < port->pps_data.min_volt ||
> + req_out_volt > port->pps_data.max_volt) {
> ret = -EINVAL;
> goto port_unlock;
> }
>
> - target_mw = (port->pps_data.op_curr * out_volt) / 1000;
> + target_mw = (port->current_limit * req_out_volt) / 1000;
> if (target_mw < port->operating_snk_mw) {
> ret = -EINVAL;
> goto port_unlock;
> @@ -5528,10 +5542,10 @@ static int tcpm_pps_set_out_volt(struct tcpm_port
> *port, u16 out_volt)
> }
>
> /* Round down output voltage to align with PPS valid steps */
> - out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP);
> + req_out_volt = req_out_volt - (req_out_volt %
> RDO_PROG_VOLT_MV_STEP);
>
> reinit_completion(&port->pps_complete);
> - port->pps_data.out_volt = out_volt;
> + port->pps_data.req_out_volt = req_out_volt;
> port->pps_status = 0;
> port->pps_pending = true;
> mutex_unlock(&port->lock);
> @@ -5589,8 +5603,8 @@ static int tcpm_pps_activate(struct tcpm_port *port,
> bool activate)
>
> /* Trigger PPS request or move back to standard PDO contract */
> if (activate) {
> - port->pps_data.out_volt = port->supply_voltage;
> - port->pps_data.op_curr = port->current_limit;
> + port->pps_data.req_out_volt = port->supply_voltage;
> + port->pps_data.req_op_curr = port->current_limit;
> }
> mutex_unlock(&port->lock);
>
> --
> 2.31.0.208.g409f899ff0-goog
On 06 April 2021 02:37, Badhri Jagan Sridharan wrote:
> power_supply_changed needs to be called to notify clients
> after the partner accepts the requested values for the pps
> case.
>
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> power_supply")
> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> ---
Missing commit information aside:
Reviewed-by: Adam Thomson <[email protected]>
> drivers/usb/typec/tcpm/tcpm.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d43774cc2ccf..7708b01009cb 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2564,6 +2564,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> port->pps_data.max_curr = port-
> >pps_data.req_max_curr;
> port->req_supply_voltage = port-
> >pps_data.req_out_volt;
> port->req_current_limit = port->pps_data.req_op_curr;
> + power_supply_changed(port->psy);
> tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> break;
> case SOFT_RESET_SEND:
> @@ -3132,7 +3133,6 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> port-
> >pps_data.req_out_volt));
> port->pps_data.req_op_curr = min(port->pps_data.max_curr,
> port->pps_data.req_op_curr);
> - power_supply_changed(port->psy);
> }
>
> return src_pdo;
> @@ -3557,8 +3557,6 @@ static void tcpm_reset_port(struct tcpm_port *port)
> port->sink_cap_done = false;
> if (port->tcpc->enable_frs)
> port->tcpc->enable_frs(port->tcpc, false);
> -
> - power_supply_changed(port->psy);
> }
>
> static void tcpm_detach(struct tcpm_port *port)
> --
> 2.31.0.208.g409f899ff0-goog
Hi Greg,
Moved to kerneldoc header in V2.
Thanks,
Badhri
On Wed, Apr 7, 2021 at 9:07 AM Adam Thomson
<[email protected]> wrote:
>
> On 06 April 2021 02:37, Badhri Jagan Sridharan wrote:
>
> > tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
> > port->pps_data.max_volt, port->pps_data.max_curr even before
> > port partner accepts the requests. This leaves incorrect values
> > in current_limit and supply_voltage that get exported by
> > "tcpm-source-psy-". Solving this problem by caching the request
> > values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
> > req_op_curr. min_volt, max_volt, max_curr gets updated once the
> > partner accepts the request. current_limit, supply_voltage gets updated
> > once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
> > current_limit and supply_voltage is enforced.
> >
> > Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> > power_supply")
> > Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> > ---
>
> Reviewed-by: Adam Thomson <[email protected]>
>
> > drivers/usb/typec/tcpm/tcpm.c | 84 ++++++++++++++++++++---------------
> > 1 file changed, 49 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 03eca5061132..d43774cc2ccf 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -269,11 +269,22 @@ struct pd_mode_data {
> > };
> >
> > struct pd_pps_data {
> > + /* Actual min voltage at the local port */
> > u32 min_volt;
> > + /* Requested min voltage to the port partner */
> > + u32 req_min_volt;
> > + /* Actual max voltage at the local port */
> > u32 max_volt;
> > + /* Requested max voltage to the port partner */
> > + u32 req_max_volt;
> > + /* Actual max current at the local port */
> > u32 max_curr;
> > - u32 out_volt;
> > - u32 op_curr;
> > + /* Requested max current of the port partner */
> > + u32 req_max_curr;
> > + /* Requested output voltage to the port partner */
> > + u32 req_out_volt;
> > + /* Requested operating current to the port partner */
> > + u32 req_op_curr;
> > bool supported;
> > bool active;
> > };
> > @@ -2498,8 +2509,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> > *port,
> > break;
> > case SNK_NEGOTIATE_PPS_CAPABILITIES:
> > /* Revert data back from any requested PPS updates */
> > - port->pps_data.out_volt = port->supply_voltage;
> > - port->pps_data.op_curr = port->current_limit;
> > + port->pps_data.req_out_volt = port->supply_voltage;
> > + port->pps_data.req_op_curr = port->current_limit;
> > port->pps_status = (type == PD_CTRL_WAIT ?
> > -EAGAIN : -EOPNOTSUPP);
> >
> > @@ -2548,8 +2559,11 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> > *port,
> > break;
> > case SNK_NEGOTIATE_PPS_CAPABILITIES:
> > port->pps_data.active = true;
> > - port->req_supply_voltage = port->pps_data.out_volt;
> > - port->req_current_limit = port->pps_data.op_curr;
> > + port->pps_data.min_volt = port-
> > >pps_data.req_min_volt;
> > + port->pps_data.max_volt = port-
> > >pps_data.req_max_volt;
> > + port->pps_data.max_curr = port-
> > >pps_data.req_max_curr;
> > + port->req_supply_voltage = port-
> > >pps_data.req_out_volt;
> > + port->req_current_limit = port->pps_data.req_op_curr;
> > tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> > break;
> > case SOFT_RESET_SEND:
> > @@ -3108,16 +3122,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> > tcpm_port *port)
> > src = port->source_caps[src_pdo];
> > snk = port->snk_pdo[snk_pdo];
> >
> > - port->pps_data.min_volt =
> > max(pdo_pps_apdo_min_voltage(src),
> > - pdo_pps_apdo_min_voltage(snk));
> > - port->pps_data.max_volt =
> > min(pdo_pps_apdo_max_voltage(src),
> > - pdo_pps_apdo_max_voltage(snk));
> > - port->pps_data.max_curr = min_pps_apdo_current(src, snk);
> > - port->pps_data.out_volt = min(port->pps_data.max_volt,
> > - max(port->pps_data.min_volt,
> > - port->pps_data.out_volt));
> > - port->pps_data.op_curr = min(port->pps_data.max_curr,
> > - port->pps_data.op_curr);
> > + port->pps_data.req_min_volt =
> > max(pdo_pps_apdo_min_voltage(src),
> > +
> > pdo_pps_apdo_min_voltage(snk));
> > + port->pps_data.req_max_volt =
> > min(pdo_pps_apdo_max_voltage(src),
> > +
> > pdo_pps_apdo_max_voltage(snk));
> > + port->pps_data.req_max_curr = min_pps_apdo_current(src,
> > snk);
> > + port->pps_data.req_out_volt = min(port->pps_data.max_volt,
> > + max(port->pps_data.min_volt,
> > + port-
> > >pps_data.req_out_volt));
> > + port->pps_data.req_op_curr = min(port->pps_data.max_curr,
> > + port->pps_data.req_op_curr);
> > power_supply_changed(port->psy);
> > }
> >
> > @@ -3245,10 +3259,10 @@ static int tcpm_pd_build_pps_request(struct
> > tcpm_port *port, u32 *rdo)
> > tcpm_log(port, "Invalid APDO selected!");
> > return -EINVAL;
> > }
> > - max_mv = port->pps_data.max_volt;
> > - max_ma = port->pps_data.max_curr;
> > - out_mv = port->pps_data.out_volt;
> > - op_ma = port->pps_data.op_curr;
> > + max_mv = port->pps_data.req_max_volt;
> > + max_ma = port->pps_data.req_max_curr;
> > + out_mv = port->pps_data.req_out_volt;
> > + op_ma = port->pps_data.req_op_curr;
> > break;
> > default:
> > tcpm_log(port, "Invalid PDO selected!");
> > @@ -3295,8 +3309,8 @@ static int tcpm_pd_build_pps_request(struct tcpm_port
> > *port, u32 *rdo)
> > tcpm_log(port, "Requesting APDO %d: %u mV, %u mA",
> > src_pdo_index, out_mv, op_ma);
> >
> > - port->pps_data.op_curr = op_ma;
> > - port->pps_data.out_volt = out_mv;
> > + port->pps_data.req_op_curr = op_ma;
> > + port->pps_data.req_out_volt = out_mv;
> >
> > return 0;
> > }
> > @@ -5429,7 +5443,7 @@ static int tcpm_try_role(struct typec_port *p, int role)
> > return ret;
> > }
> >
> > -static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> > +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 req_op_curr)
> > {
> > unsigned int target_mw;
> > int ret;
> > @@ -5447,12 +5461,12 @@ static int tcpm_pps_set_op_curr(struct tcpm_port
> > *port, u16 op_curr)
> > goto port_unlock;
> > }
> >
> > - if (op_curr > port->pps_data.max_curr) {
> > + if (req_op_curr > port->pps_data.max_curr) {
> > ret = -EINVAL;
> > goto port_unlock;
> > }
> >
> > - target_mw = (op_curr * port->pps_data.out_volt) / 1000;
> > + target_mw = (req_op_curr * port->supply_voltage) / 1000;
> > if (target_mw < port->operating_snk_mw) {
> > ret = -EINVAL;
> > goto port_unlock;
> > @@ -5466,10 +5480,10 @@ static int tcpm_pps_set_op_curr(struct tcpm_port
> > *port, u16 op_curr)
> > }
> >
> > /* Round down operating current to align with PPS valid steps */
> > - op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP);
> > + req_op_curr = req_op_curr - (req_op_curr %
> > RDO_PROG_CURR_MA_STEP);
> >
> > reinit_completion(&port->pps_complete);
> > - port->pps_data.op_curr = op_curr;
> > + port->pps_data.req_op_curr = req_op_curr;
> > port->pps_status = 0;
> > port->pps_pending = true;
> > mutex_unlock(&port->lock);
> > @@ -5490,7 +5504,7 @@ static int tcpm_pps_set_op_curr(struct tcpm_port
> > *port, u16 op_curr)
> > return ret;
> > }
> >
> > -static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
> > +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 req_out_volt)
> > {
> > unsigned int target_mw;
> > int ret;
> > @@ -5508,13 +5522,13 @@ static int tcpm_pps_set_out_volt(struct tcpm_port
> > *port, u16 out_volt)
> > goto port_unlock;
> > }
> >
> > - if (out_volt < port->pps_data.min_volt ||
> > - out_volt > port->pps_data.max_volt) {
> > + if (req_out_volt < port->pps_data.min_volt ||
> > + req_out_volt > port->pps_data.max_volt) {
> > ret = -EINVAL;
> > goto port_unlock;
> > }
> >
> > - target_mw = (port->pps_data.op_curr * out_volt) / 1000;
> > + target_mw = (port->current_limit * req_out_volt) / 1000;
> > if (target_mw < port->operating_snk_mw) {
> > ret = -EINVAL;
> > goto port_unlock;
> > @@ -5528,10 +5542,10 @@ static int tcpm_pps_set_out_volt(struct tcpm_port
> > *port, u16 out_volt)
> > }
> >
> > /* Round down output voltage to align with PPS valid steps */
> > - out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP);
> > + req_out_volt = req_out_volt - (req_out_volt %
> > RDO_PROG_VOLT_MV_STEP);
> >
> > reinit_completion(&port->pps_complete);
> > - port->pps_data.out_volt = out_volt;
> > + port->pps_data.req_out_volt = req_out_volt;
> > port->pps_status = 0;
> > port->pps_pending = true;
> > mutex_unlock(&port->lock);
> > @@ -5589,8 +5603,8 @@ static int tcpm_pps_activate(struct tcpm_port *port,
> > bool activate)
> >
> > /* Trigger PPS request or move back to standard PDO contract */
> > if (activate) {
> > - port->pps_data.out_volt = port->supply_voltage;
> > - port->pps_data.op_curr = port->current_limit;
> > + port->pps_data.req_out_volt = port->supply_voltage;
> > + port->pps_data.req_op_curr = port->current_limit;
> > }
> > mutex_unlock(&port->lock);
> >
> > --
> > 2.31.0.208.g409f899ff0-goog
>
>> The reason for this change is missing in the patch description,
>> or am I missing something ?
Ah yes forgot to state this in the commit description which I have
updated in V2.
Removed a redundant power_supply_changed as one was already made a
couple of lines before this one.
Thanks,
Badhri
On Wed, Apr 7, 2021 at 9:08 AM Adam Thomson
<[email protected]> wrote:
>
> On 06 April 2021 02:37, Badhri Jagan Sridharan wrote:
>
> > power_supply_changed needs to be called to notify clients
> > after the partner accepts the requested values for the pps
> > case.
> >
> > Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> > power_supply")
> > Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> > ---
>
> Missing commit information aside:
>
> Reviewed-by: Adam Thomson <[email protected]>
>
> > drivers/usb/typec/tcpm/tcpm.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index d43774cc2ccf..7708b01009cb 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -2564,6 +2564,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> > *port,
> > port->pps_data.max_curr = port-
> > >pps_data.req_max_curr;
> > port->req_supply_voltage = port-
> > >pps_data.req_out_volt;
> > port->req_current_limit = port->pps_data.req_op_curr;
> > + power_supply_changed(port->psy);
> > tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> > break;
> > case SOFT_RESET_SEND:
> > @@ -3132,7 +3133,6 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> > tcpm_port *port)
> > port-
> > >pps_data.req_out_volt));
> > port->pps_data.req_op_curr = min(port->pps_data.max_curr,
> > port->pps_data.req_op_curr);
> > - power_supply_changed(port->psy);
> > }
> >
> > return src_pdo;
> > @@ -3557,8 +3557,6 @@ static void tcpm_reset_port(struct tcpm_port *port)
> > port->sink_cap_done = false;
> > if (port->tcpc->enable_frs)
> > port->tcpc->enable_frs(port->tcpc, false);
> > -
> > - power_supply_changed(port->psy);
> > }
> >
> > static void tcpm_detach(struct tcpm_port *port)
> > --
> > 2.31.0.208.g409f899ff0-goog
>