| Message ID | 1529528155-26852-1-git-send-email-michael@amarulasolutions.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [U-Boot] video: mxsfb: Support data-enable and pixclock polarity | expand |
Hi Michael, On Wed, 20 Jun 2018 22:55:55 +0200 Michael Trimarchi michael@amarulasolutions.com wrote: > Add extra feature to support data-enable and clock-polarity > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > --- > drivers/video/mxsfb.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c > index 02fde05..4e3e3d7 100644 > --- a/drivers/video/mxsfb.c > +++ b/drivers/video/mxsfb.c > @@ -20,6 +20,9 @@ > > #define PS2KHZ(ps) (1000000000UL / (ps)) > > +#define FB_SYNC_DATA_ENABLE_HIGH_ACT 0x80000000 > +#define FB_SYNC_DOTCLK_FALLING_ACT 0x40000000 shouldn't these be defined in drivers/video/videomodes.h? I assume that some external code will set these flags in the mode struct, so these must be known (and included) elsewhere. > @@ -50,7 +53,7 @@ static void mxs_lcd_init(GraphicDevice *panel, > struct ctfb_res_modes *mode, int bpp) > { > struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE; > - uint32_t word_len = 0, bus_width = 0; > + uint32_t word_len = 0, bus_width = 0, flags = 0; > uint8_t valid_data = 0; > > /* Kick in the LCDIF clock */ > @@ -94,10 +97,22 @@ static void mxs_lcd_init(GraphicDevice *panel, > writel((mode->yres << LCDIF_TRANSFER_COUNT_V_COUNT_OFFSET) | mode->xres, > ®s->hw_lcdif_transfer_count); > > - writel(LCDIF_VDCTRL0_ENABLE_PRESENT | LCDIF_VDCTRL0_ENABLE_POL | > + if (mode->sync & FB_SYNC_HOR_HIGH_ACT) > + flags |= LCDIF_VDCTRL0_VSYNC_POL; you check for HSYNC high flag, but set VSYNC bit? > + if (mode->sync & FB_SYNC_VERT_HIGH_ACT) > + flags |= LCDIF_VDCTRL0_HSYNC_POL; you check for VSYNC high flag, but set HSYNC bit. Please fix. > + > + if (mode->sync & FB_SYNC_DATA_ENABLE_HIGH_ACT) > + flags |= LCDIF_VDCTRL0_ENABLE_POL; previously data enable polarity bit was always set in the ctrl register, now it will be only set if requested by new flag in mode sync. This is going to break current users, I'm afraid. Who is going to request this for all driver users? Will it be supplied via environment mode variable "video=ctfb..." ? This should be compatible with existing video mode environments then (which we cannot easily update everywhere). -- Anatolij
Hi On Thu, Jun 21, 2018 at 9:17 AM, Anatolij Gustschin <agust@denx.de> wrote: > Hi Michael, > > On Wed, 20 Jun 2018 22:55:55 +0200 > Michael Trimarchi michael@amarulasolutions.com wrote: > >> Add extra feature to support data-enable and clock-polarity >> >> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> >> --- >> drivers/video/mxsfb.c | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c >> index 02fde05..4e3e3d7 100644 >> --- a/drivers/video/mxsfb.c >> +++ b/drivers/video/mxsfb.c >> @@ -20,6 +20,9 @@ >> >> #define PS2KHZ(ps) (1000000000UL / (ps)) >> >> +#define FB_SYNC_DATA_ENABLE_HIGH_ACT 0x80000000 >> +#define FB_SYNC_DOTCLK_FALLING_ACT 0x40000000 > > shouldn't these be defined in drivers/video/videomodes.h? I assume > that some external code will set these flags in the mode struct, so > these must be known (and included) elsewhere. No, those are what we call host->flags and they can not land there. Those mapping is already used for those special host implementation and we can not diverge from linux. I found two different way to work with panel, video and display > >> @@ -50,7 +53,7 @@ static void mxs_lcd_init(GraphicDevice *panel, >> struct ctfb_res_modes *mode, int bpp) >> { >> struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE; >> - uint32_t word_len = 0, bus_width = 0; >> + uint32_t word_len = 0, bus_width = 0, flags = 0; >> uint8_t valid_data = 0; >> >> /* Kick in the LCDIF clock */ >> @@ -94,10 +97,22 @@ static void mxs_lcd_init(GraphicDevice *panel, >> writel((mode->yres << LCDIF_TRANSFER_COUNT_V_COUNT_OFFSET) | mode->xres, >> ®s->hw_lcdif_transfer_count); >> >> - writel(LCDIF_VDCTRL0_ENABLE_PRESENT | LCDIF_VDCTRL0_ENABLE_POL | >> + if (mode->sync & FB_SYNC_HOR_HIGH_ACT) >> + flags |= LCDIF_VDCTRL0_VSYNC_POL; > > you check for HSYNC high flag, but set VSYNC bit? > >> + if (mode->sync & FB_SYNC_VERT_HIGH_ACT) >> + flags |= LCDIF_VDCTRL0_HSYNC_POL; > > you check for VSYNC high flag, but set HSYNC bit. Please fix. > Yes right >> + >> + if (mode->sync & FB_SYNC_DATA_ENABLE_HIGH_ACT) >> + flags |= LCDIF_VDCTRL0_ENABLE_POL; > > previously data enable polarity bit was always set in the ctrl register, > now it will be only set if requested by new flag in mode sync. This > is going to break current users, I'm afraid. Who is going to request > this for all driver users? Will it be supplied via environment mode > variable "video=ctfb..." ? This should be compatible with existing > video mode environments then (which we cannot easily update everywhere). I have notice this one, but the previsius implementation was wrong. I'm planning to move this one in DM_VIDEO anyway later Michael > > -- > Anatolij
diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c index 02fde05..4e3e3d7 100644 --- a/drivers/video/mxsfb.c +++ b/drivers/video/mxsfb.c @@ -20,6 +20,9 @@ #define PS2KHZ(ps) (1000000000UL / (ps)) +#define FB_SYNC_DATA_ENABLE_HIGH_ACT 0x80000000 +#define FB_SYNC_DOTCLK_FALLING_ACT 0x40000000 + static GraphicDevice panel; struct mxs_dma_desc desc; @@ -50,7 +53,7 @@ static void mxs_lcd_init(GraphicDevice *panel, struct ctfb_res_modes *mode, int bpp) { struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE; - uint32_t word_len = 0, bus_width = 0; + uint32_t word_len = 0, bus_width = 0, flags = 0; uint8_t valid_data = 0; /* Kick in the LCDIF clock */ @@ -94,10 +97,22 @@ static void mxs_lcd_init(GraphicDevice *panel, writel((mode->yres << LCDIF_TRANSFER_COUNT_V_COUNT_OFFSET) | mode->xres, ®s->hw_lcdif_transfer_count); - writel(LCDIF_VDCTRL0_ENABLE_PRESENT | LCDIF_VDCTRL0_ENABLE_POL | + if (mode->sync & FB_SYNC_HOR_HIGH_ACT) + flags |= LCDIF_VDCTRL0_VSYNC_POL; + + if (mode->sync & FB_SYNC_VERT_HIGH_ACT) + flags |= LCDIF_VDCTRL0_HSYNC_POL; + + if (mode->sync & FB_SYNC_DATA_ENABLE_HIGH_ACT) + flags |= LCDIF_VDCTRL0_ENABLE_POL; + + if (mode->sync & FB_SYNC_DOTCLK_FALLING_ACT) + flags |= LCDIF_VDCTRL0_DOTCLK_POL; + + writel(LCDIF_VDCTRL0_ENABLE_PRESENT | LCDIF_VDCTRL0_VSYNC_PERIOD_UNIT | LCDIF_VDCTRL0_VSYNC_PULSE_WIDTH_UNIT | - mode->vsync_len, ®s->hw_lcdif_vdctrl0); + mode->vsync_len | flags, ®s->hw_lcdif_vdctrl0); writel(mode->upper_margin + mode->lower_margin + mode->vsync_len + mode->yres, ®s->hw_lcdif_vdctrl1);
Add extra feature to support data-enable and clock-polarity Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> --- drivers/video/mxsfb.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)