mbox series

[v3,0/2] Apple Mailbox Controller support

Message ID 20211017114054.67737-1-sven@svenpeter.dev
Headers show
Series Apple Mailbox Controller support | expand

Message

Sven Peter Oct. 17, 2021, 11:40 a.m. UTC 
Hi,

This is the second version of my series which adds support for the mailbox
controllers found on the Apple M1.

v1: https://lore.kernel.org/lkml/20210907145501.69161-1-sven@svenpeter.dev/
v2: https://lore.kernel.org/lkml/20210916154911.3168-1-sven@svenpeter.dev/

Thanks again to Jassi and Mark for the review. I've addressed your comments with the
following changes from v2 to v3:

 - removed dma barriers since the mbox client will take care of these
 - moved the of_device_id table and related code to the bottom of the file
 - removed of_xlate
 - dropped clock handling from the code and the binding since we now understand
   that these are actually power domains

Changes from v1 to v2:
 - switched to txdone_irq instead of introducing a new mode
 - switched to a threaded interrupt handler for receiving messages
 - added co-processor examples to the device tree binding
 - reformatted the register defines and clarified multiple comments

Best,

Sven

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

 .../bindings/mailbox/apple,mailbox.yaml       |  79 ++++
 MAINTAINERS                                   |   3 +
 drivers/mailbox/Kconfig                       |  12 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/apple-mailbox.c               | 374 ++++++++++++++++++
 include/linux/apple-mailbox.h                 |  18 +
 6 files changed, 488 insertions(+)
 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

Comments

Alyssa Rosenzweig Oct. 17, 2021, 12:02 p.m. UTC | #1 
> 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.
> 
> Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>

In the future, Reviewed-by tags should be dropped after making major
changes to a patch.

> +		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);

Nit: weird wrapping, much easier to read as:

+		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);

> +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,
> +};

Nit: consider dropping the `has_irq_controls = false` assignment.
Clearly there are none, or you'd have to fill out the irq_* fields too.

> +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 },
> +	{}
> +};

No generic compatibles? I assume this driver hasn't changed much in
recent iPhones, and hopefully it won't change much in M1X...

> +/* 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

Given this file lacks the context of the driver, and the questions
raised in v2 review, it might be beneficial to add a quick comment to
apple_mbox_msg explaiing that no, really, this is a 96-bit message.
Sven Peter Oct. 17, 2021, 12:11 p.m. UTC | #2 
Hi,

On Sun, Oct 17, 2021, at 14:02, Alyssa Rosenzweig wrote:
>> 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.
>> 
>> Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>
> In the future, Reviewed-by tags should be dropped after making major
> changes to a patch.

I don't think there were any major changes since v2, but sure, I'll drop your
tag in the future. All your comments below already apply to v2.

>
>> +		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);
>
> Nit: weird wrapping, much easier to read as:
>
> +		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);
>

This is just what clang-format does and I'd rather keep it this way.
Note that the first two lines are the first argument combined with an OR.

>> +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,
>> +};
>
> Nit: consider dropping the `has_irq_controls = false` assignment.
> Clearly there are none, or you'd have to fill out the irq_* fields too.

I'd rather keep it explicit as false here to make the intent clear.

>
>> +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 },
>> +	{}
>> +};
>
> No generic compatibles? I assume this driver hasn't changed much in
> recent iPhones, and hopefully it won't change much in M1X...

Then we can always have apple,tXXX-asc-mailbox, apple,t8103-asc-mailbox in the
device tree. From what I can tell this specific mailbox has only appeared in
this SoC generation.

>
>> +/* 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
>
> Given this file lacks the context of the driver, and the questions
> raised in v2 review, it might be beneficial to add a quick comment to
> apple_mbox_msg explaiing that no, really, this is a 96-bit message.

I don't think that's necessary but it doesn't hurt either I guess.


Sven