| Message ID | 1479801258-3765-1-git-send-email-martin@kaiser.cx (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)