Re: [PATCH v3 2/2] mailbox: arm_mhuv3: Add driver
From: Cristian Marussi
Date: Mon Apr 08 2024 - 06:40:37 EST
On Sun, Apr 07, 2024 at 08:14:23PM -0500, Jassi Brar wrote:
> On Thu, Apr 4, 2024 at 1:25 AM 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.
> >
Hi Jassi,
thanks for having a look at this !
> > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> > ---
> > v1 -> v2
> > - fixed checkpatch warnings about side-effects
> > - fixed sparse errors as reported
> > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202403290015.tCLXudqC-lkp@xxxxxxxxx/
> > ---
> > MAINTAINERS | 9 +
> > drivers/mailbox/Kconfig | 11 +
> > drivers/mailbox/Makefile | 2 +
> > drivers/mailbox/arm_mhuv3.c | 1063 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 1085 insertions(+)
> > create mode 100644 drivers/mailbox/arm_mhuv3.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index aa3b947fb080..e957b9d9e32a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12998,6 +12998,15 @@ F: Documentation/devicetree/bindings/mailbox/arm,mhuv2.yaml
> > F: drivers/mailbox/arm_mhuv2.c
> > F: include/linux/mailbox/arm_mhuv2_message.h
> >
> > +MAILBOX ARM MHUv3
> > +M: Sudeep Holla <sudeep.holla@xxxxxxx>
> > +M: Cristian Marussi <cristian.marussi@xxxxxxx>
> > +L: linux-kernel@xxxxxxxxxxxxxxx
> > +L: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx (moderated for non-subscribers)
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> > +F: drivers/mailbox/arm_mhuv3.c
> > +
> > MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7
> > M: Alejandro Colomar <alx@xxxxxxxxxx>
> > L: linux-man@xxxxxxxxxxxxxxx
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 42940108a187..d20cdae65cfe 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -23,6 +23,17 @@ config ARM_MHU_V2
> > Say Y here if you want to build the ARM MHUv2 controller driver,
> > which provides unidirectional mailboxes between processing elements.
> >
> > +config ARM_MHU_V3
> > + tristate "ARM MHUv3 Mailbox"
> > + depends on ARM64 || COMPILE_TEST
> > + help
> > + Say Y here if you want to build the ARM MHUv3 controller driver,
> > + which provides unidirectional mailboxes between processing elements.
> > +
> > + ARM MHUv3 controllers can implement a varying number of extensions
> > + that provides different means of transports: supported extensions
> > + will be discovered and possibly managed at probe-time.
> > +
> > config IMX_MBOX
> > tristate "i.MX Mailbox"
> > depends on ARCH_MXC || COMPILE_TEST
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 18793e6caa2f..5cf2f54debaf 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -9,6 +9,8 @@ obj-$(CONFIG_ARM_MHU) += arm_mhu.o arm_mhu_db.o
> >
> > obj-$(CONFIG_ARM_MHU_V2) += arm_mhuv2.o
> >
> > +obj-$(CONFIG_ARM_MHU_V3) += arm_mhuv3.o
> > +
> > obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> >
> > obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX) += armada-37xx-rwtm-mailbox.o
> > 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 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ARM Message Handling Unit Version 3 (MHUv3) driver.
> > + *
> > + * Copyright (C) 2024 ARM Ltd.
> > + *
> > + * Based on ARM MHUv2 driver.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +
> > +/* ====== MHUv3 Registers ====== */
> > +
> > +/* Maximum number of Doorbell channel windows */
> > +#define MHUV3_DBCW_MAX 128
> > +/* Number of DBCH combined interrupt status registers */
> > +#define MHUV3_DBCH_CMB_INT_ST_REG_CNT 4
> > +#define MHUV3_INVALID_DOORBELL 0xFFFFFFFFUL
> > +
> > +/* Number of FFCH combined interrupt status registers */
> > +#define MHUV3_FFCH_CMB_INT_ST_REG_CNT 2
> > +
> > +#define MHUV3_STAT_BYTES (sizeof(u32))
> >
> Simply 4 please.
>
Ok.
> > +#define MHUV3_STAT_BITS (MHUV3_STAT_BYTES * __CHAR_BIT__)
> >
> just 32.
>
Ok.
> > +
> > +/* Not a typo ... */
> > +#define MHUV3_MAJOR_VERSION 2
> > +
> > +enum {
> > + MHUV3_MBOX_CELL_TYPE,
> > + MHUV3_MBOX_CELL_CHWN,
> > + MHUV3_MBOX_CELL_PARAM,
> > + MHUV3_MBOX_CELLS
> > +};
> > +
> > +/* CTRL_Page */
> > +
> > +struct blk_id {
> > + u32 blk_id : 4;
>
> Please avoid name clashes.
>
I'll fix.
> > + u32 pad : 28;
> > +} __packed;
> > +
> > +struct feat_spt0 {
> > + u32 dbe_spt : 4;
> > + u32 fe_spt : 4;
> > + u32 fce_spt : 4;
> > + u32 tze_spt : 4;
> > + u32 rme_spt : 4;
> > + u32 rase_spt : 4;
> > + u32 pad: 8;
> > +} __packed;
> > +
> > +struct feat_spt1 {
> > + u32 auto_op_spt : 4;
> > + u32 pad: 28;
> > +} __packed;
> > +
> > +struct dbch_cfg0 {
> > + u32 num_dbch : 8;
> > + u32 pad: 24;
> > +} __packed;
> > +
> > +struct ffch_cfg0 {
> > + u32 num_ffch : 8;
> > + u32 x8ba_spt : 1;
> > + u32 x16ba_spt : 1;
> > + u32 x32ba_spt : 1;
> > + u32 x64ba_spt : 1;
> > + u32 pad : 4;
> > + u32 ffch_depth : 10;
> > + u32 pad2 : 6;
> > +} __packed;
> > +
> > +struct fch_cfg0 {
> > + u32 num_fch : 10;
> > + /* MBX only registers */
> > + u32 fcgi_spt : 1;
> > + /* ------------------ */
> > + u32 num_fcg : 5;
> > + u32 num_fch_per_grp : 5;
> > + u32 fch_ws : 8;
> > + u32 pad : 3;
> > +} __packed;
> > +
> > +struct ctrl {
> > + u32 op_req : 1;
> > + u32 ch_op_mask : 1;
> > + u32 pad : 30;
> > +} __packed;
> > +
> > +struct fch_ctrl {
> > + u32 pad : 2;
> > + u32 int_en : 1;
> > + u32 pad2 : 29;
> > +} __packed;
> > +
> > +struct iidr {
> > + u32 implementer : 12;
> > + u32 revision : 4;
> > + u32 variant : 4;
> > + u32 product_id : 12;
> > +} __packed;
> > +
> > +struct aidr {
> > + u32 arch_minor_rev : 4;
> > + u32 arch_major_rev : 4;
> > + u32 pad : 24;
> > +} __packed;
> > +
> I am not sure about using bitfields on register values. I know v2
> driver also uses bitfields but this still is not very portable and is
> dependent on compiler behaviour. We may actually save some loc by not
> having unused fields if we use shifts and masks. Though I don't
> strongly feel either way.
>
Yes, indeed seemed a bit odd way of handling regs when I saw it in mhuv2,
BUT it seemed it had its advantages in terms of clarity of usage....did
not know about possible drawbacks, though. I'll re-think about the pros
and cons of this approach.
> > +struct ctrl_page {
> > + struct blk_id blk_id;
> > + u8 pad[0x10 - 0x4];
> > + struct feat_spt0 feat_spt0;
> > + struct feat_spt1 feat_spt1;
> > + u8 pad1[0x20 - 0x18];
> > + struct dbch_cfg0 dbch_cfg0;
> > + u8 pad2[0x30 - 0x24];
> > + struct ffch_cfg0 ffch_cfg0;
> > + u8 pad3[0x40 - 0x34];
> > + struct fch_cfg0 fch_cfg0;
> > + u8 pad4[0x100 - 0x44];
> > + struct ctrl ctrl;
> > + /* MBX only registers */
> > + u8 pad5[0x140 - 0x104];
> > + struct fch_ctrl fch_ctrl;
> > + u32 fcg_int_en;
> > + u8 pad6[0x400 - 0x148];
> > + /* ------------------ */
> Why the decoration ? Maybe comment on what different starts from here.
>
PBX and MBX Ctrl page are exactly the same, BUT for some registers banks
that does not exist in the PBX: this decoration is indeed the end, not
the start, of the MBX only regs that starts 5 lines above with the related
comment...was trying to avoid to use 2 different types for the basically
the same data...of course it works just because the PBX code refrains
from accessing the areas where only regs known to MBX lives.
> > + u32 dbch_int_st[MHUV3_DBCH_CMB_INT_ST_REG_CNT];
> > + u32 ffch_int_st[MHUV3_FFCH_CMB_INT_ST_REG_CNT];
> > + /* MBX only registers */
> > + u8 pad7[0x470 - 0x418];
> > + u32 fcg_int_st;
> > + u8 pad8[0x480 - 0x474];
> > + u32 fcg_grp_int_st[32];
> > + u8 pad9[0xFC8 - 0x500];
> > + /* ------------------ */
Same here.
> > + struct iidr iidr;
> > + struct aidr aidr;
> > + u32 imp_def_id[12];
> > +} __packed;
> > +
> > +/* DBCW_Page */
> > +
> > +struct xbcw_ctrl {
> > + u32 comb_en : 1;
> > + u32 pad : 31;
> > +} __packed;
> > +
> > +struct pdbcw_int {
> > + u32 tfr_ack : 1;
> > + u32 pad : 31;
> > +} __packed;
> > +
> > +struct pdbcw_page {
> > + u32 st;
> > + u8 pad[0xC - 0x4];
> > + u32 set;
> > + struct pdbcw_int int_st;
> > + struct pdbcw_int int_clr;
> > + struct pdbcw_int int_en;
> > + struct xbcw_ctrl ctrl;
> > +} __packed;
> > +
> > +struct mdbcw_page {
> > + u32 st;
> > + u32 st_msk;
> > + u32 clr;
> > + u8 pad[0x10 - 0xC];
> > + u32 msk_st;
> > + u32 msk_set;
> > + u32 msk_clr;
> > + struct xbcw_ctrl ctrl;
> > +} __packed;
> > +
> > +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];
> > + 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); \
> > + })
> > +
> > +/* ====== MHUv3 data structures ====== */
> > +
> > +enum mhuv3_frame {
> > + PBX_FRAME,
> > + MBX_FRAME
> > +};
> > +
> > +static char *mhuv3_str[] = {
> > + "PBX",
> > + "MBX"
> > +};
> > +
> > +enum mhuv3_extension_type {
> > + FIRST_EXT = 0,
> > + DBE_EXT = FIRST_EXT,
> > + FCE_EXT,
> > + FE_EXT,
> > + MAX_EXT
> > +};
> > +
> > +struct mhuv3;
> > +
> > +/**
> > + * struct mhuv3_protocol_ops - MHUv3 operations
> > + *
> > + * @rx_startup: Receiver startup callback.
> > + * @rx_shutdown: Receiver shutdown callback.
> > + * @read_data: Read available Sender in-band LE data (if any).
> > + * @rx_complete: Acknowledge data reception to the Sender. Any out-of-band data
> > + * has to have been already retrieved before calling this.
> > + * @tx_startup: Sender startup callback.
> > + * @tx_shutdown: Sender shutdown callback.
> > + * @last_tx_done: Report back to the Sender if the last transfer has completed.
> > + * @send_data: Send data to the receiver.
> > + *
> > + * Each supported transport protocol provides its own implementation of
> > + * these operations.
> > + */
> > +struct mhuv3_protocol_ops {
> > + int (*rx_startup)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*rx_shutdown)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void *(*read_data)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*rx_complete)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*tx_startup)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + void (*tx_shutdown)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + int (*last_tx_done)(struct mhuv3 *mhu, struct mbox_chan *chan);
> > + int (*send_data)(struct mhuv3 *mhu, struct mbox_chan *chan, void *arg);
> > +};
> > +
> > +/**
> > + * struct mhuv3_mbox_chan_priv - MHUv3 channel private information
> > + *
> > + * @ch_idx: Channel window index associated to this mailbox channel.
> > + * @doorbell: Doorbell bit number within the @ch_idx window.
> > + * Only relevant to Doorbell transport.
> > + * @ops: Transport protocol specific operations for this channel.
> > + *
> > + * Transport specific data attached to mmailbox channel priv data.
> > + */
> > +struct mhuv3_mbox_chan_priv {
> > + u32 ch_idx;
> > + u32 doorbell;
> > + const struct mhuv3_protocol_ops *ops;
> > +};
> > +
> > +/**
> > + * struct mhuv3_extension - MHUv3 extension descriptor
> > + *
> > + * @type: Type of extension
> > + * @max_chans: Max number of channels found for this extension.
> > + * @base_ch_idx: First channel number assigned to this extension, picked from
> > + * the set of all mailbox channels descriptors created.
> > + * @mbox_of_xlate: Extension specific helper to parse DT and lookup associated
> > + * channel from the related 'mboxes' property.
> > + * @combined_irq_setup: Extension specific helper to setup the combined irq.
> > + * @channels_init: Extension specific helper to initialize channels.
> > + * @chan_from_comb_irq_get: Extension specific helper to lookup which channel
> > + * triggered the combined irq.
> > + * @pending_db: Array of per-channel pending doorbells.
> > + * @pending_lock: Protect access to pending_db.
> > + */
> > +struct mhuv3_extension {
> > + enum mhuv3_extension_type type;
> > + unsigned int max_chans;
> > + unsigned int base_ch_idx;
> > + struct mbox_chan *(*mbox_of_xlate)(struct mhuv3 *mhu,
> > + unsigned int channel,
> > + unsigned int param);
> > + void (*combined_irq_setup)(struct mhuv3 *mhu);
> > + int (*channels_init)(struct mhuv3 *mhu);
> > + struct mbox_chan *(*chan_from_comb_irq_get)(struct mhuv3 *mhu);
> > + u32 pending_db[MHUV3_DBCW_MAX];
> > + /* Protect access to pending_db */
> > + spinlock_t pending_lock;
> > +};
> > +
> > +/**
> > + * struct mhuv3 - MHUv3 mailbox controller data
> > + *
> > + * @frame: Frame type: MBX_FRAME or PBX_FRAME.
> > + * @auto_op_full: Flag to indicate if the MHU supports AutoOp full mode.
> > + * @major: MHUv3 controller architectural major version.
> > + * @minor: MHUv3 controller architectural minor version.
> > + * @tot_chans: The total number of channnels discovered across all extensions.
> > + * @cmb_irq: Combined IRQ number if any found defined.
> > + * @ctrl: A reference to the MHUv3 control page for this block.
> > + * @pbx: Base address of the PBX register mapping region.
> > + * @mbx: Base address of the MBX register mapping region.
> > + * @ext: Array holding descriptors for any found implemented extension.
> > + * @mbox: Mailbox controller belonging to the MHU frame.
> > + */
> > +struct mhuv3 {
> > + enum mhuv3_frame frame;
> > + bool auto_op_full;
> > + unsigned int major;
> > + unsigned int minor;
> > + unsigned int tot_chans;
> >
> may be num_chans or chan_count ?
>
Ok.
>
> > + int cmb_irq;
> > + struct ctrl_page __iomem *ctrl;
> > + union {
> > + struct mhu3_pbx_frame_reg __iomem *pbx;
> > + struct mhu3_mbx_frame_reg __iomem *mbx;
> > + };
> > + struct mhuv3_extension *ext[MAX_EXT];
> > + struct mbox_controller mbox;
> > +};
> > +
> > +#define mhu_from_mbox(_mbox) container_of(_mbox, struct mhuv3, mbox)
> > +
> > +typedef int (*mhuv3_extension_initializer)(struct mhuv3 *mhu);
> > +
> > +/* =================== Doorbell transport protocol operations =============== */
> > +
> > +static void mhuv3_doorbell_tx_startup(struct mhuv3 *mhu, struct mbox_chan *chan)
> > +{
> > + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> > +
> > + /* Enable Transfer Acknowledgment events */
> > + writel_relaxed_bitfield(0x1, &mhu->pbx->dbcw[priv->ch_idx].int_en, tfr_ack);
> > +}
> > +
> > +static void mhuv3_doorbell_tx_shutdown(struct mhuv3 *mhu, struct mbox_chan *chan)
> > +{
> > + unsigned long flags;
> > + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> > + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> > +
> In order of decreasing line-lengths please everywhere.
>
Sure.
> > + /* Disable Channel Transfer Ack events */
> > + writel_relaxed_bitfield(0x0, &mhu->pbx->dbcw[priv->ch_idx].int_en, tfr_ack);
> > +
> > + /* Clear Channel Transfer Ack and pending doorbells */
> > + writel_relaxed_bitfield(0x1, &mhu->pbx->dbcw[priv->ch_idx].int_clr, tfr_ack);
> > + spin_lock_irqsave(&e->pending_lock, flags);
> > + e->pending_db[priv->ch_idx] = 0;
> > + spin_unlock_irqrestore(&e->pending_lock, flags);
> > +}
[snip]
> > +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);
>
> __ffs instead of __builtin_ctz please.
>
ok.
> > + if (channel >= e->max_chans) {
> > + dev_err(dev, "Invalid %s channel:%d\n",
> > + mhuv3_str[mhu->frame], channel);
> > + break;
> > + }
> > +
[snip]
> > +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)) {
> >
> 'continue' for error instead, to have fewer indented lines.
>
ok.
> > + 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)) {
> 'continue' for error instead, to have fewer indented lines.
>
ok.
Thanks,
Cristian