On Thu, Sep 16, 2021 at 10:49 AM Sven Peter <sven@xxxxxxxxxxxxx> 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.
....
> +
> +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.
......
> +
> +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.
> +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()
> 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.
Cheers!
