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

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

On 4/8/26 4:08 PM, Alexey Klimov wrote:
> Hi Tudor,

Hi!

>> I find it strange that the SoCs use different registers. Are you sure you're
>> using the right direction? i.e. ring the doorbell to APM and not to AP?
> 
> Well, I am not sure I correctly understood the questions and comment. So,
> this all was tested with ACPM TMU code with 3 temp sensors and it seems
> to work and sensors react in the right way.
> 
> Downstream clearly does the following (see also [1],[2]) when sending
> ACPM msg:
> 
> static void apm_interrupt_gen(unsigned int id)
> {
> 	/* APM NVIC INTERRUPT GENERATE */
> 	writel((1 << id) << 16, acpm_ipc->intr + INTGR0);
> }
> 

Indeed, it looks alright, thanks for the pointers.

> I am aware that gs101 downstream uses INTGR1 in apm_interrupt_gen().
> 
> When I use INTGR1 for e850 then I observe acpm timeouts. Hence, out of
> curiosity, what's the expected behaviour when/if I ring the doorbell to

If you raise your own interrupt the APM remains unaware and the AP times out.
You also have a spurious interrupt on yourself.

> AP (to itself as far as I understand)? My understanding that it won't
> work at all in such case unless APM firmware does some very fast
> polling.
> 
> 
> [1]: https://gitlab.com/Linaro/96boards/e850-96/kernel/-/blob/android-exynos-4.14-linaro/drivers/soc/samsung/acpm/acpm_ipc.c?ref_type=heads#L423
> [2]: https://github.com/samsungexynos850/android_kernel_samsung_exynos850/blob/0af517be2336bf8e09c59d576c4c314446713101/drivers/soc/samsung/acpm/acpm_ipc.c#L426
> 
>>>  static int exynos_mbox_send_data(struct mbox_chan *chan, void *data)
>>> @@ -57,7 +104,8 @@ 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);
>>> +	writel(BIT(msg->chan_id) << exynos_mbox->data->irq_doorbell_shift,
>>> +	       exynos_mbox->regs + exynos_mbox->data->irq_doorbell_offset);
>>
>> Use FIELD_PREP from <linux/bitfield.h> please. You will use a mask instead of
>> a shift.
>>
>> I would rename irq_doorbell_offset to intgr. It aligns with the register name
>> from the datasheet. You won't need to prepend _offset to the name, we already
>> see it's an offset when doing the writel().
> 
> Sure. Thanks. Let's use FIELD_PREP.
> 
> "doorbell" naming was chosen for readability and maintainability reasons.
> It seems to be more generic enough name that better reflects the workflow
> of what's going on in ACPM+mailbox machinery. We can rename it to just
> "doorbell" for instance.
> 
> From platform data it will be clear to which register it is set, INTGR0
> or INTGR1, to align it with datasheet (which is closed anyway).
> 
> Regarding intgr vs doorbell name, the intgr is a bit unclear for a
> reader if it means interrupt generation register or something else.

interrupt generation registers sounds sane to me

> But if you prefer, I can go with "intgr".

I think I prefer intgr, yes. If you choose doorbell, you'll have:

writel(FIELD_PREP(data->doorbell_mask), BIT(msg->chan_id),
       exynos_mbox->regs + data->doorbell);

or maybe s/doorbell/doorbell_reg? But that would duplicate
exynos_mbox->regs, we already see that doorbell is a reg offset.
Doorbell is too generic for my taste.

And then how would you refer to the interrupt mask register? You
already have a doorbell_mask in the example above. 

I won't push back too hard, I'll let you choose. If you can find a good
naming scheme for the interrupt generation reg and interrupt mask reg,
then fine.

> 
> One more option is add a comment, smth like /* Ring the doorbell */
> before that writel().

I'm okay with such comment.

Cheers,
ta