| Message ID | 20230417223738.1811110-1-arnd@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: nxp: imx8-isi: fix buiding on 32-bit | expand |
Hi Arnd, Thank you for the patch. On Tue, Apr 18, 2023 at 12:37:27AM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The #if check is wrong, leading to a build failure: > > drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c: In function 'mxc_isi_channel_set_inbuf': > drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c:33:5: error: "CONFIG_ARCH_DMA_ADDR_T_64BIT" is not defined, evaluates to 0 [-Werror=undef] > 33 | #if CONFIG_ARCH_DMA_ADDR_T_64BIT > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This could just be an #ifdef, but it seems nicer to just remove > the check entirely. Apparently the only reason for the #ifdef > is to avoid another warning: > > drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c:55:24: error: right shift count >= width of type [-Werror=shift-count-overflow] > > But this is best avoided by using the lower_32_bits()/upper_32_bits() > helpers. I've submitted a patch yesterday that uses #ifdef, but I like this one better. One could argue that it leads to dead code on 32-bit platforms, but the ISI is only present in 64-bit SoCs, so that's not an issue. > Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > .../media/platform/nxp/imx8-isi/imx8-isi-hw.c | 41 ++++++++++--------- > 1 file changed, 21 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c > index db538f3d88ec..f6112b83866a 100644 > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c > @@ -29,11 +29,10 @@ static inline void mxc_isi_write(struct mxc_isi_pipe *pipe, u32 reg, u32 val) > > void mxc_isi_channel_set_inbuf(struct mxc_isi_pipe *pipe, dma_addr_t dma_addr) > { > - mxc_isi_write(pipe, CHNL_IN_BUF_ADDR, dma_addr); > -#if CONFIG_ARCH_DMA_ADDR_T_64BIT > + mxc_isi_write(pipe, CHNL_IN_BUF_ADDR, lower_32_bits(dma_addr)); > if (pipe->isi->pdata->has_36bit_dma) > - mxc_isi_write(pipe, CHNL_IN_BUF_XTND_ADDR, dma_addr >> 32); > -#endif > + mxc_isi_write(pipe, CHNL_IN_BUF_XTND_ADDR, > + upper_32_bits(dma_addr)); > } > > void mxc_isi_channel_set_outbuf(struct mxc_isi_pipe *pipe, > @@ -45,34 +44,36 @@ void mxc_isi_channel_set_outbuf(struct mxc_isi_pipe *pipe, > val = mxc_isi_read(pipe, CHNL_OUT_BUF_CTRL); > > if (buf_id == MXC_ISI_BUF1) { > - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y, dma_addrs[0]); > - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_U, dma_addrs[1]); > - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_V, dma_addrs[2]); > -#if CONFIG_ARCH_DMA_ADDR_T_64BIT > + mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y, > + lower_32_bits(dma_addrs[0])); Could you please align this with the ( ? I can also do so in my tree, but I expect Mauro to pick this patch directly from the list, so a v2 would make it easier. You can then add my Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_U, > + lower_32_bits(dma_addrs[1])); > + mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_V, > + lower_32_bits(dma_addrs[2])); > if (pipe->isi->pdata->has_36bit_dma) { > mxc_isi_write(pipe, CHNL_Y_BUF1_XTND_ADDR, > - dma_addrs[0] >> 32); > + upper_32_bits(dma_addrs[0])); > mxc_isi_write(pipe, CHNL_U_BUF1_XTND_ADDR, > - dma_addrs[1] >> 32); > + upper_32_bits(dma_addrs[1])); > mxc_isi_write(pipe, CHNL_V_BUF1_XTND_ADDR, > - dma_addrs[2] >> 32); > + upper_32_bits(dma_addrs[2])); > } > -#endif > val ^= CHNL_OUT_BUF_CTRL_LOAD_BUF1_ADDR; > } else { > - mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_Y, dma_addrs[0]); > - mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_U, dma_addrs[1]); > - mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_V, dma_addrs[2]); > -#if CONFIG_ARCH_DMA_ADDR_T_64BIT > + mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_Y, > + lower_32_bits(dma_addrs[0])); > + mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_U, > + lower_32_bits(dma_addrs[1])); > + mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_V, > + lower_32_bits(dma_addrs[2])); > if (pipe->isi->pdata->has_36bit_dma) { > mxc_isi_write(pipe, CHNL_Y_BUF2_XTND_ADDR, > - dma_addrs[0] >> 32); > + upper_32_bits(dma_addrs[0])); > mxc_isi_write(pipe, CHNL_U_BUF2_XTND_ADDR, > - dma_addrs[1] >> 32); > + upper_32_bits(dma_addrs[1])); > mxc_isi_write(pipe, CHNL_V_BUF2_XTND_ADDR, > - dma_addrs[2] >> 32); > + upper_32_bits(dma_addrs[2])); > } > -#endif > val ^= CHNL_OUT_BUF_CTRL_LOAD_BUF2_ADDR; > } >
On Tue, Apr 18, 2023, at 05:20, Laurent Pinchart wrote: > On Tue, Apr 18, 2023 at 12:37:27AM +0200, Arnd Bergmann wrote: >> drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c:55:24: error: right shift count >= width of type [-Werror=shift-count-overflow] >> >> But this is best avoided by using the lower_32_bits()/upper_32_bits() >> helpers. > > I've submitted a patch yesterday that uses #ifdef, but I like this one > better. One could argue that it leads to dead code on 32-bit platforms, > but the ISI is only present in 64-bit SoCs, so that's not an issue. I think there are actually some people using 32-bit kernels on imx8, but they tend to do it for the wrong reasons, and I'm not worried about wasting a few bytes in those setups. My first draft kept an IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) inside each of the three if(), but I dropped that for readability. >> if (buf_id == MXC_ISI_BUF1) { >> - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y, dma_addrs[0]); >> - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_U, dma_addrs[1]); >> - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_V, dma_addrs[2]); >> -#if CONFIG_ARCH_DMA_ADDR_T_64BIT >> + mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y, >> + lower_32_bits(dma_addrs[0])); > > Could you please align this with the ( ? I can also do so in my tree, > but I expect Mauro to pick this patch directly from the list, so a v2 > would make it easier. You can then add my > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Ok, I sent that out now, but now I see I forgot to add your Reviewed-by. Arnd
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c index db538f3d88ec..f6112b83866a 100644 --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c @@ -29,11 +29,10 @@ static inline void mxc_isi_write(struct mxc_isi_pipe *pipe, u32 reg, u32 val) void mxc_isi_channel_set_inbuf(struct mxc_isi_pipe *pipe, dma_addr_t dma_addr) { - mxc_isi_write(pipe, CHNL_IN_BUF_ADDR, dma_addr); -#if CONFIG_ARCH_DMA_ADDR_T_64BIT + mxc_isi_write(pipe, CHNL_IN_BUF_ADDR, lower_32_bits(dma_addr)); if (pipe->isi->pdata->has_36bit_dma) - mxc_isi_write(pipe, CHNL_IN_BUF_XTND_ADDR, dma_addr >> 32); -#endif + mxc_isi_write(pipe, CHNL_IN_BUF_XTND_ADDR, + upper_32_bits(dma_addr)); } void mxc_isi_channel_set_outbuf(struct mxc_isi_pipe *pipe, @@ -45,34 +44,36 @@ void mxc_isi_channel_set_outbuf(struct mxc_isi_pipe *pipe, val = mxc_isi_read(pipe, CHNL_OUT_BUF_CTRL); if (buf_id == MXC_ISI_BUF1) { - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y, dma_addrs[0]); - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_U, dma_addrs[1]); - mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_V, dma_addrs[2]); -#if CONFIG_ARCH_DMA_ADDR_T_64BIT + mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y, + lower_32_bits(dma_addrs[0])); + mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_U, + lower_32_bits(dma_addrs[1])); + mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_V, + lower_32_bits(dma_addrs[2])); if (pipe->isi->pdata->has_36bit_dma) { mxc_isi_write(pipe, CHNL_Y_BUF1_XTND_ADDR, - dma_addrs[0] >> 32); + upper_32_bits(dma_addrs[0])); mxc_isi_write(pipe, CHNL_U_BUF1_XTND_ADDR, - dma_addrs[1] >> 32); + upper_32_bits(dma_addrs[1])); mxc_isi_write(pipe, CHNL_V_BUF1_XTND_ADDR, - dma_addrs[2] >> 32); + upper_32_bits(dma_addrs[2])); } -#endif val ^= CHNL_OUT_BUF_CTRL_LOAD_BUF1_ADDR; } else { - mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_Y, dma_addrs[0]); - mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_U, dma_addrs[1]); - mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_V, dma_addrs[2]); -#if CONFIG_ARCH_DMA_ADDR_T_64BIT + mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_Y, + lower_32_bits(dma_addrs[0])); + mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_U, + lower_32_bits(dma_addrs[1])); + mxc_isi_write(pipe, CHNL_OUT_BUF2_ADDR_V, + lower_32_bits(dma_addrs[2])); if (pipe->isi->pdata->has_36bit_dma) { mxc_isi_write(pipe, CHNL_Y_BUF2_XTND_ADDR, - dma_addrs[0] >> 32); + upper_32_bits(dma_addrs[0])); mxc_isi_write(pipe, CHNL_U_BUF2_XTND_ADDR, - dma_addrs[1] >> 32); + upper_32_bits(dma_addrs[1])); mxc_isi_write(pipe, CHNL_V_BUF2_XTND_ADDR, - dma_addrs[2] >> 32); + upper_32_bits(dma_addrs[2])); } -#endif val ^= CHNL_OUT_BUF_CTRL_LOAD_BUF2_ADDR; }