Re: [RFC v2 2/2] usb: typec: tcpm: Add support for parsing time dt properties

[Amit Sunil Dhamne <amitsd@xxxxxxxxxx>]

Hi Heikki,

On 9/19/24 6:32 AM, Heikki Krogerus wrote:
> On Thu, Sep 19, 2024 at 12:51:14AM -0700, Amit Sunil Dhamne wrote:
> > Add support for DT time properties to allow users to define platform
> > specific timing deadlines of certain timers rather than using hardcoded
> > ones. For values that have not been explicitly defined in DT using this
> > property, default values will be set therefore, making this change
> > backward compatible.
> >
> > Signed-off-by: Amit Sunil Dhamne <amitsd@xxxxxxxxxx>
> > ---
> >   drivers/usb/typec/tcpm/tcpm.c | 81 ++++++++++++++++++++++++++++-------
> >   1 file changed, 65 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 4b02d6474259..e6c243bc44f7 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -310,6 +310,17 @@ struct pd_data {
> >   	unsigned int operating_snk_mw;
> >   };
> >   
> > +/*
> > + * @sink_wait_cap_time: Deadline (in ms) for tTypeCSinkWaitCap timer
> > + * @ps_src_wait_off_time: Deadline (in ms) for tPSSourceOff timer
> > + * @cc_debounce_time: Deadline (in ms) for tCCDebounce timer
> > + */
> > +struct pd_timings {
> > +	u32 sink_wait_cap_time;
> > +	u32 ps_src_off_time;
> > +	u32 cc_debounce_time;
> > +};
> > +
> >   struct tcpm_port {
> >   	struct device *dev;
> >   
> > @@ -552,6 +563,9 @@ struct tcpm_port {
> >   	 */
> >   	unsigned int message_id_prime;
> >   	unsigned int rx_msgid_prime;
> > +
> > +	/* Timer deadline values configured at runtime */
> > +	struct pd_timings timings;
> >   #ifdef CONFIG_DEBUG_FS
> >   	struct dentry *dentry;
> >   	struct mutex logbuffer_lock;	/* log buffer access lock */
> > @@ -4639,15 +4653,15 @@ static void run_state_machine(struct tcpm_port *port)
> >   	case SRC_ATTACH_WAIT:
> >   		if (tcpm_port_is_debug(port))
> >   			tcpm_set_state(port, DEBUG_ACC_ATTACHED,
> > -				       PD_T_CC_DEBOUNCE);
> > +				       port->timings.cc_debounce_time);
> >   		else if (tcpm_port_is_audio(port))
> >   			tcpm_set_state(port, AUDIO_ACC_ATTACHED,
> > -				       PD_T_CC_DEBOUNCE);
> > +				       port->timings.cc_debounce_time);
> >   		else if (tcpm_port_is_source(port) && port->vbus_vsafe0v)
> >   			tcpm_set_state(port,
> >   				       tcpm_try_snk(port) ? SNK_TRY
> >   							  : SRC_ATTACHED,
> > -				       PD_T_CC_DEBOUNCE);
> > +				       port->timings.cc_debounce_time);
> >   		break;
> >   
> >   	case SNK_TRY:
> > @@ -4698,7 +4712,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   		}
> >   		break;
> >   	case SRC_TRYWAIT_DEBOUNCE:
> > -		tcpm_set_state(port, SRC_ATTACHED, PD_T_CC_DEBOUNCE);
> > +		tcpm_set_state(port, SRC_ATTACHED, port->timings.cc_debounce_time);
> >   		break;
> >   	case SRC_TRYWAIT_UNATTACHED:
> >   		tcpm_set_state(port, SNK_UNATTACHED, 0);
> > @@ -4901,7 +4915,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   		    (port->cc1 != TYPEC_CC_OPEN &&
> >   		     port->cc2 == TYPEC_CC_OPEN))
> >   			tcpm_set_state(port, SNK_DEBOUNCED,
> > -				       PD_T_CC_DEBOUNCE);
> > +				       port->timings.cc_debounce_time);
> >   		else if (tcpm_port_is_disconnected(port))
> >   			tcpm_set_state(port, SNK_UNATTACHED,
> >   				       PD_T_PD_DEBOUNCE);
> > @@ -4941,7 +4955,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   		break;
> >   	case SNK_TRYWAIT:
> >   		tcpm_set_cc(port, TYPEC_CC_RD);
> > -		tcpm_set_state(port, SNK_TRYWAIT_VBUS, PD_T_CC_DEBOUNCE);
> > +		tcpm_set_state(port, SNK_TRYWAIT_VBUS, port->timings.cc_debounce_time);
> >   		break;
> >   	case SNK_TRYWAIT_VBUS:
> >   		/*
> > @@ -5014,7 +5028,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   		break;
> >   	case SNK_DISCOVERY_DEBOUNCE:
> >   		tcpm_set_state(port, SNK_DISCOVERY_DEBOUNCE_DONE,
> > -			       PD_T_CC_DEBOUNCE);
> > +			       port->timings.cc_debounce_time);
> >   		break;
> >   	case SNK_DISCOVERY_DEBOUNCE_DONE:
> >   		if (!tcpm_port_is_disconnected(port) &&
> > @@ -5041,10 +5055,10 @@ static void run_state_machine(struct tcpm_port *port)
> >   		if (port->vbus_never_low) {
> >   			port->vbus_never_low = false;
> >   			tcpm_set_state(port, SNK_SOFT_RESET,
> > -				       PD_T_SINK_WAIT_CAP);
> > +				       port->timings.sink_wait_cap_time);
> >   		} else {
> >   			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
> > -				       PD_T_SINK_WAIT_CAP);
> > +				       port->timings.sink_wait_cap_time);
> >   		}
> >   		break;
> >   	case SNK_WAIT_CAPABILITIES_TIMEOUT:
> > @@ -5066,7 +5080,8 @@ static void run_state_machine(struct tcpm_port *port)
> >   		if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
> >   			tcpm_set_state_cond(port, hard_reset_state(port), 0);
> >   		else
> > -			tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
> > +			tcpm_set_state(port, hard_reset_state(port),
> > +				       port->timings.sink_wait_cap_time);
> >   		break;
> >   	case SNK_NEGOTIATE_CAPABILITIES:
> >   		port->pd_capable = true;
> > @@ -5203,7 +5218,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   			tcpm_set_state(port, ACC_UNATTACHED, 0);
> >   		break;
> >   	case AUDIO_ACC_DEBOUNCE:
> > -		tcpm_set_state(port, ACC_UNATTACHED, PD_T_CC_DEBOUNCE);
> > +		tcpm_set_state(port, ACC_UNATTACHED, port->timings.cc_debounce_time);
> >   		break;
> >   
> >   	/* Hard_Reset states */
> > @@ -5420,7 +5435,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   		tcpm_set_state(port, ERROR_RECOVERY, 0);
> >   		break;
> >   	case FR_SWAP_SNK_SRC_TRANSITION_TO_OFF:
> > -		tcpm_set_state(port, ERROR_RECOVERY, PD_T_PS_SOURCE_OFF);
> > +		tcpm_set_state(port, ERROR_RECOVERY, port->timings.ps_src_off_time);
> >   		break;
> >   	case FR_SWAP_SNK_SRC_NEW_SINK_READY:
> >   		if (port->vbus_source)
> > @@ -5475,7 +5490,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   		tcpm_set_cc(port, TYPEC_CC_RD);
> >   		/* allow CC debounce */
> >   		tcpm_set_state(port, PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED,
> > -			       PD_T_CC_DEBOUNCE);
> > +			       port->timings.cc_debounce_time);
> >   		break;
> >   	case PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED:
> >   		/*
> > @@ -5510,7 +5525,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   						       port->pps_data.active, 0);
> >   		tcpm_set_charge(port, false);
> >   		tcpm_set_state(port, hard_reset_state(port),
> > -			       PD_T_PS_SOURCE_OFF);
> > +			       port->timings.ps_src_off_time);
> >   		break;
> >   	case PR_SWAP_SNK_SRC_SOURCE_ON:
> >   		tcpm_enable_auto_vbus_discharge(port, true);
> > @@ -5666,7 +5681,7 @@ static void run_state_machine(struct tcpm_port *port)
> >   	case PORT_RESET_WAIT_OFF:
> >   		tcpm_set_state(port,
> >   			       tcpm_default_state(port),
> > -			       port->vbus_present ? PD_T_PS_SOURCE_OFF : 0);
> > +			       port->vbus_present ? port->timings.ps_src_off_time : 0);
> >   		break;
> >   
> >   	/* AMS intermediate state */
> > @@ -6157,7 +6172,7 @@ static void _tcpm_pd_vbus_vsafe0v(struct tcpm_port *port)
> >   	case SRC_ATTACH_WAIT:
> >   		if (tcpm_port_is_source(port))
> >   			tcpm_set_state(port, tcpm_try_snk(port) ? SNK_TRY : SRC_ATTACHED,
> > -				       PD_T_CC_DEBOUNCE);
> > +				       port->timings.cc_debounce_time);
> >   		break;
> >   	case SRC_STARTUP:
> >   	case SRC_SEND_CAPABILITIES:
> > @@ -7053,6 +7068,35 @@ static int tcpm_port_register_pd(struct tcpm_port *port)
> >   	return ret;
> >   }
> >   
> > +static int tcpm_fw_get_timings(struct tcpm_port *port, struct fwnode_handle *fwnode)
> > +{
> > +	int ret;
> > +	u32 val;
> > +
> > +	if (!fwnode)
> > +		return -EINVAL;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "sink-wait-cap-time-ms", &val);
> > +	if (!ret)
> > +		port->timings.sink_wait_cap_time = val;
> > +	else
> > +		port->timings.sink_wait_cap_time = PD_T_SINK_WAIT_CAP;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "ps-source-off-time-ms", &val);
> > +	if (!ret)
> > +		port->timings.ps_src_off_time = val;
> > +	else
> > +		port->timings.ps_src_off_time = PD_T_PS_SOURCE_OFF;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "cc-debounce-time-ms", &val);
> > +	if (!ret)
> > +		port->timings.cc_debounce_time = val;
> > +	else
> > +		port->timings.cc_debounce_time = PD_T_CC_DEBOUNCE;
> > +
> > +	return 0;
> > +}
> > +
> >   static int tcpm_fw_get_caps(struct tcpm_port *port, struct fwnode_handle *fwnode)
> >   {
> >   	struct fwnode_handle *capabilities, *child, *caps = NULL;
> > @@ -7608,9 +7652,14 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
> >   	init_completion(&port->pps_complete);
> >   	tcpm_debugfs_init(port);
> >   
> > +	err = tcpm_fw_get_timings(port, tcpc->fwnode);
> > +	if (err < 0)
> > +		goto out_destroy_wq;
> This is somehow wrong. You are using default values in case of
> failure, so this should not be a reason to fail port registration
> under any circumstance. That function should just return void.
>
> I would also just call it after tcpm_fw_get_caps() (or maybe even from
> tcpm_fw_get_caps()), because tcpm_fw_get_caps() checks fwnode in any
> case.

Thanks for the review! I will update in the next revision with this 
change. Will send the next revision once I get review
from DT maintainers for DT patch.


Regards,

Amit

> >   	err = tcpm_fw_get_caps(port, tcpc->fwnode);
> >   	if (err < 0)
> >   		goto out_destroy_wq;
> > +
> >   	err = tcpm_fw_get_snk_vdos(port, tcpc->fwnode);
> >   	if (err < 0)
> >   		goto out_destroy_wq;
> thanks,