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

[Conor.Dooley at microchip.com]

On 02/01/2021 13:01, Jonathan Neuschäfer wrote

>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.
>
the more detailed feedback the better in my book, if i dont mention it youll probably see in changed in the next day or two
...
...
>
>
>> +    /* 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.

both of these are hang overs from the bare metal driver and will be dropped
>> +
>> +    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.
>

Thanks for catching this bug, both are related and were introduced during refactoring.
The only dependent drivers implemented so far don't use the full sending functionality
so it went unnoticed.

...
>
>
>> +
>> +    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.
>

true, i had put them in thinking it made it more clear, but on reflection it doesnt.

...
>> +++ 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?
>

yeah, while thats correct for now it wont remain that way for long. ill drop the mailbox reference

>
>Best regards,
>Jonathan Neuschäfer