Hi,
This series adds support for the new ARM Message Handling Unit v3 mailbox
controller [1].
The ARM MHUv3 can optionally support various extensions, enabling the
usage of different transport protocols.
Patch [2/2] adds a platform driver which, as of now, provides support only
for the Doorbell extension using the combined interrupt.
On the other side, bindings in [1/2] are introduced for all the extensions
described by the specification, as long as they are of interest to an
entity running from Normal world, like Linux: as such, Doorbell, FIFO and
FastChannel extensions are documented.
In these regards, note that the ARM MHUv3 controller can optionally
implement a considerable number of interrupts to express a great deal of
events and many of such interrupts are defined as being per-channel: with
the total maximum amount of possibly implemented channels across all
extensions being 1216 (1024+128+64), it would mean *a lot* of
interrupt-names to enumerate in the bindings.
For the sake of simplicity the binding as of now only introduces interrupt
names for a mere 8-channels in the range (0,7) for each per-channel
interrupt type: the idea is to leave open the possibility to add more to
this list of numbered items only when (and if) new real HW appears that
effectively needs more than 8 channels. (like AMBA, where the maximum
number of IRQ was progressively increased when needed, AFAIU).
Based on v6.9-rc1, tested on ARM TCS23 [2]
(TCS23 reference SW stack is still to be made fully publicly available)
Thanks,
Cristian
[1]: https://developer.arm.com/documentation/aes0072/aa/?lang=en
[2]: https://community.arm.com/arm-community-blogs/b/tools-software-ides-blog/posts/total-compute-solutions-platform-software-stack-and-fvp
---
v2 -> v3
- fixed spurious tabs/spaces in DT binding
v1 -> v2
- clarified DT bindings extension descriptions around configurability
and discoverability
- removed unused labels from the DT example
- using pattern properties to define DT interrupt-names
- bumped DT interrupt maxItems to 74 (allowing uo to 8 channels per extension)
- fixed checkpatch warnings about side-effects on write/read bitfield macros
- fixed sparse errors as reported
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Cristian Marussi (2):
dt-bindings: mailbox: arm,mhuv3: Add bindings
mailbox: arm_mhuv3: Add driver
.../bindings/mailbox/arm,mhuv3.yaml | 217 ++++
MAINTAINERS | 9 +
drivers/mailbox/Kconfig | 11 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/arm_mhuv3.c | 1063 +++++++++++++++++
5 files changed, 1302 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
create mode 100644 drivers/mailbox/arm_mhuv3.c
--
2.34.1
Add bindings for the ARM MHUv3 Mailbox controller.
Signed-off-by: Cristian Marussi <[email protected]>
---
v2 -> v3
- fixed spurious tabs in dt_binding_check
v1 -> v2
- clarified extension descriptions around configurability and discoverability
- removed unused labels from the example
- using pattern properties to define interrupt-names
- bumped interrupt maxItems to 74 (allowing uo to 8 channels per extension)
---
.../bindings/mailbox/arm,mhuv3.yaml | 217 ++++++++++++++++++
1 file changed, 217 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
new file mode 100644
index 000000000000..32a8bb711464
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
@@ -0,0 +1,217 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/arm,mhuv3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM MHUv3 Mailbox Controller
+
+maintainers:
+ - Sudeep Holla <[email protected]>
+ - Cristian Marussi <[email protected]>
+
+description: |
+ The Arm Message Handling Unit (MHU) Version 3 is a mailbox controller that
+ enables unidirectional communications with remote processors through various
+ possible transport protocols.
+ The controller can optionally support a varying number of extensions that, in
+ turn, enable different kinds of transport to be used for communication.
+ Number, type and characteristics of each supported extension can be discovered
+ dynamically at runtime.
+
+ Given the unidirectional nature of the controller, an MHUv3 mailbox controller
+ is composed of a MHU Sender (MHUS) containing a PostBox (PBX) block and a MHU
+ Receiver (MHUR) containing a MailBox (MBX) block, where
+
+ PBX is used to
+ - Configure the MHU
+ - Send Transfers to the Receiver
+ - Optionally receive acknowledgment of a Transfer from the Receiver
+
+ MBX is used to
+ - Configure the MHU
+ - Receive Transfers from the Sender
+ - Optionally acknowledge Transfers sent by the Sender
+
+ Both PBX and MBX need to be present and defined in the DT description if you
+ need to establish a bidirectional communication, since you will have to
+ acquire two distinct unidirectional channels, one for each block.
+
+ As a consequence both blocks needs to be represented separately and specified
+ as distinct DT nodes in order to properly describe their resources.
+
+ Note that, though, thanks to the runtime discoverability, there is no need to
+ identify the type of blocks with distinct compatibles.
+
+ Following are the MHUv3 possible extensions.
+
+ - Doorbell Extension (DBE): DBE defines a type of channel called a Doorbell
+ Channel (DBCH). DBCH enables a single bit Transfer to be sent from the
+ Sender to Receiver. The Transfer indicates that an event has occurred.
+ When DBE is implemented, the number of DBCHs that an implementation of the
+ MHU can support is between 1 and 128, numbered starting from 0 in ascending
+ order and discoverable at run-time.
+ Each DBCH contains 32 individual fields, referred to as flags, each of which
+ can be used independently. It is possible for the Sender to send multiple
+ Transfers at once using a single DBCH, so long as each Transfer uses
+ a different flag in the DBCH.
+ Optionally, data may be transmitted through an out-of-band shared memory
+ region, wherein the MHU Doorbell is used strictly as an interrupt generation
+ mechanism, but this is out of the scope of these bindings.
+
+ - FastChannel Extension (FCE): FCE defines a type of channel called a Fast
+ Channel (FCH). FCH is intended for lower overhead communication between
+ Sender and Receiver at the expense of determinism. An FCH allows the Sender
+ to update the channel value at any time, regardless of whether the previous
+ value has been seen by the Receiver. When the Receiver reads the channel's
+ content it gets the last value written to the channel.
+ FCH is considered lossy in nature, and means that the Sender has no way of
+ knowing if, or when, the Receiver will act on the Transfer.
+ FCHs are expected to behave as RAM which generates interrupts when writes
+ occur to the locations within the RAM.
+ When FCE is implemented, the number of FCHs that an implementation of the
+ MHU can support is between 1-1024, if the FastChannel word-size is 32-bits,
+ or between 1-512, when the FastChannel word-size is 64-bits.
+ FCHs are numbered from 0 in ascending order.
+ Note that the number of FCHs and the word-size are implementation defined,
+ not configurable but discoverable at run-time.
+ Optionally, data may be transmitted through an out-of-band shared memory
+ region, wherein the MHU FastChannel is used as an interrupt generation
+ mechanism which carries also a pointer to such out-of-band data, but this
+ is out of the scope of these bindings.
+
+ - FIFO Extension (FE): FE defines a Channel type called a FIFO Channel (FFCH).
+ FFCH allows a Sender to send
+ - Multiple Transfers to the Receiver without having to wait for the
+ previous Transfer to be acknowledged by the Receiver, as long as the
+ FIFO has room for the Transfer.
+ - Transfers which require the Receiver to provide acknowledgment.
+ - Transfers which have in-band payload.
+ In all cases, the data is guaranteed to be observed by the Receiver in the
+ same order which the Sender sent it.
+ When FE is implemented, the number of FFCHs that an implementation of the
+ MHU can support is between 1 and 64, numbered starting from 0 in ascending
+ order. The number of FFCHs, their depth (same for all implemented FFCHs) and
+ the access-granularity are implementation defined, not configurable but
+ discoverable at run-time.
+ Optionally, additional data may be transmitted through an out-of-band shared
+ memory region, wherein the MHU FIFO is used to transmit, in order, a small
+ part of the payload (like a header) and a reference to the shared memory
+ area holding the remaining, bigger, chunk of the payload, but this is out of
+ the scope of these bindings.
+
+properties:
+ compatible:
+ const: arm,mhuv3
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ minItems: 1
+ maxItems: 74
+
+ interrupt-names:
+ description: |
+ The MHUv3 controller generates a number of events some of which are used
+ to generate interrupts; as a consequence it can expose a varying number of
+ optional PBX/MBX interrupts, representing the events generated during the
+ operation of the various transport protocols associated with different
+ extensions. All interrupts of the MHU are level-sensitive.
+ Some of these optional interrupts are defined per-channel, where the
+ number of channels effectively available is implementation defined and
+ run-time discoverable.
+ In the following names are enumerated using patterns, with per-channel
+ interrupts implicitly capped at the maximum channels allowed by the
+ specification for each extension type.
+ For the sake of simplicity maxItems is anyway capped to a most plausible
+ number, assuming way less channels would be implemented than actually
+ possible.
+
+ The only mandatory interrupts on the MHU are:
+ - combined
+ - mbx-fch-xfer-<N> but only if mbx-fcgrp-xfer-<N> is not implemented.
+
+ minItems: 1
+ maxItems: 74
+ items:
+ oneOf:
+ - const: combined
+ description: PBX/MBX Combined interrupt
+ - const: combined-ffch
+ description: PBX/MBX FIFO Combined interrupt
+ - pattern: '^ffch-low-tide-[0-9]+$'
+ description: PBX/MBX FIFO Channel <N> Low Tide interrupt
+ - pattern: '^ffch-high-tide-[0-9]+$'
+ description: PBX/MBX FIFO Channel <N> High Tide interrupt
+ - pattern: '^ffch-flush-[0-9]+$'
+ description: PBX/MBX FIFO Channel <N> Flush interrupt
+ - pattern: '^mbx-dbch-xfer-[0-9]+$'
+ description: MBX Doorbell Channel <N> Transfer interrupt
+ - pattern: '^mbx-fch-xfer-[0-9]+$'
+ description: MBX FastChannel <N> Transfer interrupt
+ - pattern: '^mbx-fchgrp-xfer-[0-9]+$'
+ description: MBX FastChannel <N> Group Transfer interrupt
+ - pattern: '^mbx-ffch-xfer-[0-9]+$'
+ description: MBX FIFO Channel <N> Transfer interrupt
+ - pattern: '^pbx-dbch-xfer-ack-[0-9]+$'
+ description: PBX Doorbell Channel <N> Transfer Ack interrupt
+ - pattern: '^pbx-ffch-xfer-ack-[0-9]+$'
+ description: PBX FIFO Channel <N> Transfer Ack interrupt
+
+ '#mbox-cells':
+ description: |
+ The first argument in the consumers 'mboxes' property represents the
+ extension type, the second is for the channel number while the third
+ depends on extension type.
+
+ Extension type for DBE is 0 and the third parameter represents the
+ doorbell flag number to use.
+ Extension type for FCE is 1, third parameter unused.
+ Extension type for FE is 2, third parameter unused.
+
+ mboxes = <&mhu 0 0 5>; // DBE, Doorbell Channel Window 0, doorbell flag 5.
+ mboxes = <&mhu 0 1 7>; // DBE, Doorbell Channel Window 1, doorbell flag 7.
+ mboxes = <&mhu 1 0 0>; // FCE, FastChannel Window 0.
+ mboxes = <&mhu 1 3 0>; // FCE, FastChannel Window 3.
+ mboxes = <&mhu 2 1 0>; // FE, FIFO Channel Window 1.
+ mboxes = <&mhu 2 7 0>; // FE, FIFO Channel Window 7.
+ const: 3
+
+ clocks:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-names
+ - '#mbox-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ mailbox@2aaa0000 {
+ compatible = "arm,mhuv3";
+ #mbox-cells = <3>;
+ reg = <0 0x2aaa0000 0 0x10000>;
+ clocks = <&clock 0>;
+ interrupt-names = "combined", "pbx-dbch-xfer-ack-1",
+ "ffch-high-tide-0";
+ interrupts = <0 36 4>, <0 37 4>;
+ };
+
+ mailbox@2ab00000 {
+ compatible = "arm,mhuv3";
+ #mbox-cells = <3>;
+ reg = <0 0x2aab0000 0 0x10000>;
+ clocks = <&clock 0>;
+ interrupt-names = "combined", "mbx-dbch-xfer-1", "ffch-low-tide-0";
+ interrupts = <0 35 4>, <0 38 4>, <0 39 4>;
+ };
+ };
--
2.34.1
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 <[email protected]>
---
v1 -> v2
- fixed checkpatch warnings about side-effects
- fixed sparse errors as reported
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
---
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 <[email protected]>
+M: Cristian Marussi <[email protected]>
+L: [email protected]
+L: [email protected] (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 <[email protected]>
L: [email protected]
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))
+#define MHUV3_STAT_BITS (MHUV3_STAT_BYTES * __CHAR_BIT__)
+
+/* 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;
+ 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;
+
+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];
+ /* ------------------ */
+ 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];
+ /* ------------------ */
+ 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;
+ 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;
+
+ /* 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);
+}
+
+static int mhuv3_doorbell_rx_startup(struct mhuv3 *mhu, struct mbox_chan *chan)
+{
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ /* Unmask Channel Transfer events */
+ writel_relaxed(BIT(priv->doorbell), &mhu->mbx->dbcw[priv->ch_idx].msk_clr);
+
+ return 0;
+}
+
+static void mhuv3_doorbell_rx_shutdown(struct mhuv3 *mhu,
+ struct mbox_chan *chan)
+{
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ /* Mask Channel Transfer events */
+ writel_relaxed(BIT(priv->doorbell), &mhu->mbx->dbcw[priv->ch_idx].msk_set);
+}
+
+static void mhuv3_doorbell_rx_complete(struct mhuv3 *mhu, struct mbox_chan *chan)
+{
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ /* Clearing the pending transfer generates the Channel Transfer Ack */
+ writel_relaxed(BIT(priv->doorbell), &mhu->mbx->dbcw[priv->ch_idx].clr);
+}
+
+static int mhuv3_doorbell_last_tx_done(struct mhuv3 *mhu,
+ struct mbox_chan *chan)
+{
+ int done;
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ done = !(readl_relaxed(&mhu->pbx->dbcw[priv->ch_idx].st) &
+ BIT(priv->doorbell));
+ if (done) {
+ unsigned long flags;
+ struct mhuv3_extension *e = mhu->ext[DBE_EXT];
+
+ /* Take care to clear the pending doorbell also when polling */
+ spin_lock_irqsave(&e->pending_lock, flags);
+ e->pending_db[priv->ch_idx] &= ~BIT(priv->doorbell);
+ spin_unlock_irqrestore(&e->pending_lock, flags);
+ }
+
+ return done;
+}
+
+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);
+ /* 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 const struct mhuv3_protocol_ops mhuv3_doorbell_ops = {
+ .tx_startup = mhuv3_doorbell_tx_startup,
+ .tx_shutdown = mhuv3_doorbell_tx_shutdown,
+ .rx_startup = mhuv3_doorbell_rx_startup,
+ .rx_shutdown = mhuv3_doorbell_rx_shutdown,
+ .rx_complete = mhuv3_doorbell_rx_complete,
+ .last_tx_done = mhuv3_doorbell_last_tx_done,
+ .send_data = mhuv3_doorbell_send_data,
+};
+
+/* Sender and receiver mailbox ops */
+static bool mhuv3_sender_last_tx_done(struct mbox_chan *chan)
+{
+ struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ return priv->ops->last_tx_done(mhu, chan);
+}
+
+static int mhuv3_sender_send_data(struct mbox_chan *chan, void *data)
+{
+ struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ if (!priv->ops->last_tx_done(mhu, chan))
+ return -EBUSY;
+
+ return priv->ops->send_data(mhu, chan, data);
+}
+
+static int mhuv3_sender_startup(struct mbox_chan *chan)
+{
+ struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ if (priv->ops->tx_startup)
+ priv->ops->tx_startup(mhu, chan);
+
+ return 0;
+}
+
+static void mhuv3_sender_shutdown(struct mbox_chan *chan)
+{
+ struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ if (priv->ops->tx_shutdown)
+ priv->ops->tx_shutdown(mhu, chan);
+}
+
+static const struct mbox_chan_ops mhuv3_sender_ops = {
+ .send_data = mhuv3_sender_send_data,
+ .startup = mhuv3_sender_startup,
+ .shutdown = mhuv3_sender_shutdown,
+ .last_tx_done = mhuv3_sender_last_tx_done,
+};
+
+static int mhuv3_receiver_startup(struct mbox_chan *chan)
+{
+ struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ return priv->ops->rx_startup(mhu, chan);
+}
+
+static void mhuv3_receiver_shutdown(struct mbox_chan *chan)
+{
+ struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
+ struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
+
+ priv->ops->rx_shutdown(mhu, chan);
+}
+
+static int mhuv3_receiver_send_data(struct mbox_chan *chan, void *data)
+{
+ dev_err(chan->mbox->dev,
+ "Trying to transmit on a MBX MHUv3 frame\n");
+ return -EIO;
+}
+
+static bool mhuv3_receiver_last_tx_done(struct mbox_chan *chan)
+{
+ dev_err(chan->mbox->dev, "Trying to Tx poll on a MBX MHUv3 frame\n");
+ return true;
+}
+
+static const struct mbox_chan_ops mhuv3_receiver_ops = {
+ .send_data = mhuv3_receiver_send_data,
+ .startup = mhuv3_receiver_startup,
+ .shutdown = mhuv3_receiver_shutdown,
+ .last_tx_done = mhuv3_receiver_last_tx_done,
+};
+
+static struct mbox_chan *mhuv3_dbe_mbox_of_xlate(struct mhuv3 *mhu,
+ unsigned int channel,
+ unsigned int doorbell)
+{
+ struct mbox_controller *mbox = &mhu->mbox;
+ struct mbox_chan *chans = mbox->chans;
+ struct mhuv3_extension *e = mhu->ext[DBE_EXT];
+
+ if (channel >= e->max_chans || doorbell >= MHUV3_STAT_BITS) {
+ dev_err(mbox->dev, "Couldn't xlate to a valid channel (%d: %d)\n",
+ channel, doorbell);
+ return ERR_PTR(-ENODEV);
+ }
+
+ return &chans[e->base_ch_idx + channel * MHUV3_STAT_BITS + doorbell];
+}
+
+static void mhuv3_dbe_combined_irq_setup(struct mhuv3 *mhu)
+{
+ int i;
+ struct mhuv3_extension *e = mhu->ext[DBE_EXT];
+
+ if (mhu->frame == PBX_FRAME) {
+ struct pdbcw_page __iomem *dbcw = mhu->pbx->dbcw;
+
+ for (i = 0; i < e->max_chans; i++) {
+ writel_relaxed_bitfield(0x1, &dbcw[i].int_clr, tfr_ack);
+ writel_relaxed_bitfield(0x0, &dbcw[i].int_en, tfr_ack);
+ writel_relaxed_bitfield(0x1, &dbcw[i].ctrl, comb_en);
+ }
+ } else {
+ struct mdbcw_page __iomem *dbcw = mhu->mbx->dbcw;
+
+ for (i = 0; i < e->max_chans; i++) {
+ writel_relaxed(0xFFFFFFFF, &dbcw[i].clr);
+ writel_relaxed(0xFFFFFFFF, &dbcw[i].msk_set);
+ writel_relaxed_bitfield(0x1, &dbcw[i].ctrl, comb_en);
+ }
+ }
+}
+
+static int mhuv3_dbe_channels_init(struct mhuv3 *mhu)
+{
+ int i;
+ struct mhuv3_extension *e = mhu->ext[DBE_EXT];
+ struct mbox_controller *mbox = &mhu->mbox;
+ struct mbox_chan *chans;
+
+ chans = mbox->chans + mbox->num_chans;
+ e->base_ch_idx = mbox->num_chans;
+ for (i = 0; i < e->max_chans; i++) {
+ int k;
+ struct mhuv3_mbox_chan_priv *priv;
+
+ for (k = 0; k < MHUV3_STAT_BITS; k++) {
+ priv = devm_kmalloc(mbox->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->ch_idx = i;
+ priv->ops = &mhuv3_doorbell_ops;
+ priv->doorbell = k;
+ chans++->con_priv = priv;
+ mbox->num_chans++;
+ }
+ }
+
+ spin_lock_init(&e->pending_lock);
+
+ return 0;
+}
+
+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);
+ 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",
+ 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);
+
+ 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++)
+ 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;
+ }
+
+ 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);
+ 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);
+
+ 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)) {
+ 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)) {
+ 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));
+ }
+
+ 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);
+ }
+
+ dev_info(dev, "Using PBX in Tx polling mode.\n");
+ 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 -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;
+ }
+
+ 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);
+ } 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);
+
+ platform_set_drvdata(pdev, mhu);
+
+ 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);
+
+ 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 <[email protected]>");
--
2.34.1
On 04/04/2024 08:23, Cristian Marussi wrote:
> Add bindings for the ARM MHUv3 Mailbox controller.
>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> v2 -> v3
> - fixed spurious tabs in dt_binding_check
Did you test this patch before sending?
Best regards,
Krzysztof
On Thu, Apr 04, 2024 at 08:30:27AM +0200, Krzysztof Kozlowski wrote:
> On 04/04/2024 08:23, Cristian Marussi wrote:
> > Add bindings for the ARM MHUv3 Mailbox controller.
> >
> > Signed-off-by: Cristian Marussi <[email protected]>
> > ---
> > v2 -> v3
> > - fixed spurious tabs in dt_binding_check
>
> Did you test this patch before sending?
>
Tested with:
make -j8 DT_SCHEMA_FILES=arm,mhuv3.yaml DT_CHECKER_FLAGS=-m dt_binding_check
(with dtschema upgraded)
..and indeed even v2 was supposedly already tested (since included a
bunch of changes as advised), then I made a small last-minute nitpick and
my editor splipped in a couple of tabs...apologies for that.
Thanks,
Cristian
On Thu, Apr 4, 2024 at 1:25 AM Cristian Marussi
<[email protected]> wrote:
>
> Add bindings for the ARM MHUv3 Mailbox controller.
>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> v2 -> v3
> - fixed spurious tabs in dt_binding_check
> v1 -> v2
> - clarified extension descriptions around configurability and discoverability
> - removed unused labels from the example
> - using pattern properties to define interrupt-names
> - bumped interrupt maxItems to 74 (allowing uo to 8 channels per extension)
> ---
> .../bindings/mailbox/arm,mhuv3.yaml | 217 ++++++++++++++++++
> 1 file changed, 217 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> new file mode 100644
> index 000000000000..32a8bb711464
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> @@ -0,0 +1,217 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm,mhuv3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM MHUv3 Mailbox Controller
> +
> +maintainers:
> + - Sudeep Holla <[email protected]>
> + - Cristian Marussi <[email protected]>
> +
> +description: |
> + The Arm Message Handling Unit (MHU) Version 3 is a mailbox controller that
> + enables unidirectional communications with remote processors through various
> + possible transport protocols.
> + The controller can optionally support a varying number of extensions that, in
> + turn, enable different kinds of transport to be used for communication.
> + Number, type and characteristics of each supported extension can be discovered
> + dynamically at runtime.
> +
> + Given the unidirectional nature of the controller, an MHUv3 mailbox controller
> + is composed of a MHU Sender (MHUS) containing a PostBox (PBX) block and a MHU
> + Receiver (MHUR) containing a MailBox (MBX) block, where
> +
> + PBX is used to
> + - Configure the MHU
> + - Send Transfers to the Receiver
> + - Optionally receive acknowledgment of a Transfer from the Receiver
> +
> + MBX is used to
> + - Configure the MHU
> + - Receive Transfers from the Sender
> + - Optionally acknowledge Transfers sent by the Sender
> +
> + Both PBX and MBX need to be present and defined in the DT description if you
> + need to establish a bidirectional communication, since you will have to
> + acquire two distinct unidirectional channels, one for each block.
> +
> + As a consequence both blocks needs to be represented separately and specified
> + as distinct DT nodes in order to properly describe their resources.
> +
> + Note that, though, thanks to the runtime discoverability, there is no need to
> + identify the type of blocks with distinct compatibles.
> +
> + Following are the MHUv3 possible extensions.
> +
> + - Doorbell Extension (DBE): DBE defines a type of channel called a Doorbell
> + Channel (DBCH). DBCH enables a single bit Transfer to be sent from the
> + Sender to Receiver. The Transfer indicates that an event has occurred.
> + When DBE is implemented, the number of DBCHs that an implementation of the
> + MHU can support is between 1 and 128, numbered starting from 0 in ascending
> + order and discoverable at run-time.
> + Each DBCH contains 32 individual fields, referred to as flags, each of which
> + can be used independently. It is possible for the Sender to send multiple
> + Transfers at once using a single DBCH, so long as each Transfer uses
> + a different flag in the DBCH.
> + Optionally, data may be transmitted through an out-of-band shared memory
> + region, wherein the MHU Doorbell is used strictly as an interrupt generation
> + mechanism, but this is out of the scope of these bindings.
> +
> + - FastChannel Extension (FCE): FCE defines a type of channel called a Fast
> + Channel (FCH). FCH is intended for lower overhead communication between
> + Sender and Receiver at the expense of determinism. An FCH allows the Sender
> + to update the channel value at any time, regardless of whether the previous
> + value has been seen by the Receiver. When the Receiver reads the channel's
> + content it gets the last value written to the channel.
> + FCH is considered lossy in nature, and means that the Sender has no way of
> + knowing if, or when, the Receiver will act on the Transfer.
> + FCHs are expected to behave as RAM which generates interrupts when writes
> + occur to the locations within the RAM.
> + When FCE is implemented, the number of FCHs that an implementation of the
> + MHU can support is between 1-1024, if the FastChannel word-size is 32-bits,
> + or between 1-512, when the FastChannel word-size is 64-bits.
> + FCHs are numbered from 0 in ascending order.
> + Note that the number of FCHs and the word-size are implementation defined,
> + not configurable but discoverable at run-time.
> + Optionally, data may be transmitted through an out-of-band shared memory
> + region, wherein the MHU FastChannel is used as an interrupt generation
> + mechanism which carries also a pointer to such out-of-band data, but this
> + is out of the scope of these bindings.
> +
> + - FIFO Extension (FE): FE defines a Channel type called a FIFO Channel (FFCH).
> + FFCH allows a Sender to send
> + - Multiple Transfers to the Receiver without having to wait for the
> + previous Transfer to be acknowledged by the Receiver, as long as the
> + FIFO has room for the Transfer.
> + - Transfers which require the Receiver to provide acknowledgment.
> + - Transfers which have in-band payload.
> + In all cases, the data is guaranteed to be observed by the Receiver in the
> + same order which the Sender sent it.
> + When FE is implemented, the number of FFCHs that an implementation of the
> + MHU can support is between 1 and 64, numbered starting from 0 in ascending
> + order. The number of FFCHs, their depth (same for all implemented FFCHs) and
> + the access-granularity are implementation defined, not configurable but
> + discoverable at run-time.
> + Optionally, additional data may be transmitted through an out-of-band shared
> + memory region, wherein the MHU FIFO is used to transmit, in order, a small
> + part of the payload (like a header) and a reference to the shared memory
> + area holding the remaining, bigger, chunk of the payload, but this is out of
> + the scope of these bindings.
> +
> +properties:
> + compatible:
> + const: arm,mhuv3
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + minItems: 1
> + maxItems: 74
> +
> + interrupt-names:
> + description: |
> + The MHUv3 controller generates a number of events some of which are used
> + to generate interrupts; as a consequence it can expose a varying number of
> + optional PBX/MBX interrupts, representing the events generated during the
> + operation of the various transport protocols associated with different
> + extensions. All interrupts of the MHU are level-sensitive.
> + Some of these optional interrupts are defined per-channel, where the
> + number of channels effectively available is implementation defined and
> + run-time discoverable.
> + In the following names are enumerated using patterns, with per-channel
> + interrupts implicitly capped at the maximum channels allowed by the
> + specification for each extension type.
> + For the sake of simplicity maxItems is anyway capped to a most plausible
> + number, assuming way less channels would be implemented than actually
> + possible.
> +
> + The only mandatory interrupts on the MHU are:
> + - combined
> + - mbx-fch-xfer-<N> but only if mbx-fcgrp-xfer-<N> is not implemented.
> +
> + minItems: 1
> + maxItems: 74
> + items:
> + oneOf:
> + - const: combined
> + description: PBX/MBX Combined interrupt
> + - const: combined-ffch
> + description: PBX/MBX FIFO Combined interrupt
> + - pattern: '^ffch-low-tide-[0-9]+$'
> + description: PBX/MBX FIFO Channel <N> Low Tide interrupt
> + - pattern: '^ffch-high-tide-[0-9]+$'
> + description: PBX/MBX FIFO Channel <N> High Tide interrupt
> + - pattern: '^ffch-flush-[0-9]+$'
> + description: PBX/MBX FIFO Channel <N> Flush interrupt
> + - pattern: '^mbx-dbch-xfer-[0-9]+$'
> + description: MBX Doorbell Channel <N> Transfer interrupt
> + - pattern: '^mbx-fch-xfer-[0-9]+$'
> + description: MBX FastChannel <N> Transfer interrupt
> + - pattern: '^mbx-fchgrp-xfer-[0-9]+$'
> + description: MBX FastChannel <N> Group Transfer interrupt
> + - pattern: '^mbx-ffch-xfer-[0-9]+$'
> + description: MBX FIFO Channel <N> Transfer interrupt
> + - pattern: '^pbx-dbch-xfer-ack-[0-9]+$'
> + description: PBX Doorbell Channel <N> Transfer Ack interrupt
> + - pattern: '^pbx-ffch-xfer-ack-[0-9]+$'
> + description: PBX FIFO Channel <N> Transfer Ack interrupt
> +
Can we have optional subnodes (with different properties as required)
for each extension type ?
> + '#mbox-cells':
> + description: |
> + The first argument in the consumers 'mboxes' property represents the
> + extension type, the second is for the channel number while the third
> + depends on extension type.
> +
> + Extension type for DBE is 0 and the third parameter represents the
> + doorbell flag number to use.
> + Extension type for FCE is 1, third parameter unused.
> + Extension type for FE is 2, third parameter unused.
> +
> + mboxes = <&mhu 0 0 5>; // DBE, Doorbell Channel Window 0, doorbell flag 5.
> + mboxes = <&mhu 0 1 7>; // DBE, Doorbell Channel Window 1, doorbell flag 7.
> + mboxes = <&mhu 1 0 0>; // FCE, FastChannel Window 0.
> + mboxes = <&mhu 1 3 0>; // FCE, FastChannel Window 3.
> + mboxes = <&mhu 2 1 0>; // FE, FIFO Channel Window 1.
> + mboxes = <&mhu 2 7 0>; // FE, FIFO Channel Window 7.
>
Please define the extension types, instead of 0, 1 and 2.
Cheers!
Jassi
On Fri, Apr 05, 2024 at 11:32:00AM +0100, Jonathan Cameron wrote:
> On Thu, 4 Apr 2024 07:23:47 +0100
> Cristian Marussi <[email protected]> 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 <[email protected]>
> Drive by review (I was curious what this was :)
>
You're welcome...thanks for having a look !
>
> > 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?
>
Yes, it is indeed cryptic, these are the holes in the MMIO regs and
are derived from the spec...I'll see how I can rework to make this more
meaningful and better commented.
> > + 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?
Yes, all the struct/bitfield based MMIO stuff is borrowed from mhuv2
since it seemed more slick than a zillions defines and bitmasks (but maybe
not exempt from issues... given what Jassi jas commented later on...), BUT
while using those I saw the opportunity to drop a parameter since v2 has a
_type arg that it can indeed derived from _regptr, so making the macros less
cumbersome to invoke....THEN sparse quicly reminded me that by using typeof()
to derive the type of the local work-variable I was also grabbing all the
related attributes attached to _regptr...namely __iomem and noderef that
triggered a bunch of warnings (unjustified since operating on a local
var NOT a real MMIO): that is the reason for the dance with __force
here, and why is not needed in v2.
> > +
> > +/* ====== 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.
>
Indeed.
> > + DBE_EXT = FIRST_EXT,
> > + FCE_EXT,
> > + FE_EXT,
> > + MAX_EXT
> That's one past normal meeting of MAX, maybe call it COUNT, or NUM?
>
Ok.
> > +};
>
> > +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.
>
Yes, I'll have a look into cleanup.h at large.
>
> > + /* 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
>
Ok.
> > + 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.
>
Yes.
> > + 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.
>
Ok.
> > +
> > + 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.
>
Ok, I'll do...I was thinking was more clear to specify what was the
start instead of a plain 0....but indeed is apparent from the context.
> > + 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)
>
Yes, indeed. I've just posted a series to use dev_err_probe on the SCMI
stack and then missed completely here...my bad. I'll do.
> > + }
> > +
> > + 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?
>
Right.
> > + 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.
Yes, I'll fix the logic.
>
> > +
> > + 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.
>
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)) {
>
> if (IS_ERR(chan))
> continue;
>
> is going to be easier to read.
>
ok.
> > + 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.
>
I will give a go at reworking.
> > +
> > + 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.
Well, the only reason is that when the Tx-Ack interrupt fails somehow to
be setup (or is not provided even though the spec mandates it), the
mailbox can work anyway fine, maybe on degraded performance...so here I was
trying to be kind and carry-on best-effort with a few warnings...since
I already bumped into a system where the Tx-Ack was supposedly present BUT
the wire was NOT ... but indeed better to be noisy and bailout so to have
the thing fixed early on when it happens...I'll revisit
>
>
> > + }
> > +
> > + dev_info(dev, "Using PBX in Tx polling mode.\n");
>
> That's noisy. dev_dbg() perhaps?
>
Ok, I was trying to be noisy indeed since operating without Tx-Ack can
be limiting in some circumstances and is a sort of anomaly given the
spec would expect the PBX combined interrupt to be provided (but the
mailboxes can work...)
> > + 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.
>
Yes I'll switch to dev_err_probe where appropriate.
> > + 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()
Ditto.
>
> > + }
> > +
> > + 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.
Ok.
>
> > + } 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");
Ditto.
>
> return 0;
> > +
> > + platform_set_drvdata(pdev, mhu);
>
> With all devm as suggested below, can I think drop this.
>
Ok.
> > +
> > + 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.
Ok, I'll switch to devm_ fully and drop remove() all along.
Thanks again for the review.
Cristian
On Sun, Apr 07, 2024 at 08:14:23PM -0500, Jassi Brar wrote:
> On Thu, Apr 4, 2024 at 1:25 AM Cristian Marussi
> <[email protected]> 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 <[email protected]>
> > ---
> > v1 -> v2
> > - fixed checkpatch warnings about side-effects
> > - fixed sparse errors as reported
> > | Reported-by: kernel test robot <[email protected]>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > ---
> > 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 <[email protected]>
> > +M: Cristian Marussi <[email protected]>
> > +L: [email protected]
> > +L: [email protected] (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 <[email protected]>
> > L: [email protected]
> > 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
On Sun, Apr 07, 2024 at 06:38:52PM -0500, Jassi Brar wrote:
> On Thu, Apr 4, 2024 at 1:25 AM Cristian Marussi
> <[email protected]> wrote:
> >
> > Add bindings for the ARM MHUv3 Mailbox controller.
> >
Hi,
> > Signed-off-by: Cristian Marussi <[email protected]>
> > ---
> > v2 -> v3
> > - fixed spurious tabs in dt_binding_check
> > v1 -> v2
> > - clarified extension descriptions around configurability and discoverability
> > - removed unused labels from the example
> > - using pattern properties to define interrupt-names
> > - bumped interrupt maxItems to 74 (allowing uo to 8 channels per extension)
> > ---
> > .../bindings/mailbox/arm,mhuv3.yaml | 217 ++++++++++++++++++
> > 1 file changed, 217 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> > new file mode 100644
> > index 000000000000..32a8bb711464
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv3.yaml
> > @@ -0,0 +1,217 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mailbox/arm,mhuv3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM MHUv3 Mailbox Controller
> > +
> > +maintainers:
> > + - Sudeep Holla <[email protected]>
> > + - Cristian Marussi <[email protected]>
> > +
> > +description: |
> > + The Arm Message Handling Unit (MHU) Version 3 is a mailbox controller that
> > + enables unidirectional communications with remote processors through various
> > + possible transport protocols.
> > + The controller can optionally support a varying number of extensions that, in
> > + turn, enable different kinds of transport to be used for communication.
> > + Number, type and characteristics of each supported extension can be discovered
> > + dynamically at runtime.
> > +
> > + Given the unidirectional nature of the controller, an MHUv3 mailbox controller
> > + is composed of a MHU Sender (MHUS) containing a PostBox (PBX) block and a MHU
> > + Receiver (MHUR) containing a MailBox (MBX) block, where
> > +
> > + PBX is used to
> > + - Configure the MHU
> > + - Send Transfers to the Receiver
> > + - Optionally receive acknowledgment of a Transfer from the Receiver
> > +
> > + MBX is used to
> > + - Configure the MHU
> > + - Receive Transfers from the Sender
> > + - Optionally acknowledge Transfers sent by the Sender
> > +
> > + Both PBX and MBX need to be present and defined in the DT description if you
> > + need to establish a bidirectional communication, since you will have to
> > + acquire two distinct unidirectional channels, one for each block.
> > +
> > + As a consequence both blocks needs to be represented separately and specified
> > + as distinct DT nodes in order to properly describe their resources.
> > +
> > + Note that, though, thanks to the runtime discoverability, there is no need to
> > + identify the type of blocks with distinct compatibles.
> > +
> > + Following are the MHUv3 possible extensions.
> > +
> > + - Doorbell Extension (DBE): DBE defines a type of channel called a Doorbell
> > + Channel (DBCH). DBCH enables a single bit Transfer to be sent from the
> > + Sender to Receiver. The Transfer indicates that an event has occurred.
> > + When DBE is implemented, the number of DBCHs that an implementation of the
> > + MHU can support is between 1 and 128, numbered starting from 0 in ascending
> > + order and discoverable at run-time.
> > + Each DBCH contains 32 individual fields, referred to as flags, each of which
> > + can be used independently. It is possible for the Sender to send multiple
> > + Transfers at once using a single DBCH, so long as each Transfer uses
> > + a different flag in the DBCH.
> > + Optionally, data may be transmitted through an out-of-band shared memory
> > + region, wherein the MHU Doorbell is used strictly as an interrupt generation
> > + mechanism, but this is out of the scope of these bindings.
> > +
> > + - FastChannel Extension (FCE): FCE defines a type of channel called a Fast
> > + Channel (FCH). FCH is intended for lower overhead communication between
> > + Sender and Receiver at the expense of determinism. An FCH allows the Sender
> > + to update the channel value at any time, regardless of whether the previous
> > + value has been seen by the Receiver. When the Receiver reads the channel's
> > + content it gets the last value written to the channel.
> > + FCH is considered lossy in nature, and means that the Sender has no way of
> > + knowing if, or when, the Receiver will act on the Transfer.
> > + FCHs are expected to behave as RAM which generates interrupts when writes
> > + occur to the locations within the RAM.
> > + When FCE is implemented, the number of FCHs that an implementation of the
> > + MHU can support is between 1-1024, if the FastChannel word-size is 32-bits,
> > + or between 1-512, when the FastChannel word-size is 64-bits.
> > + FCHs are numbered from 0 in ascending order.
> > + Note that the number of FCHs and the word-size are implementation defined,
> > + not configurable but discoverable at run-time.
> > + Optionally, data may be transmitted through an out-of-band shared memory
> > + region, wherein the MHU FastChannel is used as an interrupt generation
> > + mechanism which carries also a pointer to such out-of-band data, but this
> > + is out of the scope of these bindings.
> > +
> > + - FIFO Extension (FE): FE defines a Channel type called a FIFO Channel (FFCH).
> > + FFCH allows a Sender to send
> > + - Multiple Transfers to the Receiver without having to wait for the
> > + previous Transfer to be acknowledged by the Receiver, as long as the
> > + FIFO has room for the Transfer.
> > + - Transfers which require the Receiver to provide acknowledgment.
> > + - Transfers which have in-band payload.
> > + In all cases, the data is guaranteed to be observed by the Receiver in the
> > + same order which the Sender sent it.
> > + When FE is implemented, the number of FFCHs that an implementation of the
> > + MHU can support is between 1 and 64, numbered starting from 0 in ascending
> > + order. The number of FFCHs, their depth (same for all implemented FFCHs) and
> > + the access-granularity are implementation defined, not configurable but
> > + discoverable at run-time.
> > + Optionally, additional data may be transmitted through an out-of-band shared
> > + memory region, wherein the MHU FIFO is used to transmit, in order, a small
> > + part of the payload (like a header) and a reference to the shared memory
> > + area holding the remaining, bigger, chunk of the payload, but this is out of
> > + the scope of these bindings.
> > +
> > +properties:
> > + compatible:
> > + const: arm,mhuv3
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + minItems: 1
> > + maxItems: 74
> > +
> > + interrupt-names:
> > + description: |
> > + The MHUv3 controller generates a number of events some of which are used
> > + to generate interrupts; as a consequence it can expose a varying number of
> > + optional PBX/MBX interrupts, representing the events generated during the
> > + operation of the various transport protocols associated with different
> > + extensions. All interrupts of the MHU are level-sensitive.
> > + Some of these optional interrupts are defined per-channel, where the
> > + number of channels effectively available is implementation defined and
> > + run-time discoverable.
> > + In the following names are enumerated using patterns, with per-channel
> > + interrupts implicitly capped at the maximum channels allowed by the
> > + specification for each extension type.
> > + For the sake of simplicity maxItems is anyway capped to a most plausible
> > + number, assuming way less channels would be implemented than actually
> > + possible.
> > +
> > + The only mandatory interrupts on the MHU are:
> > + - combined
> > + - mbx-fch-xfer-<N> but only if mbx-fcgrp-xfer-<N> is not implemented.
> > +
> > + minItems: 1
> > + maxItems: 74
> > + items:
> > + oneOf:
> > + - const: combined
> > + description: PBX/MBX Combined interrupt
> > + - const: combined-ffch
> > + description: PBX/MBX FIFO Combined interrupt
> > + - pattern: '^ffch-low-tide-[0-9]+$'
> > + description: PBX/MBX FIFO Channel <N> Low Tide interrupt
> > + - pattern: '^ffch-high-tide-[0-9]+$'
> > + description: PBX/MBX FIFO Channel <N> High Tide interrupt
> > + - pattern: '^ffch-flush-[0-9]+$'
> > + description: PBX/MBX FIFO Channel <N> Flush interrupt
> > + - pattern: '^mbx-dbch-xfer-[0-9]+$'
> > + description: MBX Doorbell Channel <N> Transfer interrupt
> > + - pattern: '^mbx-fch-xfer-[0-9]+$'
> > + description: MBX FastChannel <N> Transfer interrupt
> > + - pattern: '^mbx-fchgrp-xfer-[0-9]+$'
> > + description: MBX FastChannel <N> Group Transfer interrupt
> > + - pattern: '^mbx-ffch-xfer-[0-9]+$'
> > + description: MBX FIFO Channel <N> Transfer interrupt
> > + - pattern: '^pbx-dbch-xfer-ack-[0-9]+$'
> > + description: PBX Doorbell Channel <N> Transfer Ack interrupt
> > + - pattern: '^pbx-ffch-xfer-ack-[0-9]+$'
> > + description: PBX FIFO Channel <N> Transfer Ack interrupt
> > +
> Can we have optional subnodes (with different properties as required)
> for each extension type ?
>
Not sure if I have understood properly you request, but the type of extensions
present in a PBX/MBX block can be discovered at runtime together with their
characteristics (like the number of channels) so the specific DT properties
can be searched (or NOT) based on the features discovered at run-time:
are you asking for subnodes as a means of adding clarity to what can be
defined in a block depending on what it is (PBX vs MBX) and what
extensions it has ?
if that is the reason ... the convoluted names like pbx/mbx-dbch- etc...
was my attempt at thhat :D ... to give a hint at what you can define in a PBX
vs MBX block and what is related to each extensions...
Note that, though, since the PBX/MBX blocks are discoverable at runtime as such,
they are not identified as such in the DT (same compatible) so I would not have
anywayy the capability to check in the DT which is which (PBX/MBX) and if the
properties are appropriate or not, nor I could know which extensions are really
implemented, so such subnodes would ony be a way of grouping props in the DT
without adding any compile time check capability nor adding any improvement to
the runtime DT parsing proces...
..BUT, of course, I could be wrong and missing a something here, so I
am happy to corrected on the subnodes utility...
>
> > + '#mbox-cells':
> > + description: |
> > + The first argument in the consumers 'mboxes' property represents the
> > + extension type, the second is for the channel number while the third
> > + depends on extension type.
> > +
> > + Extension type for DBE is 0 and the third parameter represents the
> > + doorbell flag number to use.
> > + Extension type for FCE is 1, third parameter unused.
> > + Extension type for FE is 2, third parameter unused.
> > +
> > + mboxes = <&mhu 0 0 5>; // DBE, Doorbell Channel Window 0, doorbell flag 5.
> > + mboxes = <&mhu 0 1 7>; // DBE, Doorbell Channel Window 1, doorbell flag 7.
> > + mboxes = <&mhu 1 0 0>; // FCE, FastChannel Window 0.
> > + mboxes = <&mhu 1 3 0>; // FCE, FastChannel Window 3.
> > + mboxes = <&mhu 2 1 0>; // FE, FIFO Channel Window 1.
> > + mboxes = <&mhu 2 7 0>; // FE, FIFO Channel Window 7.
> >
> Please define the extension types, instead of 0, 1 and 2.
>
I'll do.
Thanks for the review.
Cristian
On 04/04/2024 08:23, Cristian Marussi wrote:
> Add bindings for the ARM MHUv3 Mailbox controller.
>
> Signed-off-by: Cristian Marussi <[email protected]>
..
> + mailbox@2aaa0000 {
> + compatible = "arm,mhuv3";
> + #mbox-cells = <3>;
> + reg = <0 0x2aaa0000 0 0x10000>;
> + clocks = <&clock 0>;
> + interrupt-names = "combined", "pbx-dbch-xfer-ack-1",
> + "ffch-high-tide-0";
> + interrupts = <0 36 4>, <0 37 4>;
> + };
> +
> + mailbox@2ab00000 {
> + compatible = "arm,mhuv3";
> + #mbox-cells = <3>;
> + reg = <0 0x2aab0000 0 0x10000>;
> + clocks = <&clock 0>;
> + interrupt-names = "combined", "mbx-dbch-xfer-1", "ffch-low-tide-0";
> + interrupts = <0 35 4>, <0 38 4>, <0 39 4>;
Use defines for GIC and level flags.
With this fixed:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Thu, Apr 4, 2024 at 1:25 AM Cristian Marussi
<[email protected]> 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 <[email protected]>
> ---
> v1 -> v2
> - fixed checkpatch warnings about side-effects
> - fixed sparse errors as reported
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> ---
> 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 <[email protected]>
> +M: Cristian Marussi <[email protected]>
> +L: [email protected]
> +L: [email protected] (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 <[email protected]>
> L: [email protected]
> 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.
> +#define MHUV3_STAT_BITS (MHUV3_STAT_BYTES * __CHAR_BIT__)
>
just 32.
> +
> +/* 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.
> + 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.
> +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.
> + 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];
> + /* ------------------ */
> + 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 ?
> + 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.
> + /* 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);
> +}
> +
> +static int mhuv3_doorbell_rx_startup(struct mhuv3 *mhu, struct mbox_chan *chan)
> +{
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + /* Unmask Channel Transfer events */
> + writel_relaxed(BIT(priv->doorbell), &mhu->mbx->dbcw[priv->ch_idx]msk_clr);
> +
> + return 0;
> +}
> +
> +static void mhuv3_doorbell_rx_shutdown(struct mhuv3 *mhu,
> + struct mbox_chan *chan)
> +{
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + /* Mask Channel Transfer events */
> + writel_relaxed(BIT(priv->doorbell), &mhu->mbx->dbcw[priv->ch_idx]msk_set);
> +}
> +
> +static void mhuv3_doorbell_rx_complete(struct mhuv3 *mhu, struct mbox_chan *chan)
> +{
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + /* Clearing the pending transfer generates the Channel Transfer Ack */
> + writel_relaxed(BIT(priv->doorbell), &mhu->mbx->dbcw[priv->ch_idx]clr);
> +}
> +
> +static int mhuv3_doorbell_last_tx_done(struct mhuv3 *mhu,
> + struct mbox_chan *chan)
> +{
> + int done;
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + done = !(readl_relaxed(&mhu->pbx->dbcw[priv->ch_idx].st) &
> + BIT(priv->doorbell));
> + if (done) {
> + unsigned long flags;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> +
> + /* Take care to clear the pending doorbell also when polling */
> + spin_lock_irqsave(&e->pending_lock, flags);
> + e->pending_db[priv->ch_idx] &= ~BIT(priv->doorbell);
> + spin_unlock_irqrestore(&e->pending_lock, flags);
> + }
> +
> + return done;
> +}
> +
> +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);
> + /* 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 const struct mhuv3_protocol_ops mhuv3_doorbell_ops = {
> + .tx_startup = mhuv3_doorbell_tx_startup,
> + .tx_shutdown = mhuv3_doorbell_tx_shutdown,
> + .rx_startup = mhuv3_doorbell_rx_startup,
> + .rx_shutdown = mhuv3_doorbell_rx_shutdown,
> + .rx_complete = mhuv3_doorbell_rx_complete,
> + .last_tx_done = mhuv3_doorbell_last_tx_done,
> + .send_data = mhuv3_doorbell_send_data,
> +};
> +
> +/* Sender and receiver mailbox ops */
> +static bool mhuv3_sender_last_tx_done(struct mbox_chan *chan)
> +{
> + struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + return priv->ops->last_tx_done(mhu, chan);
> +}
> +
> +static int mhuv3_sender_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + if (!priv->ops->last_tx_done(mhu, chan))
> + return -EBUSY;
> +
> + return priv->ops->send_data(mhu, chan, data);
> +}
> +
> +static int mhuv3_sender_startup(struct mbox_chan *chan)
> +{
> + struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + if (priv->ops->tx_startup)
> + priv->ops->tx_startup(mhu, chan);
> +
> + return 0;
> +}
> +
> +static void mhuv3_sender_shutdown(struct mbox_chan *chan)
> +{
> + struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + if (priv->ops->tx_shutdown)
> + priv->ops->tx_shutdown(mhu, chan);
> +}
> +
> +static const struct mbox_chan_ops mhuv3_sender_ops = {
> + .send_data = mhuv3_sender_send_data,
> + .startup = mhuv3_sender_startup,
> + .shutdown = mhuv3_sender_shutdown,
> + .last_tx_done = mhuv3_sender_last_tx_done,
> +};
> +
> +static int mhuv3_receiver_startup(struct mbox_chan *chan)
> +{
> + struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + return priv->ops->rx_startup(mhu, chan);
> +}
> +
> +static void mhuv3_receiver_shutdown(struct mbox_chan *chan)
> +{
> + struct mhuv3 *mhu = mhu_from_mbox(chan->mbox);
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + priv->ops->rx_shutdown(mhu, chan);
> +}
> +
> +static int mhuv3_receiver_send_data(struct mbox_chan *chan, void *data)
> +{
> + dev_err(chan->mbox->dev,
> + "Trying to transmit on a MBX MHUv3 frame\n");
> + return -EIO;
> +}
> +
> +static bool mhuv3_receiver_last_tx_done(struct mbox_chan *chan)
> +{
> + dev_err(chan->mbox->dev, "Trying to Tx poll on a MBX MHUv3 frame\n");
> + return true;
> +}
> +
> +static const struct mbox_chan_ops mhuv3_receiver_ops = {
> + .send_data = mhuv3_receiver_send_data,
> + .startup = mhuv3_receiver_startup,
> + .shutdown = mhuv3_receiver_shutdown,
> + .last_tx_done = mhuv3_receiver_last_tx_done,
> +};
> +
> +static struct mbox_chan *mhuv3_dbe_mbox_of_xlate(struct mhuv3 *mhu,
> + unsigned int channel,
> + unsigned int doorbell)
> +{
> + struct mbox_controller *mbox = &mhu->mbox;
> + struct mbox_chan *chans = mbox->chans;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> +
> + if (channel >= e->max_chans || doorbell >= MHUV3_STAT_BITS) {
> + dev_err(mbox->dev, "Couldn't xlate to a valid channel (%d: %d)\n",
> + channel, doorbell);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + return &chans[e->base_ch_idx + channel * MHUV3_STAT_BITS + doorbell];
> +}
> +
> +static void mhuv3_dbe_combined_irq_setup(struct mhuv3 *mhu)
> +{
> + int i;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> +
> + if (mhu->frame == PBX_FRAME) {
> + struct pdbcw_page __iomem *dbcw = mhu->pbx->dbcw;
> +
> + for (i = 0; i < e->max_chans; i++) {
> + writel_relaxed_bitfield(0x1, &dbcw[i].int_clr, tfr_ack);
> + writel_relaxed_bitfield(0x0, &dbcw[i].int_en, tfr_ack);
> + writel_relaxed_bitfield(0x1, &dbcw[i].ctrl, comb_en);
> + }
> + } else {
> + struct mdbcw_page __iomem *dbcw = mhu->mbx->dbcw;
> +
> + for (i = 0; i < e->max_chans; i++) {
> + writel_relaxed(0xFFFFFFFF, &dbcw[i].clr);
> + writel_relaxed(0xFFFFFFFF, &dbcw[i].msk_set);
> + writel_relaxed_bitfield(0x1, &dbcw[i].ctrl, comb_en);
> + }
> + }
> +}
> +
> +static int mhuv3_dbe_channels_init(struct mhuv3 *mhu)
> +{
> + int i;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> + struct mbox_controller *mbox = &mhu->mbox;
> + struct mbox_chan *chans;
> +
> + chans = mbox->chans + mbox->num_chans;
> + e->base_ch_idx = mbox->num_chans;
> + for (i = 0; i < e->max_chans; i++) {
> + int k;
> + struct mhuv3_mbox_chan_priv *priv;
> +
> + for (k = 0; k < MHUV3_STAT_BITS; k++) {
> + priv = devm_kmalloc(mbox->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->ch_idx = i;
> + priv->ops = &mhuv3_doorbell_ops;
> + priv->doorbell = k;
> + chans++->con_priv = priv;
> + mbox->num_chans++;
> + }
> + }
> +
> + spin_lock_init(&e->pending_lock);
> +
> + return 0;
> +}
> +
> +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.
> + if (channel >= e->max_chans) {
> + dev_err(dev, "Invalid %s channel:%d\n",
> + mhuv3_str[mhu->frame], channel);
> + 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",
> + 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);
> +
> + 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++)
> + 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;
> + }
> +
> + 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);
> + 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);
> +
> + 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)) {
>
'continue' for error instead, to have fewer indented lines.
> + 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.
> + 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));
> + }
> +
> + 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);
> + }
> +
> + dev_info(dev, "Using PBX in Tx polling mode.\n");
> + 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 -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;
> + }
> +
> + 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);
> + } 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);
> +
> + platform_set_drvdata(pdev, mhu);
> +
> + 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);
> +
> + 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 <[email protected]>");
> --
> 2.34.1
>
On Thu, 4 Apr 2024 07:23:47 +0100
Cristian Marussi <[email protected]> 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 <[email protected]>
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 <[email protected]>");
On Mon, Apr 08, 2024 at 11:40:24AM +0100, Cristian Marussi wrote:
> On Sun, Apr 07, 2024 at 08:14:23PM -0500, Jassi Brar wrote:
> > On Thu, Apr 4, 2024 at 1:25 AM Cristian Marussi
> > <[email protected]> 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 !
>
[snip]
> > > Signed-off-by: Cristian Marussi <[email protected]>
> > > ---
> > > +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.
>
.replying to myself here...I am dropping bitfields too in favour of
bitmasks...I've read too many horror stories this afternoon about bitfields
and compilers interactions and their flaky usage while handling reg maps to
still feel comfortable using them...
Thanks,
Cristian