From 75dfbcbfd781f826c73ebe42b78d49c23179d082 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:51 +0000 Subject: [PATCH 1/8] mailbox: mpfs: fix an incorrect mask width The system controller registers on PolarFire SoC are 32 bits wide, so 16 + 16 as the first input to GENMASK_ULL() gives a 33 bit wide mask. It probably should have been immediately obvious when it was pointed out during review that the width required using GENMASK_ULL() - but I scarcely knew what I was doing at the time and missed it. The mistake ends up being moot as it is a mask after all, but it is incorrect and should be fixed. No functional change intended. Acked-by: Jassi Brar Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/mailbox/mailbox-mpfs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c index 853901acaeec..d37560e91116 100644 --- a/drivers/mailbox/mailbox-mpfs.c +++ b/drivers/mailbox/mailbox-mpfs.c @@ -39,7 +39,7 @@ #define SCB_CTRL_NOTIFY_MASK BIT(SCB_CTRL_NOTIFY) #define SCB_CTRL_POS (16) -#define SCB_CTRL_MASK GENMASK_ULL(SCB_CTRL_POS + SCB_MASK_WIDTH, SCB_CTRL_POS) +#define SCB_CTRL_MASK GENMASK(SCB_CTRL_POS + SCB_MASK_WIDTH - 1, SCB_CTRL_POS) /* SCBCTRL service status register */ @@ -118,6 +118,7 @@ static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data) } opt_sel = ((msg->mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu)); + tx_trigger = (opt_sel << SCB_CTRL_POS) & SCB_CTRL_MASK; tx_trigger |= SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK; writel_relaxed(tx_trigger, mbox->ctrl_base + SERVICES_CR_OFFSET); From b5984a9844fc45cd301a28fb56f3de95f7e20f3c Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:52 +0000 Subject: [PATCH 2/8] mailbox: mpfs: switch to txdone_poll The system controller on PolarFire SoC has no interrupt to signify that the TX has been completed. The interrupt instead signals that a service requested by the mailbox client has succeeded. If a service fails, there will be no interrupt delivered. Switch to polling the busy register to determine whether transmission has completed. Fixes: 83d7b1560810 ("mbox: add polarfire soc system controller mailbox") Acked-by: Jassi Brar Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/mailbox/mailbox-mpfs.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c index d37560e91116..e0e825bdbad9 100644 --- a/drivers/mailbox/mailbox-mpfs.c +++ b/drivers/mailbox/mailbox-mpfs.c @@ -79,6 +79,13 @@ static bool mpfs_mbox_busy(struct mpfs_mbox *mbox) return status & SCB_STATUS_BUSY_MASK; } +static bool mpfs_mbox_last_tx_done(struct mbox_chan *chan) +{ + struct mpfs_mbox *mbox = (struct mpfs_mbox *)chan->con_priv; + + return !mpfs_mbox_busy(mbox); +} + static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data) { struct mpfs_mbox *mbox = (struct mpfs_mbox *)chan->con_priv; @@ -183,7 +190,6 @@ static irqreturn_t mpfs_mbox_inbox_isr(int irq, void *data) mpfs_mbox_rx_data(chan); - mbox_chan_txdone(chan, 0); return IRQ_HANDLED; } @@ -213,6 +219,7 @@ static const struct mbox_chan_ops mpfs_mbox_ops = { .send_data = mpfs_mbox_send_data, .startup = mpfs_mbox_startup, .shutdown = mpfs_mbox_shutdown, + .last_tx_done = mpfs_mbox_last_tx_done, }; static int mpfs_mbox_probe(struct platform_device *pdev) @@ -248,7 +255,8 @@ static int mpfs_mbox_probe(struct platform_device *pdev) mbox->controller.num_chans = 1; mbox->controller.chans = mbox->chans; mbox->controller.ops = &mpfs_mbox_ops; - mbox->controller.txdone_irq = true; + mbox->controller.txdone_poll = true; + mbox->controller.txpoll_period = 10u; ret = devm_mbox_controller_register(&pdev->dev, &mbox->controller); if (ret) { From da82f95f7c0752f36cdb8ff99d543004e4e3acdf Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:53 +0000 Subject: [PATCH 3/8] mailbox: mpfs: ditch a useless busy check mpfs_mbox_rx_data() already checks if the system controller is busy before attempting to do anything, so drop the second check before reading any data. No functional change intended. Acked-by: Jassi Brar Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/mailbox/mailbox-mpfs.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c index e0e825bdbad9..0d176aba3462 100644 --- a/drivers/mailbox/mailbox-mpfs.c +++ b/drivers/mailbox/mailbox-mpfs.c @@ -170,12 +170,10 @@ static void mpfs_mbox_rx_data(struct mbox_chan *chan) if (response->resp_status) return; - if (!mpfs_mbox_busy(mbox)) { - for (i = 0; i < num_words; i++) { - response->resp_msg[i] = - readl_relaxed(mbox->mbox_base - + mbox->resp_offset + i * 0x4); - } + for (i = 0; i < num_words; i++) { + response->resp_msg[i] = + readl_relaxed(mbox->mbox_base + + mbox->resp_offset + i * 0x4); } mbox_chan_received_data(chan, response); From 37e3430176ff333afb013314a6c949660d75cc92 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:54 +0000 Subject: [PATCH 4/8] mailbox: mpfs: check the service status in .tx_done() Services are supposed to generate an interrupt once completed, whether or not they have do so successfully. What appears to be a bug in the system controller means that interrupts are only generated for *successful* services. Currently, the status of a service is only checked in the mpfs_mbox_rx_data() once an interrupt is received. As it turns out, this is not really helpful where the potentially buggy behaviour is present, as we'll only see the status for successes where it is moot anyway. Jassi suggested moving the check to the .tx_done() callback instead. This makes sense, as the busy bit that tx_done() is polling will be lowered on completion, regardless of whether the service passed or failed. That allows us to check the status bits for all services, whether they generate an interrupt or not & pass something more informative than -EBADMSG back to the drivers implementing individual services. Suggested-by: Jassi Brar Acked-by: Jassi Brar Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/mailbox/mailbox-mpfs.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c index 0d176aba3462..162df49654fb 100644 --- a/drivers/mailbox/mailbox-mpfs.c +++ b/drivers/mailbox/mailbox-mpfs.c @@ -82,8 +82,22 @@ static bool mpfs_mbox_busy(struct mpfs_mbox *mbox) static bool mpfs_mbox_last_tx_done(struct mbox_chan *chan) { struct mpfs_mbox *mbox = (struct mpfs_mbox *)chan->con_priv; + struct mpfs_mss_response *response = mbox->response; + u32 val; - return !mpfs_mbox_busy(mbox); + if (mpfs_mbox_busy(mbox)) + return false; + + /* + * The service status is stored in bits 31:16 of the SERVICES_SR + * register & is only valid when the system controller is not busy. + * Failed services are intended to generated interrupts, but in reality + * this does not happen, so the status must be checked here. + */ + val = readl_relaxed(mbox->ctrl_base + SERVICES_SR_OFFSET); + response->resp_status = (val & SCB_STATUS_MASK) >> SCB_STATUS_POS; + + return true; } static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data) @@ -138,7 +152,7 @@ static void mpfs_mbox_rx_data(struct mbox_chan *chan) struct mpfs_mbox *mbox = (struct mpfs_mbox *)chan->con_priv; struct mpfs_mss_response *response = mbox->response; u16 num_words = ALIGN((response->resp_size), (4)) / 4U; - u32 i, status; + u32 i; if (!response->resp_msg) { dev_err(mbox->dev, "failed to assign memory for response %d\n", -ENOMEM); @@ -146,8 +160,6 @@ static void mpfs_mbox_rx_data(struct mbox_chan *chan) } /* - * The status is stored in bits 31:16 of the SERVICES_SR register. - * It is only valid when BUSY == 0. * We should *never* get an interrupt while the controller is * still in the busy state. If we do, something has gone badly * wrong & the content of the mailbox would not be valid. @@ -158,18 +170,6 @@ static void mpfs_mbox_rx_data(struct mbox_chan *chan) return; } - status = readl_relaxed(mbox->ctrl_base + SERVICES_SR_OFFSET); - - /* - * If the status of the individual servers is non-zero, the service has - * failed. The contents of the mailbox at this point are not be valid, - * so don't bother reading them. Set the status so that the driver - * implementing the service can handle the result. - */ - response->resp_status = (status & SCB_STATUS_MASK) >> SCB_STATUS_POS; - if (response->resp_status) - return; - for (i = 0; i < num_words; i++) { response->resp_msg[i] = readl_relaxed(mbox->mbox_base From 5ca631ec757bed5843e39c91c8f52d1789991756 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:55 +0000 Subject: [PATCH 5/8] soc: microchip: mpfs: fix some horrible alignment mpfs_sys_controller_delete() has some horrible alignment that upsets my OCD... Move the RHS of the assignment to a new line for greater satifaction. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index 6e20207b5756..12039cb38b33 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -66,8 +66,8 @@ static void rx_callback(struct mbox_client *client, void *msg) static void mpfs_sys_controller_delete(struct kref *kref) { - struct mpfs_sys_controller *sys_controller = container_of(kref, struct mpfs_sys_controller, - consumers); + struct mpfs_sys_controller *sys_controller = + container_of(kref, struct mpfs_sys_controller, consumers); mbox_free_channel(sys_controller->chan); kfree(sys_controller); From 4f739af1934ad7195db35f74cfbdca264668d8fc Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:56 +0000 Subject: [PATCH 6/8] soc: microchip: mpfs: use a consistent completion timeout Completion timeouts use jiffies, so passing a number directly will produce inconsistent timeouts depending on config. Define the timeout in ms and convert it to jiffies instead. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index 12039cb38b33..738ecd624d64 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -11,12 +11,15 @@ #include #include #include +#include #include #include #include #include #include +#define MPFS_SYS_CTRL_TIMEOUT_MS 100 + static DEFINE_MUTEX(transaction_lock); struct mpfs_sys_controller { @@ -28,6 +31,7 @@ struct mpfs_sys_controller { int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct mpfs_mss_msg *msg) { + unsigned long timeout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS); int ret, err; err = mutex_lock_interruptible(&transaction_lock); @@ -38,7 +42,7 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct ret = mbox_send_message(sys_controller->chan, msg); if (ret >= 0) { - if (wait_for_completion_timeout(&sys_controller->c, HZ)) { + if (wait_for_completion_timeout(&sys_controller->c, timeout)) { ret = 0; } else { ret = -ETIMEDOUT; From 7606f4dfffa7a4e4aadd8aef918cc8ac1f1e2196 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:57 +0000 Subject: [PATCH 7/8] soc: microchip: mpfs: simplify error handling in mpfs_blocking_transaction() The error handling has a kinda weird nested-if setup that is not really adding anything. Switch it to more of an early return arrangement as a predatory step for adding different handing for timeouts and failed services. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 27 ++++++++++----------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index 738ecd624d64..e61ba9b7aae3 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -32,28 +32,27 @@ struct mpfs_sys_controller { int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct mpfs_mss_msg *msg) { unsigned long timeout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS); - int ret, err; + int ret; - err = mutex_lock_interruptible(&transaction_lock); - if (err) - return err; + ret = mutex_lock_interruptible(&transaction_lock); + if (ret) + return ret; reinit_completion(&sys_controller->c); ret = mbox_send_message(sys_controller->chan, msg); - if (ret >= 0) { - if (wait_for_completion_timeout(&sys_controller->c, timeout)) { - ret = 0; - } else { - ret = -ETIMEDOUT; - dev_warn(sys_controller->client.dev, - "MPFS sys controller transaction timeout\n"); - } + if (ret < 0) + goto out; + + if (!wait_for_completion_timeout(&sys_controller->c, timeout)) { + ret = -ETIMEDOUT; + dev_warn(sys_controller->client.dev, "MPFS sys controller transaction timeout\n"); } else { - dev_err(sys_controller->client.dev, - "mpfs sys controller transaction returned %d\n", ret); + /* mbox_send_message() returns positive integers on success */ + ret = 0; } +out: mutex_unlock(&transaction_lock); return ret; From 8f943dd12eeff71857b9a1ca45adbaaba379bf30 Mon Sep 17 00:00:00 2001 From: Conor Dooley Date: Tue, 7 Mar 2023 20:22:58 +0000 Subject: [PATCH 8/8] soc: microchip: mpfs: handle timeouts and failed services differently The system controller will only deliver an interrupt if a service succeeds. This leaves us in the unfortunate position with current code where there is no way to differentiate between a legitimate timeout where the service has not completed & where it has completed, but failed. mbox_send_message() has its own completion, and it will time out of the system controller does not lower the busy flag. In this case, a timeout has occurred and the error can be propagated back to the caller. If the busy flag is lowered, but no interrupt has arrived to trigger the rx callback, the service can be deemed to have failed. Report -EBADMSG in this case so that callers can differentiate. Tested-by: Valentina Fernandez Signed-off-by: Conor Dooley --- drivers/soc/microchip/mpfs-sys-controller.c | 27 +++++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c index e61ba9b7aae3..ceaeebc1fc6b 100644 --- a/drivers/soc/microchip/mpfs-sys-controller.c +++ b/drivers/soc/microchip/mpfs-sys-controller.c @@ -18,7 +18,11 @@ #include #include -#define MPFS_SYS_CTRL_TIMEOUT_MS 100 +/* + * This timeout must be long, as some services (example: image authentication) + * take significant time to complete + */ +#define MPFS_SYS_CTRL_TIMEOUT_MS 30000 static DEFINE_MUTEX(transaction_lock); @@ -41,14 +45,26 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct reinit_completion(&sys_controller->c); ret = mbox_send_message(sys_controller->chan, msg); - if (ret < 0) + if (ret < 0) { + dev_warn(sys_controller->client.dev, "MPFS sys controller service timeout\n"); goto out; + } + /* + * Unfortunately, the system controller will only deliver an interrupt + * if a service succeeds. mbox_send_message() will block until the busy + * flag is gone. If the busy flag is gone but no interrupt has arrived + * to trigger the rx callback then the service can be deemed to have + * failed. + * The caller can then interrogate msg::response::resp_status to + * determine the cause of the failure. + * mbox_send_message() returns positive integers in the success path, so + * ret needs to be cleared if we do get an interrupt. + */ if (!wait_for_completion_timeout(&sys_controller->c, timeout)) { - ret = -ETIMEDOUT; - dev_warn(sys_controller->client.dev, "MPFS sys controller transaction timeout\n"); + ret = -EBADMSG; + dev_warn(sys_controller->client.dev, "MPFS sys controller service failed\n"); } else { - /* mbox_send_message() returns positive integers on success */ ret = 0; } @@ -107,6 +123,7 @@ static int mpfs_sys_controller_probe(struct platform_device *pdev) sys_controller->client.dev = dev; sys_controller->client.rx_callback = rx_callback; sys_controller->client.tx_block = 1U; + sys_controller->client.tx_tout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS); sys_controller->chan = mbox_request_channel(&sys_controller->client, 0); if (IS_ERR(sys_controller->chan)) {