[PATCH 1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message and mbox_msg_t

[PATCH 1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message and mbox_msg_t

[Andrew Davis]

The type of message sent using omap-mailbox is always u32. The definition
of mbox_msg_t is uintptr_t which is wrong as that type changes based on
the architecture (32bit vs 64bit). Use u32 unconditionally and remove
the now unneeded omap-mailbox.h include.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
index 3555b535b1683..33b30cfb86c9d 100644
--- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
@@ -11,7 +11,6 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_reserved_mem.h>
-#include <linux/omap-mailbox.h>
 #include <linux/platform_device.h>
 #include <linux/remoteproc.h>
 #include <linux/reset.h>
@@ -113,7 +112,7 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
 						  client);
 	struct device *dev = kproc->rproc->dev.parent;
 	const char *name = kproc->rproc->name;
-	u32 msg = omap_mbox_message(data);
+	u32 msg = (u32)(uintptr_t)(data);
 
 	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
 
@@ -152,11 +151,11 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
 {
 	struct k3_dsp_rproc *kproc = rproc->priv;
 	struct device *dev = rproc->dev.parent;
-	mbox_msg_t msg = (mbox_msg_t)vqid;
+	u32 msg = vqid;
 	int ret;
 
 	/* send the index of the triggered virtqueue in the mailbox payload */
-	ret = mbox_send_message(kproc->mbox, (void *)msg);
+	ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
 	if (ret < 0)
 		dev_err(dev, "failed to send mailbox message (%pe)\n",
 			ERR_PTR(ret));
-- 
2.39.2

[PATCH 2/3] remoteproc: k3-r5: Fix usage of omap_mbox_message and mbox_msg_t

[Andrew Davis]

The type of message sent using omap-mailbox is always u32. The definition
of mbox_msg_t is uintptr_t which is wrong as that type changes based on
the architecture (32bit vs 64bit). Use u32 unconditionally and remove
the now unneeded omap-mailbox.h include.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index ad3415a3851b2..3bcde6d00b56a 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -16,7 +16,6 @@
 #include <linux/of_address.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/of_platform.h>
-#include <linux/omap-mailbox.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/remoteproc.h>
@@ -188,7 +187,7 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client *client, void *data)
 						client);
 	struct device *dev = kproc->rproc->dev.parent;
 	const char *name = kproc->rproc->name;
-	u32 msg = omap_mbox_message(data);
+	u32 msg = (u32)(uintptr_t)(data);
 
 	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
 
