Re: [PATCH v2 20/22] firmware: ti: ti_sci_pm: Move pm ops into its own file

[Kendall Willis]

Hi Beleswar,
On 21:36-20260930, Beleswar Padhi wrote:
> From: Andrew Davis <afd@xxxxxx>
> 
> Extract the power management operations related functions and services
> from ti_sci_core.c into its own file. This breaks down the single
> large file into smaller logical files for better readability and
> maintenance.
> 
> The pm ops need access to the TI SCI instance to get the device used for
> error reporting. Move struct ti_sci_xfer, struct ti_sci_xfers_info,
> struct ti_sci_desc, struct ti_sci_info and handle_to_ti_sci_info() from
> ti_sci_core.c into ti_sci.h, along with the includes they need, so that
> they can be shared with the split files. Upcoming splits also need
> access to the instance state, so this is done once here.
> 
> Signed-off-by: Andrew Davis <afd@xxxxxx>
> Co-developed-by: Beleswar Padhi <b-padhi@xxxxxx>
> Signed-off-by: Beleswar Padhi <b-padhi@xxxxxx>

Would it be possible to add the following LPM related functions to
ti_sci_pm.c?
  ti_sci_partial_io_wakeup_enabled
  ti_sci_prepare_system_suspend

> ---
> v2: Changelog:
> 1. None to this patch.
> 
> Link to v1:
> https://lore.kernel.org/all/20260929201746.4078803-21-b-padhi@xxxxxx/
> 
>  drivers/firmware/ti/Makefile      |   1 +
>  drivers/firmware/ti/ti_sci.h      | 100 ++++++++++++++
>  drivers/firmware/ti/ti_sci_core.c | 220 +-----------------------------
>  drivers/firmware/ti/ti_sci_pm.c   | 145 ++++++++++++++++++++
>  4 files changed, 247 insertions(+), 219 deletions(-)
>  create mode 100644 drivers/firmware/ti/ti_sci_pm.c
> 
> diff --git a/drivers/firmware/ti/Makefile b/drivers/firmware/ti/Makefile
> index c4233542ad57d..bbfe73587a3c7 100644
> --- a/drivers/firmware/ti/Makefile
> +++ b/drivers/firmware/ti/Makefile
> @@ -3,4 +3,5 @@
>  ti_sci-y			:= ti_sci_core.o
>  ti_sci-y			+= ti_sci_clock.o
>  ti_sci-y			+= ti_sci_device.o
> +ti_sci-y			+= ti_sci_pm.o
>  obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
> diff --git a/drivers/firmware/ti/ti_sci.h b/drivers/firmware/ti/ti_sci.h
> index cce59cac35ed6..7a707c632276e 100644
> --- a/drivers/firmware/ti/ti_sci.h
> +++ b/drivers/firmware/ti/ti_sci.h
> @@ -13,8 +13,107 @@
>  #define __TI_SCI_H
>  
>  #include <linux/compiler_types.h>
> +#include <linux/completion.h>
> +#include <linux/container_of.h>
> +#include <linux/hashtable.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
> +#include <linux/semaphore.h>
> +#include <linux/soc/ti/ti-msgmgr.h>
> +#include <linux/soc/ti/ti_sci_protocol.h>
> +#include <linux/spinlock.h>
>  #include <linux/types.h>
>  
> +/**
> + * struct ti_sci_xfer - Structure representing a message flow
> + * @tx_message:	Transmit message
> + * @rx_buf:	Pointer to store received message
> + * @rx_len:	Receive message length
> + * @done:	completion event
> + */
> +struct ti_sci_xfer {
> +	struct ti_msgmgr_message tx_message;
> +	void *rx_buf;
> +	u8 rx_len;
> +	struct completion done;
> +};
> +
> +/**
> + * struct ti_sci_xfers_info - Structure to manage transfer information
> + * @sem_xfer_count:	Counting Semaphore for managing max simultaneous
> + *			Messages.
> + * @xfer_block:		Preallocated Message array
> + * @xfer_alloc_table:	Bitmap table for allocated messages.
> + *			Index of this bitmap table is also used for message
> + *			sequence identifier.
> + * @xfer_lock:		Protection for message allocation
> + */
> +struct ti_sci_xfers_info {
> +	struct semaphore sem_xfer_count;
> +	struct ti_sci_xfer *xfer_block;
> +	unsigned long *xfer_alloc_table;
> +	/* protect transfer allocation */
> +	spinlock_t xfer_lock;
> +};
> +
> +/**
> + * struct ti_sci_desc - Description of SoC integration
> + * @default_host_id:	Host identifier representing the compute entity
> + * @max_rx_timeout_ms:	Timeout for communication with SoC (in Milliseconds)
> + * @max_msgs: Maximum number of messages that can be pending
> + *		  simultaneously in the system
> + * @max_msg_size: Maximum size of data per message that can be handled.
> + */
> +struct ti_sci_desc {
> +	u8 default_host_id;
> +	int max_rx_timeout_ms;
> +	int max_msgs;
> +	int max_msg_size;
> +};
> +
> +/**
> + * struct ti_sci_info - Structure representing a TI SCI instance
> + * @dev:	Device pointer
> + * @desc:	SoC description for this instance
> + * @d:		Debugfs file entry
> + * @debug_region: Memory region where the debug message are available
> + * @debug_region_size: Debug region size
> + * @debug_buffer: Buffer allocated to copy debug messages.
> + * @handle:	Instance of TI SCI handle to send to clients.
> + * @cl:		Mailbox Client
> + * @chan_tx:	Transmit mailbox channel
> + * @chan_rx:	Receive mailbox channel
> + * @minfo:	Message info
> + * @node:	list head
> + * @irqs:	List of allocated irqs
> + * @irq_lock:	Protection for irq hash list
> + * @host_id:	Host ID
> + * @fw_caps:	FW/SoC low power capabilities
> + * @users:	Number of users of this instance
> + */
> +struct ti_sci_info {
> +	struct device *dev;
> +	const struct ti_sci_desc *desc;
> +	struct dentry *d;
> +	void __iomem *debug_region;
> +	char *debug_buffer;
> +	size_t debug_region_size;
> +	struct ti_sci_handle handle;
> +	struct mbox_client cl;
> +	struct mbox_chan *chan_tx;
> +	struct mbox_chan *chan_rx;
> +	struct ti_sci_xfers_info minfo;
> +	struct list_head node;
> +	DECLARE_HASHTABLE(irqs, 8);
> +	struct mutex irq_lock;
> +	u8 host_id;
> +	u64 fw_caps;
> +	/* protected by ti_sci_list_mutex */
> +	int users;
> +};
> +
> +#define handle_to_ti_sci_info(h) container_of(h, struct ti_sci_info, handle)
> +
>  /* Generic Messages */
>  #define TI_SCI_MSG_ENABLE_WDT	0x0000
>  #define TI_SCI_MSG_WAKE_RESET	0x0001
> @@ -1567,5 +1666,6 @@ 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_clock_ops(struct ti_sci_clk_ops *cops);
> +void ti_sci_setup_pm_ops(struct ti_sci_pm_ops *pmops);
>  
>  #endif /* __TI_SCI_H */
> diff --git a/drivers/firmware/ti/ti_sci_core.c b/drivers/firmware/ti/ti_sci_core.c
> index 848c040f598e0..60d0d64ac4ae8 100644
> --- a/drivers/firmware/ti/ti_sci_core.c
> +++ b/drivers/firmware/ti/ti_sci_core.c
> @@ -13,11 +13,9 @@
>  #include <linux/cpu.h>
>  #include <linux/debugfs.h>
>  #include <linux/export.h>
> -#include <linux/hashtable.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> -#include <linux/mailbox_client.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
> @@ -25,10 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_qos.h>
>  #include <linux/property.h>
> -#include <linux/semaphore.h>
>  #include <linux/slab.h>
> -#include <linux/soc/ti/ti-msgmgr.h>
> -#include <linux/soc/ti/ti_sci_protocol.h>
>  #include <linux/suspend.h>
>  #include <linux/sys_soc.h>
>  #include <linux/reboot.h>
> @@ -40,53 +35,6 @@ static LIST_HEAD(ti_sci_list);
>  /* Protection for the entire list */
>  static DEFINE_MUTEX(ti_sci_list_mutex);
>  
> -/**
> - * struct ti_sci_xfer - Structure representing a message flow
> - * @tx_message:	Transmit message
> - * @rx_buf:	Pointer to store received message
> - * @rx_len:	Receive message length
> - * @done:	completion event
> - */
> -struct ti_sci_xfer {
> -	struct ti_msgmgr_message tx_message;
> -	void *rx_buf;
> -	u8 rx_len;
> -	struct completion done;
> -};
> -
> -/**
> - * struct ti_sci_xfers_info - Structure to manage transfer information
> - * @sem_xfer_count:	Counting Semaphore for managing max simultaneous
> - *			Messages.
> - * @xfer_block:		Preallocated Message array
> - * @xfer_alloc_table:	Bitmap table for allocated messages.
> - *			Index of this bitmap table is also used for message
> - *			sequence identifier.
> - * @xfer_lock:		Protection for message allocation
> - */
> -struct ti_sci_xfers_info {
> -	struct semaphore sem_xfer_count;
> -	struct ti_sci_xfer *xfer_block;
> -	unsigned long *xfer_alloc_table;
> -	/* protect transfer allocation */
> -	spinlock_t xfer_lock;
> -};
> -
> -/**
> - * struct ti_sci_desc - Description of SoC integration
> - * @default_host_id:	Host identifier representing the compute entity
> - * @max_rx_timeout_ms:	Timeout for communication with SoC (in Milliseconds)
> - * @max_msgs: Maximum number of messages that can be pending
> - *		  simultaneously in the system
> - * @max_msg_size: Maximum size of data per message that can be handled.
> - */
> -struct ti_sci_desc {
> -	u8 default_host_id;
> -	int max_rx_timeout_ms;
> -	int max_msgs;
> -	int max_msg_size;
> -};
> -
>  /**
>   * struct ti_sci_irq - Description of allocated irqs
>   * @node: Link to hash table
> @@ -97,49 +45,7 @@ struct ti_sci_irq {
>  	struct ti_sci_msg_req_manage_irq desc;
>  };
>  
> -/**
> - * struct ti_sci_info - Structure representing a TI SCI instance
> - * @dev:	Device pointer
> - * @desc:	SoC description for this instance
> - * @d:		Debugfs file entry
> - * @debug_region: Memory region where the debug message are available
> - * @debug_region_size: Debug region size
> - * @debug_buffer: Buffer allocated to copy debug messages.
> - * @handle:	Instance of TI SCI handle to send to clients.
> - * @cl:		Mailbox Client
> - * @chan_tx:	Transmit mailbox channel
> - * @chan_rx:	Receive mailbox channel
> - * @minfo:	Message info
> - * @node:	list head
> - * @irqs:	List of allocated irqs
> - * @irq_lock:	Protection for irq hash list
> - * @host_id:	Host ID
> - * @fw_caps:	FW/SoC low power capabilities
> - * @users:	Number of users of this instance
> - */
> -struct ti_sci_info {
> -	struct device *dev;
> -	const struct ti_sci_desc *desc;
> -	struct dentry *d;
> -	void __iomem *debug_region;
> -	char *debug_buffer;
> -	size_t debug_region_size;
> -	struct ti_sci_handle handle;
> -	struct mbox_client cl;
> -	struct mbox_chan *chan_tx;
> -	struct mbox_chan *chan_rx;
> -	struct ti_sci_xfers_info minfo;
> -	struct list_head node;
> -	DECLARE_HASHTABLE(irqs, 8);
> -	struct mutex irq_lock;
> -	u8 host_id;
> -	u64 fw_caps;
> -	/* protected by ti_sci_list_mutex */
> -	int users;
> -};
> -
>  #define cl_to_ti_sci_info(c)	container_of(c, struct ti_sci_info, cl)
> -#define handle_to_ti_sci_info(h) container_of(h, struct ti_sci_info, handle)
>  
>  #ifdef CONFIG_DEBUG_FS
>  
> @@ -687,128 +593,6 @@ static int ti_sci_cmd_set_io_isolation(const struct ti_sci_handle *handle,
>  	return 0;
>  }
>  
> -/**
> - * ti_sci_msg_cmd_lpm_wake_reason() - Get the wakeup source from LPM
> - * @handle:		Pointer to TI SCI handle
> - * @source:		The wakeup source that woke the SoC from LPM
> - * @timestamp:		Timestamp of the wakeup event
> - * @pin:		The pin that has triggered wake up
> - * @mode:		The last entered low power mode
> - *
> - * Return: 0 if all went well, else returns appropriate error value.
> - */
> -static int ti_sci_msg_cmd_lpm_wake_reason(const struct ti_sci_handle *handle,
> -					  u32 *source, u64 *timestamp, u8 *pin, u8 *mode)
> -{
> -	struct ti_sci_msg_hdr req;
> -	struct ti_sci_msg_resp_lpm_wake_reason resp;
> -	struct device *dev;
> -	int ret = 0;
> -
> -	if (IS_ERR(handle))
> -		return PTR_ERR(handle);
> -	if (!handle)
> -		return -EINVAL;
> -
> -	dev = handle_to_ti_sci_info(handle)->dev;
> -
> -	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_LPM_WAKE_REASON,
> -			     TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> -			     &req, sizeof(req),
> -			     &resp, sizeof(resp));
> -	if (ret) {
> -		dev_err(dev, "Failed to get wake reason\n");
> -		return ret;
> -	}
> -
> -	if (source)
> -		*source = resp.wake_source;
> -	if (timestamp)
> -		*timestamp = resp.wake_timestamp;
> -	if (pin)
> -		*pin = resp.wake_pin;
> -	if (mode)
> -		*mode = resp.mode;
> -
> -	return 0;
> -}
> -
> -/**
> - * ti_sci_cmd_set_device_constraint() - Set LPM constraint on behalf of a device
> - * @handle:	pointer to TI SCI handle
> - * @id:	Device identifier
> - * @state:	The desired state of device constraint: set or clear
> - *
> - * Return: 0 if all went well, else returns appropriate error value.
> - */
> -static int ti_sci_cmd_set_device_constraint(const struct ti_sci_handle *handle,
> -					    u32 id, u8 state)
> -{
> -	struct ti_sci_msg_req_lpm_set_device_constraint req;
> -	struct ti_sci_msg_hdr resp;
> -	struct device *dev;
> -	int ret = 0;
> -
> -	if (IS_ERR(handle))
> -		return PTR_ERR(handle);
> -	if (!handle)
> -		return -EINVAL;
> -
> -	dev = handle_to_ti_sci_info(handle)->dev;
> -
> -	req.id = id;
> -	req.state = state;
> -
> -	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_LPM_SET_DEVICE_CONSTRAINT,
> -			     TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> -			     &req, sizeof(req),
> -			     &resp, sizeof(resp));
> -	if (ret) {
> -		dev_err(dev, "Failed to set device constraint\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -/**
> - * ti_sci_cmd_set_latency_constraint() - Set LPM resume latency constraint
> - * @handle:	pointer to TI SCI handle
> - * @latency:	maximum acceptable latency (in ms) to wake up from LPM
> - * @state:	The desired state of latency constraint: set or clear
> - *
> - * Return: 0 if all went well, else returns appropriate error value.
> - */
> -static int ti_sci_cmd_set_latency_constraint(const struct ti_sci_handle *handle,
> -					     u16 latency, u8 state)
> -{
> -	struct ti_sci_msg_req_lpm_set_latency_constraint req;
> -	struct ti_sci_msg_hdr resp;
> -	struct device *dev;
> -	int ret = 0;
> -
> -	if (IS_ERR(handle))
> -		return PTR_ERR(handle);
> -	if (!handle)
> -		return -EINVAL;
> -
> -	dev = handle_to_ti_sci_info(handle)->dev;
> -
> -	req.latency = latency;
> -	req.state = state;
> -
> -	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_LPM_SET_LATENCY_CONSTRAINT,
> -			     TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> -			     &req, sizeof(req),
> -			     &resp, sizeof(resp));
> -	if (ret) {
> -		dev_err(dev, "Failed to set device constraint\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * ti_sci_cmd_lpm_abort() - Abort entry to LPM by clearing selection of LPM to enter
>   * @dev:	Device pointer corresponding to the SCI entity
> @@ -1708,9 +1492,7 @@ static void ti_sci_setup_ops(struct ti_sci_info *info)
>  
>  	if (info->fw_caps & MSG_FLAG_CAPS_LPM_DM_MANAGED) {
>  		pr_debug("detected DM managed LPM in fw_caps\n");
> -		pmops->lpm_wake_reason = ti_sci_msg_cmd_lpm_wake_reason;
> -		pmops->set_device_constraint = ti_sci_cmd_set_device_constraint;
> -		pmops->set_latency_constraint = ti_sci_cmd_set_latency_constraint;
> +		ti_sci_setup_pm_ops(pmops);
>  	}
>  
>  	rm_core_ops->get_range = ti_sci_cmd_get_resource_range;
> diff --git a/drivers/firmware/ti/ti_sci_pm.c b/drivers/firmware/ti/ti_sci_pm.c
> new file mode 100644
> index 0000000000000..6ea09a7e342e7
> --- /dev/null
> +++ b/drivers/firmware/ti/ti_sci_pm.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Texas Instruments System Control Interface Protocol Driver
> + *
> + * Copyright (C) 2015-2026 Texas Instruments Incorporated - https://www.ti.com/
> + *	Nishanth Menon
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/soc/ti/ti_sci_protocol.h>
> +
> +#include "ti_sci.h"
> +
> +/**
> + * ti_sci_msg_cmd_lpm_wake_reason() - Get the wakeup source from LPM
> + * @handle:		Pointer to TI SCI handle
> + * @source:		The wakeup source that woke the SoC from LPM
> + * @timestamp:		Timestamp of the wakeup event
> + * @pin:		The pin that has triggered wake up
> + * @mode:		The last entered low power mode
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_msg_cmd_lpm_wake_reason(const struct ti_sci_handle *handle,
> +					  u32 *source, u64 *timestamp, u8 *pin, u8 *mode)
> +{
> +	struct ti_sci_msg_hdr req;
> +	struct ti_sci_msg_resp_lpm_wake_reason resp;
> +	struct device *dev;
> +	int ret = 0;
> +
> +	if (IS_ERR(handle))
> +		return PTR_ERR(handle);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	dev = handle_to_ti_sci_info(handle)->dev;
> +
> +	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_LPM_WAKE_REASON,
> +			     TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> +			     &req, sizeof(req),
> +			     &resp, sizeof(resp));
> +	if (ret) {
> +		dev_err(dev, "Failed to get wake reason\n");
> +		return ret;
> +	}
> +
> +	if (source)
> +		*source = resp.wake_source;
> +	if (timestamp)
> +		*timestamp = resp.wake_timestamp;
> +	if (pin)
> +		*pin = resp.wake_pin;
> +	if (mode)
> +		*mode = resp.mode;
> +
> +	return 0;
> +}
> +
> +/**
> + * ti_sci_cmd_set_device_constraint() - Set LPM constraint on behalf of a device
> + * @handle:	pointer to TI SCI handle
> + * @id:	Device identifier
> + * @state:	The desired state of device constraint: set or clear
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_set_device_constraint(const struct ti_sci_handle *handle,
> +					    u32 id, u8 state)
> +{
> +	struct ti_sci_msg_req_lpm_set_device_constraint req;
> +	struct ti_sci_msg_hdr resp;
> +	struct device *dev;
> +	int ret = 0;
> +
> +	if (IS_ERR(handle))
> +		return PTR_ERR(handle);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	dev = handle_to_ti_sci_info(handle)->dev;
> +
> +	req.id = id;
> +	req.state = state;
> +
> +	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_LPM_SET_DEVICE_CONSTRAINT,
> +			     TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> +			     &req, sizeof(req),
> +			     &resp, sizeof(resp));
> +	if (ret) {
> +		dev_err(dev, "Failed to set device constraint\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ti_sci_cmd_set_latency_constraint() - Set LPM resume latency constraint
> + * @handle:	pointer to TI SCI handle
> + * @latency:	maximum acceptable latency (in ms) to wake up from LPM
> + * @state:	The desired state of latency constraint: set or clear
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_set_latency_constraint(const struct ti_sci_handle *handle,
> +					     u16 latency, u8 state)
> +{
> +	struct ti_sci_msg_req_lpm_set_latency_constraint req;
> +	struct ti_sci_msg_hdr resp;
> +	struct device *dev;
> +	int ret = 0;
> +
> +	if (IS_ERR(handle))
> +		return PTR_ERR(handle);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	dev = handle_to_ti_sci_info(handle)->dev;
> +
> +	req.latency = latency;
> +	req.state = state;
> +
> +	ret = ti_sci_do_xfer(handle, TI_SCI_MSG_LPM_SET_LATENCY_CONSTRAINT,
> +			     TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> +			     &req, sizeof(req),
> +			     &resp, sizeof(resp));
> +	if (ret) {
> +		dev_err(dev, "Failed to set device constraint\n");

Could you correct this line to:

  if (ret) {
             dev_err(dev, "Failed to set latency constraint\n");

Best,
Kendall

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * ti_sci_setup_pm_ops() - Setup the pm operations structures
> + * @pmops:	pointer to TISCI pm ops structure
> + */
> +void ti_sci_setup_pm_ops(struct ti_sci_pm_ops *pmops)
> +{
> +	pmops->lpm_wake_reason = ti_sci_msg_cmd_lpm_wake_reason;
> +	pmops->set_device_constraint = ti_sci_cmd_set_device_constraint;
> +	pmops->set_latency_constraint = ti_sci_cmd_set_latency_constraint;
> +}
> -- 
> 2.34.1
> 
>