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

[Tudor Ambarus]

On 5/18/26 6:53 PM, Alexey Klimov wrote:
> Hi Tudor,
> 
Hi!

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

Exactly. The mailbox controller driver should model the underlying
hardware's capabilities, not the firmware's specific usage limits.
Since the Exynos850 hardware register uses a 16-bit mask, exposing
16 hardware channels is the right move. Moving num_chans into
struct exynos_mbox_driver_data alongside the register masks for
each SoC is the cleanest approach.

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

There is no need to call the ACPM firmware driver. The mailbox
controller merely exposes the available pipes.

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

I don't remember, but regardless of how many channels the gs101 acpm
firmware actually utilizes, its hw mask is also 16 bit, so exposing
16 hardware channels for it is correct.

Cheers,
ta