[PATCH 0/3] Apple Mailbox Controller support

[PATCH 0/3] Apple Mailbox Controller support

[Sven Peter]

Hi,

This series adds support for the mailbox HW found in the Apple M1. These SoCs
have various co-processors controlling different peripherals (NVMe, display
controller, SMC (required for WiFi), Thunderbolt, and probably more
we don't know about yet). All these co-processors communicate with the main CPU
using these mailboxes. These mailboxes transmit 64+32 bit messages, are
backed by a hardware FIFO and have four interrupts (FIFO empty and FIFO not
empty for the transmit and receive FIFO each).

The hardware itself allows to send 64+32 bit message using two hardware
registers. A write to or read from the second register transmits or receives a
message. Usually, the first 64 bit register is used for the message itself and 
8 bits of the second register are used as an endpoint. I originally considered
to have the endpoint exposed as a mailbox-channel, but finally decided against
it: The hardware itself only provides a single channel to the co-processor and
the endpoint bits are only an implementation detail of the firmware. There's
even one co-processor (SEP) which uses 8 bits of the first register as its
endpoint number instead.
There was a similar discussion about the BCM2835 / Raspberry Pi mailboxes
which came to the same conclusion [1].

These mailboxes also have a hardware FIFO which make implementing them with the
current mailbox a bit tricky: There is no "transmission done" interrupt because
most transmissions are "done" immediately. There is only a "transmission fifo
empty" level interrupt. I have instead implemented this by adding a fast-path to
the core mailbox code as a new txready_fifo mode.
The other possibilities (which would not require any changes to the core mailbox
code) are to either use the polling mode or to enable the "tx fifo empty"
interrupt in send_message and then call txready from the irq handler before
disabling it again. I'd like to avoid those though since so far I've never seen
the TX FIFO run full which allows to almost always avoid the context switch when
sending a message. I can easily switch to one of these modes if you prefer to
keep the core code untouched though.


Best,

Sven

Sven Peter (3):
  mailbox: Add txdone_fifo
  dt-bindings: mailbox: Add Apple mailbox bindings
  mailbox: apple: Add driver for Apple mailboxes

 .../bindings/mailbox/apple,mailbox.yaml       |  81 ++++
 MAINTAINERS                                   |   3 +
 drivers/mailbox/Kconfig                       |  12 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/apple-mailbox.c               | 432 ++++++++++++++++++
 drivers/mailbox/mailbox.c                     |  66 ++-
 drivers/mailbox/mailbox.h                     |   1 +
 include/linux/apple-mailbox.h                 |  18 +
 include/linux/mailbox_controller.h            |  15 +
 9 files changed, 621 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
 create mode 100644 drivers/mailbox/apple-mailbox.c
 create mode 100644 include/linux/apple-mailbox.h

-- 
2.25.1

[PATCH 1/3] mailbox: Add txdone_fifo

[Sven Peter]

Some mailbox controllers are backed by a hardware FIFO and only provide
a "FIFO not full" or "FIFO empty" interrupt. In the most common case
it is however not neccesarry to wait for this interrupt since there
is enough space in the FIFO and the message can be accepted directly.
txdone_fifo adds supports for such controllers. Messages are assumed
to have been transmitted as long as ops->send_data() returns success.
Only if the FIFO is full and ops->send_data() returns -EBUSY the
interrupt is enabled with ops->request_irq().

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/mailbox/mailbox.c          | 66 ++++++++++++++++++++++++++----
 drivers/mailbox/mailbox.h          |  1 +
 include/linux/mailbox_controller.h | 15 +++++++
 3 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 3e7d4b20ab34..cd628d98d211 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -50,6 +50,15 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 	return idx;
 }
 
