Re: [PATCH 5/6] firmware: ti: ti_sci_clock: Add ABI5.0 compliant Clock APIs

["Kumar, Udit" <u-kumar1@ti.com>]

On 10/1/2026 1:11 AM, Beleswar Padhi wrote:
> The previous TI K2G and K3 architectures were tightly coupled systems in
> terms of Safety and Security. All the TI-SCI messages from HLOS (Linux)
> were sent to a single sec proxy thread terminating at the core running
> Safety service (DM). In K2G systems, DM handled both Safety & Security
> requests by itself. Whereas in K3 systems, DM serviced the Safety
> requests itself, and forwarded the Security requests to core running
> Security service (TIFS) on the HLOS's behalf. This posed various
> limitations:
>    a. Increased latency: An extra forwarding hop through DM adds latency
>       in requests and responses.
>    b. Freedom From Interference (FFI) concerns: Having Safety service
>       proxy Security's requests (& vice versa) undermines the isolation
>       and FFI of the system.
>    c. Potential Deadlock Scenario: Forwarding messages between Safety
>       and Security cores can result in a deadlock if there are circular
>       dependencies.
> 
> To eliminate all the above limitations, the TI TDA54 class of SoCs
> introduce an architectural change: Isolation of Security and Safety
> services. To realize this enhanced isolation in hardware, two
> independent sec proxy threads are now provided for HLOS to communicate
> separately with the Security and Safety cores respectively. And the
> TI-SCI message forwarding has been eliminated in Software. The System
> Firmware ABI5.0 version necessitates this improved isolation and FFI
> change.
> 

Till this point, all patches has repeated commit msg, I don't think you
need this repeat in all patches

> In ABI5.0, the message structures for set_clock_state, get_clock_state,
> query_clock_freq, set_clock_freq, and get_clock_freq APIs have been
> updated to combine the existing u8 clk_id and u32 clk_id_32 into a
> single u32 clk_id. This is a good cleanup that came as a by-product of
> the major ABI update. So, add new ABI5.0 compliant APIs to account for
> this change.
> 

Good clean up to get rid from two entries for clock id.

> Note: The following TI-SCI messages are not supported with ABI5.0, since
> parent clock configuration is done internally by PDM firmware:
> TI_SCI_MSG_SET_CLOCK_PARENT, TI_SCI_MSG_GET_CLOCK_PARENT, and
> TI_SCI_MSG_GET_NUM_CLOCK_PARENTS. Hence, the corresponding clock parent
> ops are left unset. Also, SSC support is not enabled in hardware on the
> TDA54 SoC, so the needs_ssc argument of get_clock is ignored.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
> Note:
> This patch is dependent on the following 2 series
> 1. https://lore.kernel.org/all/20260930172948.2901336-1-b-padhi@ti.com/
> 2. https://lore.kernel.org/all/20260930160607.2674980-1-b-padhi@ti.com/
> 
>  drivers/firmware/ti/ti_sci.h       | 138 +++++++++++
>  drivers/firmware/ti/ti_sci_clock.c | 380 +++++++++++++++++++++++++++++
>  drivers/firmware/ti/ti_sci_core.c  |   2 +
>  3 files changed, 520 insertions(+)
> 
> diff --git a/drivers/firmware/ti/ti_sci.h b/drivers/firmware/ti/ti_sci.h
> index 2f633aeab70b1..5e0a35b394d19 100644
> --- a/drivers/firmware/ti/ti_sci.h
> +++ b/drivers/firmware/ti/ti_sci.h
> @@ -483,6 +483,34 @@ struct ti_sci_msg_req_set_clock_state {
>  	u32 clk_id_32;
>  } __packed;
>  
> +/**
> + * struct ti_sci_msg_req_set_clock_state_abi5_pdm - Request to setup a Clock state
> + * @hdr:	Generic Header, Certain flags can be set specific to the clocks.
> + *		Uses the MSG_FLAG_CLOCK_* flags defined for
> + *		&struct ti_sci_msg_req_set_clock_state.
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *		Each device has it's own set of clock inputs. This indexes
> + *		which clock input to modify.
> + * @request_state: Request the state for the clock to be set to.
> + *		Uses the MSG_CLOCK_SW_STATE_* values defined for
> + *		&struct ti_sci_msg_req_set_clock_state.
> + *
> + * Normally, all required clocks are managed by TISCI entity, this is used
> + * only for specific control *IF* required. Auto managed state is
> + * MSG_CLOCK_SW_STATE_AUTO, in other states, TISCI entity assume remote
> + * will explicitly control.
> + *
> + * Request type is TI_SCI_MSG_SET_CLOCK_STATE, response is a generic
> + * ACK or NACK message.
> + */
> +struct ti_sci_msg_req_set_clock_state_abi5_pdm {
> +	struct ti_sci_msg_hdr hdr;
> +	u32 dev_id;
> +	u32 clk_id;
> +	u8 request_state;
> +} __packed;
> +