@@ -222,11 +221,11 @@ static void k3_r5_rproc_kick(struct rproc *rproc, int vqid)
 {
 	struct k3_r5_rproc *kproc = rproc->priv;
 	struct device *dev = rproc->dev.parent;
-	mbox_msg_t msg = (mbox_msg_t)vqid;
+	u32 msg = vqid;
 	int ret;
 
 	/* send the index of the triggered virtqueue in the mailbox payload */
-	ret = mbox_send_message(kproc->mbox, (void *)msg);
+	ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
 	if (ret < 0)
 		dev_err(dev, "failed to send mailbox message, status = %d\n",
 			ret);
-- 
2.39.2

[PATCH 3/3] remoteproc: omap: Remove unused header omap-mailbox.h

[Andrew Davis]

This header no longer used, remove this include.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/remoteproc/omap_remoteproc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c
index 8f50ab80e56f4..bde04e3e6d966 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -29,7 +29,6 @@
 #include <linux/remoteproc.h>
 #include <linux/mailbox_client.h>
 #include <linux/omap-iommu.h>
-#include <linux/omap-mailbox.h>
 #include <linux/regmap.h>
 #include <linux/mfd/syscon.h>
 #include <linux/reset.h>
-- 
2.39.2

Re: [PATCH 1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message and mbox_msg_t

[Mathieu Poirier]

Hi Andrew,

On Mon, Mar 25, 2024 at 11:58:06AM -0500, Andrew Davis wrote:
> The type of message sent using omap-mailbox is always u32. The definition
> of mbox_msg_t is uintptr_t which is wrong as that type changes based on
> the architecture (32bit vs 64bit). Use u32 unconditionally and remove
> the now unneeded omap-mailbox.h include.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> index 3555b535b1683..33b30cfb86c9d 100644
> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -11,7 +11,6 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_reserved_mem.h>
> -#include <linux/omap-mailbox.h>
>  #include <linux/platform_device.h>
>  #include <linux/remoteproc.h>
>  #include <linux/reset.h>
> @@ -113,7 +112,7 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
>  						  client);
>  	struct device *dev = kproc->rproc->dev.parent;
>  	const char *name = kproc->rproc->name;
> -	u32 msg = omap_mbox_message(data);
> +	u32 msg = (u32)(uintptr_t)(data);
>  

Looking at omap-mailbox.h and unless I'm missing something, the end result is
the same.


>  	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
>  
> @@ -152,11 +151,11 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
>  {
>  	struct k3_dsp_rproc *kproc = rproc->priv;
>  	struct device *dev = rproc->dev.parent;
> -	mbox_msg_t msg = (mbox_msg_t)vqid;
> +	u32 msg = vqid;
>  	int ret;
> 

Here @vqid becomes a 'u32' rather than a 'uintptr'...

>  	/* send the index of the triggered virtqueue in the mailbox payload */
> -	ret = mbox_send_message(kproc->mbox, (void *)msg);
> +	ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);

... but here it is casted as a 'uintptr_t', which yields the same result.


I am puzzled - other than getting rid of a header file I don't see what else
this patch does.

Thanks,
Mathieu

>  	if (ret < 0)
>  		dev_err(dev, "failed to send mailbox message (%pe)\n",
>  			ERR_PTR(ret));
> -- 
> 2.39.2
> 

Re: [PATCH 1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message and mbox_msg_t

[Andrew Davis]

On 3/28/24 10:28 AM, Mathieu Poirier wrote:
> Hi Andrew,
> 
> On Mon, Mar 25, 2024 at 11:58:06AM -0500, Andrew Davis wrote:
>> The type of message sent using omap-mailbox is always u32. The definition
>> of mbox_msg_t is uintptr_t which is wrong as that type changes based on
>> the architecture (32bit vs 64bit). Use u32 unconditionally and remove
>> the now unneeded omap-mailbox.h include.
>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   drivers/remoteproc/ti_k3_dsp_remoteproc.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> index 3555b535b1683..33b30cfb86c9d 100644
>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> @@ -11,7 +11,6 @@
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <linux/of_reserved_mem.h>
>> -#include <linux/omap-mailbox.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/remoteproc.h>
>>   #include <linux/reset.h>
>> @@ -113,7 +112,7 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
>>   						  client);
>>   	struct device *dev = kproc->rproc->dev.parent;
>>   	const char *name = kproc->rproc->name;
>> -	u32 msg = omap_mbox_message(data);
>> +	u32 msg = (u32)(uintptr_t)(data);
>>   
> 
> Looking at omap-mailbox.h and unless I'm missing something, the end result is
> the same.
> 
> 
>>   	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
>>   
>> @@ -152,11 +151,11 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
>>   {
>>   	struct k3_dsp_rproc *kproc = rproc->priv;
>>   	struct device *dev = rproc->dev.parent;
>> -	mbox_msg_t msg = (mbox_msg_t)vqid;
>> +	u32 msg = vqid;
>>   	int ret;
>>
> 
> Here @vqid becomes a 'u32' rather than a 'uintptr'...
> 

u32 is the correct type for messages sent with OMAP mailbox. It
only sends 32bit messages, uintptr is 64bit when compiled on
64bit hardware (like our ARM64 cores on K3). mbox_msg_t should
have been defined as u32, this was a mistake we missed as we only
ever used to compile it for 32bit cores (where uintptr is 32bit).

>>   	/* send the index of the triggered virtqueue in the mailbox payload */
>> -	ret = mbox_send_message(kproc->mbox, (void *)msg);
>> +	ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
> 
> ... but here it is casted as a 'uintptr_t', which yields the same result.
> 

The function mbox_send_message() takes a void*, so we need to cast our 32bit
message to that first, it is cast back to u32 inside the OMAP mailbox driver.
Doing that in one step (u32 -> void*) causes a warning when void* is 64bit
(cast from int to pointer of different size).

> 
> I am puzzled - other than getting rid of a header file I don't see what else
> this patch does.
> 

Getting rid of the header is the main point of this patch (I have a later
series that needs that header gone). But the difference this patch makes is that
before we passed a pointer to a 64bit int to OMAP mailbox which takes a pointer
to a 32bit int. Sure, the result is the same in little-endian systems, but that
isn't a strictly correct in general.

Thanks,
Andrew

> Thanks,
> Mathieu
> 
>>   	if (ret < 0)
>>   		dev_err(dev, "failed to send mailbox message (%pe)\n",
>>   			ERR_PTR(ret));
>> -- 
>> 2.39.2
>>

Re: [PATCH 1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message and mbox_msg_t

[Mathieu Poirier]

On Thu, Mar 28, 2024 at 11:26:24AM -0500, Andrew Davis wrote:
> On 3/28/24 10:28 AM, Mathieu Poirier wrote:
> > Hi Andrew,
> > 
> > On Mon, Mar 25, 2024 at 11:58:06AM -0500, Andrew Davis wrote:
> > > The type of message sent using omap-mailbox is always u32. The definition
> > > of mbox_msg_t is uintptr_t which is wrong as that type changes based on
> > > the architecture (32bit vs 64bit). Use u32 unconditionally and remove
> > > the now unneeded omap-mailbox.h include.
> > > 
> > > Signed-off-by: Andrew Davis <afd@ti.com>
> > > ---
> > >   drivers/remoteproc/ti_k3_dsp_remoteproc.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> > > index 3555b535b1683..33b30cfb86c9d 100644
> > > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> > > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> > > @@ -11,7 +11,6 @@
> > >   #include <linux/module.h>
> > >   #include <linux/of.h>
> > >   #include <linux/of_reserved_mem.h>
> > > -#include <linux/omap-mailbox.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/remoteproc.h>
> > >   #include <linux/reset.h>
> > > @@ -113,7 +112,7 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
> > >   						  client);
> > >   	struct device *dev = kproc->rproc->dev.parent;
> > >   	const char *name = kproc->rproc->name;
> > > -	u32 msg = omap_mbox_message(data);
> > > +	u32 msg = (u32)(uintptr_t)(data);
> > 
> > Looking at omap-mailbox.h and unless I'm missing something, the end result is
> > the same.
> > 
> > 
> > >   	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
> > > @@ -152,11 +151,11 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
> > >   {
> > >   	struct k3_dsp_rproc *kproc = rproc->priv;
> > >   	struct device *dev = rproc->dev.parent;
> > > -	mbox_msg_t msg = (mbox_msg_t)vqid;
> > > +	u32 msg = vqid;
> > >   	int ret;
> > > 
> > 
> > Here @vqid becomes a 'u32' rather than a 'uintptr'...
> > 
> 
> u32 is the correct type for messages sent with OMAP mailbox. It
> only sends 32bit messages, uintptr is 64bit when compiled on
> 64bit hardware (like our ARM64 cores on K3). mbox_msg_t should
> have been defined as u32, this was a mistake we missed as we only
> ever used to compile it for 32bit cores (where uintptr is 32bit).
> 
> > >   	/* send the index of the triggered virtqueue in the mailbox payload */
> > > -	ret = mbox_send_message(kproc->mbox, (void *)msg);
> > > +	ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
> > 
> > ... but here it is casted as a 'uintptr_t', which yields the same result.
> > 
> 
> The function mbox_send_message() takes a void*, so we need to cast our 32bit
> message to that first, it is cast back to u32 inside the OMAP mailbox driver.
> Doing that in one step (u32 -> void*) causes a warning when void* is 64bit
> (cast from int to pointer of different size).
> 
> > 
> > I am puzzled - other than getting rid of a header file I don't see what else
> > this patch does.
> > 
> 
> Getting rid of the header is the main point of this patch (I have a later
> series that needs that header gone). But the difference this patch makes is that
> before we passed a pointer to a 64bit int to OMAP mailbox which takes a pointer
> to a 32bit int. Sure, the result is the same in little-endian systems, but that
> isn't a strictly correct in general.

From your explanation above this patchset is about two things:

1) Getting rid of a compilation warning when void* is 64bit wide
2) Getting rid of omap-mailbox.h

This is what the changelog should describe.  And next time, please add a cover
letter to your work.

Thanks,
Mathieu

> > >   	if (ret < 0)
> > >   		dev_err(dev, "failed to send mailbox message (%pe)\n",
> > >   			ERR_PTR(ret));
> > > -- 
> > > 2.39.2
> > >