[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <mhng-33aa5a63-85de-4a94-a1d5-d4507a0f55c7@palmer-ri-x1c9>
Date: Fri, 09 Dec 2022 11:14:40 -0800 (PST)
From: Palmer Dabbelt <palmer@...osinc.com>
To: Conor Dooley <conor@...nel.org>
CC: Conor Dooley <conor.dooley@...rochip.com>,
jassisinghbrar@...il.com, daire.mcnamara@...rochip.com,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] mailbox: mpfs: read the system controller's status
On Wed, 23 Nov 2022 09:56:52 PST (-0800), Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@...rochip.com>
>
> Some services explicitly return an error code in their response, but
> others rely on the system controller to set a status in its status
> register. The meaning of the bits varies based on what service is
> requested, so pass it back up to the driver that requested the service
> in the first place. The field in the message struct already existed, but
> was unused until now.
>
> If the system controller is busy, in which case we should never actually
> be in the interrupt handler, or if the service fails the mailbox itself
> should not be read. Callers should check the status before operating on
> the response.
>
> There's an existing, but unused, #define for the mailbox mask - but it
> was incorrect. It was doing a GENMASK_ULL(32, 16) which should've just
> been a GENMASK(31, 16), so fix that up and start using it.
>
> Fixes: 83d7b1560810 ("mbox: add polarfire soc system controller mailbox")
> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
> ---
> drivers/mailbox/mailbox-mpfs.c | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c
> index cfacb3f320a6..853901acaeec 100644
> --- a/drivers/mailbox/mailbox-mpfs.c
> +++ b/drivers/mailbox/mailbox-mpfs.c
> @@ -2,7 +2,7 @@
> /*
> * Microchip PolarFire SoC (MPFS) system controller/mailbox controller driver
> *
> - * Copyright (c) 2020 Microchip Corporation. All rights reserved.
> + * Copyright (c) 2020-2022 Microchip Corporation. All rights reserved.
> *
> * Author: Conor Dooley <conor.dooley@...rochip.com>
> *
> @@ -56,7 +56,7 @@
> #define SCB_STATUS_NOTIFY_MASK BIT(SCB_STATUS_NOTIFY)
>
> #define SCB_STATUS_POS (16)
> -#define SCB_STATUS_MASK GENMASK_ULL(SCB_STATUS_POS + SCB_MASK_WIDTH, SCB_STATUS_POS)
> +#define SCB_STATUS_MASK GENMASK(SCB_STATUS_POS + SCB_MASK_WIDTH - 1, SCB_STATUS_POS)
>
> struct mpfs_mbox {
> struct mbox_controller controller;
> @@ -130,13 +130,38 @@ 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;
> + u32 i, status;
>
> if (!response->resp_msg) {
> dev_err(mbox->dev, "failed to assign memory for response %d\n", -ENOMEM);
> return;
> }
>
> + /*
> + * 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.
> + */
> + if (mpfs_mbox_busy(mbox)) {
> + dev_err(mbox->dev, "got an interrupt but system controller is busy\n");
> + response->resp_status = 0xDEAD;
> + 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;
> +
> if (!mpfs_mbox_busy(mbox)) {
> for (i = 0; i < num_words; i++) {
> response->resp_msg[i] =
Reviewed-by: Palmer Dabbelt <palmer@...osinc.com>
I'm assuming this is aimed at some other tree.
Powered by blists - more mailing lists