[PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM

[PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM

[Martin Kaiser]

The HM and TM fields in the LCDC DMA Control Register are 7 bits wide.
Use the correct mask to allow setting all possible bits.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---

This bug was discovered on a board that uses DMACR_TM(16). We ended up
with TM==0 in the register, the upper three bits were filtered out.

The LCD DMA Control Register is described in section 33.3.16 of the
IMX25 reference manual.

 include/linux/platform_data/video-imxfb.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/platform_data/video-imxfb.h b/include/linux/platform_data/video-imxfb.h
index 18e9083..858c66d 100644
--- a/include/linux/platform_data/video-imxfb.h
+++ b/include/linux/platform_data/video-imxfb.h
@@ -48,8 +48,8 @@
 #define LSCR1_GRAY1(x)            (((x) & 0xf))
 
 #define DMACR_BURST	(1 << 31)
-#define DMACR_HM(x)	(((x) & 0xf) << 16)
-#define DMACR_TM(x)	((x) & 0xf)
+#define DMACR_HM(x)	(((x) & 0x7f) << 16)
+#define DMACR_TM(x)	((x) & 0x7f)
 
 struct imx_fb_videomode {
 	struct fb_videomode mode;
-- 
1.7.10.4

Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM

[Uwe Kleine-König]

On Tue, Nov 22, 2016 at 08:54:18AM +0100, Martin Kaiser wrote:
> The HM and TM fields in the LCDC DMA Control Register are 7 bits wide.
> Use the correct mask to allow setting all possible bits.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> 
> This bug was discovered on a board that uses DMACR_TM(16). We ended up
> with TM==0 in the register, the upper three bits were filtered out.
> 
> The LCD DMA Control Register is described in section 33.3.16 of the
> IMX25 reference manual.

For the MX1 which is also supported by this driver, the definitions are
right. So this needs a more sophisticated patch. Also I wonder why the
register definition is in include/linux/platform_data and not in the
driver directly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM

[Martin Kaiser]

Hello Uwe, all,

Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):

> For the MX1 which is also supported by this driver, the definitions are
> right.

ok, understood. I wasn't able to dig up an imx1 specification. Do you
know if it's publicly available?

> So this needs a more sophisticated patch. Also I wonder why the
> register definition is in include/linux/platform_data and not in the
> driver directly.

The DMACR_HM() and _TM() macros are meant to be used when we initialize
imx_fb_platform_data's dmacr component for a platform device. It's not
straightforward to distinguish between imx1 and imx21 at initialization
time.

We could modify imx_fb_platform_data to use different components for
dmacr_burst, dmacr_hm, dmacr_tm and calculate the dmacr register value
in the driver where is_imx1_fb() is available. Device tree is also using
a single dmacr entry, it's probably not a good idea to do this
differently for platform devices...

We could also define DMACR_HM_IMX1(), DMACR_HM_IMX21(), ...

Or we could just remove the macros, they are not used by any boards in
the mainline kernel. If we don't want to break proprietary board
definitions, we could at least add a comment that the macros are
incorrect for imx21.

Thoughts?

Best regards,
Martin

Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM

[Uwe Kleine-König]

On Wed, Nov 23, 2016 at 10:31:13AM +0100, Martin Kaiser wrote:
> Hello Uwe, all,
> 
> Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):
> 
> > For the MX1 which is also supported by this driver, the definitions are
> > right.
> 
> ok, understood. I wasn't able to dig up an imx1 specification. Do you
> know if it's publicly available?

http://www.nxp.com/assets/documents/data/en/reference-manuals/MC9328MX1RM.pdf

> > So this needs a more sophisticated patch. Also I wonder why the
> > register definition is in include/linux/platform_data and not in the
> > driver directly.
> 
> The DMACR_HM() and _TM() macros are meant to be used when we initialize
> imx_fb_platform_data's dmacr component for a platform device. It's not
> straightforward to distinguish between imx1 and imx21 at initialization
> time.

So you put the values to use in the device tree? Then the right thing to
do is to check the device type in the driver and mask accordingly when
the values are written to the hardware.

> We could modify imx_fb_platform_data to use different components for
> dmacr_burst, dmacr_hm, dmacr_tm and calculate the dmacr register value
> in the driver where is_imx1_fb() is available. Device tree is also using
> a single dmacr entry, it's probably not a good idea to do this
> differently for platform devices...
> 
> We could also define DMACR_HM_IMX1(), DMACR_HM_IMX21(), ...
> 
> Or we could just remove the macros, they are not used by any boards in
> the mainline kernel. If we don't want to break proprietary board
> definitions, we could at least add a comment that the macros are
> incorrect for imx21.

IMHO dropping the macros is the right thing to do. Maybe even just
believing the dt and write this value into the register might be
acceptable.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM

[Martin Kaiser]

Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):

> > ok, understood. I wasn't able to dig up an imx1 specification. Do you
> > know if it's publicly available?

