[PATCH 0/2] mailbox: ti-msgmgr: Use polled mode of operation during suspend

[PATCH 0/2] mailbox: ti-msgmgr: Use polled mode of operation during suspend

[Dave Gerlach]

Hi,
This series switches the ti-msgmgr driver to operate in polled mode
during system suspend rather than the current IRQ driven operation.
This allows users of this mailbox, such as the ti_sci firmware driver,
to communicate with TISCI firmware without the need of interrupts, which
is critical during the system suspend path.  Currently, the
ti_sci_pm_domains genpd driver will send messages to disable devices
during the noirq phase, so this must be done using a polled
communication layer. It still makes sense to use IRQ mode during normal
operation, so continue to support that as well, and only switch to
polled mode during low power path.

This is accomplished by setting a flag for polled mode in the system
suspend handler that then causes the driver to immediately poll a
mailbox RX channel in the send_data mailbox op, which will either
timeout and return or follow the normal mailbox flow and call
mbox_chan_received_data before returning from mbox_send_message.

These changes do not affect the normal operation of the ti-msgmgr driver
and are transparent outside of the system suspend path. The suspend path
already sees timeouts with messages sent during noirq phase today and
will continue to until a follow on patch for the ti_sci driver is sent
to make communication completely safe during noirq, but this series can
be merged without affecting operation.

Regards,
Dave

Dave Gerlach (2):
  mailbox: ti-msgmgr: Refactor message read during interrupt handler
  mailbox: ti-msgmgr: Operate mailbox in polled mode during system
    suspend

 drivers/mailbox/ti-msgmgr.c      | 181 ++++++++++++++++++++++++-------
 include/linux/soc/ti/ti-msgmgr.h |   8 +-
 2 files changed, 147 insertions(+), 42 deletions(-)

-- 
2.35.0

[PATCH 1/2] mailbox: ti-msgmgr: Refactor message read during interrupt handler

[Dave Gerlach]

Refactor the portion of code that actually reads received messages from
a queue into its own function, ti_msgmgr_queue_rx_data, that is called
by the interrupt handler instead of reading directly from the handler.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/mailbox/ti-msgmgr.c | 88 +++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 39 deletions(-)

diff --git a/drivers/mailbox/ti-msgmgr.c b/drivers/mailbox/ti-msgmgr.c
index efb43b038596..f860cd0c907a 100644
--- a/drivers/mailbox/ti-msgmgr.c
+++ b/drivers/mailbox/ti-msgmgr.c
@@ -190,6 +190,53 @@ static inline bool ti_msgmgr_queue_is_error(const struct ti_msgmgr_desc *d,
 	return val ? true : false;
 }
 
