Re: [PATCH v3 2/2] mailbox: arm_mhuv3: Add driver

[Jonathan Cameron]

On Thu,  4 Apr 2024 07:23:47 +0100
Cristian Marussi <cristian.marussi@xxxxxxx> wrote:

> Add support for ARM MHUv3 mailbox controller.
> 
> Support is limited to the MHUv3 Doorbell extension using only the PBX/MBX
> combined interrupts.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
Drive by review (I was curious what this was :)


> diff --git a/drivers/mailbox/arm_mhuv3.c b/drivers/mailbox/arm_mhuv3.c
> new file mode 100644
> index 000000000000..e4125568bec0
> --- /dev/null
> +++ b/drivers/mailbox/arm_mhuv3.c
> @@ -0,0 +1,1063 @@

> +struct dummy_page {
> +	u8 pad[0x1000];
> +} __packed;
> +
> +struct mhu3_pbx_frame_reg {
> +	struct ctrl_page ctrl;
> +	struct pdbcw_page dbcw[MHUV3_DBCW_MAX];
> +	struct dummy_page ffcw;
> +	struct dummy_page fcw;
> +	u8 pad[0xF000 - 0x4000];
> +	struct dummy_page impdef;
> +} __packed;
> +
> +struct mhu3_mbx_frame_reg {
> +	struct ctrl_page ctrl;
> +	struct mdbcw_page dbcw[MHUV3_DBCW_MAX];
> +	struct dummy_page ffcw;
> +	struct dummy_page fcw;
> +	u8 pad[0xF000 - 0x4000];
Magic, numbers,  Maybe give them a definition or base them on something
meaningful such as structure offsets? 

> +	struct dummy_page impdef;
> +} __packed;
> +
> +/* Macro for reading a bitfield within a physically mapped packed struct */
> +#define readl_relaxed_bitfield(_regptr, _field)				\
> +	({								\
> +		u32 _rval;						\
> +		typeof(_regptr) _rptr = _regptr;			\
> +		_rval = readl_relaxed(_rptr);				\
> +		((typeof(*_rptr) __force *)(&_rval))->_field;		\
> +	})
> +
> +/* Macro for writing a bitfield within a physically mapped packed struct */
> +#define writel_relaxed_bitfield(_value, _regptr, _field)		\
> +	({								\
> +		u32 _rval;						\
> +		typeof(_regptr) _rptr = _regptr;			\
> +		_rval = readl_relaxed(_rptr);				\
> +		((typeof(*_rptr) __force *)(&_rval))->_field = _value;	\
> +		writel_relaxed(_rval, _rptr);				\
> +	})
Similar, yet slightly different from ones in arm_mhuv2.c?  Why the differences
and can these be shared code in a suitable header?
> +
> +/* ====== MHUv3 data structures ====== */
> +
> +enum mhuv3_frame {
> +	PBX_FRAME,
> +	MBX_FRAME
Trailing commas for last entries in enums unless they are in some sense terminators.
> +};
> +
> +static char *mhuv3_str[] = {
> +	"PBX",
> +	"MBX"
> +};
> +
> +enum mhuv3_extension_type {
> +	FIRST_EXT = 0,
As mentioned inline, 0 is kind of default assumption for first so I wouldn't define it.

> +	DBE_EXT = FIRST_EXT,
> +	FCE_EXT,
> +	FE_EXT,
> +	MAX_EXT
That's one past normal meeting of MAX,  maybe call it COUNT, or NUM?

> +};

> +static int mhuv3_doorbell_send_data(struct mhuv3 *mhu, struct mbox_chan *chan,
> +				    void *arg)
> +{
> +	int ret = 0;
> +	struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +	struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&e->pending_lock, flags);

guard()  then you can do earlier returns and end up with cleaner code.


