[PATCH v4 1/5] mbox: add polarfire soc system controller mailbox

[Jassi Brar]

On Thu, Mar 11, 2021 at 5:34 AM <conor.dooley at microchip.com> wrote:

> diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c
> new file mode 100644
> index 000000000000..7aa6c8c87ea0
> --- /dev/null
> +++ b/drivers/mailbox/mailbox-mpfs.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip PolarFire SoC (MPFS) system controller/mailbox controller driver
> + *
> + * Copyright (c) 2020 Microchip Corporation. All rights reserved.
> + *
> + * Author: Conor Dooley <conor.dooley at microchip.com>
> + *
> + */
> +
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/mailbox_controller.h>
> +#include <soc/microchip/mpfs.h>
> +
> +#define SERVICES_CR_OFFSET             0x50u
> +#define SERVICES_SR_OFFSET             0x54u
> +#define MAILBOX_REG_OFFSET             0x800u
> +#define MSS_SYS_MAILBOX_DATA_OFFSET    0u
> +#define SCB_MASK_WIDTH                 16u
> +
> +/* SCBCTRL service control register */
> +
> +#define SCB_CTRL_REQ (0)
> +#define SCB_CTRL_REQ_MASK BIT(SCB_CTRL_REQ)
> +
> +#define SCB_CTRL_BUSY (1)
> +#define SCB_CTRL_BUSY_MASK BIT(SCB_CTRL_BUSY)
> +
> +#define SCB_CTRL_ABORT (2)
> +#define SCB_CTRL_ABORT_MASK BIT(SCB_CTRL_ABORT)
> +
> +#define SCB_CTRL_NOTIFY (3)
> +#define SCB_CTRL_NOTIFY_MASK BIT(SCB_CTRL_NOTIFY)
> +
> +#define SCB_CTRL_POS (16)
> +#define SCB_CTRL_MASK GENMASK(SCB_CTRL_POS + SCB_MASK_WIDTH, SCB_CTRL_POS)
> +
> +/* SCBCTRL service status register */
> +
> +#define SCB_STATUS_REQ (0)
> +#define SCB_STATUS_REQ_MASK BIT(SCB_STATUS_REQ)
> +
> +#define SCB_STATUS_BUSY (1)
> +#define SCB_STATUS_BUSY_MASK BIT(SCB_STATUS_BUSY)
> +
> +#define SCB_STATUS_ABORT (2)
> +#define SCB_STATUS_ABORT_MASK BIT(SCB_STATUS_ABORT)
> +
> +#define SCB_STATUS_NOTIFY (3)
> +#define SCB_STATUS_NOTIFY_MASK BIT(SCB_STATUS_NOTIFY)
> +
> +#define SCB_STATUS_POS (16)
> +#define SCB_STATUS_MASK GENMASK(SCB_STATUS_POS + SCB_MASK_WIDTH, SCB_STATUS_POS)
> +
Please run checkpatch with strict option on the patchset.


> +
> +static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +       u32 mailbox_val = 0u;
> +       u16 options_select;
> +       u32 tx_trigger;
>
just a nit... here and elsewhere, can the variables be lesser verbose?


> +
> +static void mpfs_mbox_rx_data(struct mbox_chan *chan)
> +{
> +       struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> +       u32 i;
> +       u16 num_words = ALIGN((mbox->response_size), (4)) / 4U; //TODO better way?
> +       struct mpfs_mss_response *response;
> +
> +       response = kmalloc(sizeof(*response), GFP_ATOMIC);
> +       response->response_size = mbox->response_size;
> +       response->response_msg = kcalloc(num_words, sizeof(response->response_msg), GFP_ATOMIC);
> +
> +       if (!response->response_msg) {
> +               dev_err(mbox->dev, "failed to assign memory for response %d\n", -ENOMEM);
> +               return;
> +       }
> +
response_size is provided by the client driver.
So why not simply get the buffer from the client and just fill it
here? That is simpler and better (avoid alloc in isr) and the right
thing to do.


> +
> +static int mpfs_mbox_probe(struct platform_device *pdev)
> +{
> +       struct mpfs_mbox *mbox;
> +       struct resource *regs;
> +       struct mbox_chan *chans;
> +       int ret;
> +
> +       mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL);
> +       if (!mbox)
> +               return -ENOMEM;
> +
> +       chans = devm_kzalloc(&pdev->dev, sizeof(*chans), GFP_KERNEL);
>
You may simply embed the mbox_chan in mpfs_mbox.

cheers.