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: <20210130022124.65083-1-mgross@linux.intel.com> <20210130022124.65083-4-mgross@linux.intel.com>
In-Reply-To: <20210130022124.65083-4-mgross@linux.intel.com>
From:   Jassi Brar <jassisinghbrar@gmail.com>
Date:   Mon, 1 Feb 2021 01:07:01 -0600
Message-ID: <CABb+yY0cE=qkU7pLx6W-3gJzGOnHbkt-ThPm67fQKGo+79fvDQ@mail.gmail.com>
Subject: Re: [PATCH v3 03/34] mailbox: vpu-ipc-mailbox: Add support for Intel
 VPU IPC mailbox
To:     mgross@linux.intel.com
Cc:     markgross@kernel.org, "arnd@arndb.de" <arnd@arndb.de>, bp@suse.de,
        damien.lemoal@wdc.com, dragan.cvetic@xilinx.com,
        Greg KH <gregkh@linuxfoundation.org>,
        Jonathan Corbet <corbet@lwn.net>, palmerdabbelt@google.com,
        paul.walmsley@sifive.com, Peng Fan <peng.fan@nxp.com>,
        Rob Herring <robh+dt@kernel.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Daniele Alessandrelli <daniele.alessandrelli@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 29, 2021 at 8:21 PM <mgross@linux.intel.com> wrote:
>
> From: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>
> Add mailbox controller enabling inter-processor communication (IPC)
> between the CPU (aka, the Application Processor - AP) and the VPU on
> Intel Movidius SoCs like Keem Bay.
>
> The controller uses HW FIFOs to enable such communication. Specifically,
> there are two FIFOs, one for the CPU and one for VPU. Each FIFO can hold
> 128 entries (messages) of 32-bit each (but only 26 bits are actually
> usable, since the 6 least-significant bits are reserved).
>
> When the Linux kernel on the AP needs to send messages to the VPU
> firmware, it writes them to the VPU FIFO; similarly, when the VPU
> firmware needs to send messages to the AP, it writes them to the CPU
> FIFO.
>
> The AP is notified of pending messages in the CPU FIFO by means of the
> 'FIFO-not-empty' interrupt, which is generated by the CPU FIFO while not
> empty. This interrupt is cleared automatically once all messages have
> been read from the FIFO (i.e., the FIFO has been emptied).
>
> The hardware doesn't provide an TX done IRQ (i.e., an IRQ that allows
> the VPU firmware to notify the AP that the message put into the VPU FIFO
> has been received); however the AP can ensure that the message has been
> successfully put into the VPU FIFO (and therefore transmitted) by
> checking the VPU FIFO status register to ensure that writing the message
> didn't cause the FIFO to overflow.
>
> Therefore, the mailbox controller is configured as capable of tx_done
> IRQs and a tasklet is used to simulate the tx_done IRQ. The tasklet is
> activated by send_data() right after the message has been put into the
> VPU FIFO and the VPU FIFO status registers has been checked. If an
> overflow is reported by the status register, the tasklet passes -EBUSY
> to mbox_chan_txdone(), to notify the mailbox client of the failed TX.
>
> The client should therefore register a tx_done() callback to properly
> handle failed transmissions.
>
> Note: the 'txdone_poll' mechanism cannot be used because it doesn't
> provide a way to report a failed transmission.
>
txdone means the last submitted transfer has been done with --
successfully or not.
So I think we can do without the tasklet as explained below....

....

> +static int vpu_ipc_mailbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct vpu_ipc_mbox *vpu_ipc_mbox = chan->con_priv;
> +       u32 entry, overflow;
> +
> +       entry = *((u32 *)data);
> +
Are all messages max 32bits wide?
Usually the controller specifies a packet format (more than just a
word but of course that's not mandatory) that a client submits the
data to be transmitted in. Esp when it has deep FIFOs.

> +       /* Ensure last 6-bits of entry are not used. */
> +       if (unlikely(entry & IPC_FIFO_ENTRY_RSVD_MASK)) {
> +               vpu_ipc_mbox->txdone_result = -EINVAL;
> +               goto exit;
> +       }
> +
> +       /* Add processor ID to entry. */
> +       entry |= IPC_FIFO_ID_CPU & IPC_FIFO_ENTRY_RSVD_MASK;
> +
> +       /* Write entry to VPU FIFO. */
> +       iowrite32(entry, vpu_ipc_mbox->vpu_fifo_base + IPC_FIFO);
> +
> +       /* Check if we overflew the VPU FIFO. */
> +       overflow = ioread32(vpu_ipc_mbox->vpu_fifo_base + IPC_FIFO_OF_FLAG0) &
> +                  BIT(IPC_FIFO_ID_CPU);
> +       if (unlikely(overflow)) {
> +               /* Reset overflow register. */
> +               iowrite32(BIT(IPC_FIFO_ID_CPU),
> +                         vpu_ipc_mbox->vpu_fifo_base + IPC_FIFO_OF_FLAG0);
> +               vpu_ipc_mbox->txdone_result = -EBUSY;
> +               goto exit;
> +       }
> +       vpu_ipc_mbox->txdone_result = 0;
> +
> +exit:
> +       /* Schedule tasklet to call mbox_chan_txdone(). */
> +       tasklet_schedule(&vpu_ipc_mbox->txdone_tasklet);
> +
> +       return 0;
> +}
> +
Maybe set txdone_poll and implement last_tx_done()  where you can wait
for FIFO to have enough space for another message, so that the next
submitted request will never return -EBUSY.

thanks