> +	/* Only one in-flight Transfer is allowed per-doorbell */
> +	if (!(e->pending_db[priv->ch_idx] & BIT(priv->doorbell))) {
> +		e->pending_db[priv->ch_idx] |= BIT(priv->doorbell);
> +		writel_relaxed(BIT(priv->doorbell),
> +			       &mhu->pbx->dbcw[priv->ch_idx].set);
> +	} else {
> +		ret = -EBUSY;
> +	}
> +	spin_unlock_irqrestore(&e->pending_lock, flags);
> +
> +	return ret;
> +}
>
> +
> +static struct mbox_chan *mhuv3_dbe_chan_from_comb_irq_get(struct mhuv3 *mhu)
> +{
> +	int i;
> +	struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> +	struct device *dev = mhu->mbox.dev;
> +
> +	for (i = 0; i < MHUV3_DBCH_CMB_INT_ST_REG_CNT; i++) {
> +		unsigned int channel, db = MHUV3_INVALID_DOORBELL;
> +		u32 cmb_st, st;
> +
> +		cmb_st = readl_relaxed(&mhu->ctrl->dbch_int_st[i]);
> +		if (!cmb_st)
> +			continue;
> +
> +		channel = i * MHUV3_STAT_BITS + __builtin_ctz(cmb_st);
> +		if (channel >= e->max_chans) {
> +			dev_err(dev, "Invalid %s channel:%d\n",
> +				mhuv3_str[mhu->frame], channel);

return here rather than breaking out the loop. It is easier to follow
given nothing is done after the loop

> +			break;
> +		}
> +
> +		if (mhu->frame == PBX_FRAME) {
> +			unsigned long flags;
> +			u32 active_dbs, fired_dbs;
> +
> +			st = readl_relaxed_bitfield(&mhu->pbx->dbcw[channel].int_st,
> +						    tfr_ack);
> +			if (!st) {
> +				dev_warn(dev, "Spurios IRQ on %s channel:%d\n",
Spell check.  Spurious.

> +					 mhuv3_str[mhu->frame], channel);
> +				continue;
> +			}
> +
> +			active_dbs = readl_relaxed(&mhu->pbx->dbcw[channel].st);
> +			spin_lock_irqsave(&e->pending_lock, flags);
> +			fired_dbs = e->pending_db[channel] & ~active_dbs;
> +			if (fired_dbs) {
> +				db = __builtin_ctz(fired_dbs);
> +				e->pending_db[channel] &= ~BIT(db);
> +				fired_dbs &= ~BIT(db);
> +			}
> +			spin_unlock_irqrestore(&e->pending_lock, flags);
> +
> +			/* Clear TFR Ack if no more doorbells pending */
> +			if (!fired_dbs)
> +				writel_relaxed_bitfield(0x1,
> +							&mhu->pbx->dbcw[channel].int_clr,
> +							tfr_ack);
> +		} else {
> +			st = readl_relaxed(&mhu->mbx->dbcw[channel].st_msk);
> +			if (!st) {
> +				dev_warn(dev, "Spurios IRQ on %s channel:%d\n",
> +					 mhuv3_str[mhu->frame], channel);
> +				continue;
> +			}
> +			db = __builtin_ctz(st);
> +		}
> +
> +		if (db != MHUV3_INVALID_DOORBELL) {
> +			dev_dbg(dev, "Found %s ch[%d]/db[%d]\n",
> +				mhuv3_str[mhu->frame], channel, db);
> +
> +			return &mhu->mbox.chans[channel * MHUV3_STAT_BITS + db];
> +		}
> +	}
> +
> +	return ERR_PTR(-EIO);
> +}
> +
> +static int mhuv3_dbe_init(struct mhuv3 *mhu)
> +{
> +	struct mhuv3_extension *e;
> +	struct device *dev = mhu->mbox.dev;
> +
> +	if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, dbe_spt))
> +		return 0;
> +
> +	dev_dbg(dev, "%s: Initializing DBE Extension.\n", mhuv3_str[mhu->frame]);
> +
> +	e = devm_kzalloc(dev, sizeof(*e), GFP_KERNEL);
> +	if (!e)
> +		return -ENOMEM;
> +
> +	e->type = DBE_EXT;
> +	/* Note that, by the spec, the number of channels is (num_dbch + 1) */
> +	e->max_chans =
> +		readl_relaxed_bitfield(&mhu->ctrl->dbch_cfg0, num_dbch) + 1;
> +	e->mbox_of_xlate = mhuv3_dbe_mbox_of_xlate;
> +	e->combined_irq_setup = mhuv3_dbe_combined_irq_setup;
> +	e->channels_init = mhuv3_dbe_channels_init;
> +	e->chan_from_comb_irq_get = mhuv3_dbe_chan_from_comb_irq_get;
> +
> +	mhu->tot_chans += e->max_chans * MHUV3_STAT_BITS;
> +	mhu->ext[DBE_EXT] = e;
> +
> +	dev_info(dev, "%s: found %d DBE channels.\n",
> +		 mhuv3_str[mhu->frame], e->max_chans);
dev_dbg() probably more appropriate.

