Re: [PATCH v3 2/2] mailbox: exynos: Add support for Exynos850 mailbox

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Thu Apr 30, 2026 at 12:01 PM BST, Tudor Ambarus wrote:
> Hi, Alexey,

Hi Tudor,

> The abstraction is clean. Few comments below.
>
> On 4/29/26 10:00 PM, Alexey Klimov wrote:
>> Exynos850-based platforms support ACPM and has similar workflow
>> of communicating with ACPM via mailbox, however mailbox controller
>> registers are located at different offsets and writes/reads could be
>> different. To distinguish between such different behaviours,
>> the registers offsets for Exynos850 and the platform-specific data
>> structs are introduced and configuration is described in such structs
>> for gs101 and exynos850 based SoCs. Probe routine now selects the
>> corresponding platform-specific data via device_get_match_data().
>> 
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>> ---
>>  drivers/mailbox/exynos-mailbox.c | 59 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 56 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/mailbox/exynos-mailbox.c b/drivers/mailbox/exynos-mailbox.c
>> index d2355b128ba4..11657dd475c0 100644
>> --- a/drivers/mailbox/exynos-mailbox.c
>> +++ b/drivers/mailbox/exynos-mailbox.c
>> @@ -31,14 +31,52 @@
>>  
>>  #define EXYNOS_MBOX_CHAN_COUNT		HWEIGHT32(EXYNOS_MBOX_INTGR1_MASK)
>>  
>> +#define EXYNOS850_MBOX_INTGR0		0x8	/* Interrupt Generation Register 0	*/
>> +#define EXYNOS850_MBOX_INTMR1		0x24	/* Interrupt Mask Register 1		*/
>> +
>> +#define EXYNOS850_MBOX_INTMR1_MASK	GENMASK(15, 0)
>> +
>> +/**
>> + * struct exynos_mbox_driver_data - platform-specific mailbox configuration.
>> + * @intgr:		offset to the IRQ generation register, doorbell
>> + *			to APM co-processor.
>> + * @intgr_shift:	shift to apply to the value written to IRQ generation
>> + *			register.
>> + * @intmr:		offset to the IRQ mask register.
>> + * @intmr_mask:		value to write to the mask register to mask out all
>> + *			interrupts.
>> + */
>> +struct exynos_mbox_driver_data {
>> +	u16 intgr;
>> +	u16 intgr_shift;
>> +	u16 intmr;
>> +	u16 intmr_mask;
>> +};
>
> using u16 for intmr_mask is slightly problematic. Down in the probe
> function, you pass it to writel():
> 	writel(data->intmr_mask, exynos_mbox->regs + data->intmr);
>
> writel() explicitly expects a 32-bit (u32) value. While the compiler will
> implicitly promote the u16 to a 32-bit integer, memory-mapped I/O masks
> should generally match the width of the register being written to. If a
> future SoC requires a 32-bit mask (e.g., GENMASK(31, 0)), the u16 will
> silently truncate it.
>
> u32 for all fields is generally preferred in kernel platform data structs
> for padding/alignment reasons.

Sure. Thanks. I can switch it to u32.

