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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Suman Anna <[email protected]>
> ---
> 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;
> }