diff mbox series

[v2] media: imx-jpeg: Leave a blank space before the configuration data

Message ID 20220527102444.19683-1-ming.qian@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v2] media: imx-jpeg: Leave a blank space before the configuration data | expand

Commit Message

Ming Qian May 27, 2022, 10:24 a.m. UTC 
There is a hardware bug that it will load
the first 128 bytes of configuration data twice,
it will led to some configure error.
so shift the configuration data 128 bytes,
and make the first 128 bytes all zero,
then hardware will load the 128 zero twice,
and ignore them as garbage.
then the configuration data can be loaded correctly

Signed-off-by: Ming Qian <ming.qian@nxp.com>
Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
v2
- add some comments about why the 0x80 offset is needed
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Tommaso Merciai May 27, 2022, 10:28 a.m. UTC | #1 
On Fri, May 27, 2022 at 06:24:44PM +0800, Ming Qian wrote:
> There is a hardware bug that it will load
> the first 128 bytes of configuration data twice,
> it will led to some configure error.
> so shift the configuration data 128 bytes,
> and make the first 128 bytes all zero,
> then hardware will load the 128 zero twice,
> and ignore them as garbage.
> then the configuration data can be loaded correctly
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
> v2
> - add some comments about why the 0x80 offset is needed
>  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 734e1b65fbc7..c0fd030d0f19 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg,
>  				     GFP_ATOMIC);
>  	if (!cfg_stm)
>  		goto err;
> +	memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
>  	jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
>  
>  skip_alloc:
> @@ -755,7 +756,13 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
>  					      u32 fourcc,
>  					      u16 w, u16 h)
>  {
> -	unsigned int offset = 0;
> +	/*
> +	 * There is a hardware issue that first 128 bytes of configuration data
> +	 * can't be loaded correctly.
> +	 * To avoid this issue, we need to write the configuration from
> +	 * an offset which should be no less than 0x80 (128 bytes).
> +	 */
> +	unsigned int offset = 0x80;
>  	u8 *cfg = (u8 *)cfg_stream_vaddr;
>  	struct mxc_jpeg_sof *sof;
>  	struct mxc_jpeg_sos *sos;
> -- 
> 2.36.1
> 

Looks good to me!

Thanks,
Tommaso
Fabio Estevam May 27, 2022, 11:11 a.m. UTC | #2 
Hi Ming,

On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
>
> There is a hardware bug that it will load
> the first 128 bytes of configuration data twice,
> it will led to some configure error.
> so shift the configuration data 128 bytes,
> and make the first 128 bytes all zero,
> then hardware will load the 128 zero twice,
> and ignore them as garbage.
> then the configuration data can be loaded correctly
>
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>

Fixes tag?
Ming Qian May 27, 2022, 11:33 a.m. UTC | #3 
> From: Fabio Estevam <festevam@gmail.com>
> Sent: 2022年5月27日 19:12
> To: Ming Qian <ming.qian@nxp.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>;
> Shawn Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Sascha Hauer <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> linux-media <linux-media@vger.kernel.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>
> Subject: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before the
> configuration data
> 
> Caution: EXT Email
> 
> Hi Ming,
> 
> On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
> >
> > There is a hardware bug that it will load the first 128 bytes of
> > configuration data twice, it will led to some configure error.
> > so shift the configuration data 128 bytes, and make the first 128
> > bytes all zero, then hardware will load the 128 zero twice, and ignore
> > them as garbage.
> > then the configuration data can be loaded correctly
> >
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> 
> Fixes tag?

Hi Fabio,
    It's a hardware issue, so I'm not sure is it a driver issue that I fix it.
    Or I just check which patch includes the code I changed, and add the fix tag?

Ming
Nicolas Dufresne May 27, 2022, 7:26 p.m. UTC | #4 
Le vendredi 27 mai 2022 à 11:33 +0000, Ming Qian a écrit :
> > From: Fabio Estevam <festevam@gmail.com>
> > Sent: 2022年5月27日 19:12
> > To: Ming Qian <ming.qian@nxp.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> > <mirela.rabulea@oss.nxp.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>;
> > Shawn Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> > Sascha Hauer <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > linux-media <linux-media@vger.kernel.org>; linux-kernel
> > <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED
> > DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; moderated
> > list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> > <linux-arm-kernel@lists.infradead.org>
> > Subject: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before
> > the
> > configuration data
> > 
> > Caution: EXT Email
> > 
> > Hi Ming,
> > 
> > On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
> > > 
> > > There is a hardware bug that it will load the first 128 bytes of
> > > configuration data twice, it will led to some configure error.
> > > so shift the configuration data 128 bytes, and make the first 128
> > > bytes all zero, then hardware will load the 128 zero twice, and ignore
> > > them as garbage.
> > > then the configuration data can be loaded correctly
> > > 
> > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > 
> > Fixes tag?
> 
> Hi Fabio,
>     It's a hardware issue, so I'm not sure is it a driver issue that I fix it.
>     Or I just check which patch includes the code I changed, and add the fix
> tag?

You can use Fixes tag even though there was no software bug. The point of the
tag is to help locate how far we can backport this patch in order to let stable
kernel users benefit.

regards,
Nicolas

> 
> Ming
Ming Qian May 30, 2022, 2:50 a.m. UTC | #5 
> From: Nicolas Dufresne <nicolas@ndufresne.ca>
> Sent: 2022年5月28日 3:26
> To: Ming Qian <ming.qian@nxp.com>; Fabio Estevam <festevam@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>;
> Shawn Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Sascha Hauer <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> linux-media <linux-media@vger.kernel.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before
> the configuration data
> 
> Caution: EXT Email
> 
> Le vendredi 27 mai 2022 à 11:33 +0000, Ming Qian a écrit :
> > > From: Fabio Estevam <festevam@gmail.com>
> > > Sent: 2022年5月27日 19:12
> > > To: Ming Qian <ming.qian@nxp.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> > > <mirela.rabulea@oss.nxp.com>; Hans Verkuil
> > > <hverkuil-cisco@xs4all.nl>; Shawn Guo <shawnguo@kernel.org>; Sascha
> > > Hauer <s.hauer@pengutronix.de>; Sascha Hauer
> > > <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > > linux-media <linux-media@vger.kernel.org>; linux-kernel
> > > <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND
> > > FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> > > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> > > <linux-arm-kernel@lists.infradead.org>
> > > Subject: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space
> > > before the configuration data
> > >
> > > Caution: EXT Email
> > >
> > > Hi Ming,
> > >
> > > On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
> > > >
> > > > There is a hardware bug that it will load the first 128 bytes of
> > > > configuration data twice, it will led to some configure error.
> > > > so shift the configuration data 128 bytes, and make the first 128
> > > > bytes all zero, then hardware will load the 128 zero twice, and
> > > > ignore them as garbage.
> > > > then the configuration data can be loaded correctly
> > > >
> > > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > > Reviewed-by: Tommaso Merciai
> > > > <tommaso.merciai@amarulasolutions.com>
> > >
> > > Fixes tag?
> >
> > Hi Fabio,
> >     It's a hardware issue, so I'm not sure is it a driver issue that I fix it.
> >     Or I just check which patch includes the code I changed, and add
> > the fix tag?
> 
> You can use Fixes tag even though there was no software bug. The point of the
> tag is to help locate how far we can backport this patch in order to let stable
> kernel users benefit.
> 
> regards,
> Nicolas
> 

Hi Nicolas,
    Thanks for your information, I'll add a fix tag in v3.
Ming.

> >
> > Ming
diff mbox series

Patch

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 734e1b65fbc7..c0fd030d0f19 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -519,6 +519,7 @@  static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg,
 				     GFP_ATOMIC);
 	if (!cfg_stm)
 		goto err;
+	memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
 	jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
 
 skip_alloc:
@@ -755,7 +756,13 @@  static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
 					      u32 fourcc,
 					      u16 w, u16 h)
 {
-	unsigned int offset = 0;
+	/*
+	 * There is a hardware issue that first 128 bytes of configuration data
+	 * can't be loaded correctly.
+	 * To avoid this issue, we need to write the configuration from
+	 * an offset which should be no less than 0x80 (128 bytes).
+	 */
+	unsigned int offset = 0x80;
 	u8 *cfg = (u8 *)cfg_stream_vaddr;
 	struct mxc_jpeg_sof *sof;
 	struct mxc_jpeg_sos *sos;