+static void complete_msg(struct mbox_chan *chan, void *msg, int r)
+{
+	if (chan->cl->tx_done)
+		chan->cl->tx_done(chan->cl, msg, r);
+
+	if (r != -ETIME && chan->cl->tx_block)
+		complete(&chan->tx_complete);
+}
+
 static void msg_submit(struct mbox_chan *chan)
 {
 	unsigned count, idx;
@@ -59,7 +68,7 @@ static void msg_submit(struct mbox_chan *chan)
 
 	spin_lock_irqsave(&chan->lock, flags);
 
-	if (!chan->msg_count || chan->active_req)
+	if (!chan->msg_count || chan->active_req || chan->irq_pending)
 		goto exit;
 
 	count = chan->msg_count;
@@ -75,15 +84,30 @@ static void msg_submit(struct mbox_chan *chan)
 		chan->cl->tx_prepare(chan->cl, data);
 	/* Try to submit a message to the MBOX controller */
 	err = chan->mbox->ops->send_data(chan, data);
-	if (!err) {
-		chan->active_req = data;
+	if (err) {
+		/* HW FIFO is full and we need to wait for an irq to continue */
+		if (chan->txdone_method & TXDONE_BY_FIFO) {
+			chan->irq_pending = true;
+			chan->mbox->ops->request_irq(chan);
+		}
+	} else {
+		/* controllers with a FIFO have already accepted the message */
+		if (!(chan->txdone_method & TXDONE_BY_FIFO))
+			chan->active_req = data;
 		chan->msg_count--;
 	}
 exit:
 	spin_unlock_irqrestore(&chan->lock, flags);
 
+	if (err)
+		return;
+
+	/* controllers with a FIFO have already accepted the message */
+	if (chan->txdone_method & TXDONE_BY_FIFO)
+		complete_msg(chan, data, 0);
+
 	/* kick start the timer immediately to avoid delays */
-	if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
+	if (chan->txdone_method & TXDONE_BY_POLL) {
 		/* but only if not already active */
 		if (!hrtimer_active(&chan->mbox->poll_hrt))
 			hrtimer_start(&chan->mbox->poll_hrt, 0, HRTIMER_MODE_REL);
@@ -107,11 +131,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
 		return;
 
 	/* Notify the client */
-	if (chan->cl->tx_done)
-		chan->cl->tx_done(chan->cl, mssg, r);
-
-	if (r != -ETIME && chan->cl->tx_block)
-		complete(&chan->tx_complete);
+	complete_msg(chan, mssg, r);
 }
 
 static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
@@ -179,6 +199,31 @@ void mbox_chan_txdone(struct mbox_chan *chan, int r)
 }
 EXPORT_SYMBOL_GPL(mbox_chan_txdone);
 
+/**
+ * mbox_chan_txfifo_ready - A way for controller driver to notify the
+ *			    framework that there is space in the FIFO
+ *			    to accept messages again.
+ * @chan: Pointer to the mailbox chan on which there is FIFO space available
+ *
+ * The controller that has IRQ for TX ACK calls this atomic API
+ * to notify that there is space in the hardware FIFO again.
+ * It works only if txdone_fifo is set by the controller.
+ * This must be called from within the IRQ handler.
+ */
+void mbox_chan_txfifo_ready(struct mbox_chan *chan)
+{
+	if (unlikely(!(chan->txdone_method & TXDONE_BY_FIFO))) {
+		dev_err(chan->mbox->dev,
+		       "Controller can't run the TX FIFO ticker\n");
+		return;
+	}
+
+	WRITE_ONCE(chan->irq_pending, false);
+	while (!READ_ONCE(chan->irq_pending) && READ_ONCE(chan->msg_count))
+		msg_submit(chan);
+}
+EXPORT_SYMBOL_GPL(mbox_chan_txfifo_ready);
+
 /**
  * mbox_client_txdone - The way for a client to run the TX state machine.
  * @chan: Mailbox channel assigned to this client.
@@ -376,6 +421,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
 	chan->msg_free = 0;
 	chan->msg_count = 0;
 	chan->active_req = NULL;
+	chan->irq_pending = false;
 	chan->cl = cl;
 	init_completion(&chan->tx_complete);
 
@@ -485,6 +531,8 @@ int mbox_controller_register(struct mbox_controller *mbox)
 
 	if (mbox->txdone_irq)
 		txdone = TXDONE_BY_IRQ;
+	else if (mbox->txdone_fifo)
+		txdone = TXDONE_BY_FIFO;
 	else if (mbox->txdone_poll)
 		txdone = TXDONE_BY_POLL;
 	else /* It has to be ACK then */
diff --git a/drivers/mailbox/mailbox.h b/drivers/mailbox/mailbox.h
index 046d6d258b32..710525341df6 100644
--- a/drivers/mailbox/mailbox.h
+++ b/drivers/mailbox/mailbox.h
@@ -6,5 +6,6 @@
 #define TXDONE_BY_IRQ	BIT(0) /* controller has remote RTR irq */
 #define TXDONE_BY_POLL	BIT(1) /* controller can read status of last TX */
 #define TXDONE_BY_ACK	BIT(2) /* S/W ACK received by Client ticks the TX */
+#define TXDONE_BY_FIFO	BIT(3) /* controller has HW FIFO with not-empty irq */
 
 #endif /* __MAILBOX_H */
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 36d6ce673503..638cc20b9ebc 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -42,6 +42,11 @@ struct mbox_chan;
  *		  Used only if txdone_poll:=true && txdone_irq:=false
  * @peek_data: Atomic check for any received data. Return true if controller
  *		  has some data to push to the client. False otherwise.
+ * @request_irq: If the controller sets 'txdone_fifo', the API calls this
+ *		 to enable the 'ready to submit' interrupt whenever a previous
+ *		 call to send_data was not successful. This interrupt must fire
+ *		 only once and must call mbox_chan_txfifo_ready when new
+ *		 messages can be accepted into the hardware FIFO again.
  */
 struct mbox_chan_ops {
 	int (*send_data)(struct mbox_chan *chan, void *data);
@@ -50,6 +55,7 @@ struct mbox_chan_ops {
 	void (*shutdown)(struct mbox_chan *chan);
 	bool (*last_tx_done)(struct mbox_chan *chan);
 	bool (*peek_data)(struct mbox_chan *chan);
+	void (*request_irq)(struct mbox_chan *chan);
 };
 
 /**
@@ -61,6 +67,11 @@ struct mbox_chan_ops {
  * @txdone_irq:		Indicates if the controller can report to API when
  *			the last transmitted data was read by the remote.
  *			Eg, if it has some TX ACK irq.
+ * @txdone_fifo:	Indicates if the controller has a hardware FIFO which
+ *			can directly accept multiple messages without delay.
+ *			In case the FIFO cannot accept a message ops->send_data
+ *			returns -EBUSY and ops->request_irq is used to request
+ *			an interrupt once the FIFO has space for new messages.
  * @txdone_poll:	If the controller can read but not report the TX
  *			done. Ex, some register shows the TX status but
  *			no interrupt rises. Ignored if 'txdone_irq' is set.
@@ -77,6 +88,7 @@ struct mbox_controller {
 	struct mbox_chan *chans;
 	int num_chans;
 	bool txdone_irq;
+	bool txdone_fifo;
 	bool txdone_poll;
 	unsigned txpoll_period;
 	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
@@ -108,6 +120,7 @@ struct mbox_controller {
  * @cl:			Pointer to the current owner of this channel
  * @tx_complete:	Transmission completion
  * @active_req:		Currently active request hook
+ * @irq_pending:	Set if the API is waiting for an interrupt to continue
  * @msg_count:		No. of mssg currently queued
  * @msg_free:		Index of next available mssg slot
  * @msg_data:		Hook for data packet
@@ -120,6 +133,7 @@ struct mbox_chan {
 	struct mbox_client *cl;
 	struct completion tx_complete;
 	void *active_req;
+	bool irq_pending;
 	unsigned msg_count, msg_free;
 	void *msg_data[MBOX_TX_QUEUE_LEN];
 	spinlock_t lock; /* Serialise access to the channel */
@@ -130,6 +144,7 @@ int mbox_controller_register(struct mbox_controller *mbox); /* can sleep */
 void mbox_controller_unregister(struct mbox_controller *mbox); /* can sleep */
 void mbox_chan_received_data(struct mbox_chan *chan, void *data); /* atomic */
 void mbox_chan_txdone(struct mbox_chan *chan, int r); /* atomic */
+void mbox_chan_txfifo_ready(struct mbox_chan *chan); /* atomic */
 
 int devm_mbox_controller_register(struct device *dev,
 				  struct mbox_controller *mbox);
-- 
2.25.1

[PATCH 2/3] dt-bindings: mailbox: Add Apple mailbox bindings

[Sven Peter]

Apple mailbox controller are found on the M1 and are used for
communication with various co-processors.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 .../bindings/mailbox/apple,mailbox.yaml       | 81 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml b/Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
new file mode 100644
index 000000000000..360d8a162cc5
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/apple,mailbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mailbox Controller
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+  - Sven Peter <sven@svenpeter.dev>
+
+description:
+  The Apple mailbox consists of two FIFOs used to exchange 64+32 bit
+  messages between the main CPU and a co-processor. Multiple instances
+  of this mailbox can be found on Apple SoCs.
+  One of the two FIFOs is used to send data to a co-processor while the other
+  FIFO is used for the other direction.
+  Various clients implement different IPC protocols based on these simple
+  messages and shared memory buffers.
+
+properties:
+  compatible:
+    oneOf:
+      - description:
+          ASC mailboxes are the most common variant found on the M1.
+        items:
+          - const: apple,t8103-asc-mailbox
+
+      - description:
+          M3 mailboxes are an older variant with a slightly different MMIO
+          interface still found on the M1.
+        items:
+          - const: apple,t8103-m3-mailbox
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 4
+    items:
+      - description: send fifo is empty interrupt
+      - description: send fifo is not empty interrupt
+      - description: receive fifo is empty interrupt
+      - description: receive fifo is not empty interrupt
+
+  interrupt-names:
+    minItems: 4
+    items:
+      - const: send-empty
+      - const: send-not-empty
+      - const: recv-empty
+      - const: recv-not-empty
+
+  clocks:
+    description:
+      Reference to the clock gate phandle(s) if required for this mailbox.
+      Optional since not all mailboxes are attached to a clock gate.
+
+  "#mbox-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+        mailbox@77408000 {
+                compatible = "apple,t8103-asc-mailbox";
+                reg = <0x77408000 0x4000>;
+                interrupts = <1 583 4>, <1 584 4>, <1 585 4>, <1 586 4>;
+                interrupt-names = "send-empty", "send-not-empty",
+                 "recv-empty", "recv-not-empty";
+                #mbox-cells = <0>;
+        };
diff --git a/MAINTAINERS b/MAINTAINERS
index cad1289793db..47de27282c98 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1720,6 +1720,7 @@ C:	irc://irc.oftc.net/asahi-dev
 T:	git https://github.com/AsahiLinux/linux.git
 F:	Documentation/devicetree/bindings/arm/apple.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
+F:	Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
-- 
2.25.1

[PATCH 3/3] mailbox: apple: Add driver for Apple mailboxes

[Sven Peter]

Apple SoCs such as the M1 come with various co-processors. Mailboxes
are used to communicate with those. This driver adds support for
two variants of those mailboxes.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 MAINTAINERS                     |   2 +
 drivers/mailbox/Kconfig         |  12 +
 drivers/mailbox/Makefile        |   2 +
 drivers/mailbox/apple-mailbox.c | 432 ++++++++++++++++++++++++++++++++
 include/linux/apple-mailbox.h   |  18 ++
 5 files changed, 466 insertions(+)
 create mode 100644 drivers/mailbox/apple-mailbox.c
 create mode 100644 include/linux/apple-mailbox.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 47de27282c98..cf0500bbea5b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1724,8 +1724,10 @@ F:	Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
+F:	drivers/mailbox/apple-mailbox.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 F:	include/dt-bindings/pinctrl/apple.h
+F:	include/linux/apple-mailbox.h
 
 ARM/ARTPEC MACHINE SUPPORT
 M:	Jesper Nilsson <jesper.nilsson@axis.com>
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c9fc06c7e685..a2caff75047f 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -8,6 +8,18 @@ menuconfig MAILBOX
 
 if MAILBOX
 
+config APPLE_MBOX
+	tristate "Apple Mailbox driver"
+	depends on ARCH_APPLE || (ARM64 && COMPILE_TEST)
+	default ARCH_APPLE
+	help
+	  Apple SoCs have various co-processors that need to be started and
+	  initialized for certain peripherals to work (NVMe, display controller,
+	  etc.). This driver adds support for the mailbox controller used to
+	  communicate with those.
+
+	  Say Y here if you have a Apple SoC.
+
 config ARM_MHU
 	tristate "ARM MHU Mailbox"
 	depends on ARM_AMBA
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index c2089f04887e..896554c7c169 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -58,3 +58,5 @@ obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
 obj-$(CONFIG_SPRD_MBOX)		+= sprd-mailbox.o
 
 obj-$(CONFIG_QCOM_IPCC)		+= qcom-ipcc.o
+
+obj-$(CONFIG_APPLE_MBOX)	+= apple-mailbox.o
diff --git a/drivers/mailbox/apple-mailbox.c b/drivers/mailbox/apple-mailbox.c
new file mode 100644
index 000000000000..d59642d9216a
--- /dev/null
+++ b/drivers/mailbox/apple-mailbox.c
@@ -0,0 +1,432 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple mailbox driver
+ *
+ * Copyright (C) 2021 The Asahi Linux Contributors
+ *
+ * This driver adds support for two mailbox variants (called ASC and M3 by
+ * Apple) found in Apple SoCs such as the M1. It consists of two FIFOs used to
+ * exchange 64+32 bit messages between the main CPU and a co-processor.
+ * Various clients implement different IPC protocols based on these simple
+ * messages and shared memory buffers.
+ *
+ * Both the main CPU and the co-processor see the same set of registers but
+ * the first FIFO (A2I) is always used to transfer messages from the application
+ * processor (us) to the I/O processor and the second one (I2A) for the
+ * other direction.
+ */
+
+#include <linux/apple-mailbox.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/gfp.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#define APPLE_ASC_MBOX_CONTROL_FULL BIT(16)
+#define APPLE_ASC_MBOX_CONTROL_EMPTY BIT(17)
+
+#define APPLE_ASC_MBOX_A2I_CONTROL 0x110
+#define APPLE_ASC_MBOX_A2I_SEND0 0x800
+#define APPLE_ASC_MBOX_A2I_SEND1 0x808
+#define APPLE_ASC_MBOX_A2I_RECV0 0x810
+#define APPLE_ASC_MBOX_A2I_RECV1 0x818
+
+#define APPLE_ASC_MBOX_I2A_CONTROL 0x114
+#define APPLE_ASC_MBOX_I2A_SEND0 0x820
+#define APPLE_ASC_MBOX_I2A_SEND1 0x828
+#define APPLE_ASC_MBOX_I2A_RECV0 0x830
+#define APPLE_ASC_MBOX_I2A_RECV1 0x838
+
+#define APPLE_M3_MBOX_A2I_CONTROL 0x50
+#define APPLE_M3_MBOX_A2I_SEND0 0x60
+#define APPLE_M3_MBOX_A2I_SEND1 0x68
+#define APPLE_M3_MBOX_A2I_RECV0 0x70
+#define APPLE_M3_MBOX_A2I_RECV1 0x78
+
+#define APPLE_M3_MBOX_I2A_CONTROL 0x80
+#define APPLE_M3_MBOX_I2A_SEND0 0x90
+#define APPLE_M3_MBOX_I2A_SEND1 0x98
+#define APPLE_M3_MBOX_I2A_RECV0 0xa0
+#define APPLE_M3_MBOX_I2A_RECV1 0xa8
+
+#define APPLE_M3_MBOX_CONTROL_FULL BIT(16)
+#define APPLE_M3_MBOX_CONTROL_EMPTY BIT(17)
+
+#define APPLE_M3_MBOX_IRQ_ENABLE 0x48
+#define APPLE_M3_MBOX_IRQ_ACK 0x4c
+#define APPLE_M3_MBOX_IRQ_A2I_EMPTY BIT(0)
+#define APPLE_M3_MBOX_IRQ_A2I_NOT_EMPTY BIT(1)
+#define APPLE_M3_MBOX_IRQ_I2A_EMPTY BIT(2)
+#define APPLE_M3_MBOX_IRQ_I2A_NOT_EMPTY BIT(3)
+
+#define APPLE_MBOX_MSG1_OUTCNT GENMASK(56, 52)
+#define APPLE_MBOX_MSG1_INCNT GENMASK(51, 48)
+#define APPLE_MBOX_MSG1_OUTPTR GENMASK(47, 44)
+#define APPLE_MBOX_MSG1_INPTR GENMASK(43, 40)
+#define APPLE_MBOX_MSG1_MSG GENMASK(31, 0)
+
+struct apple_mbox_hw {
+	unsigned int control_full;
+	unsigned int control_empty;
+
+	unsigned int a2i_control;
+	unsigned int a2i_send0;
+	unsigned int a2i_send1;
+
+	unsigned int i2a_control;
+	unsigned int i2a_recv0;
+	unsigned int i2a_recv1;
+
+	bool has_irq_controls;
+	unsigned int irq_enable;
+	unsigned int irq_ack;
+	unsigned int irq_bit_recv_not_empty;
+	unsigned int irq_bit_send_empty;
+};
+
+static const struct apple_mbox_hw apple_mbox_asc_hw = {
+	.control_full = APPLE_ASC_MBOX_CONTROL_FULL,
+	.control_empty = APPLE_ASC_MBOX_CONTROL_EMPTY,
+
+	.a2i_control = APPLE_ASC_MBOX_A2I_CONTROL,
+	.a2i_send0 = APPLE_ASC_MBOX_A2I_SEND0,
+	.a2i_send1 = APPLE_ASC_MBOX_A2I_SEND1,
+
+	.i2a_control = APPLE_ASC_MBOX_I2A_CONTROL,
+	.i2a_recv0 = APPLE_ASC_MBOX_I2A_RECV0,
+	.i2a_recv1 = APPLE_ASC_MBOX_I2A_RECV1,
+
+	.has_irq_controls = false,
+};
+
+static const struct apple_mbox_hw apple_mbox_m3_hw = {
+	.control_full = APPLE_M3_MBOX_CONTROL_FULL,
+	.control_empty = APPLE_M3_MBOX_CONTROL_EMPTY,
+
+	.a2i_control = APPLE_M3_MBOX_A2I_CONTROL,
+	.a2i_send0 = APPLE_M3_MBOX_A2I_SEND0,
+	.a2i_send1 = APPLE_M3_MBOX_A2I_SEND1,
+
+	.i2a_control = APPLE_M3_MBOX_I2A_CONTROL,
+	.i2a_recv0 = APPLE_M3_MBOX_I2A_RECV0,
+	.i2a_recv1 = APPLE_M3_MBOX_I2A_RECV1,
+
+	.has_irq_controls = true,
+	.irq_enable = APPLE_M3_MBOX_IRQ_ENABLE,
+	.irq_ack = APPLE_M3_MBOX_IRQ_ACK,
+	.irq_bit_recv_not_empty = APPLE_M3_MBOX_IRQ_I2A_NOT_EMPTY,
+	.irq_bit_send_empty = APPLE_M3_MBOX_IRQ_A2I_EMPTY,
+};
+
+static const struct of_device_id apple_mbox_of_match[] = {
+	{ .compatible = "apple,t8103-asc-mailbox", .data = &apple_mbox_asc_hw },
+	{ .compatible = "apple,t8103-m3-mailbox", .data = &apple_mbox_m3_hw },
+	{}
+};
+MODULE_DEVICE_TABLE(of, apple_mbox_of_match);
+
+struct apple_mbox {
+	void __iomem *regs;
+	const struct apple_mbox_hw *hw;
+
+	int irq_recv_not_empty;
+	int irq_send_empty;
+
+	struct clk_bulk_data *clks;
+	int num_clks;
+
+	struct mbox_chan chan;
+
+	struct device *dev;
+	struct mbox_controller controller;
+};
+
+static bool apple_mbox_hw_can_send(struct apple_mbox *apple_mbox)
+{
+	u32 mbox_ctrl =
+		readl_relaxed(apple_mbox->regs + apple_mbox->hw->a2i_control);
+
+	return !(mbox_ctrl & apple_mbox->hw->control_full);
+}
+
+static void apple_mbox_hw_send(struct apple_mbox *apple_mbox,
+			       struct apple_mbox_msg *msg)
+{
+	WARN_ON(!apple_mbox_hw_can_send(apple_mbox));
+
+	dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1);
+
+	/*
+	 * This message may be related to a shared memory buffer and we must
+	 * ensure all previous writes to normal memory are visible before
+	 * submitting it.
+	 */
+	dma_wmb();
+
+	writeq_relaxed(msg->msg0, apple_mbox->regs + apple_mbox->hw->a2i_send0);
+	writeq_relaxed(FIELD_PREP(APPLE_MBOX_MSG1_MSG, msg->msg1),
+		       apple_mbox->regs + apple_mbox->hw->a2i_send1);
+}
+
+static bool apple_mbox_hw_can_recv(struct apple_mbox *apple_mbox)
+{
+	u32 mbox_ctrl =
+		readl_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_control);
+
+	return !(mbox_ctrl & apple_mbox->hw->control_empty);
+}
+
+static void apple_mbox_hw_recv(struct apple_mbox *apple_mbox,
+			       struct apple_mbox_msg *msg)
+{
+	WARN_ON(!apple_mbox_hw_can_recv(apple_mbox));
+
+	msg->msg0 = readq_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_recv0);
+	msg->msg1 = FIELD_GET(
+		APPLE_MBOX_MSG1_MSG,
+		readq_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_recv1));
+
+	dev_dbg(apple_mbox->dev, "< RX %016llx %08x\n", msg->msg0, msg->msg1);
+
+	/*
+	 * This message may be related to a shared memory buffer and we must
+	 * ensure any following reads from normal memory only happen after
+	 * having read this message.
+	 */
+	dma_rmb();
+}
+
+static int apple_mbox_chan_send_data(struct mbox_chan *chan, void *data)
+{
+	struct apple_mbox *apple_mbox = chan->con_priv;
+	struct apple_mbox_msg *msg = data;
+
+	if (!apple_mbox_hw_can_send(apple_mbox)) {
+		dev_dbg(apple_mbox->dev, "FIFO full, waiting for IRQ\n");
+		return -EBUSY;
+	}
+
+	apple_mbox_hw_send(apple_mbox, msg);
+	return 0;
+}
+
+static irqreturn_t apple_mbox_send_empty_irq(int irq, void *data)
+{
+	struct apple_mbox *apple_mbox = data;
+
+	dev_dbg(apple_mbox->dev, "got FIFO empty IRQ\n");
+
+	/*
+	 * We don't need to acknowledge the interrupt at the mailbox level
+	 * here even if supported by the hardware. It will keep firing but that
+	 * doesn't matter since it's disabled at the main interrupt controller.
+	 * apple_mbox_chan_request_irq will acknowledge it before enabling
+	 * it at the main controller again.
+	 */
+	disable_irq_nosync(apple_mbox->irq_send_empty);
+	mbox_chan_txfifo_ready(&apple_mbox->chan);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
+{
+	struct apple_mbox *apple_mbox = data;
+	struct apple_mbox_msg msg;
+
+	while (apple_mbox_hw_can_recv(apple_mbox)) {
+		apple_mbox_hw_recv(apple_mbox, &msg);
+		mbox_chan_received_data(&apple_mbox->chan, (void *)&msg);
+	}
+
+	/*
+	 * The interrupt will keep firing even if there are no more messages
+	 * unless we also acknowledge it at the mailbox level here.
+	 * There's no race if a message comes in between the check in the while
+	 * loop above and the ack below: If a new messages arrives inbetween
+	 * those two the interrupt will just fire again immediately after the
+	 * ack since it's level sensitive.
+	 */
+	if (apple_mbox->hw->has_irq_controls)
+		writel_relaxed(apple_mbox->hw->irq_bit_recv_not_empty,
+			       apple_mbox->regs + apple_mbox->hw->irq_ack);
+
+	return IRQ_HANDLED;
+}
+
+static struct mbox_chan *apple_mbox_of_xlate(struct mbox_controller *mbox,
+					     const struct of_phandle_args *spec)
+{
+	struct apple_mbox *apple_mbox = dev_get_drvdata(mbox->dev);
+
+	if (spec->args_count != 0)
+		return ERR_PTR(-EINVAL);
+	if (apple_mbox->chan.con_priv)
+		return ERR_PTR(-EBUSY);
+
+	apple_mbox->chan.con_priv = apple_mbox;
+	return &apple_mbox->chan;
+}
+
+static int apple_mbox_chan_startup(struct mbox_chan *chan)
+{
+	struct apple_mbox *apple_mbox = chan->con_priv;
+
+	/*
+	 * Only some variants of this mailbox HW provide interrupt control
+	 * at the mailbox level. We therefore need to handle enabling/disabling
+	 * interrupts at the main interrupt controller anyway for hardware that
+	 * doesn't. Just always keep the interrupts we care about enabled at
+	 * the mailbox level so that both hardware revisions behave almost
+	 * the same.
+	 */
+	if (apple_mbox->hw->has_irq_controls)
+		writel_relaxed(apple_mbox->hw->irq_bit_recv_not_empty |
+				       apple_mbox->hw->irq_bit_send_empty,
+			       apple_mbox->regs + apple_mbox->hw->irq_enable);
+
+	enable_irq(apple_mbox->irq_recv_not_empty);
+	return 0;
+}
+
+static void apple_mbox_chan_shutdown(struct mbox_chan *chan)
+{
+	struct apple_mbox *apple_mbox = chan->con_priv;
+
+	disable_irq(apple_mbox->irq_recv_not_empty);
+}
+
+static void apple_mbox_chan_request_irq(struct mbox_chan *chan)
+{
+	struct apple_mbox *apple_mbox = chan->con_priv;
+
+	/*
+	 * The interrupt is level sensitive and will keep firing as long as the
+	 * FIFO is empty. It will also keep firing if the FIFO was empty
+	 * at any point in the past until it has been acknowledged at the
+	 * mailbox level. By acknowledging it here we can ensure that we will
+	 * only get the interrupt once the FIFO has been cleared again.
+	 * If the FIFO is already empty before the ack it will fire again
+	 * immediately after the ack.
+	 */
+	if (apple_mbox->hw->has_irq_controls)
+		writel_relaxed(apple_mbox->hw->irq_bit_send_empty,
+			       apple_mbox->regs + apple_mbox->hw->irq_ack);
+	enable_irq(apple_mbox->irq_send_empty);
+}
+
+static const struct mbox_chan_ops apple_mbox_ops = {
+	.send_data = apple_mbox_chan_send_data,
+	.request_irq = apple_mbox_chan_request_irq,
+	.startup = apple_mbox_chan_startup,
+	.shutdown = apple_mbox_chan_shutdown,
+};
+
+static int apple_mbox_probe(struct platform_device *pdev)
+{
+	int ret;
+	const struct of_device_id *match;
+	char *irqname;
+	struct apple_mbox *mbox;
+	struct device *dev = &pdev->dev;
+
+	match = of_match_node(apple_mbox_of_match, pdev->dev.of_node);
+	if (!match)
+		return -EINVAL;
+	if (!match->data)
+		return -EINVAL;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, mbox);
+
+	mbox->dev = dev;
+	mbox->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mbox->regs))
+		return PTR_ERR(mbox->regs);
+
+	mbox->hw = match->data;
+	mbox->irq_recv_not_empty =
+		platform_get_irq_byname(pdev, "recv-not-empty");
+	if (mbox->irq_recv_not_empty < 0)
+		return -ENODEV;
+
+	mbox->irq_send_empty = platform_get_irq_byname(pdev, "send-empty");
+	if (mbox->irq_send_empty < 0)
+		return -ENODEV;
+
+	ret = devm_clk_bulk_get_all(dev, &mbox->clks);
+	if (ret < 0)
+		return ret;
+	mbox->num_clks = ret;
+
+	ret = clk_bulk_prepare_enable(mbox->num_clks, mbox->clks);
+	if (ret)
+		return ret;
+
+	mbox->controller.dev = mbox->dev;
+	mbox->controller.num_chans = 1;
+	mbox->controller.chans = &mbox->chan;
+	mbox->controller.ops = &apple_mbox_ops;
+	mbox->controller.of_xlate = &apple_mbox_of_xlate;
+	mbox->controller.txdone_fifo = true;
+
+	irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-recv", dev_name(dev));
+	if (!irqname) {
+		ret = -ENOMEM;
+		goto err_clk_disable;
+	}
+	ret = devm_request_irq(dev, mbox->irq_recv_not_empty,
+			       apple_mbox_recv_irq, IRQF_NO_AUTOEN, irqname,
+			       mbox);
+	if (ret)
+		goto err_clk_disable;
+
+	irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-send", dev_name(dev));
+	if (!irqname) {
+		ret = -ENOMEM;
+		goto err_clk_disable;
+	}
+	ret = devm_request_irq(dev, mbox->irq_send_empty,
+			       apple_mbox_send_empty_irq, IRQF_NO_AUTOEN,
+			       irqname, mbox);
+	if (ret)
+		goto err_clk_disable;
+
+	ret = devm_mbox_controller_register(dev, &mbox->controller);
+	if (ret)
+		goto err_clk_disable;
+	return ret;
+
+err_clk_disable:
+	clk_bulk_disable_unprepare(mbox->num_clks, mbox->clks);
+	return ret;
+}
+
+static int apple_mbox_remove(struct platform_device *pdev)
+{
+	struct apple_mbox *apple_mbox = dev_get_drvdata(&pdev->dev);
+
+	clk_bulk_disable_unprepare(apple_mbox->num_clks, apple_mbox->clks);
+	return 0;
+}
+
+static struct platform_driver apple_mbox_driver = {
+	.driver = {
+		.name = "apple-mailbox",
+		.of_match_table = apple_mbox_of_match,
+	},
+	.probe = apple_mbox_probe,
+	.remove = apple_mbox_remove,
+};
+module_platform_driver(apple_mbox_driver);
+
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>");
+MODULE_DESCRIPTION("Apple Mailbox driver");
diff --git a/include/linux/apple-mailbox.h b/include/linux/apple-mailbox.h
new file mode 100644
index 000000000000..c455e0f9c73b
--- /dev/null
+++ b/include/linux/apple-mailbox.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
+/*
+ * Apple mailbox message format
+ *
+ * Copyright (C) 2021 The Asahi Linux Contributors
+ */
+
+#ifndef _LINUX_APPLE_MAILBOX_H_
+#define _LINUX_APPLE_MAILBOX_H_
+
+#include <linux/types.h>
+
+struct apple_mbox_msg {
+	u64 msg0;
+	u32 msg1;
+};
+
+#endif
-- 
2.25.1

