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@xxxxxx> > > > --- > > > 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 > > >