Messages in this thread | | | Date | Fri, 09 Dec 2022 11:14:40 -0800 (PST) | Subject | Re: [PATCH v3 2/2] mailbox: mpfs: read the system controller's status | From | Palmer Dabbelt <> |
| |
On Wed, 23 Nov 2022 09:56:52 PST (-0800), Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.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@microchip.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@microchip.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@rivosinc.com>
I'm assuming this is aimed at some other tree.
| |