Re: [PATCH 3/3] mailbox: apple: Add driver for Apple mailboxes
flat view
From: Alyssa Rosenzweig <hidden>
Date: 2021-09-07 18:57:18
Also in:
linux-arm-kernel, lkml
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ * 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*?
quoted hunk ↗ jump to hunk
+#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.
quoted hunk ↗ jump to hunk
+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)
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+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 ....