diff mbox series

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

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

Commit Message

Ming Qian May 27, 2022, 7:54 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>
---
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Tommaso Merciai May 27, 2022, 9:38 a.m. UTC | #1 
Hi Ming,
I think have some comments on the code for this would be nice for the
future

On Fri, May 27, 2022 at 03:54:35PM +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,

From what I've understood you initialize cfg_stm with zeros then
you start to write the configuration from 0x80 (128 bytes), avoiding the hw issue right?

> 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>
> ---
>  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
>  1 file changed, 2 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..ad4213e020f3 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,7 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
>  					      u32 fourcc,
>  					      u16 w, u16 h)
>  {
> -	unsigned int offset = 0;
> +	unsigned int offset = 0x80;
>  	u8 *cfg = (u8 *)cfg_stream_vaddr;
>  	struct mxc_jpeg_sof *sof;
>  	struct mxc_jpeg_sos *sos;
> -- 
> 2.36.1
> 

Thanks,
Tommaso
Ming Qian May 27, 2022, 9:52 a.m. UTC | #2 
> From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Sent: 2022年5月27日 17:38
> To: Ming Qian <ming.qian@nxp.com>
> Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the
> configuration data
> 
> Caution: EXT Email
> 
> Hi Ming,
> I think have some comments on the code for this would be nice for the future
> 
> On Fri, May 27, 2022 at 03:54:35PM +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,
> 
> From what I've understood you initialize cfg_stm with zeros then you start to
> write the configuration from 0x80 (128 bytes), avoiding the hw issue right?
> 

HI Tommaso,
    Yes, you're right, there is a hardware bug.
I want to write the configuration data from the offset 0x80,
And set the first 128 bytes to zero.
Then the hardware bug can be avoided.

Ming.


> > 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>
> > ---
> >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
> >  1 file changed, 2 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..ad4213e020f3 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,7 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void
> *cfg_stream_vaddr,
> >                                             u32 fourcc,
> >                                             u16 w, u16 h)  {
> > -     unsigned int offset = 0;
> > +     unsigned int offset = 0x80;
> >       u8 *cfg = (u8 *)cfg_stream_vaddr;
> >       struct mxc_jpeg_sof *sof;
> >       struct mxc_jpeg_sos *sos;
> > --
> > 2.36.1
> >
> 
> Thanks,
> Tommaso
> 
> --
> Tommaso Merciai
> Embedded Linux Engineer
> tommaso.merciai@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> info@amarulasolutions.com
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.am
> arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C1
> d15c3ca5ba248ae53bc08da3fc4a75b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C637892411093606090%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> C3000%7C%7C%7C&amp;sdata=2%2FZlmaidIXmpurQBNW56roQgaWnY7IElP
> OJzhFaZlow%3D&amp;reserved=0
Tommaso Merciai May 27, 2022, 10:05 a.m. UTC | #3 
On Fri, May 27, 2022 at 09:52:48AM +0000, Ming Qian wrote:
> > From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > Sent: 2022年5月27日 17:38
> > To: Ming Qian <ming.qian@nxp.com>
> > Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> > <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the
> > configuration data
> > 
> > Caution: EXT Email
> > 
> > Hi Ming,
> > I think have some comments on the code for this would be nice for the future
> > 
> > On Fri, May 27, 2022 at 03:54:35PM +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,
> > 
> > From what I've understood you initialize cfg_stm with zeros then you start to
> > write the configuration from 0x80 (128 bytes), avoiding the hw issue right?
> > 
> 

Hi Ming,

> HI Tommaso,
>     Yes, you're right, there is a hardware bug.
> I want to write the configuration data from the offset 0x80,
> And set the first 128 bytes to zero.
> Then the hardware bug can be avoided.

Thanks for your clarification!
Unfortunately I can't test this on a real board but the implementation looks
good to me. Only thing is missing I think we need some notes to keep
track of this known issue (for the sake of clarity just a note on why we
start to write from 0x80)

Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>

Thanks,
Tommaso

> 
> Ming.
> 
> 
> > > 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>
> > > ---
> > >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
> > >  1 file changed, 2 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..ad4213e020f3 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,7 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void
> > *cfg_stream_vaddr,
> > >                                             u32 fourcc,
> > >                                             u16 w, u16 h)  {
> > > -     unsigned int offset = 0;
> > > +     unsigned int offset = 0x80;
> > >       u8 *cfg = (u8 *)cfg_stream_vaddr;
> > >       struct mxc_jpeg_sof *sof;
> > >       struct mxc_jpeg_sos *sos;
> > > --
> > > 2.36.1
> > >
> > 
> > Thanks,
> > Tommaso
> > 
> > --
> > Tommaso Merciai
> > Embedded Linux Engineer
> > tommaso.merciai@amarulasolutions.com
> > __________________________________
> > 
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> > info@amarulasolutions.com
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.am
> > arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C1
> > d15c3ca5ba248ae53bc08da3fc4a75b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > 35%7C0%7C0%7C637892411093606090%7CUnknown%7CTWFpbGZsb3d8eyJ
> > WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> > C3000%7C%7C%7C&amp;sdata=2%2FZlmaidIXmpurQBNW56roQgaWnY7IElP
> > OJzhFaZlow%3D&amp;reserved=0
Ming Qian May 27, 2022, 10:15 a.m. UTC | #4 
> From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Sent: 2022年5月27日 18:06
> To: Ming Qian <ming.qian@nxp.com>
> Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space before the
> configuration data
> 
> Caution: EXT Email
> 
> On Fri, May 27, 2022 at 09:52:48AM +0000, Ming Qian wrote:
> > > From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > > Sent: 2022年5月27日 17:38
> > > To: Ming Qian <ming.qian@nxp.com>
> > > Cc: mchehab@kernel.org; Mirela Rabulea (OSS)
> > > <mirela.rabulea@oss.nxp.com>; hverkuil-cisco@xs4all.nl;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > > Subject: [EXT] Re: [PATCH] media: imx-jpeg: Leave a blank space
> > > before the configuration data
> > >
> > > Caution: EXT Email
> > >
> > > Hi Ming,
> > > I think have some comments on the code for this would be nice for
> > > the future
> > >
> > > On Fri, May 27, 2022 at 03:54:35PM +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,
> > >
> > > From what I've understood you initialize cfg_stm with zeros then you
> > > start to write the configuration from 0x80 (128 bytes), avoiding the hw
> issue right?
> > >
> >
> 
> Hi Ming,
> 
> > HI Tommaso,
> >     Yes, you're right, there is a hardware bug.
> > I want to write the configuration data from the offset 0x80, And set
> > the first 128 bytes to zero.
> > Then the hardware bug can be avoided.
> 
> Thanks for your clarification!
> Unfortunately I can't test this on a real board but the implementation looks
> good to me. Only thing is missing I think we need some notes to keep track of
> this known issue (for the sake of clarity just a note on why we start to write
> from 0x80)
> 
> Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> 
> Thanks,
> Tommaso
> 

I'll add some comments in code, thanks for your suggestion

> >
> > Ming.
> >
> >
> > > > 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>
> > > > ---
> > > >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++-
> > > >  1 file changed, 2 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..ad4213e020f3 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,7 @@ static unsigned int
> > > > mxc_jpeg_setup_cfg_stream(void
> > > *cfg_stream_vaddr,
> > > >                                             u32 fourcc,
> > > >                                             u16 w, u16 h)  {
> > > > -     unsigned int offset = 0;
> > > > +     unsigned int offset = 0x80;
> > > >       u8 *cfg = (u8 *)cfg_stream_vaddr;
> > > >       struct mxc_jpeg_sof *sof;
> > > >       struct mxc_jpeg_sos *sos;
> > > > --
> > > > 2.36.1
> > > >
> > >
> > > Thanks,
> > > Tommaso
> > >
> > > --
> > > Tommaso Merciai
> > > Embedded Linux Engineer
> > > tommaso.merciai@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions SRL
> > > Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> > > info@amarulasolutions.com
> > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> > > .am%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C75180f2a39b
> b437bb0b6
> > >
> 08da3fc87e42%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6378
> 924275
> > >
> 86457594%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzI
> > >
> iLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=PDj76
> PwhSl
> > > cOyexz%2BR7ljkwx%2FEG7YTB%2BffIbz8atgFs%3D&amp;reserved=0
> > >
> arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C1
> > >
> d15c3ca5ba248ae53bc08da3fc4a75b%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > >
> 35%7C0%7C0%7C637892411093606090%7CUnknown%7CTWFpbGZsb3d8eyJ
> > >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> > >
> C3000%7C%7C%7C&amp;sdata=2%2FZlmaidIXmpurQBNW56roQgaWnY7IElP
> > > OJzhFaZlow%3D&amp;reserved=0
> 
> --
> Tommaso Merciai
> Embedded Linux Engineer
> tommaso.merciai@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310
> info@amarulasolutions.com
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.am
> arulasolutions.com%2F&amp;data=05%7C01%7Cming.qian%40nxp.com%7C7
> 5180f2a39bb437bb0b608da3fc87e42%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C637892427586457594%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&amp;sdata=NW%2FGyXSBrpkfzW%2Fqdh4q9JyZ8rTX8x2
> M7r%2FMJiRH7tQ%3D&amp;reserved=0
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..ad4213e020f3 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,7 @@  static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
 					      u32 fourcc,
 					      u16 w, u16 h)
 {
-	unsigned int offset = 0;
+	unsigned int offset = 0x80;
 	u8 *cfg = (u8 *)cfg_stream_vaddr;
 	struct mxc_jpeg_sof *sof;
 	struct mxc_jpeg_sos *sos;