+static int ti_msgmgr_queue_rx_data(struct mbox_chan *chan, struct ti_queue_inst *qinst,
+				   const struct ti_msgmgr_desc *desc)
+{
+	int num_words;
+	struct ti_msgmgr_message message;
+	void __iomem *data_reg;
+	u32 *word_data;
+
+	/*
+	 * I have no idea about the protocol being used to communicate with the
+	 * remote producer - 0 could be valid data, so I wont make a judgement
+	 * of how many bytes I should be reading. Let the client figure this
+	 * out.. I just read the full message and pass it on..
+	 */
+	message.len = desc->max_message_size;
+	message.buf = (u8 *)qinst->rx_buff;
+
+	/*
+	 * NOTE about register access involved here:
+	 * the hardware block is implemented with 32bit access operations and no
+	 * support for data splitting.  We don't want the hardware to misbehave
+	 * with sub 32bit access - For example: if the last register read is
+	 * split into byte wise access, it can result in the queue getting
+	 * stuck or indeterminate behavior. An out of order read operation may
+	 * result in weird data results as well.
+	 * Hence, we do not use memcpy_fromio or __ioread32_copy here, instead
+	 * we depend on readl for the purpose.
+	 *
+	 * Also note that the final register read automatically marks the
+	 * queue message as read.
+	 */
+	for (data_reg = qinst->queue_buff_start, word_data = qinst->rx_buff,
+	     num_words = (desc->max_message_size / sizeof(u32));
+	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
+		*word_data = readl(data_reg);
+
+	/*
+	 * Last register read automatically clears the IRQ if only 1 message
+	 * is pending - so send the data up the stack..
+	 * NOTE: Client is expected to be as optimal as possible, since
+	 * we invoke the handler in IRQ context.
+	 */
+	mbox_chan_received_data(chan, (void *)&message);
+
+	return 0;
+}
+
 /**
  * ti_msgmgr_queue_rx_interrupt() - Interrupt handler for receive Queue
  * @irq:	Interrupt number
@@ -206,10 +253,7 @@ static irqreturn_t ti_msgmgr_queue_rx_interrupt(int irq, void *p)
 	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
 	struct ti_queue_inst *qinst = chan->con_priv;
 	const struct ti_msgmgr_desc *desc;
-	int msg_count, num_words;
-	struct ti_msgmgr_message message;
-	void __iomem *data_reg;
-	u32 *word_data;
+	int msg_count;
 
 	if (WARN_ON(!inst)) {
 		dev_err(dev, "no platform drv data??\n");
@@ -237,41 +281,7 @@ static irqreturn_t ti_msgmgr_queue_rx_interrupt(int irq, void *p)
 		return IRQ_NONE;
 	}
 
-	/*
-	 * I have no idea about the protocol being used to communicate with the
-	 * remote producer - 0 could be valid data, so I won't make a judgement
-	 * of how many bytes I should be reading. Let the client figure this
-	 * out.. I just read the full message and pass it on..
-	 */
-	message.len = desc->max_message_size;
-	message.buf = (u8 *)qinst->rx_buff;
-
-	/*
-	 * NOTE about register access involved here:
-	 * the hardware block is implemented with 32bit access operations and no
-	 * support for data splitting.  We don't want the hardware to misbehave
-	 * with sub 32bit access - For example: if the last register read is
-	 * split into byte wise access, it can result in the queue getting
-	 * stuck or indeterminate behavior. An out of order read operation may
-	 * result in weird data results as well.
-	 * Hence, we do not use memcpy_fromio or __ioread32_copy here, instead
-	 * we depend on readl for the purpose.
-	 *
-	 * Also note that the final register read automatically marks the
-	 * queue message as read.
-	 */
-	for (data_reg = qinst->queue_buff_start, word_data = qinst->rx_buff,
-	     num_words = (desc->max_message_size / sizeof(u32));
-	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
-		*word_data = readl(data_reg);
-
-	/*
-	 * Last register read automatically clears the IRQ if only 1 message
-	 * is pending - so send the data up the stack..
-	 * NOTE: Client is expected to be as optimal as possible, since
-	 * we invoke the handler in IRQ context.
-	 */
-	mbox_chan_received_data(chan, (void *)&message);
+	ti_msgmgr_queue_rx_data(chan, qinst, desc);
 
 	return IRQ_HANDLED;
 }
-- 
2.35.0

Re: [PATCH 1/2] mailbox: ti-msgmgr: Refactor message read during interrupt handler

[Suman Anna]

On 2/9/22 22:16, Dave Gerlach wrote:
> Refactor the portion of code that actually reads received messages from
> a queue into its own function, ti_msgmgr_queue_rx_data, that is called
> by the interrupt handler instead of reading directly from the handler.
> 
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>

Acked-by: Suman Anna <s-anna@ti.com>

> ---
>  drivers/mailbox/ti-msgmgr.c | 88 +++++++++++++++++++++----------------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/mailbox/ti-msgmgr.c b/drivers/mailbox/ti-msgmgr.c
> index efb43b038596..f860cd0c907a 100644
> --- a/drivers/mailbox/ti-msgmgr.c
> +++ b/drivers/mailbox/ti-msgmgr.c
> @@ -190,6 +190,53 @@ static inline bool ti_msgmgr_queue_is_error(const struct ti_msgmgr_desc *d,
>  	return val ? true : false;
>  }
>  
> +static int ti_msgmgr_queue_rx_data(struct mbox_chan *chan, struct ti_queue_inst *qinst,
> +				   const struct ti_msgmgr_desc *desc)
> +{
> +	int num_words;
> +	struct ti_msgmgr_message message;
> +	void __iomem *data_reg;
> +	u32 *word_data;
> +
> +	/*
> +	 * I have no idea about the protocol being used to communicate with the
> +	 * remote producer - 0 could be valid data, so I wont make a judgement
> +	 * of how many bytes I should be reading. Let the client figure this
> +	 * out.. I just read the full message and pass it on..
> +	 */
> +	message.len = desc->max_message_size;
> +	message.buf = (u8 *)qinst->rx_buff;
> +
> +	/*
> +	 * NOTE about register access involved here:
> +	 * the hardware block is implemented with 32bit access operations and no
> +	 * support for data splitting.  We don't want the hardware to misbehave
> +	 * with sub 32bit access - For example: if the last register read is
> +	 * split into byte wise access, it can result in the queue getting
> +	 * stuck or indeterminate behavior. An out of order read operation may
> +	 * result in weird data results as well.
> +	 * Hence, we do not use memcpy_fromio or __ioread32_copy here, instead
> +	 * we depend on readl for the purpose.
> +	 *
> +	 * Also note that the final register read automatically marks the
> +	 * queue message as read.
> +	 */
> +	for (data_reg = qinst->queue_buff_start, word_data = qinst->rx_buff,
> +	     num_words = (desc->max_message_size / sizeof(u32));
> +	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
> +		*word_data = readl(data_reg);
> +
> +	/*
> +	 * Last register read automatically clears the IRQ if only 1 message
> +	 * is pending - so send the data up the stack..
> +	 * NOTE: Client is expected to be as optimal as possible, since
> +	 * we invoke the handler in IRQ context.
> +	 */
> +	mbox_chan_received_data(chan, (void *)&message);
> +
> +	return 0;
> +}
> +
>  /**
>   * ti_msgmgr_queue_rx_interrupt() - Interrupt handler for receive Queue
>   * @irq:	Interrupt number
> @@ -206,10 +253,7 @@ static irqreturn_t ti_msgmgr_queue_rx_interrupt(int irq, void *p)
>  	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>  	struct ti_queue_inst *qinst = chan->con_priv;
>  	const struct ti_msgmgr_desc *desc;
> -	int msg_count, num_words;
> -	struct ti_msgmgr_message message;
> -	void __iomem *data_reg;
> -	u32 *word_data;
> +	int msg_count;
>  
>  	if (WARN_ON(!inst)) {
>  		dev_err(dev, "no platform drv data??\n");
> @@ -237,41 +281,7 @@ static irqreturn_t ti_msgmgr_queue_rx_interrupt(int irq, void *p)
>  		return IRQ_NONE;
>  	}
>  
> -	/*
> -	 * I have no idea about the protocol being used to communicate with the
> -	 * remote producer - 0 could be valid data, so I won't make a judgement
> -	 * of how many bytes I should be reading. Let the client figure this
> -	 * out.. I just read the full message and pass it on..
> -	 */
> -	message.len = desc->max_message_size;
> -	message.buf = (u8 *)qinst->rx_buff;
> -
> -	/*
> -	 * NOTE about register access involved here:
> -	 * the hardware block is implemented with 32bit access operations and no
> -	 * support for data splitting.  We don't want the hardware to misbehave
> -	 * with sub 32bit access - For example: if the last register read is
> -	 * split into byte wise access, it can result in the queue getting
> -	 * stuck or indeterminate behavior. An out of order read operation may
> -	 * result in weird data results as well.
> -	 * Hence, we do not use memcpy_fromio or __ioread32_copy here, instead
> -	 * we depend on readl for the purpose.
> -	 *
> -	 * Also note that the final register read automatically marks the
> -	 * queue message as read.
> -	 */
> -	for (data_reg = qinst->queue_buff_start, word_data = qinst->rx_buff,
> -	     num_words = (desc->max_message_size / sizeof(u32));
> -	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
> -		*word_data = readl(data_reg);
> -
> -	/*
> -	 * Last register read automatically clears the IRQ if only 1 message
> -	 * is pending - so send the data up the stack..
> -	 * NOTE: Client is expected to be as optimal as possible, since
> -	 * we invoke the handler in IRQ context.
> -	 */
> -	mbox_chan_received_data(chan, (void *)&message);
> +	ti_msgmgr_queue_rx_data(chan, qinst, desc);
>  
>  	return IRQ_HANDLED;
>  }

[PATCH 2/2] mailbox: ti-msgmgr: Operate mailbox in polled mode during system suspend

[Dave Gerlach]

During the system suspend path we must set all queues to operate in
polled mode as it is possible for any protocol built using this mailbox,
such as TISCI, to require communication during the no irq phase of suspend,
and we cannot rely on interrupts there.

Polled mode is implemented by allowing the mailbox user to define an
RX channel as part of the message that is sent which is what gets polled
for a response. If polled mode is enabled, this will immediately be
polled for a response at the end of the mailbox send_data op before
returning success for the data send or timing out if no response is
received.

Finally, to ensure polled mode is always enabled during system suspend,
iterate through all queues to set RX queues to polled mode during system
suspend and disable polled mode for all in the resume handler.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/mailbox/ti-msgmgr.c      | 93 +++++++++++++++++++++++++++++++-
 include/linux/soc/ti/ti-msgmgr.h |  8 ++-
 2 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/ti-msgmgr.c b/drivers/mailbox/ti-msgmgr.c
index f860cd0c907a..ddac423ac1a9 100644
--- a/drivers/mailbox/ti-msgmgr.c
+++ b/drivers/mailbox/ti-msgmgr.c
@@ -2,7 +2,7 @@
 /*
  * Texas Instruments' Message Manager Driver
  *
- * Copyright (C) 2015-2017 Texas Instruments Incorporated - https://www.ti.com/
+ * Copyright (C) 2015-2022 Texas Instruments Incorporated - https://www.ti.com/
  *	Nishanth Menon
  */
 
@@ -11,6 +11,7 @@
 #include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/mailbox_controller.h>
 #include <linux/module.h>
@@ -100,6 +101,7 @@ struct ti_msgmgr_desc {
  * @queue_ctrl: Queue Control register
  * @chan:	Mailbox channel
  * @rx_buff:	Receive buffer pointer allocated at probe, max_message_size
+ * @polled_rx_mode: Use polling for rx instead of interrupts
  */
 struct ti_queue_inst {
 	char name[30];
@@ -113,6 +115,7 @@ struct ti_queue_inst {
 	void __iomem *queue_ctrl;
 	struct mbox_chan *chan;
 	u32 *rx_buff;
+	bool polled_rx_mode;
 };
 
 /**
@@ -237,6 +240,26 @@ static int ti_msgmgr_queue_rx_data(struct mbox_chan *chan, struct ti_queue_inst
 	return 0;
 }
 
+static int ti_msgmgr_queue_rx_poll_timeout(struct mbox_chan *chan, int timeout_us)
+{
+	struct device *dev = chan->mbox->dev;
+	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
+	struct ti_queue_inst *qinst = chan->con_priv;
+	const struct ti_msgmgr_desc *desc = inst->desc;
+	int msg_count;
+	int ret;
+
+	ret = readl_poll_timeout_atomic(qinst->queue_state, msg_count,
+					(msg_count & desc->status_cnt_mask),
+					10, timeout_us);
+	if (ret != 0)
+		return ret;
+
+	ti_msgmgr_queue_rx_data(chan, qinst, desc);
+
+	return 0;
+}
+
 /**
  * ti_msgmgr_queue_rx_interrupt() - Interrupt handler for receive Queue
  * @irq:	Interrupt number
@@ -346,6 +369,17 @@ static bool ti_msgmgr_last_tx_done(struct mbox_chan *chan)
 	return msg_count ? false : true;
 }
 
+static bool ti_msgmgr_chan_has_polled_queue_rx(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst;
+
+	if (!chan)
+		return false;
+
+	qinst = chan->con_priv;
+	return qinst->polled_rx_mode;
+}
+
 /**
  * ti_msgmgr_send_data() - Send data
  * @chan:	Channel Pointer
@@ -363,6 +397,7 @@ static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
 	struct ti_msgmgr_message *message = data;
 	void __iomem *data_reg;
 	u32 *word_data;
+	int ret = 0;
 
 	if (WARN_ON(!inst)) {
 		dev_err(dev, "no platform drv data??\n");
@@ -404,7 +439,12 @@ static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
 	if (data_reg <= qinst->queue_buff_end)
 		writel(0, qinst->queue_buff_end);
 
-	return 0;
+	/* If we are in polled mode, wait for a response before proceeding */
+	if (ti_msgmgr_chan_has_polled_queue_rx(message->chan_rx))
+		ret = ti_msgmgr_queue_rx_poll_timeout(message->chan_rx,
+						      message->timeout_rx_ms * 1000);
+
+	return ret;
 }
 
 /**
@@ -652,6 +692,54 @@ static int ti_msgmgr_queue_setup(int idx, struct device *dev,
 	return 0;
 }
 
+static int ti_msgmgr_queue_rx_set_polled_mode(struct ti_queue_inst *qinst, bool enable)
+{
+	if (enable) {
+		disable_irq(qinst->irq);
+		qinst->polled_rx_mode = true;
+	} else {
+		enable_irq(qinst->irq);
+		qinst->polled_rx_mode = false;
+	}
+
+	return 0;
+}
+
+static int ti_msgmgr_suspend(struct device *dev)
+{
+	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
+	struct ti_queue_inst *qinst;
+	int i;
+
+	/*
+	 * We must switch operation to polled mode now as drivers and the genpd
+	 * layer may make late TI SCI calls to change clock and device states
+	 * from the noirq phase of suspend.
+	 */
+	for (qinst = inst->qinsts, i = 0; i < inst->num_valid_queues; qinst++, i++) {
+		if (!qinst->is_tx)
+			ti_msgmgr_queue_rx_set_polled_mode(qinst, true);
+	}
+
+	return 0;
+}
+
+static int ti_msgmgr_resume(struct device *dev)
+{
+	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
+	struct ti_queue_inst *qinst;
+	int i;
+
+	for (qinst = inst->qinsts, i = 0; i < inst->num_valid_queues; qinst++, i++) {
+		if (!qinst->is_tx)
+			ti_msgmgr_queue_rx_set_polled_mode(qinst, false);
+	}
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ti_msgmgr_pm_ops, ti_msgmgr_suspend, ti_msgmgr_resume);
+
 /* Queue operations */
 static const struct mbox_chan_ops ti_msgmgr_chan_ops = {
 	.startup = ti_msgmgr_queue_startup,
@@ -839,6 +927,7 @@ static struct platform_driver ti_msgmgr_driver = {
 	.driver = {
 		   .name = "ti-msgmgr",
 		   .of_match_table = of_match_ptr(ti_msgmgr_of_match),
+		   .pm = &ti_msgmgr_pm_ops,
 	},
 };
 module_platform_driver(ti_msgmgr_driver);
diff --git a/include/linux/soc/ti/ti-msgmgr.h b/include/linux/soc/ti/ti-msgmgr.h
index 1f6e76d423cf..69a8d7682c4b 100644
--- a/include/linux/soc/ti/ti-msgmgr.h
+++ b/include/linux/soc/ti/ti-msgmgr.h
@@ -1,7 +1,7 @@
 /*
  * Texas Instruments' Message Manager
  *
- * Copyright (C) 2015-2016 Texas Instruments Incorporated - https://www.ti.com/
+ * Copyright (C) 2015-2022 Texas Instruments Incorporated - https://www.ti.com/
  *	Nishanth Menon
  *
  * This program is free software; you can redistribute it and/or modify
@@ -17,10 +17,14 @@
 #ifndef TI_MSGMGR_H
 #define TI_MSGMGR_H
 
+struct mbox_chan;
+
 /**
  * struct ti_msgmgr_message - Message Manager structure
  * @len: Length of data in the Buffer
  * @buf: Buffer pointer
+ * @chan_rx: Expected channel for response, must be provided to use polled rx
+ * @timeout_rx_ms: Timeout value to use if polling for response
  *
  * This is the structure for data used in mbox_send_message
  * the length of data buffer used depends on the SoC integration
@@ -30,6 +34,8 @@
 struct ti_msgmgr_message {
 	size_t len;
 	u8 *buf;
+	struct mbox_chan *chan_rx;
+	int timeout_rx_ms;
 };
 
 #endif /* TI_MSGMGR_H */
-- 
2.35.0