Hello,
I've added review comments below. Some of them might be more detailed
than necessary, and reflect my opinion rather than something that must
be fixed. Anyway, I hope my comments make sense.
On Wed, Dec 23, 2020 at 04:32:55PM +0000, conor.dooley@xxxxxxxxxxxxx wrote:
> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>
> This driver adds support for the single mailbox channel of the MSS
> system controller on the Microchip PolarFire SoC.
>
> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> ---
[...]
> +#define SERVICES_CR_OFFSET 0x50u
> +#define SERVICES_SR_OFFSET 0x54u
> +#define MAILBOX_REG_OFFSET 0x800u
> +#define MSS_SYS_BUSY -EBUSY
> +#define MSS_SYS_PARAM_ERR -EINVAL
Defining your own aliases for -Esomething is rather uncommon.
> +#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 registers */
registers -> register ?
It seems to be only one 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)
> +
> +struct mpfs_mbox {
> + struct mbox_controller controller;
> + struct device *dev;
> + int irq;
> + void __iomem *mailbox_base;
> + void __iomem *int_reg;
> + struct mbox_chan *chan;
> + u16 response_size;
> + u16 response_offset;
> +};
> +
> +static bool mpfs_mbox_busy(struct mpfs_mbox *mbox)
> +{
> + u32 status;
> +
> + status = readl_relaxed(mbox->mailbox_base + SERVICES_SR_OFFSET);
> +
> + return status & SCB_STATUS_BUSY_MASK;
> +}
> +
> +static struct mpfs_mbox *mbox_chan_to_mpfs_mbox(struct mbox_chan *chan)
> +{
> + if (!chan)
> + return NULL;
> +
> + return (struct mpfs_mbox *)chan->con_priv;
> +}
> +
> +static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> + u32 index;
> + u32 *word_buf;
> + u8 *byte_buf;
> + u8 byte_off;
> + u8 extra_bits;
> + u8 i;
> + u32 mailbox_val = 0u;
> + u16 mbox_offset;
> + u16 mbox_options_select;
> + u32 mbox_tx_trigger;
the mbox_ prefix seems unnecessary because the whole driver deals with
a mailbox.
Some of the variables are only used in if/for scopes below. I'd move
their declaration down into these scopes, to make the outer scope of the
function a little easier to understand.
> + struct mpfs_mss_msg *msg = data;
> + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> +
> + mbox_offset = msg->mailbox_offset;
mbox_offset is only used once. The indirection of storing the value in
another variable makes the code slightly slower to read, IMO.
> + mbox->response_size = msg->response_size;
> + mbox->response_offset = msg->response_offset;
> +
> + if (mpfs_mbox_busy(mbox))
> + return MSS_SYS_BUSY;
> +
> + mbox_options_select = ((mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu));
> + mbox_tx_trigger = (((mbox_options_select << SCB_CTRL_POS) &
> + SCB_CTRL_MASK) | SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK);
Slightly reformatted, you could save a few parentheses:
mbox_options_select = (mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu);
mbox_tx_trigger = (mbox_options_select << SCB_CTRL_POS) & SCB_CTRL_MASK;
mbox_tx_trigger |= SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK;
> + /* Code for MSS_SYS_PARAM_ERR is not implemented with this version of driver. */
What is the "code for MSS_SYS_PARAM_ERR" semantically? Input validation?
> + writel_relaxed(0, mbox->int_reg);
What does a write to mbox->int_reg do? Does it acknowledge an interrupt?
It would be interesting to have an explaining comment here.
> +
> + if (msg->cmd_data_size) {
> + byte_buf = msg->cmd_data;
> +
> + for (index = 0; index < (msg->cmd_data_size / 4); index++)
> + writel_relaxed(word_buf[index],
> + mbox->mailbox_base + MAILBOX_REG_OFFSET + index);
word_buf is uninitialized.
In mpfs_mbox_rx_data, you access the registers at mbox->mailbox_base +
MAILBOX_REG_OFFSET in steps of four bytes, here you increment the offset
in steps of one byte, because the index isn't scaled. This seems wrong.
> + extra_bits = msg->cmd_data_size & 3;
> + if (extra_bits) {
> + byte_off = ALIGN_DOWN(msg->cmd_data_size, 4);
> + byte_buf = msg->cmd_data + byte_off;
> +
> + mailbox_val = readl_relaxed(mbox->mailbox_base +
> + MAILBOX_REG_OFFSET + index);
> +
> + for (i = 0u; i < extra_bits; i++) {
> + mailbox_val &= ~(0xffu << (i * 8u));
> + mailbox_val |= (byte_buf[i] << (i * 8u));
> + }
> +
> + writel_relaxed(mailbox_val,
> + mbox->mailbox_base + MAILBOX_REG_OFFSET + index);
> + }
> + }
> +
I'd move the calculation of mbox_tx_trigger down here, right before its
use, because it's not necessary for the code above (if I'm reading it
correctly).
> + writel_relaxed(mbox_tx_trigger, mbox->mailbox_base + SERVICES_CR_OFFSET);
> +
> + return 0;
> +}
> +
> +static inline bool mpfs_mbox_pending(struct mpfs_mbox *mbox)
> +{
> + u32 status;
> +
> + status = readl_relaxed(mbox->mailbox_base + SERVICES_SR_OFFSET);
> +
> + return !(status & SCB_STATUS_BUSY_MASK);
> +}
> +
> +static void mpfs_mbox_rx_data(struct mbox_chan *chan)
> +{
> + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> + u32 *data;
> + u32 i;
> + u32 response_limit;
> +
> + data = devm_kzalloc(mbox->dev, sizeof(*data) * mbox->response_size, GFP_ATOMIC);
The use of devm_ seems bogus here, because the allocated data does not
have the same lifetime as the device; it is allocated and freed for
every RX event.
However, please use kcalloc instead of kzalloc, so you don't have to do
the multiplication manually. kcalloc checks for overflows.
> + if (!data)
> + dev_err(mbox->dev, "failed to assign memory for response\n", -ENOMEM);
There is no format specifier for parameter -ENOMEM.
In the !data case, the code will still run into the loop below, and
probably cause a crash.
> +
> + response_limit = mbox->response_size + mbox->response_offset;
> + if (mpfs_mbox_pending(mbox) && mbox->response_size > 0U) {
> + for (i = mbox->response_offset; i < response_limit; i++) {
> + data[i - mbox->response_offset] =
> + readl_relaxed(mbox->mailbox_base + MAILBOX_REG_OFFSET + i * 0x4);
> + }
> + }
It seems slightly easier to me to let the loop run from zero to
mbox->response_size-1. Most importantly, it becomes easy to see that
data is not accessed out of bounds. (But it's your choice)
if (mpfs_mbox_pending(mbox)) {
for (i = 0; i < mbox->response_size; i++) {
data[i] = readl_relaxed(mbox->mailbox_base + MAILBOX_REG_OFFSET +
(i + mbox->response_offset) * 0x4);
}
}
> +
> + mbox_chan_received_data(chan, (void *)data);
> + devm_kfree(mbox->dev, data);
> +}
> +
> +static irqreturn_t mpfs_mbox_inbox_isr(int irq, void *data)
> +{
> + struct mbox_chan *chan = (struct mbox_chan *)data;
This cast and the one at the end of mpfs_mbox_rx_data are somewhat
uncessary, because C allows implicit conversion of void pointers to and
from other pointer types.
> + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> +
> + writel_relaxed(0, mbox->int_reg);
> +
> + mpfs_mbox_rx_data(chan);
> +
> + mbox_chan_txdone(chan, 0);
> + return IRQ_HANDLED;
> +}
[...]
> +++ b/include/soc/microchip/mpfs.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *
> + * Microchip PolarFire SoC (MPFS) mailbox
This is mpfs.h, but the comment implies it's only for mailbox-related
code, not for all of the Microchip PolarFire SoC. Is this intentional?
Best regards,
Jonathan Neuschäfer
Attachment:
signature.asc
Description: PGP signature