Re: [PATCH 3/3] mailbox: apple: Add driver for Apple mailboxes

[Alyssa Rosenzweig]

> +	  Apple SoCs have various co-processors that need to be started and
> +	  initialized for certain peripherals to work (NVMe, display controller,
> +	  etc.). This driver adds support for the mailbox controller used to
> +	  communicate with those.

This makes it seem like it's just a boot time issue. Maybe that's the
case for NVMe? In general the mailbox is needed 100% of the time. I
suggest something like:

	  Apple SoCs have various co-processors required for certain
	  peripherals to work (NVMe, display controller, etc.). This
	  driver adds support for the mailbox controller used to
	  communicate with those.

> + * Copyright (C) 2021 The Asahi Linux Contributors

Isn't this all you?

> + * Various clients implement different IPC protocols based on these simple

s/clients/coprocessors/ or firmware or something?

> + * Both the main CPU and the co-processor see the same set of registers but
> + * the first FIFO (A2I) is always used to transfer messages from the application
> + * processor (us) to the I/O processor and the second one (I2A) for the
> + * other direction.

Do we actually know what the copro sees? It seems obvious, but *know*?

> +#define APPLE_ASC_MBOX_CONTROL_FULL BIT(16)
> +#define APPLE_ASC_MBOX_CONTROL_EMPTY BIT(17)
> +
> +#define APPLE_ASC_MBOX_A2I_CONTROL 0x110
> +#define APPLE_ASC_MBOX_A2I_SEND0 0x800
> +#define APPLE_ASC_MBOX_A2I_SEND1 0x808
> +#define APPLE_ASC_MBOX_A2I_RECV0 0x810
> +#define APPLE_ASC_MBOX_A2I_RECV1 0x818
> +
> +#define APPLE_ASC_MBOX_I2A_CONTROL 0x114
> +#define APPLE_ASC_MBOX_I2A_SEND0 0x820
> +#define APPLE_ASC_MBOX_I2A_SEND1 0x828
> +#define APPLE_ASC_MBOX_I2A_RECV0 0x830
> +#define APPLE_ASC_MBOX_I2A_RECV1 0x838
> +
> +#define APPLE_M3_MBOX_A2I_CONTROL 0x50
> +#define APPLE_M3_MBOX_A2I_SEND0 0x60
> +#define APPLE_M3_MBOX_A2I_SEND1 0x68
> +#define APPLE_M3_MBOX_A2I_RECV0 0x70
> +#define APPLE_M3_MBOX_A2I_RECV1 0x78
> +
> +#define APPLE_M3_MBOX_I2A_CONTROL 0x80
> +#define APPLE_M3_MBOX_I2A_SEND0 0x90
> +#define APPLE_M3_MBOX_I2A_SEND1 0x98
> +#define APPLE_M3_MBOX_I2A_RECV0 0xa0
> +#define APPLE_M3_MBOX_I2A_RECV1 0xa8
> +
> +#define APPLE_M3_MBOX_CONTROL_FULL BIT(16)
> +#define APPLE_M3_MBOX_CONTROL_EMPTY BIT(17)