> http://www.nxp.com/assets/documents/data/en/reference-manuals/MC9328MX1RM.pdf

Thanks.

> So you put the values to use in the device tree? Then the right thing to
> do is to check the device type in the driver and mask accordingly when
> the values are written to the hardware.

Device tree and platform data contain the entire register, not the
individual components. The macros are provided to build the register
value from the components, but nobody's using them.

> IMHO dropping the macros is the right thing to do.

Ok, I'll submit a patch for this.

Best regards,
Martin

[PATCH] video: imxfb: remove the macros for initializing the DMACR

[Martin Kaiser]

The current definitions of DMACR_HM() and DMACR_TM() are correct only
for imx1, they're wrong for imx21.

The macros are meant for legacy board files only, they're not applicable
for boards using device tree.

At the moment, there are no boards are using the macros. So it should be
safe to drop them rather than making them work for both imx1 and imx21,
which would require a change in the platform data struct.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 include/linux/platform_data/video-imxfb.h |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/platform_data/video-imxfb.h b/include/linux/platform_data/video-imxfb.h
index 18e9083..a5c0a71 100644
--- a/include/linux/platform_data/video-imxfb.h
+++ b/include/linux/platform_data/video-imxfb.h
@@ -47,10 +47,6 @@
 #define LSCR1_GRAY2(x)            (((x) & 0xf) << 4)
 #define LSCR1_GRAY1(x)            (((x) & 0xf))
 
-#define DMACR_BURST	(1 << 31)
-#define DMACR_HM(x)	(((x) & 0xf) << 16)
-#define DMACR_TM(x)	((x) & 0xf)
-
 struct imx_fb_videomode {
 	struct fb_videomode mode;
 	u32 pcr;
-- 
1.7.10.4

Re: [PATCH] video: imxfb: remove the macros for initializing the DMACR

[Uwe Kleine-König]

On Mon, Nov 28, 2016 at 11:43:09PM +0100, Martin Kaiser wrote:
> The current definitions of DMACR_HM() and DMACR_TM() are correct only
> for imx1, they're wrong for imx21.
> 
> The macros are meant for legacy board files only, they're not applicable
> for boards using device tree.
> 
> At the moment, there are no boards are using the macros. So it should be
s/are using the/using these/

> safe to drop them rather than making them work for both imx1 and imx21,
> which would require a change in the platform data struct.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2] video: imxfb: remove the macros for initializing the DMACR

[Martin Kaiser]

The current definitions of DMACR_HM() and DMACR_TM() are correct only
for imx1, they're wrong for imx21.

The macros are meant for legacy board files only, they're not applicable
for boards using device tree.

At the moment, there are no boards using these macros. So it should be
safe to drop them rather than making them work for both imx1 and imx21,
which would require a change in the platform data struct.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
Change in v2:
    fixed the commit message

 include/linux/platform_data/video-imxfb.h |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/platform_data/video-imxfb.h b/include/linux/platform_data/video-imxfb.h
index 18e9083..a5c0a71 100644
--- a/include/linux/platform_data/video-imxfb.h
+++ b/include/linux/platform_data/video-imxfb.h
@@ -47,10 +47,6 @@
 #define LSCR1_GRAY2(x)            (((x) & 0xf) << 4)
 #define LSCR1_GRAY1(x)            (((x) & 0xf))
 
-#define DMACR_BURST	(1 << 31)
-#define DMACR_HM(x)	(((x) & 0xf) << 16)
-#define DMACR_TM(x)	((x) & 0xf)
-
 struct imx_fb_videomode {
 	struct fb_videomode mode;
 	u32 pcr;
-- 
1.7.10.4

Re: [PATCH v2] video: imxfb: remove the macros for initializing the DMACR

[Uwe Kleine-König]

On Tue, Nov 29, 2016 at 08:50:39PM +0100, Martin Kaiser wrote:
> The current definitions of DMACR_HM() and DMACR_TM() are correct only
> for imx1, they're wrong for imx21.
> 
> The macros are meant for legacy board files only, they're not applicable
> for boards using device tree.
> 
> At the moment, there are no boards using these macros. So it should be
> safe to drop them rather than making them work for both imx1 and imx21,
> which would require a change in the platform data struct.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2] video: imxfb: remove the macros for initializing the DMACR

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, November 29, 2016 08:57:21 PM Uwe Kleine-König wrote:
> On Tue, Nov 29, 2016 at 08:50:39PM +0100, Martin Kaiser wrote:
> > The current definitions of DMACR_HM() and DMACR_TM() are correct only
> > for imx1, they're wrong for imx21.
> > 
> > The macros are meant for legacy board files only, they're not applicable
> > for boards using device tree.
> > 
> > At the moment, there are no boards using these macros. So it should be
> > safe to drop them rather than making them work for both imx1 and imx21,
> > which would require a change in the platform data struct.
> > 
> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks, queued for 4.11.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics