Re: [PATCH v2 2/2] mailbox: apple: Add driver for Apple mailboxes

["Sven Peter" <sven@svenpeter.dev>]

Hi Jassi,

Thanks a lot for the review!

On Sat, Oct 16, 2021, at 21:04, Jassi Brar wrote:
> On Thu, Sep 16, 2021 at 10:49 AM Sven Peter <sven@svenpeter.dev> wrote:
>
> ....
>> +static const struct apple_mbox_hw apple_mbox_asc_hw = {
>> +       .control_full = APPLE_ASC_MBOX_CONTROL_FULL,
>> +       .control_empty = APPLE_ASC_MBOX_CONTROL_EMPTY,
>> +
>> +       .a2i_control = APPLE_ASC_MBOX_A2I_CONTROL,
>> +       .a2i_send0 = APPLE_ASC_MBOX_A2I_SEND0,
>> +       .a2i_send1 = APPLE_ASC_MBOX_A2I_SEND1,
>> +
>> +       .i2a_control = APPLE_ASC_MBOX_I2A_CONTROL,
>> +       .i2a_recv0 = APPLE_ASC_MBOX_I2A_RECV0,
>> +       .i2a_recv1 = APPLE_ASC_MBOX_I2A_RECV1,
>> +
>> +       .has_irq_controls = false,
>> +};
>> +
>> +static const struct apple_mbox_hw apple_mbox_m3_hw = {
>> +       .control_full = APPLE_M3_MBOX_CONTROL_FULL,
>> +       .control_empty = APPLE_M3_MBOX_CONTROL_EMPTY,
>> +
>> +       .a2i_control = APPLE_M3_MBOX_A2I_CONTROL,
>> +       .a2i_send0 = APPLE_M3_MBOX_A2I_SEND0,
>> +       .a2i_send1 = APPLE_M3_MBOX_A2I_SEND1,
>> +
>> +       .i2a_control = APPLE_M3_MBOX_I2A_CONTROL,
>> +       .i2a_recv0 = APPLE_M3_MBOX_I2A_RECV0,
>> +       .i2a_recv1 = APPLE_M3_MBOX_I2A_RECV1,
>> +
>> +       .has_irq_controls = true,
>> +       .irq_enable = APPLE_M3_MBOX_IRQ_ENABLE,
>> +       .irq_ack = APPLE_M3_MBOX_IRQ_ACK,
>> +       .irq_bit_recv_not_empty = APPLE_M3_MBOX_IRQ_I2A_NOT_EMPTY,
>> +       .irq_bit_send_empty = APPLE_M3_MBOX_IRQ_A2I_EMPTY,
>> +};
>> +
>> +static const struct of_device_id apple_mbox_of_match[] = {
>> +       { .compatible = "apple,t8103-asc-mailbox", .data = &apple_mbox_asc_hw },
>> +       { .compatible = "apple,t8103-m3-mailbox", .data = &apple_mbox_m3_hw },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(of, apple_mbox_of_match);
>> +
> Traditionally, these go at the end of file.

Ack.

>
> ....
>> +
>> +static int apple_mbox_hw_send(struct apple_mbox *apple_mbox,
>> +                             struct apple_mbox_msg *msg)
>> +{
>> +       if (!apple_mbox_hw_can_send(apple_mbox))
>> +               return -EBUSY;
>> +
>> +       dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1);
>> +
>> +       /*
>> +        * This message may be related to a shared memory buffer and we must
>> +        * ensure all previous writes to normal memory are visible before
>> +        * submitting it.
>> +        */
>> +       dma_wmb();
>> +
> This may cause unnecessary overhead.
> mbox_client.tx_prepare() is where the SHMEM data is copied, which
> should also call dma_wmb() just before return.
> ......
>

Ok, I'll just drop it here then.

>> +
>> +static int apple_mbox_hw_recv(struct apple_mbox *apple_mbox,
>> +                             struct apple_mbox_msg *msg)
>> +{
>> +       if (!apple_mbox_hw_can_recv(apple_mbox))
>> +               return -ENOMSG;
>> +
>> +       msg->msg0 = readq_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_recv0);
>> +       msg->msg1 = FIELD_GET(
>> +               APPLE_MBOX_MSG1_MSG,
>> +               readq_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_recv1));
>> +
>> +       dev_dbg(apple_mbox->dev, "< RX %016llx %08x\n", msg->msg0, msg->msg1);
>> +
>> +       /*
>> +        * This message may be related to a shared memory buffer and we must
>> +        * ensure any following reads from normal memory only happen after
>> +        * having read this message.
>> +        */
>> +       dma_rmb();
>> +
> .... similarly should be done by the client as the first thing in recv callback.

Ok.

>
>
>> +static struct mbox_chan *apple_mbox_of_xlate(struct mbox_controller *mbox,
>> +                                            const struct of_phandle_args *spec)
>> +{
>> +       struct apple_mbox *apple_mbox = dev_get_drvdata(mbox->dev);
>> +
>> +       if (spec->args_count != 0)
>> +               return ERR_PTR(-EINVAL);
>> +       if (apple_mbox->chan.con_priv)
>> +               return ERR_PTR(-EBUSY);
>> +
>> +       apple_mbox->chan.con_priv = apple_mbox;
>> +       return &apple_mbox->chan;
>> +}
>> +
> we could do without of_xlate callback, by assigning  chan.con_priv
> already in the .probe()

Sure, will do that.

>
>
>> diff --git a/include/linux/apple-mailbox.h b/include/linux/apple-mailbox.h
>> +
>> +struct apple_mbox_msg {
>> +       u64 msg0;
>> +       u32 msg1;
>> +};
>> +
> Aren't msg0/1 the Tx and Rx channels? If so you may want to separate
> them out as such. But of course, I don't know the h/w details so I may
> be wrong.

This hardware essentially only has a single channel.
Depending on the firmware running on the other side sometimes msg1 is used
as an endpoint index and sometimes 8bits of msg0 are. This is similar to the
discussion about the raspberry pi mailbox hardware [1].



Thanks,


Sven


[1] https://lore.kernel.org/all/550C7534.8070100@wwwdotorg.org/