The ordering here is asymmetric (control bits on top or on bottom). Also
might be nicer to align things.

> +struct apple_mbox {
> +	void __iomem *regs;
> +	const struct apple_mbox_hw *hw;
> +     ...
> +};

This requires a lot of pointer chasing to send/receive messages. Will
this become a perf issue in any case? It'd be good to get ballparks on
how often these mboxes are used. (For DCP, it doesn't matter, only a few
hundred messages per second. Other copros may have different behaviour)

> +static bool apple_mbox_hw_can_send(struct apple_mbox *apple_mbox)
> +{
> +	u32 mbox_ctrl =
> +		readl_relaxed(apple_mbox->regs + apple_mbox->hw->a2i_control);
> +
> +	return !(mbox_ctrl & apple_mbox->hw->control_full);
> +}

If you do the pointer chasing, I suspect you want accessor
functions/macros at least to make it less intrusive...

> +	dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1);

Isn't "TX" redundant here?

> +	dev_dbg(apple_mbox->dev, "got FIFO empty IRQ\n");

I realize it's a dev_dbg but this still seems unnecessarily noisy.

> +static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
> +{
> +	struct apple_mbox *apple_mbox = data;
> +	struct apple_mbox_msg msg;
> +
> +	while (apple_mbox_hw_can_recv(apple_mbox)) {
> +		apple_mbox_hw_recv(apple_mbox, &msg);
> +		mbox_chan_received_data(&apple_mbox->chan, (void *)&msg);
> +	}
```

A comment is warranted why this loop is safe and will always terminate,
especially given this is an IRQ context. (Ah... can a malicious
coprocessor DoS the AP by sending messages faster than the AP can
process them? freezing the system since preemption might be disabled
here? I suppose thee's no good way to protect against that level of
goofy.)

> +	 * There's no race if a message comes in between the check in the while
> +	 * loop above and the ack below: If a new messages arrives inbetween
> +	 * those two the interrupt will just fire again immediately after the
> +	 * ack since it's level sensitive.

I don't quite understand the reasoning. Why have IRQ controls at all
then on the M3?

> +	if (apple_mbox->hw->has_irq_controls)
> +		writel_relaxed(apple_mbox->hw->irq_bit_recv_not_empty,
> +			       apple_mbox->regs + apple_mbox->hw->irq_ack);

Nit: { braces } since this is two lines. Unless kernel likes this sort
of scary aesthetic. Would go away with an accessor, of course.

> +	/*
> +	 * Only some variants of this mailbox HW provide interrupt control
> +	 * at the mailbox level. We therefore need to handle enabling/disabling
> +	 * interrupts at the main interrupt controller anyway for hardware that
> +	 * doesn't. Just always keep the interrupts we care about enabled at
> +	 * the mailbox level so that both hardware revisions behave almost
> +	 * the same.
> +	 */

Cute. Does macOS do this? Are there any tradeoffs?

> +	irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-recv", dev_name(dev));
...
> +	irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-send", dev_name(dev));

Not sure this is better or worse than specifying IRQ names in the device
tree... probably less greppable.

> +++ b/include/linux/apple-mailbox.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> +/*
> + * Apple mailbox message format
> + *
> + * Copyright (C) 2021 The Asahi Linux Contributors
> + */
> +
> +#ifndef _LINUX_APPLE_MAILBOX_H_
> +#define _LINUX_APPLE_MAILBOX_H_
> +
> +#include <linux/types.h>
> +
> +struct apple_mbox_msg {
> +	u64 msg0;
> +	u32 msg1;
> +};
> +
> +#endif

Seems like such a waste to have a dedicated file for a single structure
:'( I guess it's needed for the rtkit library but still ....

Re: [PATCH 2/3] dt-bindings: mailbox: Add Apple mailbox bindings

[Alyssa Rosenzweig]

> +      - description:
> +          M3 mailboxes are an older variant with a slightly different MMIO
> +          interface still found on the M1.
> +        items:
> +          - const: apple,t8103-m3-mailbox

Would be nice to document an example of where an M3 mailbox is found.

> +  interrupts:
> +    minItems: 4
> +    items:
> +      - description: send fifo is empty interrupt
> +      - description: send fifo is not empty interrupt
> +      - description: receive fifo is empty interrupt
> +      - description: receive fifo is not empty interrupt
> +
> +  interrupt-names:
> +    minItems: 4
> +    items:
> +      - const: send-empty
> +      - const: send-not-empty
> +      - const: recv-empty
> +      - const: recv-not-empty

If the names became not-constant the asprintf thing goes away, not sure
that's better or worse.

> +  clocks:
> +    description:
> +      Reference to the clock gate phandle(s) if required for this mailbox.
> +      Optional since not all mailboxes are attached to a clock gate.

Do we do anything with the clocks at this point?

Re: [PATCH 3/3] mailbox: apple: Add driver for Apple mailboxes

[Sven Peter]

Hi,

Thanks for the review!

On Tue, Sep 7, 2021, at 20:54, Alyssa Rosenzweig wrote:
> > +	  Apple SoCs have various co-processors that need to be started and
> > +	  initialized for certain peripherals to work (NVMe, display controller,
> > +	  etc.). This driver adds support for the mailbox controller used to
> > +	  communicate with those.
> 
> This makes it seem like it's just a boot time issue. Maybe that's the
> case for NVMe? In general the mailbox is needed 100% of the time. I
> suggest something like:
> 
> 	  Apple SoCs have various co-processors required for certain
> 	  peripherals to work (NVMe, display controller, etc.). This
> 	  driver adds support for the mailbox controller used to
> 	  communicate with those.

Sure.

> 
> > + * Copyright (C) 2021 The Asahi Linux Contributors
> 
> Isn't this all you?

Yes, but see e.g. the discussion on the M1 bringup series [1][2].

[1] https://www.linuxfoundation.org/en/blog/copyright-notices-in-open-source-software-projects/
[2] https://lore.kernel.org/linux-arm-kernel/d8454963-3242-699b-4c20-e85d26b19796@marcan.st/

> 
> > + * Various clients implement different IPC protocols based on these simple
> 
> s/clients/coprocessors/ or firmware or something?

Sure.

> 
> > + * Both the main CPU and the co-processor see the same set of registers but
> > + * the first FIFO (A2I) is always used to transfer messages from the application
> > + * processor (us) to the I/O processor and the second one (I2A) for the
> > + * other direction.
> 
> Do we actually know what the copro sees? It seems obvious, but *know*?

Yes, I know. They see the exact same set of registers. I wouldn't have written
it like that otherwise.

> 
> > +#define APPLE_ASC_MBOX_CONTROL_FULL BIT(16)
> > +#define APPLE_ASC_MBOX_CONTROL_EMPTY BIT(17)
> > +
> > +#define APPLE_ASC_MBOX_A2I_CONTROL 0x110
> > +#define APPLE_ASC_MBOX_A2I_SEND0 0x800
> > +#define APPLE_ASC_MBOX_A2I_SEND1 0x808
> > +#define APPLE_ASC_MBOX_A2I_RECV0 0x810
> > +#define APPLE_ASC_MBOX_A2I_RECV1 0x818
> > +
> > +#define APPLE_ASC_MBOX_I2A_CONTROL 0x114
> > +#define APPLE_ASC_MBOX_I2A_SEND0 0x820
> > +#define APPLE_ASC_MBOX_I2A_SEND1 0x828
> > +#define APPLE_ASC_MBOX_I2A_RECV0 0x830
> > +#define APPLE_ASC_MBOX_I2A_RECV1 0x838
> > +
> > +#define APPLE_M3_MBOX_A2I_CONTROL 0x50
> > +#define APPLE_M3_MBOX_A2I_SEND0 0x60
> > +#define APPLE_M3_MBOX_A2I_SEND1 0x68
> > +#define APPLE_M3_MBOX_A2I_RECV0 0x70
> > +#define APPLE_M3_MBOX_A2I_RECV1 0x78
> > +
> > +#define APPLE_M3_MBOX_I2A_CONTROL 0x80
> > +#define APPLE_M3_MBOX_I2A_SEND0 0x90
> > +#define APPLE_M3_MBOX_I2A_SEND1 0x98
> > +#define APPLE_M3_MBOX_I2A_RECV0 0xa0
> > +#define APPLE_M3_MBOX_I2A_RECV1 0xa8
> > +
> > +#define APPLE_M3_MBOX_CONTROL_FULL BIT(16)
> > +#define APPLE_M3_MBOX_CONTROL_EMPTY BIT(17)
> 
> The ordering here is asymmetric (control bits on top or on bottom). Also
> might be nicer to align things.

Sure, I'll move them to the top for M3 as well.

> 
> > +struct apple_mbox {
> > +	void __iomem *regs;
> > +	const struct apple_mbox_hw *hw;
> > +     ...
> > +};
> 
> This requires a lot of pointer chasing to send/receive messages. Will
> this become a perf issue in any case? It'd be good to get ballparks on
> how often these mboxes are used. (For DCP, it doesn't matter, only a few
> hundred messages per second. Other copros may have different behaviour)

If this actually becomes a problem I'm happy to fix it but right now
this feels like premature optimization to me. DCP is going to be the
worst offender followed by SMC (at most a few per second when it's really
busy during boot time) and SEP (I've also never seen more than a few per
second here). The rest of them are mostly quiet after they have booted.

> 
> > +static bool apple_mbox_hw_can_send(struct apple_mbox *apple_mbox)
> > +{
> > +	u32 mbox_ctrl =
> > +		readl_relaxed(apple_mbox->regs + apple_mbox->hw->a2i_control);
> > +
> > +	return !(mbox_ctrl & apple_mbox->hw->control_full);
> > +}
> 
> If you do the pointer chasing, I suspect you want accessor
> functions/macros at least to make it less intrusive...

There's regmap but that can't easily be used here because I need 32bit
and 64bit writes. I also don't really see the advantage of replacing
this with some custom functions like e.g.

	mbox_ctrl = apple_mbox_hw_readl(apple_mbox, apple_mbox->hw->a2i_control);

which is almost as long. And if I introduce a separate function just to read the
control reg this just becomes a lot of boilerplate and harder to follow.

Or did you have anything else in mind?

> 
> > +	dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1);
> 
> Isn't "TX" redundant here?

Sure, but it also doesn't hurt in a debug message. I can spot the TX easier
but I'm sure there are people who prefer >.

> 
> > +	dev_dbg(apple_mbox->dev, "got FIFO empty IRQ\n");
> 
> I realize it's a dev_dbg but this still seems unnecessarily noisy.

This code path is almost never reached. I've only been able to trigger
it by forcing send_message to return EBUSY even when it could still
move messages to the FIFO to test this path.
This also can't be triggered more often than the TX debug message.
If this triggers more often there's a bug somewhere that kept the interrupt
enabled and then the whole machine will hang due an interrupt storm anyway. In
that case I'd prefer to have this as noisy as possible.

> 
> > +static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
> > +{
> > +	struct apple_mbox *apple_mbox = data;
> > +	struct apple_mbox_msg msg;
> > +
> > +	while (apple_mbox_hw_can_recv(apple_mbox)) {
> > +		apple_mbox_hw_recv(apple_mbox, &msg);
> > +		mbox_chan_received_data(&apple_mbox->chan, (void *)&msg);
> > +	}
> ```
> 
> A comment is warranted why this loop is safe and will always terminate,
> especially given this is an IRQ context. (Ah... can a malicious
> coprocessor DoS the AP by sending messages faster than the AP can
> process them? freezing the system since preemption might be disabled
> here? I suppose thee's no good way to protect against that level of
> goofy.)

The only way this can't terminate is if the co-processor tries to stall
us with messages, but what's your threat model here? If the co-processor wants
to be evil it usually has many other ways to annoy us (e.g. ANS could just disable
NVMe, SMC could just trigger a shutdown, etc.)

I could move this to threaded interrupt context though which would make that a bit
harder to pull off at the cost of a bit more latency from incoming messages.
Not sure that's worth it though.

> 
> > +	 * There's no race if a message comes in between the check in the while
> > +	 * loop above and the ack below: If a new messages arrives inbetween
> > +	 * those two the interrupt will just fire again immediately after the
> > +	 * ack since it's level sensitive.
> 
> I don't quite understand the reasoning. Why have IRQ controls at all
> then on the M3?

Re-read the two lines just above this one. If the interrupt is not acked here
it will keep firing even when there are no new messages.
But it's fine to ack it even when there are message available since it will
just trigger again immediately.

> 
> > +	if (apple_mbox->hw->has_irq_controls)
> > +		writel_relaxed(apple_mbox->hw->irq_bit_recv_not_empty,
> > +			       apple_mbox->regs + apple_mbox->hw->irq_ack);
> 
> Nit: { braces } since this is two lines. Unless kernel likes this sort
> of scary aesthetic. Would go away with an accessor, of course.

Sure.

> 
> > +	/*
> > +	 * Only some variants of this mailbox HW provide interrupt control
> > +	 * at the mailbox level. We therefore need to handle enabling/disabling
> > +	 * interrupts at the main interrupt controller anyway for hardware that
> > +	 * doesn't. Just always keep the interrupts we care about enabled at
> > +	 * the mailbox level so that both hardware revisions behave almost
> > +	 * the same.
> > +	 */
> 
> Cute. Does macOS do this? Are there any tradeoffs?

Not sure if macOS does is and I also don't see a reason to care what it
does exactly. I've verified that this works with the M3 mailboxes.
I also don't see why there would there be any tradeoffs.
Do you have anything specific in mind?

I suspect this HW was used in previous SoCs where all four interrupts
shared a single line and required this chained interrupt controller
at the mailbox level. But since they are all separate on the M1 it's
just a nuisance we have to deal with - especially since the ASC
variant got rid of the interrupt controls.

> 
> > +	irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-recv", dev_name(dev));
> ...
> > +	irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-send", dev_name(dev));
> 
> Not sure this is better or worse than specifying IRQ names in the device
> tree... probably less greppable.

This name will appear in e.g. /proc/interrupts and at least the device name
should be in there. Usually only dev_name is used here but since we have two
interrupts they should have different names. The names from the DT are taken a
few lines above when calling platform_get_irq_byname.

> 
> > +++ b/include/linux/apple-mailbox.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> > +/*
> > + * Apple mailbox message format
> > + *
> > + * Copyright (C) 2021 The Asahi Linux Contributors
> > + */
> > +
> > +#ifndef _LINUX_APPLE_MAILBOX_H_
> > +#define _LINUX_APPLE_MAILBOX_H_
> > +
> > +#include <linux/types.h>
> > +
> > +struct apple_mbox_msg {
> > +	u64 msg0;
> > +	u32 msg1;
> > +};
> > +
> > +#endif
> 
> Seems like such a waste to have a dedicated file for a single structure
> :'( I guess it's needed for the rtkit library but still ....
> 

It's needed for all clients that will use this mailbox controller, but yeah,
no way around this.



Sven

Re: [PATCH 2/3] dt-bindings: mailbox: Add Apple mailbox bindings

[Sven Peter]

Hi,


On Tue, Sep 7, 2021, at 20:56, Alyssa Rosenzweig wrote:
> > +      - description:
> > +          M3 mailboxes are an older variant with a slightly different MMIO
> > +          interface still found on the M1.
> > +        items:
> > +          - const: apple,t8103-m3-mailbox
> 
> Would be nice to document an example of where an M3 mailbox is found.

Sure, I can add a comment that this is used for the coprocessor controlling Thunderbolt.

> 
> > +  interrupts:
> > +    minItems: 4
> > +    items:
> > +      - description: send fifo is empty interrupt
> > +      - description: send fifo is not empty interrupt
> > +      - description: receive fifo is empty interrupt
> > +      - description: receive fifo is not empty interrupt
> > +
> > +  interrupt-names:
> > +    minItems: 4
> > +    items:
> > +      - const: send-empty
> > +      - const: send-not-empty
> > +      - const: recv-empty
> > +      - const: recv-not-empty
> 
> If the names became not-constant the asprintf thing goes away, not sure
> that's better or worse.

I'm not sure I understand your comment here. This property just gives a name
to the interrupts so that they can be referenced by that instead of a magic
number between 0 and 4 in the driver.

> 
> > +  clocks:
> > +    description:
> > +      Reference to the clock gate phandle(s) if required for this mailbox.
> > +      Optional since not all mailboxes are attached to a clock gate.
> 
> Do we do anything with the clocks at this point?
> 

The device tree bindings describe the hardware (as best as we can without proper
documentation) and some of these mailboxes have clock gates which need to be turned
on before accessing their MMIO. This driver already tries to do that and works fine
with the downstream clock driver(s) we have.



Sven

Re: [PATCH 2/3] dt-bindings: mailbox: Add Apple mailbox bindings

[Alyssa Rosenzweig]

> > > +      - description:
> > > +          M3 mailboxes are an older variant with a slightly different MMIO
> > > +          interface still found on the M1.
> > > +        items:
> > > +          - const: apple,t8103-m3-mailbox
> > 
> > Would be nice to document an example of where an M3 mailbox is found.
> 
> Sure, I can add a comment that this is used for the coprocessor controlling Thunderbolt.

That raises another issue ... how do we know the M3 code works at all
without TB support yet? It "looks" correct but some of the IRQ handling
stuff is nontrivial.

> > > +  interrupts:
> > > +    minItems: 4
> > > +    items:
> > > +      - description: send fifo is empty interrupt
> > > +      - description: send fifo is not empty interrupt
> > > +      - description: receive fifo is empty interrupt
> > > +      - description: receive fifo is not empty interrupt
> > > +
> > > +  interrupt-names:
> > > +    minItems: 4
> > > +    items:
> > > +      - const: send-empty
> > > +      - const: send-not-empty
> > > +      - const: recv-empty
> > > +      - const: recv-not-empty
> > 
> > If the names became not-constant the asprintf thing goes away, not sure
> > that's better or worse.
> 
> I'm not sure I understand your comment here. This property just gives a name
> to the interrupts so that they can be referenced by that instead of a magic
> number between 0 and 4 in the driver.

D'oh, right, retracted. (Both this comment and the corresponding comment
on the driver itself). Sorry about that.

