Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1527284 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM Martin Kaiser <martin@kaiser.cx> - 2016-11-22 09:00 +0100
    Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2016-11-22 09:50 +0100
      Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM Martin Kaiser <martin@kaiser.cx> - 2016-11-23 10:40 +0100
        Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2016-11-23 11:00 +0100
          Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM Martin Kaiser <martin@kaiser.cx> - 2016-11-25 09:50 +0100

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

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

[toc] | [next] | [standalone]

#1527312

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/  |

[toc] | [prev] | [next] | [standalone]

#1528244

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

[toc] | [prev] | [next] | [standalone]

#1528265

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/  |

[toc] | [prev] | [next] | [standalone]

#1529948

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web