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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

Hi, Alexey,

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.

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

> +
> +	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.

Cheers,
ta