> > > +  clocks:
> > > +    description:
> > > +      Reference to the clock gate phandle(s) if required for this mailbox.
> > > +      Optional since not all mailboxes are attached to a clock gate.
> > 
> > Do we do anything with the clocks at this point?
> > 
> 
> The device tree bindings describe the hardware (as best as we can without proper
> documentation) and some of these mailboxes have clock gates which need to be turned
> on before accessing their MMIO. This driver already tries to do that and works fine
> with the downstream clock driver(s) we have.

Good enough for me, thanks for clarifying 👍

Commit r-b, though Rob will surely point out problems and I'll need to
rereview 😉

Re: [PATCH 3/3] mailbox: apple: Add driver for Apple mailboxes

[Alyssa Rosenzweig]

> > > + * Both the main CPU and the co-processor see the same set of registers but
> > > + * the first FIFO (A2I) is always used to transfer messages from the application
> > > + * processor (us) to the I/O processor and the second one (I2A) for the
> > > + * other direction.
> > 
> > Do we actually know what the copro sees? It seems obvious, but *know*?
> 
> Yes, I know. They see the exact same set of registers. I wouldn't have written
> it like that otherwise.

Ack.

> > > +struct apple_mbox {
> > > +	void __iomem *regs;
> > > +	const struct apple_mbox_hw *hw;
> > > +     ...
> > > +};
> > 
> > This requires a lot of pointer chasing to send/receive messages. Will
> > this become a perf issue in any case? It'd be good to get ballparks on
> > how often these mboxes are used. (For DCP, it doesn't matter, only a few
> > hundred messages per second. Other copros may have different behaviour)
> 
> If this actually becomes a problem I'm happy to fix it but right now
> this feels like premature optimization to me. DCP is going to be the
> worst offender followed by SMC (at most a few per second when it's really
> busy during boot time) and SEP (I've also never seen more than a few per
> second here). The rest of them are mostly quiet after they have booted.