while you are at this, can you think of getting rid of state and
encapsulate state with set-freq ie set-freq 0 means disabled and any
positive number is enable

>  /**
>   * struct ti_sci_msg_req_get_clock_state - Request for clock state
>   * @hdr:	Generic Header
> @@ -505,6 +533,23 @@ struct ti_sci_msg_req_get_clock_state {
>  	u32 clk_id_32;
>  } __packed;
>  
> +/**
> + * struct ti_sci_msg_req_get_clock_state_abi5_pdm - Request for clock state
> + * @hdr:	Generic Header
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *		Each device has it's own set of clock inputs. This indexes
> + *		which clock input to get state of.
> + *
> + * Request type is TI_SCI_MSG_GET_CLOCK_STATE, response is state
> + * of the clock
> + */
> +struct ti_sci_msg_req_get_clock_state_abi5_pdm {
> +	struct ti_sci_msg_hdr hdr;
> +	u32 dev_id;
> +	u32 clk_id;
> +} __packed;
> +
>  /**
>   * struct ti_sci_msg_resp_get_clock_state - Response to get clock state
>   * @hdr:	Generic Header
> @@ -659,6 +704,37 @@ struct ti_sci_msg_req_query_clock_freq {
>  	u32 clk_id_32;
>  } __packed;
>  
> +/**
> + * struct ti_sci_msg_req_query_clock_freq_abi5_pdm - Request to query a frequency
> + * @hdr:	Generic Header
> + * @dev_id:	Device identifier this request is for
> + * @min_freq_hz: The minimum allowable frequency in Hz. This is the minimum
> + *		allowable programmed frequency and does not account for clock
> + *		tolerances and jitter.
> + * @target_freq_hz: The target clock frequency. A frequency will be found
> + *		as close to this target frequency as possible.
> + * @max_freq_hz: The maximum allowable frequency in Hz. This is the maximum
> + *		allowable programmed frequency and does not account for clock
> + *		tolerances and jitter.
> + * @clk_id:	Clock identifier for the device for this request.
> + *
> + * NOTE: Normally clock frequency management is automatically done by TISCI
> + * entity. In case of specific requests, TISCI evaluates capability to achieve
> + * requested frequency within provided range and responds with
> + * result message.
> + *
> + * Request type is TI_SCI_MSG_QUERY_CLOCK_FREQ, response is appropriate message,
> + * or NACK in case of inability to satisfy request.
> + */
> +struct ti_sci_msg_req_query_clock_freq_abi5_pdm {
> +	struct ti_sci_msg_hdr hdr;
> +	u32 dev_id;
> +	u64 min_freq_hz;
> +	u64 target_freq_hz;
> +	u64 max_freq_hz;
> +	u32 clk_id;
> +} __packed;
> +
>  /**
>   * struct ti_sci_msg_resp_query_clock_freq - Response to a clock frequency query
>   * @hdr:	Generic Header
> @@ -718,6 +794,49 @@ struct ti_sci_msg_req_set_clock_freq {
>  	u32 clk_id_32;
>  } __packed;
>  
> +/**
> + * struct ti_sci_msg_req_set_clock_freq_abi5_pdm - Request to setup a clock frequency
> + * @hdr:	Generic Header
> + * @dev_id:	Device identifier this request is for
> + * @min_freq_hz: The minimum allowable frequency in Hz. This is the minimum
> + *		allowable programmed frequency and does not account for clock
> + *		tolerances and jitter.
> + * @target_freq_hz: The target clock frequency. The clock will be programmed
> + *		at a rate as close to this target frequency as possible.
> + * @max_freq_hz: The maximum allowable frequency in Hz. This is the maximum
> + *		allowable programmed frequency and does not account for clock
> + *		tolerances and jitter.
> + * @clk_id:	Clock identifier for the device for this request.
> + *
> + * NOTE: Normally clock frequency management is automatically done by TISCI
> + * entity. In case of specific requests, TISCI evaluates capability to achieve
> + * requested range and responds with success/failure message.
> + *
> + * This sets the desired frequency for a clock within an allowable
> + * range. This message will fail on an enabled clock unless
> + * MSG_FLAG_CLOCK_ALLOW_FREQ_CHANGE is set for the clock. Additionally,
> + * if other clocks have their frequency modified due to this message,
> + * they also must have the MSG_FLAG_CLOCK_ALLOW_FREQ_CHANGE or be disabled.
> + *
> + * Calling set frequency on a clock input to the SoC pseudo-device will
> + * inform the PMMC of that clock's frequency. Setting a frequency of
> + * zero will indicate the clock is disabled.
> + *
> + * Calling set frequency on clock outputs from the SoC pseudo-device will
> + * function similarly to setting the clock frequency on a device.
> + *
> + * Request type is TI_SCI_MSG_SET_CLOCK_FREQ, response is a generic ACK/NACK
> + * message.
> + */
> +struct ti_sci_msg_req_set_clock_freq_abi5_pdm {
> +	struct ti_sci_msg_hdr hdr;
> +	u32 dev_id;
> +	u64 min_freq_hz;
> +	u64 target_freq_hz;
> +	u64 max_freq_hz;
> +	u32 clk_id;
> +} __packed;

