Hi Tomi,
Thank you for the patch.
On Mon, Apr 12, 2021 at 02:34:48PM +0300, Tomi Valkeinen wrote:
> I have not noticed any errors due to this, but the DMA configuration
> looks racy. Setting the DMA mode bitfield in CAL_WR_DMA_CTRL supposedly
> enables the DMA. However, the driver currently a) continues configuring
> the DMA after setting the mode, and b) enables the DMA interrupts only
> after setting the mode.
>
> This probably doesn't cause any issues as there should be no data coming
> in to the DMA yet, but it's still better to fix this.
>
> Add a new function, cal_ctx_wr_dma_enable(), to set the DMA mode field,
> and call that function only after the DMA config and the irq enabling
> has been done.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/ti-vpe/cal.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index a1d173bd4613..0fef892854ef 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -409,8 +409,6 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx)
> CAL_WR_DMA_CTRL_YSIZE_MASK);
> cal_set_field(&val, CAL_WR_DMA_CTRL_DTAG_PIX_DAT,
> CAL_WR_DMA_CTRL_DTAG_MASK);
> - cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST,
> - CAL_WR_DMA_CTRL_MODE_MASK);
> cal_set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR,
> CAL_WR_DMA_CTRL_PATTERN_MASK);
> cal_set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK);
> @@ -442,6 +440,15 @@ void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr)
> cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->dma_ctx), addr);
> }
>
> +static void cal_ctx_wr_dma_enable(struct cal_ctx *ctx)
> +{
> + u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx));
> +
> + cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST,
> + CAL_WR_DMA_CTRL_MODE_MASK);
> + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val);
> +}
> +
> static void cal_ctx_wr_dma_disable(struct cal_ctx *ctx)
> {
> u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx));
> @@ -500,6 +507,8 @@ void cal_ctx_start(struct cal_ctx *ctx)
> CAL_HL_IRQ_WDMA_END_MASK(ctx->dma_ctx));
> cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(2),
> CAL_HL_IRQ_WDMA_START_MASK(ctx->dma_ctx));
> +
> + cal_ctx_wr_dma_enable(ctx);
> }
You could also move the IRQ enabling before the call to
cal_ctx_wr_dma_config(), and reorder the configuration in
cal_ctx_wr_dma_config() to write CAL_WR_DMA_CTRL last. That would save a
read-modify-write cycle.
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> void cal_ctx_stop(struct cal_ctx *ctx)
--
Regards,
Laurent Pinchart
