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

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

[Sivansh Gupta]

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@oss.qualcomm.com>
---
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@oss.qualcomm.com/#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);
+		u64 val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i));
 		struct mbox_chan *chan = &cpucp->chans[i];
 		unsigned long flags;
 
-- 
2.43.0

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

[Sibi Sankar]

On 9/28/26 10:59, 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.

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 results in a masked out value being passed on to the mailbox
clients.

Fix this by using readq instead and pass on the correct value to the
mailbox clients.

^^ seems more appropriate with these minor nits fixed.

Reviewed-by: Sibi Sankar <sibi.sankar@oss.qualcomm.com>

> 
> Fixes: 0e2a9a03106c ("mailbox: Add support for QTI CPUCP mailbox controller")
> Signed-off-by: Sivansh Gupta <sivansh.gupta@oss.qualcomm.com>
> ---
> 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@oss.qualcomm.com/#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);
> +		u64 val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i));
>   		struct mbox_chan *chan = &cpucp->chans[i];
>   		unsigned long flags;
>   

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

[Konrad Dybcio]

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@oss.qualcomm.com>
> ---
> 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@oss.qualcomm.com/#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

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

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@oss.qualcomm.com>
> > ---
> > 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@oss.qualcomm.com/#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

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

[Sivansh Gupta]

On 2026-10-13 18:02 +0530, 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@oss.qualcomm.com>
> > > ---
> > > 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@oss.qualcomm.com/#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

Hey Konrad, you might have missed my message, so just sending a gentle
reminder!

Sivansh