> +
> +	return 0;
> +}
> +
> +static int mhuv3_fce_init(struct mhuv3 *mhu)
> +{
> +	struct device *dev = mhu->mbox.dev;
> +
> +	if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, fce_spt))
> +		return 0;
> +
> +	dev_dbg(dev, "%s: FCE Extension not supported by driver.\n",
> +		mhuv3_str[mhu->frame]);
> +
> +	return 0;
> +}
> +
> +static int mhuv3_fe_init(struct mhuv3 *mhu)
> +{
> +	struct device *dev = mhu->mbox.dev;
> +
> +	if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, fe_spt))
> +		return 0;
> +
> +	dev_dbg(dev, "%s: FE Extension not supported by driver.\n",
> +		mhuv3_str[mhu->frame]);
> +
> +	return 0;
> +}
> +
> +static mhuv3_extension_initializer mhuv3_extension_init[MAX_EXT] = {
> +	mhuv3_dbe_init,
> +	mhuv3_fce_init,
> +	mhuv3_fe_init,
> +};
> +
> +static int mhuv3_initialize_channels(struct device *dev, struct mhuv3 *mhu)
> +{
> +	int i, ret = 0;
> +	struct mbox_controller *mbox = &mhu->mbox;
> +
> +	mbox->chans = devm_kcalloc(dev, mhu->tot_chans,
> +				   sizeof(*mbox->chans), GFP_KERNEL);
> +	if (!mbox->chans)
> +		return -ENOMEM;
> +
> +	for (i = FIRST_EXT; i < MAX_EXT && !ret; i++)
Why this dance with FIRST_EXT if it is always 0?  Cleaner to just use 0.

> +		if (mhu->ext[i])
> +			ret = mhu->ext[i]->channels_init(mhu);
> +
> +	return ret;
> +}
> +
> +static struct mbox_chan *mhuv3_mbox_of_xlate(struct mbox_controller *mbox,
> +					     const struct of_phandle_args *pa)
> +{
> +	unsigned int type, channel, param;
> +	struct mhuv3 *mhu = mhu_from_mbox(mbox);
> +
> +	if (pa->args_count != MHUV3_MBOX_CELLS)
> +		return ERR_PTR(-EINVAL);
> +
> +	type = pa->args[MHUV3_MBOX_CELL_TYPE];
> +	if (type >= MAX_EXT)
> +		return ERR_PTR(-EINVAL);
> +
> +	channel = pa->args[MHUV3_MBOX_CELL_CHWN];
> +	param = pa->args[MHUV3_MBOX_CELL_PARAM];
> +
> +	return mhu->ext[type]->mbox_of_xlate(mhu, channel, param);
> +}
> +
> +static int mhuv3_frame_init(struct mhuv3 *mhu, void __iomem *regs)
> +{
> +	int i, ret = 0;
> +	struct device *dev = mhu->mbox.dev;
> +
> +	mhu->ctrl = regs;
> +	mhu->frame = readl_relaxed_bitfield(&mhu->ctrl->blk_id, blk_id);
> +	if (mhu->frame > MBX_FRAME) {
> +		dev_err(dev, "Invalid Frame type- %d\n", mhu->frame);
> +		return -EINVAL;
dev_err_probe() etc (see later)

> +	}
> +
> +	mhu->major = readl_relaxed_bitfield(&mhu->ctrl->aidr, arch_major_rev);
> +	mhu->minor = readl_relaxed_bitfield(&mhu->ctrl->aidr, arch_minor_rev);
> +	if (mhu->major != MHUV3_MAJOR_VERSION) {
> +		dev_warn(dev, "Unsupported MHU %s block - major:%d  minor:%d\n",
> +			 mhuv3_str[mhu->frame], mhu->major, mhu->minor);

You are treating it as an error, so why only a warning?

> +		return -EINVAL;
> +	}
> +	mhu->auto_op_full = !!readl_relaxed_bitfield(&mhu->ctrl->feat_spt1,
> +						     auto_op_spt);
> +	/* Request the PBX/MBX to remain operational */
> +	if (mhu->auto_op_full)
> +		writel_relaxed_bitfield(0x1, &mhu->ctrl->ctrl, op_req);
> +
> +	dev_dbg(dev, "Found MHU %s block - major:%d  minor:%d\n",
> +		mhuv3_str[mhu->frame], mhu->major, mhu->minor);
> +
> +	if (mhu->frame == PBX_FRAME)
> +		mhu->pbx = regs;
> +	else
> +		mhu->mbx = regs;
> +
> +	for (i = FIRST_EXT; i < MAX_EXT && !ret; i++)
> +		ret = mhuv3_extension_init[i](mhu);

Only dbe_init() returns any errors, so if I ready this correctly you always
eat that error. 

> +
> +	return ret;
> +}
> +
> +static irqreturn_t mhuv3_pbx_comb_interrupt(int irq, void *arg)
> +{
> +	int ret = IRQ_NONE;
> +	unsigned int i, found = 0;
> +	struct mhuv3 *mhu = arg;
> +	struct device *dev = mhu->mbox.dev;
> +	struct mbox_chan *chan;
> +
> +	for (i = FIRST_EXT; i < MAX_EXT; i++) {
> +		/* FCE does not participate to the PBX combined */
> +		if (i == FCE_EXT || !mhu->ext[i])
> +			continue;
> +
> +		chan = mhu->ext[i]->chan_from_comb_irq_get(mhu);
> +		if (!IS_ERR(chan)) {

		if (IS_ERR(chan))
			continue;

will reduce indent and give more readable code.

> +			struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> +			found++;
> +			if (chan->cl) {
> +				mbox_chan_txdone(chan, 0);
> +				ret = IRQ_HANDLED;
> +			} else {
> +				dev_warn(dev,
> +					 "TX Ack on UNBOUND channel (%u)\n",
> +					 priv->ch_idx);
> +			}
> +		}
> +	}
> +
> +	if (!found)
> +		dev_warn_once(dev, "Failed to find channel for the TX interrupt\n");
> +
> +	return ret;
> +}
> +
> +static irqreturn_t mhuv3_mbx_comb_interrupt(int irq, void *arg)
> +{
> +	int ret = IRQ_NONE;
> +	unsigned int i, found = 0;
> +	struct mhuv3 *mhu = arg;
> +	struct device *dev = mhu->mbox.dev;
> +	struct mbox_chan *chan;
> +
> +	for (i = FIRST_EXT; i < MAX_EXT; i++) {
> +		if (!mhu->ext[i])
> +			continue;
> +
> +		/* Process any extension which could be source of the IRQ */
> +		chan = mhu->ext[i]->chan_from_comb_irq_get(mhu);
> +		if (!IS_ERR(chan)) {

		if (IS_ERR(chan))
			continue;

is going to be easier to read.

> +			void *data = NULL;
> +			struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> +			found++;
> +			/* Read and acknowledge optional in-band LE data first. */
> +			if (priv->ops->read_data)
> +				data = priv->ops->read_data(mhu, chan);
> +
> +			if (chan->cl && !IS_ERR(data)) {
> +				mbox_chan_received_data(chan, data);
> +				ret = IRQ_HANDLED;
> +			} else if (!chan->cl) {
> +				dev_warn(dev,
> +					 "RX Data on UNBOUND channel (%u)\n",
> +					 priv->ch_idx);
> +			} else {
> +				dev_err(dev, "Failed to read data: %lu\n",
> +					PTR_ERR(data));
> +			}

I'd be tempted to factor out this code block into another function as I think
that will allow you to deal with the errors more directly.

> +
> +			if (!IS_ERR(data))
> +				kfree(data);
> +
> +			/*
> +			 * Acknowledge transfer after any possible optional
> +			 * out-of-band data has also been retrieved via
> +			 * mbox_chan_received_data().
> +			 */
> +			if (priv->ops->rx_complete)
> +				priv->ops->rx_complete(mhu, chan);
> +		}
> +	}
> +
> +	if (!found)
> +		dev_warn_once(dev, "Failed to find channel for the RX interrupt\n");
> +
> +	return ret;
> +}
> +
> +static int mhuv3_setup_pbx(struct mhuv3 *mhu)
> +{
> +	struct device *dev = mhu->mbox.dev;
> +
> +	mhu->mbox.ops = &mhuv3_sender_ops;
> +
> +	if (mhu->cmb_irq > 0) {
> +		int ret;
> +
> +		ret = devm_request_threaded_irq(dev, mhu->cmb_irq, NULL,
> +						mhuv3_pbx_comb_interrupt,
> +						IRQF_ONESHOT, "mhuv3-pbx", mhu);
> +		if (!ret) {
> +			int i;
> +
> +			mhu->mbox.txdone_irq = true;
> +			mhu->mbox.txdone_poll = false;
> +
> +			for (i = FIRST_EXT; i < MAX_EXT; i++)
> +				if (mhu->ext[i])
> +					mhu->ext[i]->combined_irq_setup(mhu);
> +
> +			dev_dbg(dev, "MHUv3 PBX IRQs initialized.\n");
> +
> +			return 0;
> +		}
> +
> +		dev_err(dev, "Failed to request PBX IRQ - ret:%d", ret);

If an irq was provided and it failed, I'd just return an error, not muddle on.
Broken system.  If it's not an 'error' then don't use dev_err()

Papering over this leads to an odd code flow with if (!ret) so it would
be nice not to bother unless there is a strong reason to carry on.


> +	}
> +
> +	dev_info(dev, "Using PBX in Tx polling mode.\n");

That's noisy.  dev_dbg() perhaps?

> +	mhu->mbox.txdone_irq = false;
> +	mhu->mbox.txdone_poll = true;
> +	mhu->mbox.txpoll_period = 1;
> +
> +	return 0;
> +}
> +
> +static int mhuv3_setup_mbx(struct mhuv3 *mhu)
> +{
> +	int ret, i;
> +	struct device *dev = mhu->mbox.dev;
> +
> +	mhu->mbox.ops = &mhuv3_receiver_ops;
> +
> +	if (mhu->cmb_irq <= 0) {
> +		dev_err(dev, "Missing MBX combined IRQ !\n");
		return dev_err_probe()
here as I think it's only called form init.  Sure you might not
need the deferred handling it provides but it still leads to
cleaner code and no one has to think about whether deferal might
happen or not.

> +		return -EINVAL;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, mhu->cmb_irq, NULL,
> +					mhuv3_mbx_comb_interrupt, IRQF_ONESHOT,
> +					"mhuv3-mbx", mhu);
> +	if (ret) {
> +		dev_err(dev, "Failed to request MBX IRQ - ret:%d\n", ret);
> +		return ret;

		return dev_err_probe()

> +	}
> +
> +	for (i = FIRST_EXT; i < MAX_EXT; i++)
> +		if (mhu->ext[i])
> +			mhu->ext[i]->combined_irq_setup(mhu);
> +
> +	dev_dbg(dev, "MHUv3 MBX IRQs initialized.\n");
> +
> +	return ret;
> +}
> +
> +static int mhuv3_irqs_init(struct mhuv3 *mhu, struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	dev_dbg(mhu->mbox.dev, "Initializing %s block.\n", mhuv3_str[mhu->frame]);
> +
> +	if (mhu->frame == PBX_FRAME) {
> +		mhu->cmb_irq = platform_get_irq_byname_optional(pdev, "combined");
> +		ret = mhuv3_setup_pbx(mhu);

		return early is both shorter and easier to follow if people
are looking at particular paths through the function.

> +	} else {
> +		mhu->cmb_irq = platform_get_irq_byname(pdev, "combined");
> +		ret = mhuv3_setup_mbx(mhu);
> +	}
> +
> +	return ret;
> +}
> +
> +static int mhuv3_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct mhuv3 *mhu;
> +	void __iomem *regs;
> +	struct device *dev = &pdev->dev;
> +
> +	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
> +	if (!mhu)
> +		return -ENOMEM;
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	mhu->mbox.dev = dev;
> +	ret = mhuv3_frame_init(mhu, regs);
> +	if (ret)
> +		return ret;
> +
> +	ret = mhuv3_irqs_init(mhu, pdev);
> +	if (ret)
> +		return ret;
> +
> +	mhu->mbox.of_xlate = mhuv3_mbox_of_xlate;
> +	ret = mhuv3_initialize_channels(dev, mhu);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mbox_controller_register(dev, &mhu->mbox);
> +	if (ret)
> +		dev_err(dev, "failed to register ARM MHUv3 driver %d\n", ret);

