Re: [PATCH v2 04/10] mailbox: tegra-hsp: Add support for shared mailboxes

[Jon Hunter <jonathanh@nvidia.com>]

On 13/11/2018 13:09, Thierry Reding wrote:
> On Tue, Nov 13, 2018 at 11:09:22AM +0000, Jon Hunter wrote:
>>
>> On 12/11/2018 15:18, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> The Tegra HSP block supports 'shared mailboxes' that are simple 32-bit
>>> registers consisting of a FULL bit in MSB position and 31 bits of data.
>>> The hardware can be configured to trigger interrupts when a mailbox
>>> is empty or full. Add support for these shared mailboxes to the HSP
>>> driver.
>>>
>>> The initial use for the mailboxes is the Tegra Combined UART. For this
>>> purpose, we use interrupts to receive data, and spinning to wait for
>>> the transmit mailbox to be emptied to minimize unnecessary overhead.
>>>
>>> Based on work by Mikko Perttunen <mperttunen@nvidia.com>.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> Changes in v2:
>>> - do not write per-mailbox interrupt enable registers on Tegra186
>>> - merge handlers for empty and full interrupts
>>> - track direction of shared mailboxes
>>>
>>>  drivers/mailbox/tegra-hsp.c | 498 +++++++++++++++++++++++++++++++-----
>>>  1 file changed, 437 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
>>> index 0cde356c11ab..0100a974149b 100644
>>> --- a/drivers/mailbox/tegra-hsp.c
>>> +++ b/drivers/mailbox/tegra-hsp.c
>>> @@ -1,5 +1,5 @@
>>>  /*
>>> - * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
>>> + * Copyright (c) 2016-2018, NVIDIA CORPORATION.  All rights reserved.
>>>   *
>>>   * This program is free software; you can redistribute it and/or modify it
>>>   * under the terms and conditions of the GNU General Public License,
>>> @@ -11,6 +11,7 @@
>>>   * more details.
>>>   */
>>>  
>>> +#include <linux/delay.h>
>>>  #include <linux/interrupt.h>
>>>  #include <linux/io.h>
>>>  #include <linux/mailbox_controller.h>
>>> @@ -21,6 +22,17 @@
>>>  
>>>  #include <dt-bindings/mailbox/tegra186-hsp.h>
>>>  
>>> +#include "mailbox.h"
>>> +
>>> +#define HSP_INT_IE(x)		(0x100 + ((x) * 4))
>>> +#define HSP_INT_IV		0x300
>>> +#define HSP_INT_IR		0x304
>>> +
>>> +#define HSP_INT_EMPTY_SHIFT	0
>>> +#define HSP_INT_EMPTY_MASK	0xff
>>> +#define HSP_INT_FULL_SHIFT	8
>>> +#define HSP_INT_FULL_MASK	0xff
>>> +
>>>  #define HSP_INT_DIMENSIONING	0x380
>>>  #define HSP_nSM_SHIFT		0
>>>  #define HSP_nSS_SHIFT		4
>>> @@ -34,6 +46,11 @@
>>>  #define HSP_DB_RAW	0x8
>>>  #define HSP_DB_PENDING	0xc
>>>  
>>> +#define HSP_SM_SHRD_MBOX	0x0
>>> +#define HSP_SM_SHRD_MBOX_FULL	BIT(31)
>>> +#define HSP_SM_SHRD_MBOX_FULL_INT_IE	0x04
>>> +#define HSP_SM_SHRD_MBOX_EMPTY_INT_IE	0x08
>>> +
>>>  #define HSP_DB_CCPLEX		1
>>>  #define HSP_DB_BPMP		3
>>>  #define HSP_DB_MAX		7
>>> @@ -55,6 +72,12 @@ struct tegra_hsp_doorbell {
>>>  	unsigned int index;
>>>  };
>>>  
>>> +struct tegra_hsp_mailbox {
>>> +	struct tegra_hsp_channel channel;
>>> +	unsigned int index;
>>> +	bool producer;
>>> +};
>>> +
>>>  struct tegra_hsp_db_map {
>>>  	const char *name;
>>>  	unsigned int master;
>>> @@ -63,13 +86,18 @@ struct tegra_hsp_db_map {
>>>  
>>>  struct tegra_hsp_soc {
>>>  	const struct tegra_hsp_db_map *map;
>>> +	bool has_per_mb_ie;
>>>  };
>>>  
>>>  struct tegra_hsp {
>>> +	struct device *dev;
>>>  	const struct tegra_hsp_soc *soc;
>>> -	struct mbox_controller mbox;
>>> +	struct mbox_controller mbox_db;
>>> +	struct mbox_controller mbox_sm;
>>>  	void __iomem *regs;
>>> -	unsigned int irq;
>>> +	unsigned int doorbell_irq;
>>> +	unsigned int *shared_irqs;
>>> +	unsigned int shared_irq;
>>>  	unsigned int num_sm;
>>>  	unsigned int num_as;
>>>  	unsigned int num_ss;
>>> @@ -78,13 +106,10 @@ struct tegra_hsp {
>>>  	spinlock_t lock;
>>>  
>>>  	struct list_head doorbells;
>>> -};
>>> +	struct tegra_hsp_mailbox *mailboxes;
>>>  
>>> -static inline struct tegra_hsp *
>>> -to_tegra_hsp(struct mbox_controller *mbox)
>>> -{
>>> -	return container_of(mbox, struct tegra_hsp, mbox);
>>> -}
>>> +	unsigned long mask;
>>> +};
>>>  
>>>  static inline u32 tegra_hsp_readl(struct tegra_hsp *hsp, unsigned int offset)
>>>  {
>>> @@ -158,7 +183,7 @@ static irqreturn_t tegra_hsp_doorbell_irq(int irq, void *data)
>>>  
>>>  	spin_lock(&hsp->lock);
>>>  
>>> -	for_each_set_bit(master, &value, hsp->mbox.num_chans) {
>>> +	for_each_set_bit(master, &value, hsp->mbox_db.num_chans) {
>>>  		struct tegra_hsp_doorbell *db;
>>>  
>>>  		db = __tegra_hsp_doorbell_get(hsp, master);
>>> @@ -182,6 +207,71 @@ static irqreturn_t tegra_hsp_doorbell_irq(int irq, void *data)
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> +static irqreturn_t tegra_hsp_shared_irq(int irq, void *data)
>>> +{
>>> +	struct tegra_hsp *hsp = data;
>>> +	unsigned long bit, mask;
>>> +	u32 status, value;
>>> +	void *msg;
>>> +
>>> +	status = tegra_hsp_readl(hsp, HSP_INT_IR) & hsp->mask;
>>> +
>>> +	/* process EMPTY interrupts first */
>>> +	mask = (status >> HSP_INT_EMPTY_SHIFT) & HSP_INT_EMPTY_MASK;
>>> +
>>> +	for_each_set_bit(bit, &mask, hsp->num_sm) {
>>> +		struct tegra_hsp_mailbox *mb = &hsp->mailboxes[bit];
>>> +
>>> +		if (mb->producer) {
>>> +			/*
>>> +			 * Disable EMPTY interrupts until data is sent with
>>> +			 * the next message. These interrupts are level-
>>> +			 * triggered, so if we kept them enabled they would
>>> +			 * constantly trigger until we next write data into
>>> +			 * the message.
>>> +			 */
>>> +			spin_lock(&hsp->lock);
>>> +
>>> +			hsp->mask &= ~BIT(HSP_INT_EMPTY_SHIFT + mb->index);
>>> +			tegra_hsp_writel(hsp, hsp->mask,
>>> +					 HSP_INT_IE(hsp->shared_irq));
>>> +
>>> +			spin_unlock(&hsp->lock);
>>> +
>>> +			mbox_chan_txdone(mb->channel.chan, 0);
>>> +		}
>>> +	}
>>> +
>>> +	/* process FULL interrupts */
>>> +	mask = (status >> HSP_INT_FULL_SHIFT) & HSP_INT_FULL_MASK;
>>> +
>>> +	for_each_set_bit(bit, &mask, hsp->num_sm) {
>>> +		struct tegra_hsp_mailbox *mb = &hsp->mailboxes[bit];
>>> +
>>> +		if (!mb->producer) {
>>> +			value = tegra_hsp_channel_readl(&mb->channel,
>>> +							HSP_SM_SHRD_MBOX);
>>> +			value &= ~HSP_SM_SHRD_MBOX_FULL;
>>> +			msg = (void *)(unsigned long)value;
>>> +			mbox_chan_received_data(mb->channel.chan, msg);
>>> +
>>> +			/*
>>> +			 * Need to clear all bits here since some producers,
>>> +			 * such as TCU, depend on fields in the register
>>> +			 * getting cleared by the consumer.
>>> +			 *
>>> +			 * The mailbox API doesn't give the consumers a way
>>> +			 * of doing that explicitly, so we have to make sure
>>> +			 * we cover all possible cases.
>>> +			 */
>>> +			tegra_hsp_channel_writel(&mb->channel, 0x0,
>>> +						 HSP_SM_SHRD_MBOX);
>>> +		}
>>> +	}
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>>  static struct tegra_hsp_channel *
>>>  tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
>>>  			  unsigned int master, unsigned int index)
>>> @@ -194,7 +284,7 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
>>>  	if (!db)
>>>  		return ERR_PTR(-ENOMEM);
>>>  
>>> -	offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) << 16;
>>> +	offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) * SZ_64K;
>>>  	offset += index * 0x100;
>>>  
>>>  	db->channel.regs = hsp->regs + offset;
>>> @@ -235,8 +325,8 @@ static int tegra_hsp_doorbell_startup(struct mbox_chan *chan)
>>>  	unsigned long flags;
>>>  	u32 value;
>>>  
>>> -	if (db->master >= hsp->mbox.num_chans) {
>>> -		dev_err(hsp->mbox.dev,
>>> +	if (db->master >= chan->mbox->num_chans) {
>>> +		dev_err(chan->mbox->dev,
>>>  			"invalid master ID %u for HSP channel\n",
>>>  			db->master);
>>>  		return -EINVAL;
>>> @@ -281,46 +371,168 @@ static void tegra_hsp_doorbell_shutdown(struct mbox_chan *chan)
>>>  	spin_unlock_irqrestore(&hsp->lock, flags);
>>>  }
>>>  
>>> -static const struct mbox_chan_ops tegra_hsp_doorbell_ops = {
>>> +static const struct mbox_chan_ops tegra_hsp_db_ops = {
>>>  	.send_data = tegra_hsp_doorbell_send_data,
>>>  	.startup = tegra_hsp_doorbell_startup,
>>>  	.shutdown = tegra_hsp_doorbell_shutdown,
>>>  };
>>>  
>>> -static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
>>> +static int tegra_hsp_mailbox_send_data(struct mbox_chan *chan, void *data)
>>> +{
>>> +	struct tegra_hsp_mailbox *mb = chan->con_priv;
>>> +	struct tegra_hsp *hsp = mb->channel.hsp;
>>> +	unsigned long flags;
>>> +	u32 value;
>>> +
>>> +	WARN_ON(!mb->producer);
>>
>> Should we return here?
> 
> Yeah, that's a good idea. I made this return -EPERM for lack of a better
> error code.
> 
>>> +
>>> +	/* copy data and mark mailbox full */
>>> +	value = (u32)(unsigned long)data;
>>> +	value |= HSP_SM_SHRD_MBOX_FULL;
>>> +
>>> +	tegra_hsp_channel_writel(&mb->channel, value, HSP_SM_SHRD_MBOX);
>>> +
>>> +	if (!irqs_disabled()) {
>>> +		/* enable EMPTY interrupt for the shared mailbox */
>>> +		spin_lock_irqsave(&hsp->lock, flags);
>>> +
>>> +		hsp->mask |= BIT(HSP_INT_EMPTY_SHIFT + mb->index);
>>> +		tegra_hsp_writel(hsp, hsp->mask, HSP_INT_IE(hsp->shared_irq));
>>> +
>>> +		spin_unlock_irqrestore(&hsp->lock, flags);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int tegra_hsp_mailbox_flush(struct mbox_chan *chan,
>>> +				   unsigned long timeout)
>>> +{
>>> +	struct tegra_hsp_mailbox *mb = chan->con_priv;
>>> +	struct tegra_hsp_channel *ch = &mb->channel;
>>> +	u32 value;
>>> +
>>> +	timeout = jiffies + msecs_to_jiffies(timeout);
>>> +
>>> +	while (time_before(jiffies, timeout)) {
>>> +		value = tegra_hsp_channel_readl(ch, HSP_SM_SHRD_MBOX);
>>> +		if ((value & HSP_SM_SHRD_MBOX_FULL) == 0) {
>>> +			mbox_chan_txdone(chan, 0);
>>> +			return 0;
>>> +		}
>>> +
>>> +		udelay(1);
>>> +	}
>>> +
>>> +	return -ETIME;
>>> +}
>>> +
>>> +static int tegra_hsp_mailbox_startup(struct mbox_chan *chan)
>>> +{
>>> +	struct tegra_hsp_mailbox *mb = chan->con_priv;
>>> +	struct tegra_hsp_channel *ch = &mb->channel;
>>> +	struct tegra_hsp *hsp = mb->channel.hsp;
>>> +	unsigned long flags;
>>> +
>>> +	chan->txdone_method = TXDONE_BY_IRQ;
>>> +
>>> +	/*
>>> +	 * Shared mailboxes start out as consumers by default. FULL and EMPTY
>>> +	 * interrupts are coalesced at the same shared interrupt.
>>> +	 *
>>> +	 * Keep EMPTY interrupts disabled at startup and only enable them when
>>> +	 * the mailbox is actually full. This is required because the FULL and
>>> +	 * EMPTY interrupts are level-triggered, so keeping EMPTY interrupts
>>> +	 * enabled all the time would cause an interrupt storm while mailboxes
>>> +	 * are idle.
>>> +	 */
>>> +
>>> +	spin_lock_irqsave(&hsp->lock, flags);
>>> +
>>> +	if (mb->producer)
>>> +		hsp->mask &= ~BIT(HSP_INT_EMPTY_SHIFT + mb->index);
>>> +	else
>>> +		hsp->mask |= BIT(HSP_INT_FULL_SHIFT + mb->index);
>>> +
>>> +	tegra_hsp_writel(hsp, hsp->mask, HSP_INT_IE(hsp->shared_irq));
>>> +
>>> +	spin_unlock_irqrestore(&hsp->lock, flags);
>>> +
>>> +	if (hsp->soc->has_per_mb_ie) {
>>> +		if (mb->producer)
>>> +			tegra_hsp_channel_writel(ch, 0x0,
>>> +						 HSP_SM_SHRD_MBOX_EMPTY_INT_IE);
>>> +		else
>>> +			tegra_hsp_channel_writel(ch, 0x1,
>>> +						 HSP_SM_SHRD_MBOX_FULL_INT_IE);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void tegra_hsp_mailbox_shutdown(struct mbox_chan *chan)
>>> +{
>>> +	struct tegra_hsp_mailbox *mb = chan->con_priv;
>>> +	struct tegra_hsp_channel *ch = &mb->channel;
>>> +	struct tegra_hsp *hsp = mb->channel.hsp;
>>> +	unsigned long flags;
>>> +
>>> +	if (hsp->soc->has_per_mb_ie) {
>>> +		if (mb->producer)
>>> +			tegra_hsp_channel_writel(ch, 0x0,
>>> +						 HSP_SM_SHRD_MBOX_EMPTY_INT_IE);
>>> +		else
>>> +			tegra_hsp_channel_writel(ch, 0x0,
>>> +						 HSP_SM_SHRD_MBOX_FULL_INT_IE);
>>> +	}
>>> +
>>> +	spin_lock_irqsave(&hsp->lock, flags);
>>> +
>>> +	if (mb->producer)
>>> +		hsp->mask &= ~BIT(HSP_INT_EMPTY_SHIFT + mb->index);
>>> +	else
>>> +		hsp->mask &= ~BIT(HSP_INT_FULL_SHIFT + mb->index);
>>> +
>>> +	tegra_hsp_writel(hsp, hsp->mask, HSP_INT_IE(hsp->shared_irq));
>>> +
>>> +	spin_unlock_irqrestore(&hsp->lock, flags);
>>> +}
>>> +
>>> +static const struct mbox_chan_ops tegra_hsp_sm_ops = {
>>> +	.send_data = tegra_hsp_mailbox_send_data,
>>> +	.flush = tegra_hsp_mailbox_flush,
>>> +	.startup = tegra_hsp_mailbox_startup,
>>> +	.shutdown = tegra_hsp_mailbox_shutdown,
>>> +};
>>> +
>>> +static struct mbox_chan *tegra_hsp_db_xlate(struct mbox_controller *mbox,
>>>  					    const struct of_phandle_args *args)
>>>  {
>>> +	struct tegra_hsp *hsp = container_of(mbox, struct tegra_hsp, mbox_db);
>>> +	unsigned int type = args->args[0], master = args->args[1];
>>>  	struct tegra_hsp_channel *channel = ERR_PTR(-ENODEV);
>>> -	struct tegra_hsp *hsp = to_tegra_hsp(mbox);
>>> -	unsigned int type = args->args[0];
>>> -	unsigned int master = args->args[1];
>>>  	struct tegra_hsp_doorbell *db;
>>>  	struct mbox_chan *chan;
>>>  	unsigned long flags;
>>>  	unsigned int i;
>>>  
>>> -	switch (type) {
>>> -	case TEGRA_HSP_MBOX_TYPE_DB:
>>> -		db = tegra_hsp_doorbell_get(hsp, master);
>>> -		if (db)
>>> -			channel = &db->channel;
>>> +	if (type != TEGRA_HSP_MBOX_TYPE_DB || !hsp->doorbell_irq)
>>> +		return ERR_PTR(-ENODEV);
>>>  
>>> -		break;
>>> -
>>> -	default:
>>> -		break;
>>> -	}
>>> +	db = tegra_hsp_doorbell_get(hsp, master);
>>> +	if (db)
>>> +		channel = &db->channel;
>>>  
>>>  	if (IS_ERR(channel))
>>>  		return ERR_CAST(channel);
>>>  
>>>  	spin_lock_irqsave(&hsp->lock, flags);
>>>  
>>> -	for (i = 0; i < hsp->mbox.num_chans; i++) {
>>> -		chan = &hsp->mbox.chans[i];
>>> +	for (i = 0; i < mbox->num_chans; i++) {
>>> +		chan = &mbox->chans[i];
>>>  		if (!chan->con_priv) {
>>> -			chan->con_priv = channel;
>>>  			channel->chan = chan;
>>> +			chan->con_priv = db;
>>>  			break;
>>>  		}
>>>  
>>> @@ -332,6 +544,29 @@ static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
>>>  	return chan ?: ERR_PTR(-EBUSY);
>>>  }
>>>  
>>> +static struct mbox_chan *tegra_hsp_sm_xlate(struct mbox_controller *mbox,
>>> +					    const struct of_phandle_args *args)
>>> +{
>>> +	struct tegra_hsp *hsp = container_of(mbox, struct tegra_hsp, mbox_sm);
>>> +	unsigned int type = args->args[0], index;
>>> +	struct tegra_hsp_mailbox *mb;
>>> +
>>> +	index = args->args[1] & TEGRA_HSP_SM_MASK;
>>> +
>>> +	if (type != TEGRA_HSP_MBOX_TYPE_SM || !hsp->shared_irqs ||
>>> +	    index >= hsp->num_sm)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	mb = &hsp->mailboxes[index];
>>> +
>>> +	if ((args->args[1] & TEGRA_HSP_SM_FLAG_TX) == 0)
>>> +		mb->producer = false;
>>> +	else
>>> +		mb->producer = true;
>>> +
>>> +	return mb->channel.chan;
>>> +}
>>> +
>>>  static void tegra_hsp_remove_doorbells(struct tegra_hsp *hsp)
>>>  {
>>>  	struct tegra_hsp_doorbell *db, *tmp;
>>> @@ -364,10 +599,65 @@ static int tegra_hsp_add_doorbells(struct tegra_hsp *hsp)
>>>  	return 0;
>>>  }
>>>  
>>> +static int tegra_hsp_add_mailboxes(struct tegra_hsp *hsp, struct device *dev)
>>> +{
>>> +	int i;
>>> +
>>> +	hsp->mailboxes = devm_kcalloc(dev, hsp->num_sm, sizeof(*hsp->mailboxes),
>>> +				      GFP_KERNEL);
>>> +	if (!hsp->mailboxes)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < hsp->num_sm; i++) {
>>> +		struct tegra_hsp_mailbox *mb = &hsp->mailboxes[i];
>>> +
>>> +		mb->index = i;
>>> +
>>> +		mb->channel.hsp = hsp;
>>> +		mb->channel.regs = hsp->regs + SZ_64K + i * SZ_32K;
>>> +		mb->channel.chan = &hsp->mbox_sm.chans[i];
>>> +		mb->channel.chan->con_priv = mb;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int tegra_hsp_request_shared_irqs(struct tegra_hsp *hsp)
>>> +{
>>> +	unsigned int i, irq = 0;
>>> +	int err;
>>> +
>>> +	for (i = 0; i < hsp->num_si; i++) {
>>> +		if (hsp->shared_irq == 0 && hsp->shared_irqs[i] > 0) {
>>> +			irq = hsp->shared_irqs[i];
>>> +			hsp->shared_irq = i;
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	if (irq > 0) {
>>> +		err = devm_request_irq(hsp->dev, irq, tegra_hsp_shared_irq, 0,
>>> +				       dev_name(hsp->dev), hsp);
>>> +		if (err < 0) {
>>> +			dev_err(hsp->dev, "failed to request interrupt: %d\n",
>>> +				err);
>>> +			return err;
>>> +		}
>>
>> Are we suppose to loop through all the shared interrupts and find one
>> that is available? Looking at the above it seems that we will try to use
>> the first shared interrupt we have a valid mapping for, regardless of if
>> it is available/in-use.
>>
>> Otherwise I am not sure why it is necessary to stored all the shared
>> irqs, because AFAICT we only use the first we have a valid mapping for.
>> Maybe I am missing something ...
> 
> Yeah, that's a good idea. In practice I don't think this matters at all
> because there just isn't another user of these interrupts, but it might
> be more explicit and self-explanatory if we try all of the interrupts
> in turn until we can request one.

By the way, any reason why we could not put the call to
platform_get_irq_byname() in the above function? May simplify the code a
bit to have a single loop.

Cheers
Jon

-- 
nvpublic