2020-08-04 06:51:53

by Badhri Jagan Sridharan

[permalink] [raw]
Subject: [PATCH v1] tcpm: Honour pSnkStdby requirement during negotiation

From PD Spec:
The Sink Shall transition to Sink Standby before a positive or
negative voltage transition of VBUS. During Sink Standby
the Sink Shall reduce its power draw to pSnkStdby. This allows
the Source to manage the voltage transition as well as
supply sufficient operating current to the Sink to maintain PD
operation during the transition. The Sink Shall
complete this transition to Sink Standby within tSnkStdby
after evaluating the Accept Message from the Source. The
transition when returning to Sink operation from Sink Standby
Shall be completed within tSnkNewPower. The
pSnkStdby requirement Shall only apply if the Sink power draw
is higher than this level.

The above requirement needs to be met to prevent hard resets
from port partner.

Introducing psnkstdby_after_accept flag to accommodate systems
where the input current limit loops are not fast enough to meet
tSnkStby(15 msec).

When not set, port requests PD_P_SNK_STDBY upon entering SNK_DISCOVERY and
the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
SNK_READY for non-pd link.

When set, port requests CC advertisement based current limit during
SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY during
SNK_TRANSITION_SINK, PD based current limit gets set after RX of
PD_CTRL_PSRDY. However, in this case it has to be made sure that the
tSnkStdby (15 msec) is met.

Signed-off-by: Badhri Jagan Sridharan <[email protected]>
---
drivers/usb/typec/tcpm/tcpm.c | 52 +++++++++++++++++++++++++++--------
include/linux/usb/pd.h | 5 +++-
2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 3ef37202ee37..e46da41940b9 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -293,9 +293,12 @@ struct tcpm_port {
unsigned int operating_snk_mw;
bool update_sink_caps;

- /* Requested current / voltage */
+ /* Set current / voltage */
u32 current_limit;
u32 supply_voltage;
+ /* current / voltage requested to partner */
+ u32 req_current_limit;
+ u32 req_supply_voltage;

/* Used to export TA voltage and current */
struct power_supply *psy;
@@ -323,13 +326,27 @@ struct tcpm_port {
struct pd_mode_data mode_data;
struct typec_altmode *partner_altmode[ALTMODE_DISCOVERY_MAX];
struct typec_altmode *port_altmode[ALTMODE_DISCOVERY_MAX];
-
/* Deadline in jiffies to exit src_try_wait state */
unsigned long max_wait;

/* port belongs to a self powered device */
bool self_powered;

+ /*
+ * Honour psnkstdby after accept is received.
+ * However, in this case it has to be made sure that the tSnkStdby (15
+ * msec) is met.
+ *
+ * When not set, port requests PD_P_SNK_STDBY_5V upon entering SNK_DISCOVERY and
+ * the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
+ * SNK_READY for non-pd link.
+ *
+ * When set, port requests CC advertisement based current limit during
+ * SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY_5V during SNK_TRANSITION_SINK,
+ * PD based current limit gets set after RX of PD_CTRL_PSRDY.
+ */
+ bool psnkstdby_after_accept;
+
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
struct mutex logbuffer_lock; /* log buffer access lock */
@@ -1787,9 +1804,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
switch (port->state) {
case SNK_TRANSITION_SINK:
if (port->vbus_present) {
- tcpm_set_current_limit(port,
- port->current_limit,
- port->supply_voltage);
+ tcpm_set_current_limit(port, port->req_current_limit,
+ port->req_supply_voltage);
port->explicit_contract = true;
tcpm_set_state(port, SNK_READY, 0);
} else {
@@ -1861,8 +1877,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
break;
case SNK_NEGOTIATE_PPS_CAPABILITIES:
port->pps_data.active = true;
- port->supply_voltage = port->pps_data.out_volt;
- port->current_limit = port->pps_data.op_curr;
+ port->req_supply_voltage = port->pps_data.out_volt;
+ port->req_current_limit = port->pps_data.op_curr;
tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
break;
case SOFT_RESET_SEND:
@@ -2463,8 +2479,8 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
}

- port->current_limit = ma;
- port->supply_voltage = mv;
+ port->req_current_limit = ma;
+ port->req_supply_voltage = mv;

return 0;
}
@@ -3235,9 +3251,11 @@ static void run_state_machine(struct tcpm_port *port)
break;
case SNK_DISCOVERY:
if (port->vbus_present) {
- tcpm_set_current_limit(port,
- tcpm_get_current_limit(port),
- 5000);
+ if (port->psnkstdby_after_accept || tcpm_get_current_limit(port) <=
+ PD_P_SNK_STDBY_5V)
+ tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
+ else
+ tcpm_set_current_limit(port, PD_P_SNK_STDBY_5V, 5000);
tcpm_set_charge(port, true);
tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
break;
@@ -3318,6 +3336,10 @@ static void run_state_machine(struct tcpm_port *port)
}
break;
case SNK_TRANSITION_SINK:
+ if (port->psnkstdby_after_accept)
+ tcpm_set_current_limit(port, tcpm_get_current_limit(port) >
+ PD_P_SNK_STDBY_5V ? PD_P_SNK_STDBY_5V :
+ tcpm_get_current_limit(port), 5000);
case SNK_TRANSITION_SINK_VBUS:
tcpm_set_state(port, hard_reset_state(port),
PD_T_PS_TRANSITION);
@@ -3331,6 +3353,10 @@ static void run_state_machine(struct tcpm_port *port)
port->pwr_opmode = TYPEC_PWR_MODE_PD;
}

+ /* Set current limit for NON-PD link when psnkstdby_after_accept is not set*/
+ if (!port->pd_capable && !port->psnkstdby_after_accept)
+ tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
+
tcpm_swap_complete(port, 0);
tcpm_typec_connect(port);
tcpm_check_send_discover(port);
@@ -4513,6 +4539,8 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
port->typec_caps.type = ret;
port->port_type = port->typec_caps.type;

+ port->psnkstdby_after_accept = fwnode_property_read_bool(fwnode, "psnkstdby-after-accept");
+
if (port->port_type == TYPEC_PORT_SNK)
goto sink;

diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index b6c233e79bd4..6bd70f77045e 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -483,5 +483,8 @@ static inline unsigned int rdo_max_power(u32 rdo)
#define PD_N_CAPS_COUNT (PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
#define PD_N_HARD_RESET_COUNT 2

-#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
+#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
+
+#define PD_P_SNK_STDBY_5V 500 /* 2500 mw - 500mA @ 5V */
+
#endif /* __LINUX_USB_PD_H */
--
2.28.0.163.g6104cc2f0b6-goog


2020-08-04 08:32:29

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v1] tcpm: Honour pSnkStdby requirement during negotiation