Good enough for me -- it won't matter for DCP, so if it doesn't get any
worse than DCP there's no point in optimizing this.

> > > +static bool apple_mbox_hw_can_send(struct apple_mbox *apple_mbox)
> > > +{
> > > +	u32 mbox_ctrl =
> > > +		readl_relaxed(apple_mbox->regs + apple_mbox->hw->a2i_control);
> > > +
> > > +	return !(mbox_ctrl & apple_mbox->hw->control_full);
> > > +}
> > 
> > If you do the pointer chasing, I suspect you want accessor
> > functions/macros at least to make it less intrusive...
> 
> There's regmap but that can't easily be used here because I need 32bit
> and 64bit writes. I also don't really see the advantage of replacing
> this with some custom functions like e.g.
> 
> 	mbox_ctrl = apple_mbox_hw_readl(apple_mbox, apple_mbox->hw->a2i_control);
> 
> which is almost as long. And if I introduce a separate function just to read the
> control reg this just becomes a lot of boilerplate and harder to follow.
> 
> Or did you have anything else in mind?

I was envisioning a macro:

>	#define apple_mbox_readl(mbox, name) \
>		readl_relaxed(mbox->regs + mbox->hw-> ## name)
>
> 	mbox_ctrl = apple_mbox_readl(apple_mbox, a2i_control);

Now that I've typed it out, however, it seems a bit too magical to
justify the minor space savings. And given you need both 32b and 64b
there would be ~4 such macros which is also a lot of boilerplate.

It's not a huge deal either way but I thought I'd raise it.

> > > +	dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1);
> > 
> > Isn't "TX" redundant here?
> 
> Sure, but it also doesn't hurt in a debug message. I can spot the TX easier
> but I'm sure there are people who prefer >.

