Re: [PATCH v3] mailbox: qcom-cpucp: read mbox hardware register using readq()

[Sivansh Gupta]

On 2026-09-30 11:55 +0200, Konrad Dybcio wrote:
> On 9/28/26 7:29 AM, Sivansh Gupta wrote:
> > Mailbox hardware registers are 64 bit wide, but the interrupt handler
> > currently reads only 32 bits with readl() at offset 0x4 (upper 32
> > bits). This returns an incorrect value, while reading using readq()
> > returns the expected value.
> > 
> > Use readq() to access the 64 bit register and pass the expected value
> > to the mailbox client.
> > 
> > Fixes: 0e2a9a03106c ("mailbox: Add support for QTI CPUCP mailbox controller")
> > Signed-off-by: Sivansh Gupta <sivansh.gupta@xxxxxxxxxxxxxxxx>
> > ---
> > v3:
> > - Removed references to an out of tree driver to focus on the incorrect
> >   value returned by readl()
> >   https://lore.kernel.org/all/93ce8588-7500-45da-9b9a-424c202590cf@xxxxxxxxxxxxxxxx/#t
> > 
> > v2:
> > - Expanded the commit message to explain why this change is required.
> > 
> >  drivers/mailbox/qcom-cpucp-mbox.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> > index 862e45e8fbd5..0b584d9aad6c 100644
> > --- a/drivers/mailbox/qcom-cpucp-mbox.c
> > +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> > @@ -61,7 +61,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> >  	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> >  
> >  	for_each_set_bit(i, (unsigned long *)&status, cpucp->mbox.num_chans) {
> > -		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> 
> This still retains the writel() later on

This is intentional, currently the existing mbox client(s) assume 32 bits
writes instead of 64 bits, changing to 64 bits here would require all
mbox clients to be updated to send 64 bits.
Currently there is only 1 upstream client (arm_scmi).

> > +		u64 val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i));
> 
> This will alter the indices of the mailbox channels, but it's ""fine""
> because it's always zero for the existing platforms..
> 
> Konrad

I am unsure what you mean here. The channel registers are spaced 8 bytes
apart. In the old code, we used to read each channel's upper 32 bits
(channel base + 4 bytes), in the new code we will be reading the entire
8 bytes.
If this is not what you meant, please let me know!

Thanks,
Sivansh