Re: [PATCH v7 02/20] mailbox: mtk-cmdq: Refine DMA address handling for the command buffer

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

On Thu, 2025-10-09 at 13:27 +0200, AngeloGioacchino Del Regno wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 27/08/25 13:37, Jason-JH Lin ha scritto:
> > GCE can only fetch the command buffer address from a 32-bit
> > register.
> > Some SoCs support a 35-bit command buffer address for GCE, which
> > requires a right shift of 3 bits before setting the address into
> > the 32-bit register. A comment has been added to the header of
> > cmdq_get_shift_pa() to explain this requirement.
> > 
> > To prevent the GCE command buffer address from being DMA mapped
> > beyond
> > its supported bit range, the DMA bit mask for the device is set
> > during
> > initialization.
> > 
> > Additionally, to ensure the correct shift is applied when setting
> > or
> > reading the register that stores the GCE command buffer address,
> > new APIs, cmdq_reg_shift_addr() and cmdq_reg_revert_addr(), have
> > been
> > introduced for consistent operations on this register.
> > 
> > The variable type for the command buffer address has been
> > standardized
> > to dma_addr_t to prevent handling issues caused by type mismatches.
> > 
> > Fixes: 0858fde496f8 ("mailbox: cmdq: variablize address shift in
> > platform")
> > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> > ---
> >   drivers/mailbox/mtk-cmdq-mailbox.c       | 43 ++++++++++++++++---
> > -----
> >   include/linux/mailbox/mtk-cmdq-mailbox.h | 10 ++++++
> >   2 files changed, 39 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 532929916e99..a60486cbb533 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -92,6 +92,16 @@ struct gce_plat {
> >       u32 gce_num;
> >   };
> > 
> > +static inline u32 cmdq_reg_shift_addr(dma_addr_t addr, const
> > struct gce_plat *pdata)
> 
> cmdq_format_gce_addr() or cmdq_pa_to_gce_addr()
> 
> (or cmdq_va_to_gce_addr() if this is not a physical address)
> 
> explains what you're doing here a bit better I think.

OK, I'll change the API name and add comments for both APIs:

static inline u32 cmdq_convert_gce_addr()
{
    /* Convert DMA addr (PA or IOVA) to GCE readable addr */
}

static inline u32 cmdq_revert_gce_addr()
{
    /* Revert GCE readable addr to DMA addr (PA or IOVA) */
}

> 
> > +{
> > +     return (addr >> pdata->shift);
> 
> You don't need parenthesis; just `return addr >> pdata->shift;` is
> fine
> 

OK, I'll remove this.

> > +}
> > +
> > +static inline dma_addr_t cmdq_reg_revert_addr(u32 addr, const
> > struct gce_plat *pdata)
> 
> cmdq_gce_to_pa_addr() or cmdq_get_pa_addr() perhaps? :-)
> 
> (same comments for s/pa/va/g if this is not physical address)
> 
> Everything else looks ok.
> 

Thanks for the reviews!

Regards,
Jason-JH Lin

> Cheers,
> Angelo