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

[Sivansh Gupta]

On 2026-11-02 16:56 +0100, Konrad Dybcio wrote:
> On 10/13/26 2:32 PM, Sivansh Gupta wrote:
> > 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!
> 
> You're right, please remove the leftover define

Hey, APSS_CPUCP_MBOX_CMD_OFF is intentionally used in 
qcom_cpucp_mbox_send_data  as it is still using writel instead of writeq, 
as we discussed.

Also, I have since learned that this patch has already been picked up!

Cheers, Sivansh

> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> 
> Konrad
> 
>