Received-SPF: pass (google.com: domain of linux-kernel+bounces-5700-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1350-g1d0a93a8fb-fm-20231218.001-g1d0a93a8
Precedence: bulk
MIME-Version: 1.0
Message-Id: <fcb410dd-f042-4469-9319-040c26099b2b@app.fastmail.com>
In-Reply-To: <20231218061201.98136-2-joshua.yeong@starfivetech.com>
References: <20231218061201.98136-1-joshua.yeong@starfivetech.com>
 <20231218061201.98136-2-joshua.yeong@starfivetech.com>
Date: Tue, 19 Dec 2023 17:38:23 +0000
From: "Arnd Bergmann" <arnd@arndb.de>
To: "Joshua Yeong" <joshua.yeong@starfivetech.com>,
 jeeheng.sia@starfivetech.com, leyfoon.tan@starfivetech.com,
 "Jassi Brar" <jassisinghbrar@gmail.com>, "Rob Herring" <robh+dt@kernel.org>,
 krzysztof.kozlowski+dt@linaro.org, "Conor Dooley" <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mailbox: starfive: Add StarFive Meu Mailbox Driver
Content-Type: text/plain

On Mon, Dec 18, 2023, at 06:11, Joshua Yeong wrote:
> This patch adds support for the StarFive Meu Mailbox driver. This enables
> communication using mailbox doorbell between AP and SCP cores.

I think the naming here is a bit confusing. As far as I can tell, this
is not a mailbox at all but just a doorbell, so maybe clarify that in the
wording.

It could be turned into a mailbox if you add a shared memory segment of
course, but neither the driver nor the binding specifies how to use that.

> +config STARFIVE_MEU_MBOX
> +	tristate "Starfive MEU Mailbox"

Please spell out MEU here, I don't think that is a well understood acronynm,
at least I have never heard of it.

> +	depends on OF
> +	depends on SOC_STARFIVE || COMPILE_TEST
> +	help
> +	  Say Y here if you want to build the Starfive MEU mailbox controller
> +	  driver.
> +
> +	  The StarFive mailbox controller has 2 channels. Each channel has a
> +	  pair of Tx/Rx link and each link has 31 slots/doorbells.

What is the significance of the "channels" as opposed to "slots"?

The binding seems to indicate that there are just 62 fully equal
doorbell channels as far as consumers are concerned. For efficiency
one might want to of course only use one doorbell per channel here
to avoid the interrupt sharing.

> +static inline struct mbox_chan *
> +meu_db_mbox_to_channel(struct mbox_controller *mbox, unsigned int 
> pchan,
> +		       unsigned int doorbell)
> +{
> +	struct meu_db_channel *chan_info;
> +	int i;
> +
> +	for (i = 0; i < mbox->num_chans; i++) {
> +		chan_info = mbox->chans[i].con_priv;
> +		if (chan_info && chan_info->pchan == pchan &&
> +		    chan_info->doorbell == doorbell)
> +			return &mbox->chans[i];
> +	}
> +
> +	return NULL;
> +}

If the number of doorbells is known in advance, maybe use
use it as the array index here?

> +static void meu_db_mbox_clear_irq(struct mbox_chan *chan)
> +{
> +	struct meu_db_channel *chan_info = chan->con_priv;
> +	void __iomem *base = chan_info->meu->mlink[chan_info->pchan].rx_reg;
> +
> +	writel_relaxed(BIT(chan_info->doorbell), base + CHAN_CLR_OFFSET);
> +}

Any reason to use writel_relaxed() instead of writel() here? Please
add a comment if this is necessary, otherwise this risks adding
races if someone tries to use the doorbell in combination with
shared memory.


> +static struct mbox_chan *
> +meu_db_mbox_irq_to_channel(struct starfive_meu *meu, unsigned int 
> pchan)
> +{
> +	void __iomem *base = meu->mlink[pchan].rx_reg;
> +	struct mbox_controller *mbox = &meu->mbox;
> +	struct mbox_chan *chan = NULL;
> +	unsigned int doorbell;
> +	unsigned long bits;
> +
> +	bits = FIELD_GET(CHAN_RX_DOORBELL, readl_relaxed(base + 
> CHAN_RX_STAT_OFFSET));
> +	if (!bits)
> +		/* No IRQs fired in specified physical channel */
> +		return NULL;
> +
> +	/* An IRQ has fired, find the associated channel */
> +	for (doorbell = 0; bits; doorbell++) {
> +		if (!test_and_clear_bit(doorbell, &bits))
> +			continue;

There is no need to use atomic updates on stack variables. Just
use for_each_set_bit() to do the loop here.

> +		chan = meu_db_mbox_to_channel(mbox, pchan, doorbell);
> +		if (chan)
> +			break;
> +
> +		/* Clear IRQ for unregistered doorbell */
> +		writel_relaxed(BIT(doorbell), base + CHAN_CLR_OFFSET);

Again, add a comment about the use of _relaxed() here.


> +static bool meu_db_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct meu_db_channel *chan_info = chan->con_priv;
> +	void __iomem *base = chan_info->meu->mlink[chan_info->pchan].tx_reg;
> +
> +	if (readl_relaxed(base + CHAN_TX_STAT_OFFSET) & 
> BIT(chan_info->doorbell))

The readl_relaxed() again prevents the use of shared memory, so
change it to a normal readl() or add a comment about how ordering
is maintained.

     Arnd