ti_sci_msg_req_query_clock_freq_abi5_pdm  is same as of above
could you combine these two ?

> +
>  /**
>   * struct ti_sci_msg_req_get_clock_freq - Request to get the clock frequency
>   * @hdr:	Generic Header
> @@ -739,6 +858,24 @@ struct ti_sci_msg_req_get_clock_freq {
>  	u32 clk_id_32;
>  } __packed;
>  
> +/**
> + * struct ti_sci_msg_req_get_clock_freq_abi5_pdm - Request to get the clock frequency
> + * @hdr:	Generic Header
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *
> + * NOTE: Normally clock frequency management is automatically done by TISCI
> + * entity. In some cases, clock frequencies are configured by host.
> + *
> + * Request type is TI_SCI_MSG_GET_CLOCK_FREQ, responded with clock frequency
> + * that the clock is currently at.
> + */
> +struct ti_sci_msg_req_get_clock_freq_abi5_pdm {
> +	struct ti_sci_msg_hdr hdr;
> +	u32 dev_id;
> +	u32 clk_id;
> +} __packed;
> +
>  /**
>   * struct ti_sci_msg_resp_get_clock_freq - Response of clock frequency request
>   * @hdr:	Generic Header
> @@ -1738,6 +1875,7 @@ int ti_sci_do_xfer(const struct ti_sci_handle *handle,
>  void ti_sci_setup_device_ops(struct ti_sci_dev_ops *dops);
>  void ti_sci_setup_device_ops_abi5(struct ti_sci_dev_ops *dops);
>  void ti_sci_setup_clock_ops(struct ti_sci_clk_ops *cops);
> +void ti_sci_setup_clock_ops_abi5(struct ti_sci_clk_ops *cops);
>  void ti_sci_setup_pm_ops(struct ti_sci_pm_ops *pmops);
>  void ti_sci_setup_rm_core_ops(struct ti_sci_rm_core_ops *rm_core_ops);
>  void ti_sci_setup_rm_irq_ops(struct ti_sci_rm_irq_ops *iops);
> diff --git a/drivers/firmware/ti/ti_sci_clock.c b/drivers/firmware/ti/ti_sci_clock.c
> index f266522fb7d08..faf37cc6df8d2 100644
> --- a/drivers/firmware/ti/ti_sci_clock.c
> +++ b/drivers/firmware/ti/ti_sci_clock.c
> @@ -50,6 +50,40 @@ static int ti_sci_set_clock_state(const struct ti_sci_handle *handle,
>  	return 0;
>  }
>  
> +/**
> + * ti_sci_set_clock_state_abi5_pdm() - Set clock state helper
> + * @handle:	pointer to TI SCI handle
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *		Each device has it's own set of clock inputs. This indexes
> + *		which clock input to modify.
> + * @flags:	Header flags as needed
> + * @state:	State to request for the clock.
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_set_clock_state_abi5_pdm(const struct ti_sci_handle *handle,
> +					   u32 dev_id, u32 clk_id,
> +					   u32 flags, u8 state)
> +{
> +	struct ti_sci_msg_req_set_clock_state_abi5_pdm req;
> +	struct ti_sci_msg_hdr resp;
> +	int ret = 0;
> +
> +	req.dev_id = dev_id;
> +	req.clk_id = clk_id;
> +	req.request_state = state;
> +
> +	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_SET_CLOCK_STATE,
> +			     flags | TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> +			     &req, sizeof(req),
> +			     &resp, sizeof(resp));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  /**
>   * ti_sci_cmd_get_clock_state() - Get clock state helper
>   * @handle:	pointer to TI SCI handle
> @@ -96,6 +130,47 @@ static int ti_sci_cmd_get_clock_state(const struct ti_sci_handle *handle,
>  	return 0;
>  }
>  
> +/**
> + * ti_sci_cmd_get_clock_state_abi5_pdm() - Get clock state helper
> + * @handle:	pointer to TI SCI handle
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *		Each device has it's own set of clock inputs. This indexes
> + *		which clock input to modify.
> + * @programmed_state:	State requested for clock to move to
> + * @current_state:	State that the clock is currently in
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_get_clock_state_abi5_pdm(const struct ti_sci_handle *handle,
> +					       u32 dev_id, u32 clk_id,
> +					       u8 *programmed_state, u8 *current_state)
> +{
> +	struct ti_sci_msg_req_get_clock_state_abi5_pdm req;
> +	struct ti_sci_msg_resp_get_clock_state resp;
> +	int ret = 0;
> +
> +	if (!programmed_state && !current_state)
> +		return -EINVAL;
> +
> +	req.dev_id = dev_id;
> +	req.clk_id = clk_id;
> +
> +	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_GET_CLOCK_STATE,
> +			     TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> +			     &req, sizeof(req),
> +			     &resp, sizeof(resp));
> +	if (ret)
> +		return ret;
> +
> +	if (programmed_state)
> +		*programmed_state = resp.programmed_state;
> +	if (current_state)
> +		*current_state = resp.current_state;

Isn't this a bug, if programed state is not equal to current_state for
clock ?

> +
> +	return 0;
> +}
> +
>  /**
>   * ti_sci_cmd_get_clock() - Get control of a clock from TI SCI
>   * @handle:	pointer to TI SCI handle
> @@ -123,6 +198,32 @@ static int ti_sci_cmd_get_clock(const struct ti_sci_handle *handle, u32 dev_id,
>  				      MSG_CLOCK_SW_STATE_REQ);
>  }
>  
> +/**
> + * ti_sci_cmd_get_clock_abi5_pdm() - Get control of a clock from TI SCI
> + * @handle:	pointer to TI SCI handle
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *		Each device has it's own set of clock inputs. This indexes
> + *		which clock input to modify.
> + * @needs_ssc:	unused, SSC is not supported with ABI5
> + * @can_change_freq: 'true' if frequency change is desired, else 'false'
> + * @enable_input_term: 'true' if input termination is desired, else 'false'
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_get_clock_abi5_pdm(const struct ti_sci_handle *handle, u32 dev_id,
> +					 u32 clk_id, bool needs_ssc,
> +					 bool can_change_freq, bool enable_input_term)
> +{
> +	u32 flags = 0;
> +
> +	flags |= can_change_freq ? MSG_FLAG_CLOCK_ALLOW_FREQ_CHANGE : 0;

get is changing freq, why ?

> +	flags |= enable_input_term ? MSG_FLAG_CLOCK_INPUT_TERM : 0;
> +
> +	return ti_sci_set_clock_state_abi5_pdm(handle, dev_id, clk_id, flags,
> +					       MSG_CLOCK_SW_STATE_REQ);
> +}
> +
>  /**
>   * ti_sci_cmd_idle_clock() - Idle a clock which is in our control
>   * @handle:	pointer to TI SCI handle
> @@ -143,6 +244,26 @@ static int ti_sci_cmd_idle_clock(const struct ti_sci_handle *handle,
>  				      MSG_CLOCK_SW_STATE_UNREQ);
>  }
>  
> +/**
> + * ti_sci_cmd_idle_clock_abi5_pdm() - Idle a clock which is in our control

Idle means what, you are leaving owner ship and keeping state as is ?
or something else, not clear from above comments

> + * @handle:	pointer to TI SCI handle
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *		Each device has it's own set of clock inputs. This indexes
> + *		which clock input to modify.
> + *
> + * NOTE: This clock must have been requested by get_clock previously.
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_idle_clock_abi5_pdm(const struct ti_sci_handle *handle,
> +					  u32 dev_id, u32 clk_id)
> +{
> +	return ti_sci_set_clock_state_abi5_pdm(handle, dev_id, clk_id,
> +					       MSG_FLAG_CLOCK_ALLOW_FREQ_CHANGE,
> +					       MSG_CLOCK_SW_STATE_UNREQ);
> +}
> +
>  /**
>   * ti_sci_cmd_put_clock() - Release a clock from our control back to TISCI
>   * @handle:	pointer to TI SCI handle
> @@ -163,6 +284,26 @@ static int ti_sci_cmd_put_clock(const struct ti_sci_handle *handle,
>  				      MSG_CLOCK_SW_STATE_AUTO);
>  }
>  
> +/**
> + * ti_sci_cmd_put_clock_abi5_pdm() - Release a clock from our control back to TISCI
> + * @handle:	pointer to TI SCI handle
> + * @dev_id:	Device identifier this request is for
> + * @clk_id:	Clock identifier for the device for this request.
> + *		Each device has it's own set of clock inputs. This indexes
> + *		which clock input to modify.
> + *
> + * NOTE: This clock must have been requested by get_clock previously.
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_put_clock_abi5_pdm(const struct ti_sci_handle *handle,
> +					 u32 dev_id, u32 clk_id)
> +{
> +	return ti_sci_set_clock_state_abi5_pdm(handle, dev_id, clk_id,
> +					       MSG_FLAG_CLOCK_ALLOW_FREQ_CHANGE,
> +					       MSG_CLOCK_SW_STATE_AUTO);

put clock is , making state is disable , any reason AUTO is selected here

> +}
> +
>  /**
>   * ti_sci_cmd_clk_is_auto() - Is the clock being auto managed
>   * @handle:	pointer to TI SCI handle
> @@ -191,6 +332,34 @@ static int ti_sci_cmd_clk_is_auto(const struct ti_sci_handle *handle,
>  	return 0;
>  }
>  
> [..]