diff mbox series

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

Message ID 20210907145501.69161-4-sven@svenpeter.dev (mailing list archive)
State New, archived
Headers show
Series Apple Mailbox Controller support | expand

Commit Message

Sven Peter Sept. 7, 2021, 2:55 p.m. UTC 
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

Comments

Alyssa Rosenzweig Sept. 7, 2021, 6:54 p.m. UTC | #1 
> +	  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 ....
Sven Peter Sept. 7, 2021, 8:23 p.m. UTC | #2 
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
Alyssa Rosenzweig Sept. 7, 2021, 9:06 p.m. UTC | #3 
> > > + * 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
Sven Peter Sept. 8, 2021, 3:38 p.m. UTC | #4 
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
diff mbox series

Patch

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