Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210916154911.3168-1-sven@svenpeter.dev> <20210916154911.3168-3-sven@svenpeter.dev>
In-Reply-To: <20210916154911.3168-3-sven@svenpeter.dev>
From:   Jassi Brar <jassisinghbrar@gmail.com>
Date:   Sat, 16 Oct 2021 14:04:34 -0500
Message-ID: <CABb+yY2XxpQa-QqsVnzVsYb+msSiOiELE1R5UFjE02Diwww2Ew@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] mailbox: apple: Add driver for Apple mailboxes
To:     Sven Peter <sven@svenpeter.dev>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Mark Kettenis <mark.kettenis@xs4all.nl>,
        Hector Martin <marcan@marcan.st>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Mohamed Mediouni <mohamed.mediouni@caramail.com>,
        Stan Skowronek <stan@corellium.com>,
        Devicetree List <devicetree@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

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