Fair enough.

> > > +	dev_dbg(apple_mbox->dev, "got FIFO empty IRQ\n");
> > 
> > I realize it's a dev_dbg but this still seems unnecessarily noisy.
> 
> This code path is almost never reached. I've only been able to trigger
> it by forcing send_message to return EBUSY even when it could still
> move messages to the FIFO to test this path.
> This also can't be triggered more often than the TX debug message.
> If this triggers more often there's a bug somewhere that kept the interrupt
> enabled and then the whole machine will hang due an interrupt storm anyway. In
> that case I'd prefer to have this as noisy as possible.

Ah! Sure, makes sense. Is it worth adding a few words to the functions
comments indicating this rarely occurs in good conditions?

> > > +static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
> > > +{
> > > +	struct apple_mbox *apple_mbox = data;
> > > +	struct apple_mbox_msg msg;
> > > +
> > > +	while (apple_mbox_hw_can_recv(apple_mbox)) {
> > > +		apple_mbox_hw_recv(apple_mbox, &msg);
> > > +		mbox_chan_received_data(&apple_mbox->chan, (void *)&msg);
> > > +	}
> > ```
> > 
> > A comment is warranted why this loop is safe and will always terminate,
> > especially given this is an IRQ context. (Ah... can a malicious
> > coprocessor DoS the AP by sending messages faster than the AP can
> > process them? freezing the system since preemption might be disabled
> > here? I suppose thee's no good way to protect against that level of
> > goofy.)
> 
> The only way this can't terminate is if the co-processor tries to stall
> us with messages, but what's your threat model here? If the co-processor wants
> to be evil it usually has many other ways to annoy us (e.g. ANS could just disable
> NVMe, SMC could just trigger a shutdown, etc.)
> 
> I could move this to threaded interrupt context though which would make that a bit
> harder to pull off at the cost of a bit more latency from incoming messages.
> Not sure that's worth it though.

Probably not worth it, no.

> > 
> > > +	 * There's no race if a message comes in between the check in the while
> > > +	 * loop above and the ack below: If a new messages arrives inbetween
> > > +	 * those two the interrupt will just fire again immediately after the
> > > +	 * ack since it's level sensitive.
> > 
> > I don't quite understand the reasoning. Why have IRQ controls at all
> > then on the M3?
> 
> Re-read the two lines just above this one. If the interrupt is not acked here
> it will keep firing even when there are no new messages.
> But it's fine to ack it even when there are message available since it will
> just trigger again immediately.

Got it, thanks.

> > > +	/*
> > > +	 * Only some variants of this mailbox HW provide interrupt control
> > > +	 * at the mailbox level. We therefore need to handle enabling/disabling
> > > +	 * interrupts at the main interrupt controller anyway for hardware that
> > > +	 * doesn't. Just always keep the interrupts we care about enabled at
> > > +	 * the mailbox level so that both hardware revisions behave almost
> > > +	 * the same.
> > > +	 */
> > 
> > Cute. Does macOS do this? Are there any tradeoffs?
> 
> Not sure if macOS does is and I also don't see a reason to care what it
> does exactly. I've verified that this works with the M3 mailboxes.
> I also don't see why there would there be any tradeoffs.
> Do you have anything specific in mind?
> 
> I suspect this HW was used in previous SoCs where all four interrupts
> shared a single line and required this chained interrupt controller
> at the mailbox level. But since they are all separate on the M1 it's
> just a nuisance we have to deal with - especially since the ASC
> variant got rid of the interrupt controls.

Makes sense for a legacy block 👍

Re: [PATCH 2/3] dt-bindings: mailbox: Add Apple mailbox bindings

[Sven Peter]

On Tue, Sep 7, 2021, at 22:48, Alyssa Rosenzweig wrote:
> > > > +      - description:
> > > > +          M3 mailboxes are an older variant with a slightly different MMIO
> > > > +          interface still found on the M1.
> > > > +        items:
> > > > +          - const: apple,t8103-m3-mailbox
> > > 
> > > Would be nice to document an example of where an M3 mailbox is found.
> > 
> > Sure, I can add a comment that this is used for the coprocessor controlling Thunderbolt.
> 
> That raises another issue ... how do we know the M3 code works at all
> without TB support yet? It "looks" correct but some of the IRQ handling
> stuff is nontrivial.

Enabling the mailbox interface just requires a few clocks to be ungated.
Then I injected messages manually to verify that the code works.
In addition I also brought up parts of the Thunderbolt controller which
then allowed the co-processor on the other end of the mailbox to boot.
After that I was also able to successfully talk to that processor using
the same protocol used by most other processors.


> 
> > > > +  interrupts:
> > > > +    minItems: 4
> > > > +    items:
> > > > +      - description: send fifo is empty interrupt
> > > > +      - description: send fifo is not empty interrupt
> > > > +      - description: receive fifo is empty interrupt
> > > > +      - description: receive fifo is not empty interrupt
> > > > +
> > > > +  interrupt-names:
> > > > +    minItems: 4
> > > > +    items:
> > > > +      - const: send-empty
> > > > +      - const: send-not-empty
> > > > +      - const: recv-empty
> > > > +      - const: recv-not-empty
> > > 
> > > If the names became not-constant the asprintf thing goes away, not sure
> > > that's better or worse.
> > 
> > I'm not sure I understand your comment here. This property just gives a name
> > to the interrupts so that they can be referenced by that instead of a magic
> > number between 0 and 4 in the driver.
> 
> D'oh, right, retracted. (Both this comment and the corresponding comment
> on the driver itself). Sorry about that.
> 
> > > > +  clocks:
> > > > +    description:
> > > > +      Reference to the clock gate phandle(s) if required for this mailbox.
> > > > +      Optional since not all mailboxes are attached to a clock gate.
> > > 
> > > Do we do anything with the clocks at this point?
> > > 
> > 
> > The device tree bindings describe the hardware (as best as we can without proper
> > documentation) and some of these mailboxes have clock gates which need to be turned
> > on before accessing their MMIO. This driver already tries to do that and works fine
> > with the downstream clock driver(s) we have.
> 
> Good enough for me, thanks for clarifying 👍
> 
> Commit r-b, though Rob will surely point out problems and I'll need to
> rereview 😉
> 

Thanks!



Sven

Re: [PATCH 3/3] mailbox: apple: Add driver for Apple mailboxes

[Sven Peter]

On Tue, Sep 7, 2021, at 23:06, Alyssa Rosenzweig wrote:
> > > > + * Both the main CPU and the co-processor see the same set of registers but
> > > > + * the first FIFO (A2I) is always used to transfer messages from the application
> > > > + * processor (us) to the I/O processor and the second one (I2A) for the
> > > > + * other direction.
> > > 
> > > Do we actually know what the copro sees? It seems obvious, but *know*?
> > 
> > Yes, I know. They see the exact same set of registers. I wouldn't have written
> > it like that otherwise.
> 
> Ack.
> 
> > > > +struct apple_mbox {
> > > > +	void __iomem *regs;
> > > > +	const struct apple_mbox_hw *hw;
> > > > +     ...
> > > > +};
> > > 
> > > This requires a lot of pointer chasing to send/receive messages. Will
> > > this become a perf issue in any case? It'd be good to get ballparks on
> > > how often these mboxes are used. (For DCP, it doesn't matter, only a few
> > > hundred messages per second. Other copros may have different behaviour)
> > 
> > If this actually becomes a problem I'm happy to fix it but right now
> > this feels like premature optimization to me. DCP is going to be the
> > worst offender followed by SMC (at most a few per second when it's really
> > busy during boot time) and SEP (I've also never seen more than a few per
> > second here). The rest of them are mostly quiet after they have booted.
> 
> Good enough for me -- it won't matter for DCP, so if it doesn't get any
> worse than DCP there's no point in optimizing this.
> 
> > > > +static bool apple_mbox_hw_can_send(struct apple_mbox *apple_mbox)
> > > > +{
> > > > +	u32 mbox_ctrl =
> > > > +		readl_relaxed(apple_mbox->regs + apple_mbox->hw->a2i_control);
> > > > +
> > > > +	return !(mbox_ctrl & apple_mbox->hw->control_full);
> > > > +}
> > > 
> > > If you do the pointer chasing, I suspect you want accessor
> > > functions/macros at least to make it less intrusive...
> > 
> > There's regmap but that can't easily be used here because I need 32bit
> > and 64bit writes. I also don't really see the advantage of replacing
> > this with some custom functions like e.g.
> > 
> > 	mbox_ctrl = apple_mbox_hw_readl(apple_mbox, apple_mbox->hw->a2i_control);
> > 
> > which is almost as long. And if I introduce a separate function just to read the
> > control reg this just becomes a lot of boilerplate and harder to follow.
> > 
> > Or did you have anything else in mind?
> 
> I was envisioning a macro:
> 
> >	#define apple_mbox_readl(mbox, name) \
> >		readl_relaxed(mbox->regs + mbox->hw-> ## name)
> >
> > 	mbox_ctrl = apple_mbox_readl(apple_mbox, a2i_control);
> 
> Now that I've typed it out, however, it seems a bit too magical to
> justify the minor space savings. And given you need both 32b and 64b
> there would be ~4 such macros which is also a lot of boilerplate.
> 
> It's not a huge deal either way but I thought I'd raise it.

Yeah, I've thought about this as well but this entire hardware is
so simple that we don't gain much from it imho.

> 
> > > > +	dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1);
> > > 
> > > Isn't "TX" redundant here?
> > 
> > Sure, but it also doesn't hurt in a debug message. I can spot the TX easier
> > but I'm sure there are people who prefer >.
> 
> Fair enough.
> 
> > > > +	dev_dbg(apple_mbox->dev, "got FIFO empty IRQ\n");
> > > 
> > > I realize it's a dev_dbg but this still seems unnecessarily noisy.
> > 
> > This code path is almost never reached. I've only been able to trigger
> > it by forcing send_message to return EBUSY even when it could still
> > move messages to the FIFO to test this path.
> > This also can't be triggered more often than the TX debug message.
> > If this triggers more often there's a bug somewhere that kept the interrupt
> > enabled and then the whole machine will hang due an interrupt storm anyway. In
> > that case I'd prefer to have this as noisy as possible.
> 
> Ah! Sure, makes sense. Is it worth adding a few words to the functions
> comments indicating this rarely occurs in good conditions?

Sure, I can add a small comment if it makes the code easier to understand.

> 
> > > > +static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
> > > > +{
> > > > +	struct apple_mbox *apple_mbox = data;
> > > > +	struct apple_mbox_msg msg;
> > > > +
> > > > +	while (apple_mbox_hw_can_recv(apple_mbox)) {
> > > > +		apple_mbox_hw_recv(apple_mbox, &msg);
> > > > +		mbox_chan_received_data(&apple_mbox->chan, (void *)&msg);
> > > > +	}
> > > ```
> > > 
> > > A comment is warranted why this loop is safe and will always terminate,
> > > especially given this is an IRQ context. (Ah... can a malicious
> > > coprocessor DoS the AP by sending messages faster than the AP can
> > > process them? freezing the system since preemption might be disabled
> > > here? I suppose thee's no good way to protect against that level of
> > > goofy.)
> > 
> > The only way this can't terminate is if the co-processor tries to stall
> > us with messages, but what's your threat model here? If the co-processor wants
> > to be evil it usually has many other ways to annoy us (e.g. ANS could just disable
> > NVMe, SMC could just trigger a shutdown, etc.)
> > 
> > I could move this to threaded interrupt context though which would make that a bit
> > harder to pull off at the cost of a bit more latency from incoming messages.
> > Not sure that's worth it though.
> 
> Probably not worth it, no.

One small advantage of the threaded interrupt would be that the mailbox clients could
detect the invalid messages and at least get a chance to shut the channel down if
the co-processor did this by accident.
Still not sure if that would actually help much but I guess it won't matter in
the end anyway. Changing this only requires to request a threaded irq in the probe
function so it's also not a big deal either way.


> 
> > > 
> > > > +	 * There's no race if a message comes in between the check in the while
> > > > +	 * loop above and the ack below: If a new messages arrives inbetween
> > > > +	 * those two the interrupt will just fire again immediately after the
> > > > +	 * ack since it's level sensitive.
> > > 
> > > I don't quite understand the reasoning. Why have IRQ controls at all
> > > then on the M3?
> > 
> > Re-read the two lines just above this one. If the interrupt is not acked here
> > it will keep firing even when there are no new messages.
> > But it's fine to ack it even when there are message available since it will
> > just trigger again immediately.
> 
> Got it, thanks.
> 
> > > > +	/*
> > > > +	 * Only some variants of this mailbox HW provide interrupt control
> > > > +	 * at the mailbox level. We therefore need to handle enabling/disabling
> > > > +	 * interrupts at the main interrupt controller anyway for hardware that
> > > > +	 * doesn't. Just always keep the interrupts we care about enabled at
> > > > +	 * the mailbox level so that both hardware revisions behave almost
> > > > +	 * the same.
> > > > +	 */
> > > 
> > > Cute. Does macOS do this? Are there any tradeoffs?
> > 
> > Not sure if macOS does is and I also don't see a reason to care what it
> > does exactly. I've verified that this works with the M3 mailboxes.
> > I also don't see why there would there be any tradeoffs.
> > Do you have anything specific in mind?
> > 
> > I suspect this HW was used in previous SoCs where all four interrupts
> > shared a single line and required this chained interrupt controller
> > at the mailbox level. But since they are all separate on the M1 it's
> > just a nuisance we have to deal with - especially since the ASC
> > variant got rid of the interrupt controls.
> 
> Makes sense for a legacy block 👍
> 


Best,


Sven

Re: [PATCH 0/3] Apple Mailbox Controller support

[Jassi Brar]

On Tue, Sep 7, 2021 at 9:55 AM Sven Peter <sven@svenpeter.dev> wrote:
>
> Hi,
>
> This series adds support for the mailbox HW found in the Apple M1. These SoCs
> have various co-processors controlling different peripherals (NVMe, display
> controller, SMC (required for WiFi), Thunderbolt, and probably more
> we don't know about yet). All these co-processors communicate with the main CPU
> using these mailboxes. These mailboxes transmit 64+32 bit messages, are
> backed by a hardware FIFO and have four interrupts (FIFO empty and FIFO not
> empty for the transmit and receive FIFO each).
>
> The hardware itself allows to send 64+32 bit message using two hardware
> registers. A write to or read from the second register transmits or receives a
> message. Usually, the first 64 bit register is used for the message itself and
> 8 bits of the second register are used as an endpoint. I originally considered
> to have the endpoint exposed as a mailbox-channel, but finally decided against
> it: The hardware itself only provides a single channel to the co-processor and
> the endpoint bits are only an implementation detail of the firmware. There's
> even one co-processor (SEP) which uses 8 bits of the first register as its
> endpoint number instead.
> There was a similar discussion about the BCM2835 / Raspberry Pi mailboxes
> which came to the same conclusion [1].
>
> These mailboxes also have a hardware FIFO which make implementing them with the
> current mailbox a bit tricky: There is no "transmission done" interrupt because
> most transmissions are "done" immediately. There is only a "transmission fifo
> empty" level interrupt. I have instead implemented this by adding a fast-path to
> the core mailbox code as a new txready_fifo mode.
> The other possibilities (which would not require any changes to the core mailbox
> code) are to either use the polling mode or to enable the "tx fifo empty"
> interrupt in send_message and then call txready from the irq handler before
> disabling it again. I'd like to avoid those though since so far I've never seen
> the TX FIFO run full which allows to almost always avoid the context switch when
> sending a message. I can easily switch to one of these modes if you prefer to
> keep the core code untouched though.
>
Yes, please keep the api unchanged.
Let us please not dig our own tunnels when the existing ways serve the purpose.

Thanks.

Re: [PATCH 0/3] Apple Mailbox Controller support

[Sven Peter]

On Wed, Sep 8, 2021, at 22:48, Jassi Brar wrote:
> On Tue, Sep 7, 2021 at 9:55 AM Sven Peter <sven@svenpeter.dev> wrote:
> >
> > Hi,
> >
> > This series adds support for the mailbox HW found in the Apple M1. These SoCs
> > have various co-processors controlling different peripherals (NVMe, display
> > controller, SMC (required for WiFi), Thunderbolt, and probably more
> > we don't know about yet). All these co-processors communicate with the main CPU
> > using these mailboxes. These mailboxes transmit 64+32 bit messages, are
> > backed by a hardware FIFO and have four interrupts (FIFO empty and FIFO not
> > empty for the transmit and receive FIFO each).
> >
> > The hardware itself allows to send 64+32 bit message using two hardware
> > registers. A write to or read from the second register transmits or receives a
> > message. Usually, the first 64 bit register is used for the message itself and
> > 8 bits of the second register are used as an endpoint. I originally considered
> > to have the endpoint exposed as a mailbox-channel, but finally decided against
> > it: The hardware itself only provides a single channel to the co-processor and
> > the endpoint bits are only an implementation detail of the firmware. There's
> > even one co-processor (SEP) which uses 8 bits of the first register as its
> > endpoint number instead.
> > There was a similar discussion about the BCM2835 / Raspberry Pi mailboxes
> > which came to the same conclusion [1].
> >
> > These mailboxes also have a hardware FIFO which make implementing them with the
> > current mailbox a bit tricky: There is no "transmission done" interrupt because
> > most transmissions are "done" immediately. There is only a "transmission fifo
> > empty" level interrupt. I have instead implemented this by adding a fast-path to
> > the core mailbox code as a new txready_fifo mode.
> > The other possibilities (which would not require any changes to the core mailbox
> > code) are to either use the polling mode or to enable the "tx fifo empty"
> > interrupt in send_message and then call txready from the irq handler before
> > disabling it again. I'd like to avoid those though since so far I've never seen
> > the TX FIFO run full which allows to almost always avoid the context switch when
> > sending a message. I can easily switch to one of these modes if you prefer to
> > keep the core code untouched though.
> >
> Yes, please keep the api unchanged.
> Let us please not dig our own tunnels when the existing ways serve the purpose.
> 

Ok, I'll use txdone_irq for v2 then and just ignore the HW FIFO.

Thanks,

Sven

Re: [PATCH 2/3] dt-bindings: mailbox: Add Apple mailbox bindings

[Mark Kettenis]

> From: Sven Peter <sven@svenpeter.dev>
> Date: Tue,  7 Sep 2021 16:55:00 +0200
> 
> Apple mailbox controller are found on the M1 and are used for
> communication with various co-processors.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  .../bindings/mailbox/apple,mailbox.yaml       | 81 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>