Hi Badhri,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on next-20200803]
[cannot apply to v5.8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Badhri-Jagan-Sridharan/tcpm-Honour-pSnkStdby-requirement-during-negotiation/20200804-145301
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/usb/typec/tcpm/tcpm.c: In function 'run_state_machine':
>> drivers/usb/typec/tcpm/tcpm.c:3339:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
3339 | if (port->psnkstdby_after_accept)
| ^
drivers/usb/typec/tcpm/tcpm.c:3343:2: note: here
3343 | case SNK_TRANSITION_SINK_VBUS:
| ^~~~
At top level:
drivers/usb/typec/tcpm/tcpm.c:1614:39: warning: 'tcpm_altmode_ops' defined but not used [-Wunused-const-variable=]
1614 | static const struct typec_altmode_ops tcpm_altmode_ops = {
| ^~~~~~~~~~~~~~~~

vim +3339 drivers/usb/typec/tcpm/tcpm.c

2943
2944 static void run_state_machine(struct tcpm_port *port)
2945 {
2946 int ret;
2947 enum typec_pwr_opmode opmode;
2948 unsigned int msecs;
2949
2950 port->enter_state = port->state;
2951 switch (port->state) {
2952 case TOGGLING:
2953 break;
2954 /* SRC states */
2955 case SRC_UNATTACHED:
2956 if (!port->non_pd_role_swap)
2957 tcpm_swap_complete(port, -ENOTCONN);
2958 tcpm_src_detach(port);
2959 if (tcpm_start_toggling(port, tcpm_rp_cc(port))) {
2960 tcpm_set_state(port, TOGGLING, 0);
2961 break;
2962 }
2963 tcpm_set_cc(port, tcpm_rp_cc(port));
2964 if (port->port_type == TYPEC_PORT_DRP)
2965 tcpm_set_state(port, SNK_UNATTACHED, PD_T_DRP_SNK);
2966 break;
2967 case SRC_ATTACH_WAIT:
2968 if (tcpm_port_is_debug(port))
2969 tcpm_set_state(port, DEBUG_ACC_ATTACHED,
2970 PD_T_CC_DEBOUNCE);
2971 else if (tcpm_port_is_audio(port))
2972 tcpm_set_state(port, AUDIO_ACC_ATTACHED,
2973 PD_T_CC_DEBOUNCE);
2974 else if (tcpm_port_is_source(port))
2975 tcpm_set_state(port,
2976 tcpm_try_snk(port) ? SNK_TRY
2977 : SRC_ATTACHED,
2978 PD_T_CC_DEBOUNCE);
2979 break;
2980
2981 case SNK_TRY:
2982 port->try_snk_count++;
2983 /*
2984 * Requirements:
2985 * - Do not drive vconn or vbus
2986 * - Terminate CC pins (both) to Rd
2987 * Action:
2988 * - Wait for tDRPTry (PD_T_DRP_TRY).
2989 * Until then, ignore any state changes.
2990 */
2991 tcpm_set_cc(port, TYPEC_CC_RD);
2992 tcpm_set_state(port, SNK_TRY_WAIT, PD_T_DRP_TRY);
2993 break;
2994 case SNK_TRY_WAIT:
2995 if (tcpm_port_is_sink(port)) {
2996 tcpm_set_state(port, SNK_TRY_WAIT_DEBOUNCE, 0);
2997 } else {
2998 tcpm_set_state(port, SRC_TRYWAIT, 0);
2999 port->max_wait = 0;
3000 }
3001 break;
3002 case SNK_TRY_WAIT_DEBOUNCE:
3003 tcpm_set_state(port, SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS,
3004 PD_T_PD_DEBOUNCE);
3005 break;
3006 case SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS:
3007 if (port->vbus_present && tcpm_port_is_sink(port)) {
3008 tcpm_set_state(port, SNK_ATTACHED, 0);
3009 } else {
3010 tcpm_set_state(port, SRC_TRYWAIT, 0);
3011 port->max_wait = 0;
3012 }
3013 break;
3014 case SRC_TRYWAIT:
3015 tcpm_set_cc(port, tcpm_rp_cc(port));
3016 if (port->max_wait == 0) {
3017 port->max_wait = jiffies +
3018 msecs_to_jiffies(PD_T_DRP_TRY);
3019 tcpm_set_state(port, SRC_TRYWAIT_UNATTACHED,
3020 PD_T_DRP_TRY);
3021 } else {
3022 if (time_is_after_jiffies(port->max_wait))
3023 tcpm_set_state(port, SRC_TRYWAIT_UNATTACHED,
3024 jiffies_to_msecs(port->max_wait -
3025 jiffies));
3026 else
3027 tcpm_set_state(port, SNK_UNATTACHED, 0);
3028 }
3029 break;
3030 case SRC_TRYWAIT_DEBOUNCE:
3031 tcpm_set_state(port, SRC_ATTACHED, PD_T_CC_DEBOUNCE);
3032 break;
3033 case SRC_TRYWAIT_UNATTACHED:
3034 tcpm_set_state(port, SNK_UNATTACHED, 0);
3035 break;
3036
3037 case SRC_ATTACHED:
3038 ret = tcpm_src_attach(port);
3039 tcpm_set_state(port, SRC_UNATTACHED,
3040 ret < 0 ? 0 : PD_T_PS_SOURCE_ON);
3041 break;
3042 case SRC_STARTUP:
3043 opmode = tcpm_get_pwr_opmode(tcpm_rp_cc(port));
3044 typec_set_pwr_opmode(port->typec_port, opmode);
3045 port->pwr_opmode = TYPEC_PWR_MODE_USB;
3046 port->caps_count = 0;
3047 port->negotiated_rev = PD_MAX_REV;
3048 port->message_id = 0;
3049 port->rx_msgid = -1;
3050 port->explicit_contract = false;
3051 tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
3052 break;
3053 case SRC_SEND_CAPABILITIES:
3054 port->caps_count++;
3055 if (port->caps_count > PD_N_CAPS_COUNT) {
3056 tcpm_set_state(port, SRC_READY, 0);
3057 break;
3058 }
3059 ret = tcpm_pd_send_source_caps(port);
3060 if (ret < 0) {
3061 tcpm_set_state(port, SRC_SEND_CAPABILITIES,
3062 PD_T_SEND_SOURCE_CAP);
3063 } else {
3064 /*
3065 * Per standard, we should clear the reset counter here.
3066 * However, that can result in state machine hang-ups.
3067 * Reset it only in READY state to improve stability.
3068 */
3069 /* port->hard_reset_count = 0; */
3070 port->caps_count = 0;
3071 port->pd_capable = true;
3072 tcpm_set_state_cond(port, SRC_SEND_CAPABILITIES_TIMEOUT,
3073 PD_T_SEND_SOURCE_CAP);
3074 }
3075 break;
3076 case SRC_SEND_CAPABILITIES_TIMEOUT:
3077 /*
3078 * Error recovery for a PD_DATA_SOURCE_CAP reply timeout.
3079 *
3080 * PD 2.0 sinks are supposed to accept src-capabilities with a
3081 * 3.0 header and simply ignore any src PDOs which the sink does
3082 * not understand such as PPS but some 2.0 sinks instead ignore
3083 * the entire PD_DATA_SOURCE_CAP message, causing contract
3084 * negotiation to fail.
3085 *
3086 * After PD_N_HARD_RESET_COUNT hard-reset attempts, we try
3087 * sending src-capabilities with a lower PD revision to
3088 * make these broken sinks work.
3089 */
3090 if (port->hard_reset_count < PD_N_HARD_RESET_COUNT) {
3091 tcpm_set_state(port, HARD_RESET_SEND, 0);
3092 } else if (port->negotiated_rev > PD_REV20) {
3093 port->negotiated_rev--;
3094 port->hard_reset_count = 0;
3095 tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
3096 } else {
3097 tcpm_set_state(port, hard_reset_state(port), 0);
3098 }
3099 break;
3100 case SRC_NEGOTIATE_CAPABILITIES:
3101 ret = tcpm_pd_check_request(port);
3102 if (ret < 0) {
3103 tcpm_pd_send_control(port, PD_CTRL_REJECT);
3104 if (!port->explicit_contract) {
3105 tcpm_set_state(port,
3106 SRC_WAIT_NEW_CAPABILITIES, 0);
3107 } else {
3108 tcpm_set_state(port, SRC_READY, 0);
3109 }
3110 } else {
3111 tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
3112 tcpm_set_state(port, SRC_TRANSITION_SUPPLY,
3113 PD_T_SRC_TRANSITION);
3114 }
3115 break;
3116 case SRC_TRANSITION_SUPPLY:
3117 /* XXX: regulator_set_voltage(vbus, ...) */
3118 tcpm_pd_send_control(port, PD_CTRL_PS_RDY);
3119 port->explicit_contract = true;
3120 typec_set_pwr_opmode(port->typec_port, TYPEC_PWR_MODE_PD);
3121 port->pwr_opmode = TYPEC_PWR_MODE_PD;
3122 tcpm_set_state_cond(port, SRC_READY, 0);
3123 break;
3124 case SRC_READY:
3125 #if 1
3126 port->hard_reset_count = 0;
3127 #endif
3128 port->try_src_count = 0;
3129
3130 tcpm_swap_complete(port, 0);
3131 tcpm_typec_connect(port);
3132
3133 tcpm_check_send_discover(port);
3134 /*
3135 * 6.3.5
3136 * Sending ping messages is not necessary if
3137 * - the source operates at vSafe5V
3138 * or
3139 * - The system is not operating in PD mode
3140 * or
3141 * - Both partners are connected using a Type-C connector
3142 *
3143 * There is no actual need to send PD messages since the local
3144 * port type-c and the spec does not clearly say whether PD is
3145 * possible when type-c is connected to Type-A/B
3146 */
3147 break;
3148 case SRC_WAIT_NEW_CAPABILITIES:
3149 /* Nothing to do... */
3150 break;
3151
3152 /* SNK states */
3153 case SNK_UNATTACHED:
3154 if (!port->non_pd_role_swap)
3155 tcpm_swap_complete(port, -ENOTCONN);
3156 tcpm_pps_complete(port, -ENOTCONN);
3157 tcpm_snk_detach(port);
3158 if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
3159 tcpm_set_state(port, TOGGLING, 0);
3160 break;
3161 }
3162 tcpm_set_cc(port, TYPEC_CC_RD);
3163 if (port->port_type == TYPEC_PORT_DRP)
3164 tcpm_set_state(port, SRC_UNATTACHED, PD_T_DRP_SRC);
3165 break;
3166 case SNK_ATTACH_WAIT:
3167 if ((port->cc1 == TYPEC_CC_OPEN &&
3168 port->cc2 != TYPEC_CC_OPEN) ||
3169 (port->cc1 != TYPEC_CC_OPEN &&
3170 port->cc2 == TYPEC_CC_OPEN))
3171 tcpm_set_state(port, SNK_DEBOUNCED,
3172 PD_T_CC_DEBOUNCE);
3173 else if (tcpm_port_is_disconnected(port))
3174 tcpm_set_state(port, SNK_UNATTACHED,
3175 PD_T_PD_DEBOUNCE);
3176 break;
3177 case SNK_DEBOUNCED:
3178 if (tcpm_port_is_disconnected(port))
3179 tcpm_set_state(port, SNK_UNATTACHED,
3180 PD_T_PD_DEBOUNCE);
3181 else if (port->vbus_present)
3182 tcpm_set_state(port,
3183 tcpm_try_src(port) ? SRC_TRY
3184 : SNK_ATTACHED,
3185 0);
3186 else
3187 /* Wait for VBUS, but not forever */
3188 tcpm_set_state(port, PORT_RESET, PD_T_PS_SOURCE_ON);
3189 break;
3190
3191 case SRC_TRY:
3192 port->try_src_count++;
3193 tcpm_set_cc(port, tcpm_rp_cc(port));
3194 port->max_wait = 0;
3195 tcpm_set_state(port, SRC_TRY_WAIT, 0);
3196 break;
3197 case SRC_TRY_WAIT:
3198 if (port->max_wait == 0) {
3199 port->max_wait = jiffies +
3200 msecs_to_jiffies(PD_T_DRP_TRY);
3201 msecs = PD_T_DRP_TRY;
3202 } else {
3203 if (time_is_after_jiffies(port->max_wait))
3204 msecs = jiffies_to_msecs(port->max_wait -
3205 jiffies);
3206 else
3207 msecs = 0;
3208 }
3209 tcpm_set_state(port, SNK_TRYWAIT, msecs);
3210 break;
3211 case SRC_TRY_DEBOUNCE:
3212 tcpm_set_state(port, SRC_ATTACHED, PD_T_PD_DEBOUNCE);
3213 break;
3214 case SNK_TRYWAIT:
3215 tcpm_set_cc(port, TYPEC_CC_RD);
3216 tcpm_set_state(port, SNK_TRYWAIT_VBUS, PD_T_CC_DEBOUNCE);
3217 break;
3218 case SNK_TRYWAIT_VBUS:
3219 /*
3220 * TCPM stays in this state indefinitely until VBUS
3221 * is detected as long as Rp is not detected for
3222 * more than a time period of tPDDebounce.
3223 */
3224 if (port->vbus_present && tcpm_port_is_sink(port)) {
3225 tcpm_set_state(port, SNK_ATTACHED, 0);
3226 break;
3227 }
3228 if (!tcpm_port_is_sink(port))
3229 tcpm_set_state(port, SNK_TRYWAIT_DEBOUNCE, 0);
3230 break;
3231 case SNK_TRYWAIT_DEBOUNCE:
3232 tcpm_set_state(port, SNK_UNATTACHED, PD_T_PD_DEBOUNCE);
3233 break;
3234 case SNK_ATTACHED:
3235 ret = tcpm_snk_attach(port);
3236 if (ret < 0)
3237 tcpm_set_state(port, SNK_UNATTACHED, 0);
3238 else
3239 tcpm_set_state(port, SNK_STARTUP, 0);
3240 break;
3241 case SNK_STARTUP:
3242 opmode = tcpm_get_pwr_opmode(port->polarity ?
3243 port->cc2 : port->cc1);
3244 typec_set_pwr_opmode(port->typec_port, opmode);
3245 port->pwr_opmode = TYPEC_PWR_MODE_USB;
3246 port->negotiated_rev = PD_MAX_REV;
3247 port->message_id = 0;
3248 port->rx_msgid = -1;
3249 port->explicit_contract = false;
3250 tcpm_set_state(port, SNK_DISCOVERY, 0);
3251 break;
3252 case SNK_DISCOVERY:
3253 if (port->vbus_present) {
3254 if (port->psnkstdby_after_accept || tcpm_get_current_limit(port) <=
3255 PD_P_SNK_STDBY_5V)
3256 tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
3257 else
3258 tcpm_set_current_limit(port, PD_P_SNK_STDBY_5V, 5000);
3259 tcpm_set_charge(port, true);
3260 tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
3261 break;
3262 }
3263 /*
3264 * For DRP, timeouts differ. Also, handling is supposed to be
3265 * different and much more complex (dead battery detection;
3266 * see USB power delivery specification, section 8.3.3.6.1.5.1).
3267 */
3268 tcpm_set_state(port, hard_reset_state(port),
3269 port->port_type == TYPEC_PORT_DRP ?
3270 PD_T_DB_DETECT : PD_T_NO_RESPONSE);
3271 break;
3272 case SNK_DISCOVERY_DEBOUNCE:
3273 tcpm_set_state(port, SNK_DISCOVERY_DEBOUNCE_DONE,
3274 PD_T_CC_DEBOUNCE);
3275 break;
3276 case SNK_DISCOVERY_DEBOUNCE_DONE:
3277 if (!tcpm_port_is_disconnected(port) &&
3278 tcpm_port_is_sink(port) &&
3279 time_is_after_jiffies(port->delayed_runtime)) {
3280 tcpm_set_state(port, SNK_DISCOVERY,
3281 jiffies_to_msecs(port->delayed_runtime -
3282 jiffies));
3283 break;
3284 }
3285 tcpm_set_state(port, unattached_state(port), 0);
3286 break;
3287 case SNK_WAIT_CAPABILITIES:
3288 ret = port->tcpc->set_pd_rx(port->tcpc, true);
3289 if (ret < 0) {
3290 tcpm_set_state(port, SNK_READY, 0);
3291 break;
3292 }
3293 /*
3294 * If VBUS has never been low, and we time out waiting
3295 * for source cap, try a soft reset first, in case we
3296 * were already in a stable contract before this boot.
3297 * Do this only once.
3298 */
3299 if (port->vbus_never_low) {
3300 port->vbus_never_low = false;
3301 tcpm_set_state(port, SOFT_RESET_SEND,
3302 PD_T_SINK_WAIT_CAP);
3303 } else {
3304 tcpm_set_state(port, hard_reset_state(port),
3305 PD_T_SINK_WAIT_CAP);
3306 }
3307 break;
3308 case SNK_NEGOTIATE_CAPABILITIES:
3309 port->pd_capable = true;
3310 port->hard_reset_count = 0;
3311 ret = tcpm_pd_send_request(port);
3312 if (ret < 0) {
3313 /* Let the Source send capabilities again. */
3314 tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
3315 } else {
3316 tcpm_set_state_cond(port, hard_reset_state(port),
3317 PD_T_SENDER_RESPONSE);
3318 }
3319 break;
3320 case SNK_NEGOTIATE_PPS_CAPABILITIES:
3321 ret = tcpm_pd_send_pps_request(port);
3322 if (ret < 0) {
3323 port->pps_status = ret;
3324 /*
3325 * If this was called due to updates to sink
3326 * capabilities, and pps is no longer valid, we should
3327 * safely fall back to a standard PDO.
3328 */
3329 if (port->update_sink_caps)
3330 tcpm_set_state(port, SNK_NEGOTIATE_CAPABILITIES, 0);
3331 else
3332 tcpm_set_state(port, SNK_READY, 0);
3333 } else {
3334 tcpm_set_state_cond(port, hard_reset_state(port),
3335 PD_T_SENDER_RESPONSE);
3336 }
3337 break;
3338 case SNK_TRANSITION_SINK:
> 3339 if (port->psnkstdby_after_accept)
3340 tcpm_set_current_limit(port, tcpm_get_current_limit(port) >
3341 PD_P_SNK_STDBY_5V ? PD_P_SNK_STDBY_5V :
3342 tcpm_get_current_limit(port), 5000);
3343 case SNK_TRANSITION_SINK_VBUS:
3344 tcpm_set_state(port, hard_reset_state(port),
3345 PD_T_PS_TRANSITION);
3346 break;
3347 case SNK_READY:
3348 port->try_snk_count = 0;
3349 port->update_sink_caps = false;
3350 if (port->explicit_contract) {
3351 typec_set_pwr_opmode(port->typec_port,
3352 TYPEC_PWR_MODE_PD);
3353 port->pwr_opmode = TYPEC_PWR_MODE_PD;
3354 }
3355
3356 /* Set current limit for NON-PD link when psnkstdby_after_accept is not set*/
3357 if (!port->pd_capable && !port->psnkstdby_after_accept)
3358 tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
3359
3360 tcpm_swap_complete(port, 0);
3361 tcpm_typec_connect(port);
3362 tcpm_check_send_discover(port);
3363 tcpm_pps_complete(port, port->pps_status);
3364
3365 power_supply_changed(port->psy);
3366
3367 break;
3368
3369 /* Accessory states */
3370 case ACC_UNATTACHED:
3371 tcpm_acc_detach(port);
3372 tcpm_set_state(port, SRC_UNATTACHED, 0);
3373 break;
3374 case DEBUG_ACC_ATTACHED:
3375 case AUDIO_ACC_ATTACHED:
3376 ret = tcpm_acc_attach(port);
3377 if (ret < 0)
3378 tcpm_set_state(port, ACC_UNATTACHED, 0);
3379 break;
3380 case AUDIO_ACC_DEBOUNCE:
3381 tcpm_set_state(port, ACC_UNATTACHED, PD_T_CC_DEBOUNCE);
3382 break;
3383
3384 /* Hard_Reset states */
3385 case HARD_RESET_SEND:
3386 tcpm_pd_transmit(port, TCPC_TX_HARD_RESET, NULL);
3387 tcpm_set_state(port, HARD_RESET_START, 0);
3388 break;
3389 case HARD_RESET_START:
3390 port->hard_reset_count++;
3391 port->tcpc->set_pd_rx(port->tcpc, false);
3392 tcpm_unregister_altmodes(port);
3393 port->send_discover = true;
3394 if (port->pwr_role == TYPEC_SOURCE)
3395 tcpm_set_state(port, SRC_HARD_RESET_VBUS_OFF,
3396 PD_T_PS_HARD_RESET);
3397 else
3398 tcpm_set_state(port, SNK_HARD_RESET_SINK_OFF, 0);
3399 break;
3400 case SRC_HARD_RESET_VBUS_OFF:
3401 tcpm_set_vconn(port, true);
3402 tcpm_set_vbus(port, false);
3403 tcpm_set_roles(port, port->self_powered, TYPEC_SOURCE,
3404 tcpm_data_role_for_source(port));
3405 tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
3406 break;
3407 case SRC_HARD_RESET_VBUS_ON:
3408 tcpm_set_vbus(port, true);
3409 port->tcpc->set_pd_rx(port->tcpc, true);
3410 tcpm_set_attached_state(port, true);
3411 tcpm_set_state(port, SRC_UNATTACHED, PD_T_PS_SOURCE_ON);
3412 break;
3413 case SNK_HARD_RESET_SINK_OFF:
3414 memset(&port->pps_data, 0, sizeof(port->pps_data));
3415 tcpm_set_vconn(port, false);
3416 if (port->pd_capable)
3417 tcpm_set_charge(port, false);
3418 tcpm_set_roles(port, port->self_powered, TYPEC_SINK,
3419 tcpm_data_role_for_sink(port));
3420 /*
3421 * VBUS may or may not toggle, depending on the adapter.
3422 * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
3423 * directly after timeout.
3424 */
3425 tcpm_set_state(port, SNK_HARD_RESET_SINK_ON, PD_T_SAFE_0V);
3426 break;
3427 case SNK_HARD_RESET_WAIT_VBUS:
3428 /* Assume we're disconnected if VBUS doesn't come back. */
3429 tcpm_set_state(port, SNK_UNATTACHED,
3430 PD_T_SRC_RECOVER_MAX + PD_T_SRC_TURN_ON);
3431 break;
3432 case SNK_HARD_RESET_SINK_ON:
3433 /* Note: There is no guarantee that VBUS is on in this state */
3434 /*
3435 * XXX:
3436 * The specification suggests that dual mode ports in sink
3437 * mode should transition to state PE_SRC_Transition_to_default.
3438 * See USB power delivery specification chapter 8.3.3.6.1.3.
3439 * This would mean to to
3440 * - turn off VCONN, reset power supply
3441 * - request hardware reset
3442 * - turn on VCONN
3443 * - Transition to state PE_Src_Startup
3444 * SNK only ports shall transition to state Snk_Startup
3445 * (see chapter 8.3.3.3.8).
3446 * Similar, dual-mode ports in source mode should transition
3447 * to PE_SNK_Transition_to_default.
3448 */
3449 if (port->pd_capable) {
3450 tcpm_set_current_limit(port,
3451 tcpm_get_current_limit(port),
3452 5000);
3453 tcpm_set_charge(port, true);
3454 }
3455 tcpm_set_attached_state(port, true);
3456 tcpm_set_state(port, SNK_STARTUP, 0);
3457 break;
3458
3459 /* Soft_Reset states */
3460 case SOFT_RESET:
3461 port->message_id = 0;
3462 port->rx_msgid = -1;
3463 tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
3464 if (port->pwr_role == TYPEC_SOURCE)
3465 tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
3466 else
3467 tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
3468 break;
3469 case SOFT_RESET_SEND:
3470 port->message_id = 0;
3471 port->rx_msgid = -1;
3472 if (tcpm_pd_send_control(port, PD_CTRL_SOFT_RESET))
3473 tcpm_set_state_cond(port, hard_reset_state(port), 0);
3474 else
3475 tcpm_set_state_cond(port, hard_reset_state(port),
3476 PD_T_SENDER_RESPONSE);
3477 break;
3478
3479 /* DR_Swap states */
3480 case DR_SWAP_SEND:
3481 tcpm_pd_send_control(port, PD_CTRL_DR_SWAP);
3482 tcpm_set_state_cond(port, DR_SWAP_SEND_TIMEOUT,
3483 PD_T_SENDER_RESPONSE);
3484 break;
3485 case DR_SWAP_ACCEPT:
3486 tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
3487 tcpm_set_state_cond(port, DR_SWAP_CHANGE_DR, 0);
3488 break;
3489 case DR_SWAP_SEND_TIMEOUT:
3490 tcpm_swap_complete(port, -ETIMEDOUT);
3491 tcpm_set_state(port, ready_state(port), 0);
3492 break;
3493 case DR_SWAP_CHANGE_DR:
3494 if (port->data_role == TYPEC_HOST) {
3495 tcpm_unregister_altmodes(port);
3496 tcpm_set_roles(port, true, port->pwr_role,
3497 TYPEC_DEVICE);
3498 } else {
3499 tcpm_set_roles(port, true, port->pwr_role,
3500 TYPEC_HOST);
3501 port->send_discover = true;
3502 }
3503 tcpm_set_state(port, ready_state(port), 0);
3504 break;
3505
3506 /* PR_Swap states */
3507 case PR_SWAP_ACCEPT:
3508 tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
3509 tcpm_set_state(port, PR_SWAP_START, 0);
3510 break;
3511 case PR_SWAP_SEND:
3512 tcpm_pd_send_control(port, PD_CTRL_PR_SWAP);
3513 tcpm_set_state_cond(port, PR_SWAP_SEND_TIMEOUT,
3514 PD_T_SENDER_RESPONSE);
3515 break;
3516 case PR_SWAP_SEND_TIMEOUT:
3517 tcpm_swap_complete(port, -ETIMEDOUT);
3518 tcpm_set_state(port, ready_state(port), 0);
3519 break;
3520 case PR_SWAP_START:
3521 if (port->pwr_role == TYPEC_SOURCE)
3522 tcpm_set_state(port, PR_SWAP_SRC_SNK_TRANSITION_OFF,
3523 PD_T_SRC_TRANSITION);
3524 else
3525 tcpm_set_state(port, PR_SWAP_SNK_SRC_SINK_OFF, 0);
3526 break;
3527 case PR_SWAP_SRC_SNK_TRANSITION_OFF:
3528 tcpm_set_vbus(port, false);
3529 port->explicit_contract = false;
3530 /* allow time for Vbus discharge, must be < tSrcSwapStdby */
3531 tcpm_set_state(port, PR_SWAP_SRC_SNK_SOURCE_OFF,
3532 PD_T_SRCSWAPSTDBY);
3533 break;
3534 case PR_SWAP_SRC_SNK_SOURCE_OFF:
3535 tcpm_set_cc(port, TYPEC_CC_RD);
3536 /* allow CC debounce */
3537 tcpm_set_state(port, PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED,
3538 PD_T_CC_DEBOUNCE);
3539 break;
3540 case PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED:
3541 /*
3542 * USB-PD standard, 6.2.1.4, Port Power Role:
3543 * "During the Power Role Swap Sequence, for the initial Source
3544 * Port, the Port Power Role field shall be set to Sink in the
3545 * PS_RDY Message indicating that the initial Source’s power
3546 * supply is turned off"
3547 */
3548 tcpm_set_pwr_role(port, TYPEC_SINK);
3549 if (tcpm_pd_send_control(port, PD_CTRL_PS_RDY)) {
3550 tcpm_set_state(port, ERROR_RECOVERY, 0);
3551 break;
3552 }
3553 tcpm_set_state_cond(port, SNK_UNATTACHED, PD_T_PS_SOURCE_ON);
3554 break;
3555 case PR_SWAP_SRC_SNK_SINK_ON:
3556 tcpm_set_state(port, SNK_STARTUP, 0);
3557 break;
3558 case PR_SWAP_SNK_SRC_SINK_OFF:
3559 tcpm_set_charge(port, false);
3560 tcpm_set_state(port, hard_reset_state(port),
3561 PD_T_PS_SOURCE_OFF);
3562 break;
3563 case PR_SWAP_SNK_SRC_SOURCE_ON:
3564 tcpm_set_cc(port, tcpm_rp_cc(port));
3565 tcpm_set_vbus(port, true);
3566 /*
3567 * allow time VBUS ramp-up, must be < tNewSrc
3568 * Also, this window overlaps with CC debounce as well.
3569 * So, Wait for the max of two which is PD_T_NEWSRC
3570 */
3571 tcpm_set_state(port, PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP,
3572 PD_T_NEWSRC);
3573 break;
3574 case PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP:
3575 /*
3576 * USB PD standard, 6.2.1.4:
3577 * "Subsequent Messages initiated by the Policy Engine,
3578 * such as the PS_RDY Message sent to indicate that Vbus
3579 * is ready, will have the Port Power Role field set to
3580 * Source."
3581 */
3582 tcpm_set_pwr_role(port, TYPEC_SOURCE);
3583 tcpm_pd_send_control(port, PD_CTRL_PS_RDY);
3584 tcpm_set_state(port, SRC_STARTUP, 0);
3585 break;
3586
3587 case VCONN_SWAP_ACCEPT:
3588 tcpm_pd_send_control(port, PD_CTRL_ACCEPT);
3589 tcpm_set_state(port, VCONN_SWAP_START, 0);
3590 break;
3591 case VCONN_SWAP_SEND:
3592 tcpm_pd_send_control(port, PD_CTRL_VCONN_SWAP);
3593 tcpm_set_state(port, VCONN_SWAP_SEND_TIMEOUT,
3594 PD_T_SENDER_RESPONSE);
3595 break;
3596 case VCONN_SWAP_SEND_TIMEOUT:
3597 tcpm_swap_complete(port, -ETIMEDOUT);
3598 tcpm_set_state(port, ready_state(port), 0);
3599 break;
3600 case VCONN_SWAP_START:
3601 if (port->vconn_role == TYPEC_SOURCE)
3602 tcpm_set_state(port, VCONN_SWAP_WAIT_FOR_VCONN, 0);
3603 else
3604 tcpm_set_state(port, VCONN_SWAP_TURN_ON_VCONN, 0);
3605 break;
3606 case VCONN_SWAP_WAIT_FOR_VCONN:
3607 tcpm_set_state(port, hard_reset_state(port),
3608 PD_T_VCONN_SOURCE_ON);
3609 break;
3610 case VCONN_SWAP_TURN_ON_VCONN:
3611 tcpm_set_vconn(port, true);
3612 tcpm_pd_send_control(port, PD_CTRL_PS_RDY);
3613 tcpm_set_state(port, ready_state(port), 0);
3614 break;
3615 case VCONN_SWAP_TURN_OFF_VCONN:
3616 tcpm_set_vconn(port, false);
3617 tcpm_set_state(port, ready_state(port), 0);
3618 break;
3619
3620 case DR_SWAP_CANCEL:
3621 case PR_SWAP_CANCEL:
3622 case VCONN_SWAP_CANCEL:
3623 tcpm_swap_complete(port, port->swap_status);
3624 if (port->pwr_role == TYPEC_SOURCE)
3625 tcpm_set_state(port, SRC_READY, 0);
3626 else
3627 tcpm_set_state(port, SNK_READY, 0);
3628 break;
3629
3630 case BIST_RX:
3631 switch (BDO_MODE_MASK(port->bist_request)) {
3632 case BDO_MODE_CARRIER2:
3633 tcpm_pd_transmit(port, TCPC_TX_BIST_MODE_2, NULL);
3634 tcpm_set_state(port, unattached_state(port),
3635 PD_T_BIST_CONT_MODE);
3636 break;
3637 case BDO_MODE_TESTDATA:
3638 if (port->tcpc->set_bist_data) {
3639 tcpm_log(port, "Enable BIST MODE TESTDATA");
3640 port->tcpc->set_bist_data(port->tcpc, true);
3641 }
3642 break;
3643 default:
3644 break;
3645 }
3646 break;
3647 case GET_STATUS_SEND:
3648 tcpm_pd_send_control(port, PD_CTRL_GET_STATUS);
3649 tcpm_set_state(port, GET_STATUS_SEND_TIMEOUT,
3650 PD_T_SENDER_RESPONSE);
3651 break;
3652 case GET_STATUS_SEND_TIMEOUT:
3653 tcpm_set_state(port, ready_state(port), 0);
3654 break;
3655 case GET_PPS_STATUS_SEND:
3656 tcpm_pd_send_control(port, PD_CTRL_GET_PPS_STATUS);
3657 tcpm_set_state(port, GET_PPS_STATUS_SEND_TIMEOUT,
3658 PD_T_SENDER_RESPONSE);
3659 break;
3660 case GET_PPS_STATUS_SEND_TIMEOUT:
3661 tcpm_set_state(port, ready_state(port), 0);
3662 break;
3663 case ERROR_RECOVERY:
3664 tcpm_swap_complete(port, -EPROTO);
3665 tcpm_pps_complete(port, -EPROTO);
3666 tcpm_set_state(port, PORT_RESET, 0);
3667 break;
3668 case PORT_RESET:
3669 tcpm_reset_port(port);
3670 tcpm_set_cc(port, TYPEC_CC_OPEN);
3671 tcpm_set_state(port, PORT_RESET_WAIT_OFF,
3672 PD_T_ERROR_RECOVERY);
3673 break;
3674 case PORT_RESET_WAIT_OFF:
3675 tcpm_set_state(port,
3676 tcpm_default_state(port),
3677 port->vbus_present ? PD_T_PS_SOURCE_OFF : 0);
3678 break;
3679 default:
3680 WARN(1, "Unexpected port state %d\n", port->state);
3681 break;
3682 }
3683 }
3684

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (29.42 kB)
.config.gz (55.25 kB)
Download all attachments

2020-08-04 17:00:47

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v1] tcpm: Honour pSnkStdby requirement during negotiation

