From: Amit Sunil Dhamne <amitsd@google.com>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: gregkh@linuxfoundation.org, robh@kernel.org,
dmitry.baryshkov@linaro.org, badhri@google.com,
kyletso@google.com, rdbabiera@google.com,
linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Re: [RFC v2 2/2] usb: typec: tcpm: Add support for parsing time dt properties
Date: Thu, 19 Sep 2024 15:15:03 -0700 [thread overview]
Message-ID: <724c1598-d41b-4795-b313-4fb73c6a24e0@google.com> (raw)
In-Reply-To: <Zuwn1Fn1DrLGvPK9@kuha.fi.intel.com>
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@google.com>
>> ---
>> 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,
>
prev parent reply other threads:[~2024-09-19 22:15 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-19 7:51 [RFC v2 0/2] Add support for time DT property in TCPM Amit Sunil Dhamne
2024-09-19 7:51 ` [RFC v2 1/2] dt-bindings: connector: Add properties to define time values Amit Sunil Dhamne
2024-09-21 0:56 ` Rob Herring
2024-09-19 7:51 ` [RFC v2 2/2] usb: typec: tcpm: Add support for parsing time dt properties Amit Sunil Dhamne
2024-09-19 8:56 ` Dmitry Baryshkov
2024-09-19 22:08 ` Amit Sunil Dhamne
2024-09-19 13:32 ` Heikki Krogerus
2024-09-19 22:15 ` Amit Sunil Dhamne [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=724c1598-d41b-4795-b313-4fb73c6a24e0@google.com \
--to=amitsd@google.com \
--cc=badhri@google.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=kyletso@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=rdbabiera@google.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®