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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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.

> +{
> +	return (addr >> pdata->shift);

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

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

Cheers,
Angelo

> +{
> +	return ((dma_addr_t)addr << pdata->shift);
> +}
> +
>   u8 cmdq_get_shift_pa(struct mbox_chan *chan)
>   {
>   	struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, mbox);
> @@ -188,13 +198,12 @@ static void cmdq_task_insert_into_thread(struct cmdq_task *task)
>   	struct cmdq_task *prev_task = list_last_entry(
>   			&thread->task_busy_list, typeof(*task), list_entry);
>   	u64 *prev_task_base = prev_task->pkt->va_base;
> +	u32 shift_addr = cmdq_reg_shift_addr(task->pa_base, task->cmdq->pdata);
>   
>   	/* let previous task jump to this task */
>   	dma_sync_single_for_cpu(dev, prev_task->pa_base,
>   				prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
> -	prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
> -		(u64)CMDQ_JUMP_BY_PA << 32 |
> -		(task->pa_base >> task->cmdq->pdata->shift);
> +	prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] = (u64)CMDQ_JUMP_BY_PA << 32 | shift_addr;
>   	dma_sync_single_for_device(dev, prev_task->pa_base,
>   				   prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
>   
> @@ -237,7 +246,8 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
>   				    struct cmdq_thread *thread)
>   {
>   	struct cmdq_task *task, *tmp, *curr_task = NULL;
> -	u32 curr_pa, irq_flag, task_end_pa;
> +	u32 irq_flag, shift_addr;
> +	dma_addr_t curr_pa, task_end_pa;
>   	bool err;
>   
>   	irq_flag = readl(thread->base + CMDQ_THR_IRQ_STATUS);
> @@ -259,7 +269,8 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
>   	else
>   		return;
>   
> -	curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) << cmdq->pdata->shift;
> +	shift_addr = readl(thread->base + CMDQ_THR_CURR_ADDR);
> +	curr_pa = cmdq_reg_revert_addr(shift_addr, cmdq->pdata);
>   
>   	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
>   				 list_entry) {
> @@ -378,7 +389,8 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
>   	struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv;
>   	struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev);
>   	struct cmdq_task *task;
> -	unsigned long curr_pa, end_pa;
> +	u32 shift_addr;
> +	dma_addr_t curr_pa, end_pa;
>   	int ret;
>   
>   	/* Client should not flush new tasks if suspended. */
> @@ -409,20 +421,20 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
>   		 */
>   		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
>   
> -		writel(task->pa_base >> cmdq->pdata->shift,
> -		       thread->base + CMDQ_THR_CURR_ADDR);
> -		writel((task->pa_base + pkt->cmd_buf_size) >> cmdq->pdata->shift,
> -		       thread->base + CMDQ_THR_END_ADDR);
> +		shift_addr = cmdq_reg_shift_addr(task->pa_base, cmdq->pdata);
> +		writel(shift_addr, thread->base + CMDQ_THR_CURR_ADDR);
> +		shift_addr = cmdq_reg_shift_addr(task->pa_base + pkt->cmd_buf_size, cmdq->pdata);
> +		writel(shift_addr, thread->base + CMDQ_THR_END_ADDR);
>   
>   		writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
>   		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
>   		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
>   	} else {
>   		WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> -		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) <<
> -			cmdq->pdata->shift;
> -		end_pa = readl(thread->base + CMDQ_THR_END_ADDR) <<
> -			cmdq->pdata->shift;
> +		shift_addr = readl(thread->base + CMDQ_THR_CURR_ADDR);
> +		curr_pa = cmdq_reg_revert_addr(shift_addr, cmdq->pdata);
> +		shift_addr = readl(thread->base + CMDQ_THR_END_ADDR);
> +		end_pa = cmdq_reg_revert_addr(shift_addr, cmdq->pdata);
>   		/* check boundary */
>   		if (curr_pa == end_pa - CMDQ_INST_SIZE ||
>   		    curr_pa == end_pa) {
> @@ -656,6 +668,9 @@ static int cmdq_probe(struct platform_device *pdev)
>   	if (err)
>   		return err;
>   
> +	dma_set_coherent_mask(dev,
> +			      DMA_BIT_MASK(sizeof(u32) * BITS_PER_BYTE + cmdq->pdata->shift));
> +
>   	cmdq->mbox.dev = dev;
>   	cmdq->mbox.chans = devm_kcalloc(dev, cmdq->pdata->thread_nr,
>   					sizeof(*cmdq->mbox.chans), GFP_KERNEL);
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 4c1a91b07de3..e1555e06e7e5 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -77,6 +77,16 @@ struct cmdq_pkt {
>   	size_t			buf_size; /* real buffer size */
>   };
>   
> +/**
> + * cmdq_get_shift_pa() - get the shift bits of physical address
> + * @chan: mailbox channel
> + *
> + * GCE can only fetch the command buffer address from a 32-bit register.
> + * Some SOCs support more than 32-bit command buffer address for GCE, which
> + * requires some shift bits to make the address fit into the 32-bit register.
> + *
> + * Return: the shift bits of physical address
> + */
>   u8 cmdq_get_shift_pa(struct mbox_chan *chan);
>   
>   #endif /* __MTK_CMDQ_MAILBOX_H__ */