On 8/3/20 11:51 PM, Badhri Jagan Sridharan wrote:
>>From PD Spec:
> The Sink Shall transition to Sink Standby before a positive or
> negative voltage transition of VBUS. During Sink Standby
> the Sink Shall reduce its power draw to pSnkStdby. This allows
> the Source to manage the voltage transition as well as
> supply sufficient operating current to the Sink to maintain PD
> operation during the transition. The Sink Shall
> complete this transition to Sink Standby within tSnkStdby
> after evaluating the Accept Message from the Source. The
> transition when returning to Sink operation from Sink Standby
> Shall be completed within tSnkNewPower. The
> pSnkStdby requirement Shall only apply if the Sink power draw
> is higher than this level.
>
> The above requirement needs to be met to prevent hard resets
> from port partner.
>
> Introducing psnkstdby_after_accept flag to accommodate systems
> where the input current limit loops are not fast enough to meet
> tSnkStby(15 msec).
>
> When not set, port requests PD_P_SNK_STDBY upon entering SNK_DISCOVERY and
> the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
> SNK_READY for non-pd link.
>
> When set, port requests CC advertisement based current limit during
> SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY during
> SNK_TRANSITION_SINK, PD based current limit gets set after RX of
> PD_CTRL_PSRDY. However, in this case it has to be made sure that the
> tSnkStdby (15 msec) is met.
>
> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> ---
> drivers/usb/typec/tcpm/tcpm.c | 52 +++++++++++++++++++++++++++--------
> include/linux/usb/pd.h | 5 +++-
> 2 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3ef37202ee37..e46da41940b9 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -293,9 +293,12 @@ struct tcpm_port {
> unsigned int operating_snk_mw;
> bool update_sink_caps;
>
> - /* Requested current / voltage */
> + /* Set current / voltage */
> u32 current_limit;
> u32 supply_voltage;
> + /* current / voltage requested to partner */
> + u32 req_current_limit;
> + u32 req_supply_voltage;

I don't see a clear delineation where, when, and why supply_voltage vs. req_supply_voltage
and current_limit vs. req_current_limit is used. Maybe someone else does and can give a
Reviewed-by: tag, but for my part I'll have to spend some time trying to understand
what each variable and its use now means. Overall this suggests that the code may now be
a bit fragile. If those two sets of variables are now indeed needed, I think it would help
to have a detailed explanation for each use. This would help reducing the probability
of errors if the code has to be touched again in the future.

>
> /* Used to export TA voltage and current */
> struct power_supply *psy;
> @@ -323,13 +326,27 @@ struct tcpm_port {
> struct pd_mode_data mode_data;
> struct typec_altmode *partner_altmode[ALTMODE_DISCOVERY_MAX];
> struct typec_altmode *port_altmode[ALTMODE_DISCOVERY_MAX];
> -
> /* Deadline in jiffies to exit src_try_wait state */
> unsigned long max_wait;
>
> /* port belongs to a self powered device */
> bool self_powered;
>
> + /*
> + * Honour psnkstdby after accept is received.
> + * However, in this case it has to be made sure that the tSnkStdby (15
> + * msec) is met.
> + *
> + * When not set, port requests PD_P_SNK_STDBY_5V upon entering SNK_DISCOVERY and
> + * the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
> + * SNK_READY for non-pd link.
> + *
> + * When set, port requests CC advertisement based current limit during
> + * SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY_5V during SNK_TRANSITION_SINK,
> + * PD based current limit gets set after RX of PD_CTRL_PSRDY.
> + */
> + bool psnkstdby_after_accept;
> +
> #ifdef CONFIG_DEBUG_FS
> struct dentry *dentry;
> struct mutex logbuffer_lock; /* log buffer access lock */
> @@ -1787,9 +1804,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
> switch (port->state) {
> case SNK_TRANSITION_SINK:
> if (port->vbus_present) {
> - tcpm_set_current_limit(port,
> - port->current_limit,
> - port->supply_voltage);
> + tcpm_set_current_limit(port, port->req_current_limit,
> + port->req_supply_voltage);
> port->explicit_contract = true;
> tcpm_set_state(port, SNK_READY, 0);
> } else {
> @@ -1861,8 +1877,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
> break;
> case SNK_NEGOTIATE_PPS_CAPABILITIES:
> port->pps_data.active = true;
> - port->supply_voltage = port->pps_data.out_volt;
> - port->current_limit = port->pps_data.op_curr;
> + port->req_supply_voltage = port->pps_data.out_volt;
> + port->req_current_limit = port->pps_data.op_curr;
> tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> break;
> case SOFT_RESET_SEND:
> @@ -2463,8 +2479,8 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
> flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> }
>
> - port->current_limit = ma;
> - port->supply_voltage = mv;
> + port->req_current_limit = ma;
> + port->req_supply_voltage = mv;
>
> return 0;
> }
> @@ -3235,9 +3251,11 @@ static void run_state_machine(struct tcpm_port *port)
> break;
> case SNK_DISCOVERY:
> if (port->vbus_present) {
> - tcpm_set_current_limit(port,
> - tcpm_get_current_limit(port),
> - 5000);
> + if (port->psnkstdby_after_accept || tcpm_get_current_limit(port) <=
> + PD_P_SNK_STDBY_5V)
> + tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
> + else
> + tcpm_set_current_limit(port, PD_P_SNK_STDBY_5V, 5000);
> tcpm_set_charge(port, true);
> tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
> break;
> @@ -3318,6 +3336,10 @@ static void run_state_machine(struct tcpm_port *port)
> }
> break;
> case SNK_TRANSITION_SINK:
> + if (port->psnkstdby_after_accept)
> + tcpm_set_current_limit(port, tcpm_get_current_limit(port) >
> + PD_P_SNK_STDBY_5V ? PD_P_SNK_STDBY_5V :
> + tcpm_get_current_limit(port), 5000);
> case SNK_TRANSITION_SINK_VBUS:
> tcpm_set_state(port, hard_reset_state(port),
> PD_T_PS_TRANSITION);
> @@ -3331,6 +3353,10 @@ static void run_state_machine(struct tcpm_port *port)
> port->pwr_opmode = TYPEC_PWR_MODE_PD;
> }
>
> + /* Set current limit for NON-PD link when psnkstdby_after_accept is not set*/
> + if (!port->pd_capable && !port->psnkstdby_after_accept)
> + tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
> +
> tcpm_swap_complete(port, 0);
> tcpm_typec_connect(port);
> tcpm_check_send_discover(port);
> @@ -4513,6 +4539,8 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
> port->typec_caps.type = ret;
> port->port_type = port->typec_caps.type;
>
> + port->psnkstdby_after_accept = fwnode_property_read_bool(fwnode, "psnkstdby-after-accept");
> +

I think this will need to be documented.

Guenter

> if (port->port_type == TYPEC_PORT_SNK)
> goto sink;
>
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index b6c233e79bd4..6bd70f77045e 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -483,5 +483,8 @@ static inline unsigned int rdo_max_power(u32 rdo)
> #define PD_N_CAPS_COUNT (PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
> #define PD_N_HARD_RESET_COUNT 2
>
> -#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
> +#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
> +
> +#define PD_P_SNK_STDBY_5V 500 /* 2500 mw - 500mA @ 5V */
> +
> #endif /* __LINUX_USB_PD_H */
>

2020-08-05 00:21:38

by Badhri Jagan Sridharan

[permalink] [raw]
Subject: Re: [PATCH v1] tcpm: Honour pSnkStdby requirement during negotiation

Hi Guenter,

Your concern is very valid ! I was myself pondering at the
significance and meaning of
current_limit, supply_voltage. I will pen down what I understood from
reading the code.
If it makes sense, I will add the documentation to the change as well.
In TOT code i.e. without the patch in discussion, current_limit and
supply_voltage gets
in three places.
1. tcpm_set_current_limit --------------> Actual current_limit
/voltage requested to be set
in the hardware.
2. tcpm_pd_build_request --------------> current_limit /voltage
requested to the port partner.
Not yet
accepted by port partner
3. case PD_CTRL_ACCEPT: ----------> PPS path: new
current_limit/voltage to be set in hw &
already
accepted by the port partner.
switch (port->state) {
case SNK_NEGOTIATE_CAPABILITIES:
port->pps_data.active = false;
tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
break;
case SNK_NEGOTIATE_PPS_CAPABILITIES:
port->pps_data.active = true;
port->supply_voltage = port->pps_data.out_volt;
port->current_limit = port->pps_data.op_curr;
tcpm_set_state(port, SNK_TRANSITION_SINK, 0);

PPS path safely differentiates between new current_limit &
supply_voltage requested
to the port-partner and current_limit & supply_voltage requested to
be set in the
hardware by caching the new requested values to partner in
port->pps_data.op_curr/_volt (in tcpm_pd_select_pps_apdo) and updates
port->supply_voltage, port->current_limit only when the port-partner responds
with an accept packet (as pointed in the above code stub).

However, when NOT IN PPS, TCPM code updates port->current_limit &
port->supply_voltage directly in tcpm_pd_build_request where the partner can
still REJECT the request and port->current_limit & port->supply_voltage
might go out of sync. So, there is already a need to introduce new variables to
cache the requested values to port partners.

Now coming to addressing pSnkStdby, Once the partner accepts the new request,
PD spec wants the sink to limit the power consumption to pSnkStdby (2.5W) till
source sends the PS_RDY. i.e. while TCPM waits in SNK_TRANSITION_SINK,
the sink's power consumption has to be limited to 2.5W. Sink can enforce or draw
new partner accepted current & voltage at the hardware level only
after PS_RDY is received from the port-partner.

With the patch that I am submitting, I am hoping to cleanly
differentiate between the
current limit/voltage requested to be set in hardware(
tcpm_set_current_limit) Vs
the current limit/voltage requested to the port partner.

current_limit, supply_voltage -> Requested to be set in hardware.
req_current_limit, req_supply_voltage -> Newly requested value to the
port partner.

Perhaps doing the following alleviates current_limit/supply_voltage ambiguity:
1. renaming current_limit, supply_voltage to hw_current_limit, hw_supply_voltage
2. making a comment saying hw_current_limit, hw_supply_voltage should only
be set in tcpm_set_current_limit.
3. Instead of calling requested values to port-partner as
req_current_limit/req_supply_voltage, maybe partner_req_current_limit,
partner_req_supply_voltage is a better name.
4. Adding a comment to state significance of hw_current_limit,
hw_supply_voltage,
partner_req_current_limit, partner_req_supply_voltage in tcpm struct.

Let me know if it makes sense I will update the patch (may be even
split if it makes
sense) and resend.

Thanks,
Badhri




On Tue, Aug 4, 2020 at 9:59 AM Guenter Roeck <[email protected]> wrote:
>
> On 8/3/20 11:51 PM, Badhri Jagan Sridharan wrote:
> >>From PD Spec:
> > The Sink Shall transition to Sink Standby before a positive or
> > negative voltage transition of VBUS. During Sink Standby
> > the Sink Shall reduce its power draw to pSnkStdby. This allows
> > the Source to manage the voltage transition as well as
> > supply sufficient operating current to the Sink to maintain PD
> > operation during the transition. The Sink Shall
> > complete this transition to Sink Standby within tSnkStdby
> > after evaluating the Accept Message from the Source. The
> > transition when returning to Sink operation from Sink Standby
> > Shall be completed within tSnkNewPower. The
> > pSnkStdby requirement Shall only apply if the Sink power draw
> > is higher than this level.
> >
> > The above requirement needs to be met to prevent hard resets
> > from port partner.
> >
> > Introducing psnkstdby_after_accept flag to accommodate systems
> > where the input current limit loops are not fast enough to meet
> > tSnkStby(15 msec).
> >
> > When not set, port requests PD_P_SNK_STDBY upon entering SNK_DISCOVERY and
> > the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
> > SNK_READY for non-pd link.
> >
> > When set, port requests CC advertisement based current limit during
> > SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY during
> > SNK_TRANSITION_SINK, PD based current limit gets set after RX of
> > PD_CTRL_PSRDY. However, in this case it has to be made sure that the
> > tSnkStdby (15 msec) is met.
> >
> > Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> > ---
> > drivers/usb/typec/tcpm/tcpm.c | 52 +++++++++++++++++++++++++++--------
> > include/linux/usb/pd.h | 5 +++-
> > 2 files changed, 44 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 3ef37202ee37..e46da41940b9 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -293,9 +293,12 @@ struct tcpm_port {
> > unsigned int operating_snk_mw;
> > bool update_sink_caps;
> >
> > - /* Requested current / voltage */
> > + /* Set current / voltage */
> > u32 current_limit;
> > u32 supply_voltage;
> > + /* current / voltage requested to partner */
> > + u32 req_current_limit;
> > + u32 req_supply_voltage;
>
> I don't see a clear delineation where, when, and why supply_voltage vs. req_supply_voltage
> and current_limit vs. req_current_limit is used. Maybe someone else does and can give a
> Reviewed-by: tag, but for my part I'll have to spend some time trying to understand
> what each variable and its use now means. Overall this suggests that the code may now be
> a bit fragile. If those two sets of variables are now indeed needed, I think it would help
> to have a detailed explanation for each use. This would help reducing the probability
> of errors if the code has to be touched again in the future.
>
> >
> > /* Used to export TA voltage and current */
> > struct power_supply *psy;
> > @@ -323,13 +326,27 @@ struct tcpm_port {
> > struct pd_mode_data mode_data;
> > struct typec_altmode *partner_altmode[ALTMODE_DISCOVERY_MAX];
> > struct typec_altmode *port_altmode[ALTMODE_DISCOVERY_MAX];
> > -
> > /* Deadline in jiffies to exit src_try_wait state */
> > unsigned long max_wait;
> >
> > /* port belongs to a self powered device */
> > bool self_powered;
> >
> > + /*
> > + * Honour psnkstdby after accept is received.
> > + * However, in this case it has to be made sure that the tSnkStdby (15
> > + * msec) is met.
> > + *
> > + * When not set, port requests PD_P_SNK_STDBY_5V upon entering SNK_DISCOVERY and
> > + * the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
> > + * SNK_READY for non-pd link.
> > + *
> > + * When set, port requests CC advertisement based current limit during
> > + * SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY_5V during SNK_TRANSITION_SINK,
> > + * PD based current limit gets set after RX of PD_CTRL_PSRDY.
> > + */
> > + bool psnkstdby_after_accept;
> > +
> > #ifdef CONFIG_DEBUG_FS
> > struct dentry *dentry;
> > struct mutex logbuffer_lock; /* log buffer access lock */
> > @@ -1787,9 +1804,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
> > switch (port->state) {
> > case SNK_TRANSITION_SINK:
> > if (port->vbus_present) {
> > - tcpm_set_current_limit(port,
> > - port->current_limit,
> > - port->supply_voltage);
> > + tcpm_set_current_limit(port, port->req_current_limit,
> > + port->req_supply_voltage);
> > port->explicit_contract = true;
> > tcpm_set_state(port, SNK_READY, 0);
> > } else {
> > @@ -1861,8 +1877,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
> > break;
> > case SNK_NEGOTIATE_PPS_CAPABILITIES:
> > port->pps_data.active = true;
> > - port->supply_voltage = port->pps_data.out_volt;
> > - port->current_limit = port->pps_data.op_curr;
> > + port->req_supply_voltage = port->pps_data.out_volt;
> > + port->req_current_limit = port->pps_data.op_curr;
> > tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> > break;
> > case SOFT_RESET_SEND:
> > @@ -2463,8 +2479,8 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
> > flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> > }
> >
> > - port->current_limit = ma;
> > - port->supply_voltage = mv;
> > + port->req_current_limit = ma;
> > + port->req_supply_voltage = mv;
> >
> > return 0;
> > }
> > @@ -3235,9 +3251,11 @@ static void run_state_machine(struct tcpm_port *port)
> > break;
> > case SNK_DISCOVERY:
> > if (port->vbus_present) {
> > - tcpm_set_current_limit(port,
> > - tcpm_get_current_limit(port),
> > - 5000);
> > + if (port->psnkstdby_after_accept || tcpm_get_current_limit(port) <=
> > + PD_P_SNK_STDBY_5V)
> > + tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
> > + else
> > + tcpm_set_current_limit(port, PD_P_SNK_STDBY_5V, 5000);
> > tcpm_set_charge(port, true);
> > tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
> > break;
> > @@ -3318,6 +3336,10 @@ static void run_state_machine(struct tcpm_port *port)
> > }
> > break;
> > case SNK_TRANSITION_SINK:
> > + if (port->psnkstdby_after_accept)
> > + tcpm_set_current_limit(port, tcpm_get_current_limit(port) >
> > + PD_P_SNK_STDBY_5V ? PD_P_SNK_STDBY_5V :
> > + tcpm_get_current_limit(port), 5000);
> > case SNK_TRANSITION_SINK_VBUS:
> > tcpm_set_state(port, hard_reset_state(port),
> > PD_T_PS_TRANSITION);
> > @@ -3331,6 +3353,10 @@ static void run_state_machine(struct tcpm_port *port)
> > port->pwr_opmode = TYPEC_PWR_MODE_PD;
> > }
> >
> > + /* Set current limit for NON-PD link when psnkstdby_after_accept is not set*/
> > + if (!port->pd_capable && !port->psnkstdby_after_accept)
> > + tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
> > +
> > tcpm_swap_complete(port, 0);
> > tcpm_typec_connect(port);
> > tcpm_check_send_discover(port);
> > @@ -4513,6 +4539,8 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
> > port->typec_caps.type = ret;
> > port->port_type = port->typec_caps.type;
> >
> > + port->psnkstdby_after_accept = fwnode_property_read_bool(fwnode, "psnkstdby-after-accept");
> > +
>
> I think this will need to be documented.
>
> Guenter
>
> > if (port->port_type == TYPEC_PORT_SNK)
> > goto sink;
> >
> > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > index b6c233e79bd4..6bd70f77045e 100644
> > --- a/include/linux/usb/pd.h
> > +++ b/include/linux/usb/pd.h
> > @@ -483,5 +483,8 @@ static inline unsigned int rdo_max_power(u32 rdo)
> > #define PD_N_CAPS_COUNT (PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
> > #define PD_N_HARD_RESET_COUNT 2
> >
> > -#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
> > +#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
> > +
> > +#define PD_P_SNK_STDBY_5V 500 /* 2500 mw - 500mA @ 5V */
> > +
> > #endif /* __LINUX_USB_PD_H */
> >
>