>>  /**
>>   * struct exynos_mbox - driver's private data.
>>   * @regs:	mailbox registers base address.
>>   * @mbox:	pointer to the mailbox controller.
>> + * @data:	pointer to driver platform-specific data.
>>   */
>>  struct exynos_mbox {
>>  	void __iomem *regs;
>>  	struct mbox_controller *mbox;
>> +	const struct exynos_mbox_driver_data *data;
>> +};
>> +
>> +static const struct exynos_mbox_driver_data exynos850_mbox_data = {
>> +	.intgr = EXYNOS850_MBOX_INTGR0,
>> +	.intgr_shift = 16,
>> +	.intmr = EXYNOS850_MBOX_INTMR1,
>> +	.intmr_mask = EXYNOS850_MBOX_INTMR1_MASK,
>> +};
>> +
>> +static const struct exynos_mbox_driver_data exynos_gs101_mbox_data = {
>> +	.intgr = EXYNOS_MBOX_INTGR1,
>> +	.intgr_shift = 0,
>> +	.intmr = EXYNOS_MBOX_INTMR0,
>> +	.intmr_mask = EXYNOS_MBOX_INTMR0_MASK,
>>  };
>>  
>>  static int exynos_mbox_send_data(struct mbox_chan *chan, void *data)
>> @@ -57,7 +95,9 @@ static int exynos_mbox_send_data(struct mbox_chan *chan, void *data)
>>  		return -EINVAL;
>>  	}
>>  
>> -	writel(BIT(msg->chan_id), exynos_mbox->regs + EXYNOS_MBOX_INTGR1);
>> +	/* Ring the doorbell */
>> +	writel(BIT(msg->chan_id) << exynos_mbox->data->intgr_shift,
>> +	       exynos_mbox->regs + exynos_mbox->data->intgr);
>>  
>>  	return 0;
>>  }
>> @@ -87,13 +127,21 @@ static struct mbox_chan *exynos_mbox_of_xlate(struct mbox_controller *mbox,
>>  }
>>  
>>  static const struct of_device_id exynos_mbox_match[] = {
>> -	{ .compatible = "google,gs101-mbox" },
>> +	{
>> +		.compatible = "google,gs101-mbox",
>> +		.data = &exynos_gs101_mbox_data
>> +	},
>> +	{
>> +		.compatible = "samsung,exynos850-mbox",
>> +		.data = &exynos850_mbox_data
>> +	},
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(of, exynos_mbox_match);
>>  
>>  static int exynos_mbox_probe(struct platform_device *pdev)
>>  {
>> +	const struct exynos_mbox_driver_data *data;
>>  	struct device *dev = &pdev->dev;
>>  	struct exynos_mbox *exynos_mbox;
>>  	struct mbox_controller *mbox;
>> @@ -122,6 +170,11 @@ static int exynos_mbox_probe(struct platform_device *pdev)
>>  		return dev_err_probe(dev, PTR_ERR(pclk),
>>  				     "Failed to enable clock.\n");
>>  
>> +	data = device_get_match_data(&pdev->dev);
>> +	if (!data)
>> +		return -ENODEV;
>
> you shall move this first thing in probe() to avoid doing allocations
> gratuitously on null match data.

Ack.

>> +
>> +	exynos_mbox->data = data;
>>  	mbox->num_chans = EXYNOS_MBOX_CHAN_COUNT;
>
> EXYNOS_MBOX_CHAN_COUNT is globally defined as:
> #define EXYNOS_MBOX_CHAN_COUNT		HWEIGHT32(EXYNOS_MBOX_INTGR1_MASK)
>
> Does the Exynos850 have the exact same number of channels as the GS101?
>
> You may move num_chans into struct exynos_mbox_driver_data alongside the
> register offsets so each SoC explicitly declares its channel capacity.

Here:

=> md 2040000 <--- sram_base + initdata_base
02040000: 000063bc 00000007 0000650c 00000013  .c.......e......
02040010: 00000000 00000007 0000000b 0000000e  ................
				^^^^
02040020: 00000000 00000000 00000013 00000009  ................
02040030: 00008000 00008008 0000800c 00008010  ................
02040040: 00000010 0000017f 00007800 00000080  .........x......
02040050: 0001f800 00004000 00000300 00000010  .....@..........
02040060: 66633931 20613261 65766164 00383130  19cfa2a dave018.
02040070: 00000000 00000000 3a393000 353a3133  .........09:31:5
02040080: 65462034 31312062 32303220 00000030  4 Feb 11 2020...
02040090: 00000000 0000001b 00000002 00ff00df  ................

So it looks like the ipc_ap_max field is equal to 0xb.

[   12.972113] exynos-acpm-protocol firmware:power-management: calling acpm_channels_init
[   12.972216] acpm_channels_init: acpm->num_chans=b.
[   12.975541] exynos-acpm-protocol firmware:power-management: ID = 0 poll = 1, mlen = 16, qlen = 15
[   12.976522] exynos-acpm-protocol firmware:power-management: calling acpm_channels_init
[   12.979336] acpm_channels_init: acpm->num_chans=b.
[   12.984133] exynos-acpm-protocol firmware:power-management: ID = 0 poll = 1, mlen = 16, qlen = 15
[   12.993849] exynos-acpm-protocol firmware:power-management: ID = 1 poll = 1, mlen = 16, qlen = 3
[   13.001756] exynos-acpm-protocol firmware:power-management: ID = 2 poll = 1, mlen = 16, qlen = 5
[   13.010519] exynos-acpm-protocol firmware:power-management: ID = 3 poll = 0, mlen = 16, qlen = 1
[   13.019317] exynos-acpm-protocol firmware:power-management: ID = 4 poll = 1, mlen = 16, qlen = 3
[   13.028073] exynos-acpm-protocol firmware:power-management: ID = 5 poll = 0, mlen = 16, qlen = 1
[   13.036805] exynos-acpm-protocol firmware:power-management: ID = 6 poll = 0, mlen = 16, qlen = 1
[   13.050945] exynos-acpm-protocol firmware:power-management: ID = 7 poll = 1, mlen = 2, qlen = 1
[   13.065791] exynos-acpm-protocol firmware:power-management: ID = 8 poll = 1, mlen = 2, qlen = 1
[   13.079592] exynos-acpm-protocol firmware:power-management: ID = 9 poll = 1, mlen = 16, qlen = 7
[   13.088398] exynos-acpm-protocol firmware:power-management: ID = 10 poll = 1, mlen = 8, qlen = 1

That's what sram + initdata provides but I guess these are implemented
number of channels of ACPM firmware (when APM communicates with AP CPU).
The mailbox hardware register though can process or consume 16 bits or
in other words HWEIGHT32(GENMASK(15, 0)). I guess this field should
indicate hardware capability of mbox hardware like max number of
possible channels? I'll change the code to use HWEIGHT32(mask) of
corresponding register then.

Or should there be a call to acpm firmware driver to query the number
of channels? Or should we get it from device tree?

Does gs101 have less than 16 number of ACPM ap channels?

Best regards,
Alexey