Use dev_err_probe() to get a few things for free in probe time error messages message.
		return dev_err_probe(dev, reg, "failed to register ARM HMUv3 driver\n");

	return 0;
> +
> +	platform_set_drvdata(pdev, mhu);

With all devm as suggested below, can I think drop this.

> +
> +	return ret;
> +}
> +
> +static int mhuv3_remove(struct platform_device *pdev)
> +{
> +	struct mhuv3 *mhu = platform_get_drvdata(pdev);
> +
> +	if (mhu->auto_op_full)
> +		writel_relaxed_bitfield(0x0, &mhu->ctrl->ctrl, op_req);
> +

From a quick glance probably better to use a
devm_add_action_or_reset() so that this is turned off at
equivalent place in remove() path as where it is turned on in _init()

Only register the callback if auto_op_full()

Mixing and matching devm_ and calls in remove is a path to weird
races and corner cases so better to go all in on devm handling.

> +	return 0;
> +}
> +
> +static const struct of_device_id mhuv3_of_match[] = {
> +	{ .compatible = "arm,mhuv3", .data = NULL },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mhuv3_of_match);
> +
> +static struct platform_driver mhuv3_driver = {
> +	.driver = {
> +		.name = "arm-mhuv3-mailbox",
> +		.of_match_table = mhuv3_of_match,
> +	},
> +	.probe = mhuv3_probe,
> +	.remove = mhuv3_remove,
> +};
> +module_platform_driver(mhuv3_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ARM MHUv3 Driver");
> +MODULE_AUTHOR("Cristian Marussi <cristian.marussi@xxxxxxx>");