| Message ID | 1356246228-26732-4-git-send-email-nikita@compulab.co.il |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hello Nikita, On 12/23/2012 08:03 AM, Nikita Kiryanov wrote: > Add support for dvi displays with user selectable dvi presets. > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> > --- > board/cm_t35/Makefile | 1 + > board/cm_t35/cm_t35.c | 3 + > board/cm_t35/display.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++ > include/configs/cm_t35.h | 6 ++ > 4 files changed, 223 insertions(+) > create mode 100644 board/cm_t35/display.c > > diff --git a/board/cm_t35/Makefile b/board/cm_t35/Makefile > index 894fa09..bde56e6 100644 > --- a/board/cm_t35/Makefile > +++ b/board/cm_t35/Makefile > @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk > LIB = $(obj)lib$(BOARD).o > > COBJS-$(CONFIG_DRIVER_OMAP34XX_I2C) += eeprom.o > +COBJS-$(CONFIG_LCD) += display.o > > COBJS := cm_t35.o leds.o $(COBJS-y) > > diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c > index edbb941..8f3d735 100644 > --- a/board/cm_t35/cm_t35.c > +++ b/board/cm_t35/cm_t35.c > @@ -216,6 +216,9 @@ static void cm_t3x_set_common_muxconf(void) > /* SB-T35 Ethernet */ > MUX_VAL(CP(GPMC_NCS4), (IEN | PTU | EN | M0)); /*GPMC_nCS4*/ > > + /* DVI enable */ > + MUX_VAL(CP(GPMC_NCS3), (IDIS | PTU | DIS | M4));/*GPMC_nCS3*/ > + > /* CM-T3x Ethernet */ > MUX_VAL(CP(GPMC_NCS5), (IDIS | PTU | DIS | M0)); /*GPMC_nCS5*/ > MUX_VAL(CP(GPMC_CLK), (IEN | PTD | DIS | M4)); /*GPIO_59*/ > diff --git a/board/cm_t35/display.c b/board/cm_t35/display.c > new file mode 100644 > index 0000000..11b8ed9 > --- /dev/null > +++ b/board/cm_t35/display.c > @@ -0,0 +1,213 @@ > +/* > + * (C) Copyright 2012 CompuLab, Ltd. <www.compulab.co.il> > + * > + * Authors: Nikita Kiryanov <nikita@compulab.co.il> > + * > + * See file CREDITS for list of people who contributed to this > + * project. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc. > + */ > +#include <common.h> > +#include <asm/gpio.h> > +#include <asm/io.h> > +#include <stdio_dev.h> > +#include <asm/arch/dss.h> > +#include <lcd.h> > +#include <asm/arch-omap3/dss.h> > + > +DECLARE_GLOBAL_DATA_PTR; > + > +enum display_type { > + NONE, > + DVI, > +}; > + > +/* > + * The frame buffer is allocated before we have the chance to parse user input. > + * To make sure enough memory is allocated for all resolutions, we define > + * vl_{col | row} to the maximal resolution supported by OMAP3. > + */ > +vidinfo_t panel_info = { > + .vl_col = 1400, > + .vl_row = 1050, > + .vl_bpix = 4, /* 16-bits pixel data */ There is no need to hardcode the magic 4, just use LCD_BPP > + .cmap = (ushort *)0x80100000, > +}; > + > +static struct panel_config panel_cfg; > +static enum display_type lcd_def; > + > +static const struct panel_config preset_dvi_640X480 = { > + .lcd_size = PANEL_LCD_SIZE(640, 480), > + .timing_h = DSS_HBP(48) | DSS_HFP(16) | DSS_HSW(96), > + .timing_v = DSS_VBP(33) | DSS_VFP(10) | DSS_VSW(2), > + .divisor = 12 | (1 << 16), > + .data_lines = LCD_INTERFACE_24_BIT, > + .panel_type = ACTIVE_DISPLAY, > + .load_mode = 2, > +}; > + > +static const struct panel_config preset_dvi_800X600 = { > + .lcd_size = PANEL_LCD_SIZE(800, 600), > + .timing_h = DSS_HBP(88) | DSS_HFP(40) | DSS_HSW(128), > + .timing_v = DSS_VBP(23) | DSS_VFP(1) | DSS_VSW(4), > + .divisor = 8 | (1 << 16), > + .data_lines = LCD_INTERFACE_24_BIT, > + .panel_type = ACTIVE_DISPLAY, > + .load_mode = 2, > +}; > + > +static const struct panel_config preset_dvi_1024X768 = { > + .lcd_size = PANEL_LCD_SIZE(1024, 768), > + .timing_h = DSS_HBP(160) | DSS_HFP(24) | DSS_HSW(136), > + .timing_v = DSS_VBP(29) | DSS_VFP(3) | DSS_VSW(6), > + .divisor = 5 | (1 << 16), > + .data_lines = LCD_INTERFACE_24_BIT, > + .panel_type = ACTIVE_DISPLAY, > + .load_mode = 2, > +}; > + > +static const struct panel_config preset_dvi_1152X864 = { > + .lcd_size = PANEL_LCD_SIZE(1152, 864), > + .timing_h = DSS_HBP(256) | DSS_HFP(64) | DSS_HSW(128), > + .timing_v = DSS_VBP(32) | DSS_VFP(1) | DSS_VSW(3), > + .divisor = 3 | (1 << 16), > + .data_lines = LCD_INTERFACE_24_BIT, > + .panel_type = ACTIVE_DISPLAY, > + .load_mode = 2, > +}; > + > +static const struct panel_config preset_dvi_1280X960 = { > + .lcd_size = PANEL_LCD_SIZE(1280, 960), > + .timing_h = DSS_HBP(312) | DSS_HFP(96) | DSS_HSW(112), > + .timing_v = DSS_VBP(36) | DSS_VFP(1) | DSS_VSW(3), > + .divisor = 3 | (1 << 16), > + .data_lines = LCD_INTERFACE_24_BIT, > + .panel_type = ACTIVE_DISPLAY, > + .load_mode = 2, > +}; > + > +static const struct panel_config preset_dvi_1280X1024 = { > + .lcd_size = PANEL_LCD_SIZE(1280, 1024), > + .timing_h = DSS_HBP(248) | DSS_HFP(48) | DSS_HSW(112), > + .timing_v = DSS_VBP(38) | DSS_VFP(1) | DSS_VSW(3), > + .divisor = 3 | (1 << 16), > + .data_lines = LCD_INTERFACE_24_BIT, > + .panel_type = ACTIVE_DISPLAY, > + .load_mode = 2, > +}; > + > +/* > + * set_resolution_params() > + * > + * Due to usage of multiple display related APIs resolution data is located in > + * more than one place. This function updates them all. > + */ > +static void set_resolution_params(int x, int y) > +{ > + panel_cfg.lcd_size = PANEL_LCD_SIZE(x, y); > + panel_info.vl_col = x; > + panel_info.vl_row = y; > + lcd_line_length = (panel_info.vl_col * NBITS(panel_info.vl_bpix)) / 8; > +} > + > +static void set_preset(const struct panel_config preset, int x_res, int y_res) > +{ > + panel_cfg = preset; > + set_resolution_params(x_res, y_res); > +} > + > +static enum display_type set_dvi_preset(const struct panel_config preset, > + int x_res, int y_res) > +{ > + set_preset(preset, x_res, y_res); > + return DVI; > +} > + > +/* > + * env_parse_displaytype() - parse display type. > + * > + * Parses the environment variable "displaytype", which contains the > + * name of the display type or preset, in which case it applies its > + * configurations. > + * > + * Returns the type of display that was specified. > + */ > +static enum display_type env_parse_displaytype(char *displaytype) > +{ > + if (!strncmp(displaytype, "dvi640x480", 10)) > + return set_dvi_preset(preset_dvi_640X480, 640, 480); > + else if (!strncmp(displaytype, "dvi800x600", 10)) > + return set_dvi_preset(preset_dvi_800X600, 800, 600); > + else if (!strncmp(displaytype, "dvi1024x768", 11)) > + return set_dvi_preset(preset_dvi_1024X768, 1024, 768); > + else if (!strncmp(displaytype, "dvi1152x864", 11)) > + return set_dvi_preset(preset_dvi_1152X864, 1152, 864); > + else if (!strncmp(displaytype, "dvi1280x960", 11)) > + return set_dvi_preset(preset_dvi_1280X960, 1280, 960); > + else if (!strncmp(displaytype, "dvi1280x1024", 12)) > + return set_dvi_preset(preset_dvi_1280X1024, 1280, 1024); > + > + return NONE; > +} I think the lcd_line_length is no longer needed. ( but I haven't checked) Wondering if this should be board specific.. > + > +int lcd_line_length; > +int lcd_color_fg; > +int lcd_color_bg; > +void *lcd_base; > +short console_col; > +short console_row; > +void *lcd_console_address; > + fyi, I try to get rid of those, see http://patchwork.ozlabs.org/patch/211562/. Will repost v2 after this... No idea how this gets merged, but you might end with some unused globals. > +void lcd_ctrl_init(void *lcdbase) > +{ > + struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE; > + struct prcm *prcm = (struct prcm *)PRCM_BASE; > + char *displaytype = getenv("displaytype"); > + > + if (displaytype == NULL) > + return; > + > + lcd_def = env_parse_displaytype(displaytype); > + if (lcd_def == NONE) > + return; > + > + panel_cfg.frame_buffer = lcdbase; > + omap3_dss_panel_config(&panel_cfg); > + /* > + * Pixel clock is defined with many divisions and only few > + * multiplications of the system clock. Since DSS FCLK divisor is set > + * to 16 by default, we need to set it to a smaller value, like 3 > + * (chosen via trial and error). > + */ bah, guessing timer values, this might help you https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c (a bit old, but simply downloading the file should work, and yes it might warn a bit) The whole routine should go to the video driver in my opinion. > + clrsetbits_le32(&prcm->clksel_dss, 0xF, 3); > + /* > + * Some of what the omap3_dss_panel_config() function did, needs to be > + * adjusted, otherwise the image will be messed up/not appear at all. > + */ > + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16 << 1); > + clrsetbits_le32(&dispc->gfx_attributes, 0x3 << 6, GFXBURSTSIZE16 << 6); > +} mmm, do you really need 16 bit support? lcd.c is easily extended to 32 bit support (I have a patch for it) and then you can drop the driver adjustment. Anyway, if you want 16 bit support it should go into the driver and not in your board in my opinion. > +void lcd_enable(void) > +{ > + if (lcd_def == DVI) { > + gpio_direction_output(54, 0); /* Turn on DVI */ > + omap3_dss_enable(); > + } > +} > + > +void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort blue) {} > diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h > index fa6eb4e..8544b15 100644 > --- a/include/configs/cm_t35.h > +++ b/include/configs/cm_t35.h > @@ -339,4 +339,10 @@ > #define CONFIG_OMAP3_GPIO_6 /* GPIO186 is in GPIO bank 6 */ > #endif > > +/* Display Configuration */ > +#define CONFIG_OMAP3_GPIO_2 > +#define CONFIG_VIDEO_OMAP3 > + > +#define CONFIG_LCD > + > #endif /* __CONFIG_H */ Regards, Jeroen
Hi Jeroen, On 01/20/2013 10:59 PM, Jeroen Hofstee wrote: > Hello Nikita, > > On 12/23/2012 08:03 AM, Nikita Kiryanov wrote: >> Add support for dvi displays with user selectable dvi presets. >> >> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> >> --- [...] >> + >> +/* >> + * The frame buffer is allocated before we have the chance to parse >> user input. >> + * To make sure enough memory is allocated for all resolutions, we >> define >> + * vl_{col | row} to the maximal resolution supported by OMAP3. >> + */ >> +vidinfo_t panel_info = { >> + .vl_col = 1400, >> + .vl_row = 1050, >> + .vl_bpix = 4, /* 16-bits pixel data */ > There is no need to hardcode the magic 4, just use LCD_BPP Good point. >> + .cmap = (ushort *)0x80100000, >> +}; >> + >> +static struct panel_config panel_cfg; >> +static enum display_type lcd_def; >> + >> +static const struct panel_config preset_dvi_640X480 = { >> + .lcd_size = PANEL_LCD_SIZE(640, 480), >> + .timing_h = DSS_HBP(48) | DSS_HFP(16) | DSS_HSW(96), >> + .timing_v = DSS_VBP(33) | DSS_VFP(10) | DSS_VSW(2), >> + .divisor = 12 | (1 << 16), >> + .data_lines = LCD_INTERFACE_24_BIT, >> + .panel_type = ACTIVE_DISPLAY, >> + .load_mode = 2, >> +}; >> + [...] >> + else if (!strncmp(displaytype, "dvi800x600", 10)) >> + return set_dvi_preset(preset_dvi_800X600, 800, 600); >> + else if (!strncmp(displaytype, "dvi1024x768", 11)) >> + return set_dvi_preset(preset_dvi_1024X768, 1024, 768); >> + else if (!strncmp(displaytype, "dvi1152x864", 11)) >> + return set_dvi_preset(preset_dvi_1152X864, 1152, 864); >> + else if (!strncmp(displaytype, "dvi1280x960", 11)) >> + return set_dvi_preset(preset_dvi_1280X960, 1280, 960); >> + else if (!strncmp(displaytype, "dvi1280x1024", 12)) >> + return set_dvi_preset(preset_dvi_1280X1024, 1280, 1024); >> + >> + return NONE; >> +} > I think the lcd_line_length is no longer needed. ( but I haven't checked) > Wondering if this should be board specific.. These variables are here because at the time the implementation of lcd.c required them to be defined by the board. If you succeed in removing them it will be a simple matter of a clean up patch. >> + >> +int lcd_line_length; >> +int lcd_color_fg; >> +int lcd_color_bg; >> +void *lcd_base; >> +short console_col; >> +short console_row; >> +void *lcd_console_address; >> + > fyi, I try to get rid of those, see > http://patchwork.ozlabs.org/patch/211562/. > Will repost v2 after this... No idea how this gets merged, but you might > end > with some unused globals. Yes that should be the extent of the "damage". >> +void lcd_ctrl_init(void *lcdbase) >> +{ >> + struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE; >> + struct prcm *prcm = (struct prcm *)PRCM_BASE; >> + char *displaytype = getenv("displaytype"); >> + >> + if (displaytype == NULL) >> + return; >> + >> + lcd_def = env_parse_displaytype(displaytype); >> + if (lcd_def == NONE) >> + return; >> + >> + panel_cfg.frame_buffer = lcdbase; >> + omap3_dss_panel_config(&panel_cfg); >> + /* >> + * Pixel clock is defined with many divisions and only few >> + * multiplications of the system clock. Since DSS FCLK divisor is >> set >> + * to 16 by default, we need to set it to a smaller value, like 3 >> + * (chosen via trial and error). >> + */ > bah, guessing timer values, this might help you > https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c > > (a bit old, but simply downloading the file should work, and yes it > might warn a bit) Thanks. I'll consider it for future extensions to this code, but for the time being the guess serves its purpose. > > The whole routine should go to the video driver in my opinion. What this function is, is predefines + call to omap3_dss_panel_config() + some corrections. Anything related to the predefines is not generic. They have many assumptions in them (such as whether the screen is active or passive) and they are selected using a user interface that is also specific to our board. The adjustment after the call to omap3_dss_panel_config() is, once again, specific to our own choices. So overall, I don't think this is fit to be a generic driver function. >> + clrsetbits_le32(&prcm->clksel_dss, 0xF, 3); >> + /* >> + * Some of what the omap3_dss_panel_config() function did, needs >> to be >> + * adjusted, otherwise the image will be messed up/not appear at >> all. >> + */ >> + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16 >> << 1); >> + clrsetbits_le32(&dispc->gfx_attributes, 0x3 << 6, GFXBURSTSIZE16 >> << 6); >> +} > mmm, do you really need 16 bit support? Yes, we do want 16 bit support. > lcd.c is easily extended > to 32 bit support (I have a patch for it) and then you can drop the driver > adjustment. Anyway, if you want 16 bit support it should go into the driver > and not in your board in my opinion. Addressed above. >> +void lcd_enable(void) >> +{ >> + if (lcd_def == DVI) { >> + gpio_direction_output(54, 0); /* Turn on DVI */ >> + omap3_dss_enable(); >> + } >> +} >> + >> +void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort >> blue) {} >> diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h >> index fa6eb4e..8544b15 100644 >> --- a/include/configs/cm_t35.h >> +++ b/include/configs/cm_t35.h >> @@ -339,4 +339,10 @@ >> #define CONFIG_OMAP3_GPIO_6 /* GPIO186 is in GPIO bank 6 */ >> #endif >> +/* Display Configuration */ >> +#define CONFIG_OMAP3_GPIO_2 >> +#define CONFIG_VIDEO_OMAP3 >> + >> +#define CONFIG_LCD >> + >> #endif /* __CONFIG_H */ > Regards, > Jeroen >
On 01/21/2013 09:12 AM, Nikita Kiryanov wrote: > >>> + else if (!strncmp(displaytype, "dvi800x600", 10)) >>> + return set_dvi_preset(preset_dvi_800X600, 800, 600); >>> + else if (!strncmp(displaytype, "dvi1024x768", 11)) >>> + return set_dvi_preset(preset_dvi_1024X768, 1024, 768); >>> + else if (!strncmp(displaytype, "dvi1152x864", 11)) >>> + return set_dvi_preset(preset_dvi_1152X864, 1152, 864); >>> + else if (!strncmp(displaytype, "dvi1280x960", 11)) >>> + return set_dvi_preset(preset_dvi_1280X960, 1280, 960); >>> + else if (!strncmp(displaytype, "dvi1280x1024", 12)) >>> + return set_dvi_preset(preset_dvi_1280X1024, 1280, 1024); >>> + >>> + return NONE; >>> +} >> I think the lcd_line_length is no longer needed. ( but I haven't >> checked) >> Wondering if this should be board specific.. > > These variables are here because at the time the implementation of > lcd.c required them to be defined by the board. If you succeed in > removing them it will be a simple matter of a clean up patch. What I meant is that Stephan Warren posted a patch to fix the lcd_line_length update in general, see http://patchwork.ozlabs.org/patch/212378/. (Which I thought was picked up, but isn't yet), instead of fixing it in boards, like you do. > >>> +void lcd_ctrl_init(void *lcdbase) >>> +{ >>> + struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE; >>> + struct prcm *prcm = (struct prcm *)PRCM_BASE; >>> + char *displaytype = getenv("displaytype"); >>> + >>> + if (displaytype == NULL) >>> + return; >>> + >>> + lcd_def = env_parse_displaytype(displaytype); >>> + if (lcd_def == NONE) >>> + return; >>> + >>> + panel_cfg.frame_buffer = lcdbase; >>> + omap3_dss_panel_config(&panel_cfg); >>> + /* >>> + * Pixel clock is defined with many divisions and only few >>> + * multiplications of the system clock. Since DSS FCLK divisor is >>> set >>> + * to 16 by default, we need to set it to a smaller value, like 3 >>> + * (chosen via trial and error). >>> + */ >> bah, guessing timer values, this might help you >> https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c >> >> >> (a bit old, but simply downloading the file should work, and yes it >> might warn a bit) > > Thanks. > I'll consider it for future extensions to this code, but for the time > being the guess serves its purpose. > What purpose does it serve exactly? The link I mentioned is not to update the code per definition, but to be a bit more explicit why the timer adjustments are needed instead of "this seems to work.." >> >> The whole routine should go to the video driver in my opinion. > > What this function is, is predefines + call to omap3_dss_panel_config() > + some corrections. > Anything related to the predefines is not generic. They have many > assumptions in them (such as whether the screen is active or passive) > and they are selected using a user interface that is also specific to > our board. > > The adjustment after the call to omap3_dss_panel_config() is, once > again, specific to our own choices. > > So overall, I don't think this is fit to be a generic driver function. > The idea would be that the driver could optionally check the env for a display configuration. If not found or not enabled call board_video_init. So there is a single driver for video and lcd and configurable by the environment and you can also have all control of it in the board. I don't have the time currently to check if this would actually work, but I don't see a reason why not (perhaps I can check something during the weekend). And I didn't mean the predefined lcd configs. I am fine with you having them in you board / tested to work / delivered with them etc. But you add custom omap frame buffers settings, set by the user and I don't think that part should go into a board, since it is at least common to omap(3/4?) (or at least should be in my opinion). > >>> + clrsetbits_le32(&prcm->clksel_dss, 0xF, 3); >>> + /* >>> + * Some of what the omap3_dss_panel_config() function did, needs >>> to be >>> + * adjusted, otherwise the image will be messed up/not appear at >>> all. >>> + */ >>> + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16 >>> << 1); >>> + clrsetbits_le32(&dispc->gfx_attributes, 0x3 << 6, GFXBURSTSIZE16 >>> << 6); >>> +} >> mmm, do you really need 16 bit support? > > Yes, we do want 16 bit support. Why? > >> lcd.c is easily extended >> to 32 bit support (I have a patch for it) and then you can drop the >> driver >> adjustment. Anyway, if you want 16 bit support it should go into the >> driver >> and not in your board in my opinion. > > Addressed above. well the same why... you drive them as 24 bit panels, why do you want a lower resolution in software? Regards, Jeroen
On 01/23/13 23:39, Jeroen Hofstee wrote: > On 01/21/2013 09:12 AM, Nikita Kiryanov wrote: >> >>>> + else if (!strncmp(displaytype, "dvi800x600", 10)) >>>> + return set_dvi_preset(preset_dvi_800X600, 800, 600); >>>> + else if (!strncmp(displaytype, "dvi1024x768", 11)) >>>> + return set_dvi_preset(preset_dvi_1024X768, 1024, 768); >>>> + else if (!strncmp(displaytype, "dvi1152x864", 11)) >>>> + return set_dvi_preset(preset_dvi_1152X864, 1152, 864); >>>> + else if (!strncmp(displaytype, "dvi1280x960", 11)) >>>> + return set_dvi_preset(preset_dvi_1280X960, 1280, 960); >>>> + else if (!strncmp(displaytype, "dvi1280x1024", 12)) >>>> + return set_dvi_preset(preset_dvi_1280X1024, 1280, 1024); >>>> + >>>> + return NONE; >>>> +} >>> I think the lcd_line_length is no longer needed. ( but I haven't checked) >>> Wondering if this should be board specific.. >> >> These variables are here because at the time the implementation of >> lcd.c required them to be defined by the board. If you succeed in >> removing them it will be a simple matter of a clean up patch. > > What I meant is that Stephan Warren posted a patch to fix the lcd_line_length > update in general, see http://patchwork.ozlabs.org/patch/212378/. (Which I > thought was picked up, but isn't yet), instead of fixing it in boards, like you do. Unless. explicitly, requested by Anatolij, we should not wait for specific patches to be merged or not merged. This does not work like this. After improvements are made, the code can be adjusted - that's what the -rc cycle is for. > >> >>>> +void lcd_ctrl_init(void *lcdbase) >>>> +{ >>>> + struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE; >>>> + struct prcm *prcm = (struct prcm *)PRCM_BASE; >>>> + char *displaytype = getenv("displaytype"); >>>> + >>>> + if (displaytype == NULL) >>>> + return; >>>> + >>>> + lcd_def = env_parse_displaytype(displaytype); >>>> + if (lcd_def == NONE) >>>> + return; >>>> + >>>> + panel_cfg.frame_buffer = lcdbase; >>>> + omap3_dss_panel_config(&panel_cfg); >>>> + /* >>>> + * Pixel clock is defined with many divisions and only few >>>> + * multiplications of the system clock. Since DSS FCLK divisor is >>>> set >>>> + * to 16 by default, we need to set it to a smaller value, like 3 >>>> + * (chosen via trial and error). >>>> + */ >>> bah, guessing timer values, this might help you >>> https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c >>> >>> (a bit old, but simply downloading the file should work, and yes it >>> might warn a bit) >> >> Thanks. >> I'll consider it for future extensions to this code, but for the time >> being the guess serves its purpose. >> > What purpose does it serve exactly? The link I mentioned is not to update the > code per definition, but to be a bit more explicit why the timer adjustments are > needed instead of "this seems to work.." Thanks for the explanation, we will look into this. Currently, I don't see a good reason we should miss the merge window just to adopt those above. We will put it on our TODO list. Thanks! >>> >>> The whole routine should go to the video driver in my opinion. >> >> What this function is, is predefines + call to omap3_dss_panel_config() >> + some corrections. >> Anything related to the predefines is not generic. They have many >> assumptions in them (such as whether the screen is active or passive) >> and they are selected using a user interface that is also specific to >> our board. >> >> The adjustment after the call to omap3_dss_panel_config() is, once >> again, specific to our own choices. >> >> So overall, I don't think this is fit to be a generic driver function. >> > The idea would be that the driver could optionally check the env for a > display configuration. If not found or not enabled call board_video_init. > So there is a single driver for video and lcd and configurable by the > environment and you can also have all control of it in the board. I don't > have the time currently to check if this would actually work, but > I don't see a reason why not (perhaps I can check something during > the weekend). It should work, but first we should decide if it is suitable for all OMAP DSS users. So I would suggest we see how it works and if people decide this is good enough to be in the generic DSS code, we can move it to live there. > > And I didn't mean the predefined lcd configs. I am fine with you having > them in you board / tested to work / delivered with them etc. But you > add custom omap frame buffers settings, set by the user and I don't > think that part should go into a board, since it is at least common to > omap(3/4?) (or at least should be in my opinion). If I get you correctly, you are suggesting to parametrize the GFXFORMAT_RGB16 and GFXBURSTSIZE16? This can be done. >> >>>> + clrsetbits_le32(&prcm->clksel_dss, 0xF, 3); >>>> + /* >>>> + * Some of what the omap3_dss_panel_config() function did, needs >>>> to be >>>> + * adjusted, otherwise the image will be messed up/not appear at >>>> all. >>>> + */ >>>> + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16 >>>> << 1); >>>> + clrsetbits_le32(&dispc->gfx_attributes, 0x3 << 6, GFXBURSTSIZE16 >>>> << 6); >>>> +} >>> mmm, do you really need 16 bit support? >> >> Yes, we do want 16 bit support. > Why? >> >>> lcd.c is easily extended >>> to 32 bit support (I have a patch for it) and then you can drop the driver >>> adjustment. Anyway, if you want 16 bit support it should go into the driver >>> and not in your board in my opinion. >> >> Addressed above. > well the same why... you drive them as 24 bit panels, why do you want a lower > resolution in software? Actually, we also have 16 and 18 bit panels, just not in this patch set. We want the basic stuff to go in and then other stuff. But, you are right, probably parameterizing these or deriving from some other setting should be done. We will look into this.
diff --git a/board/cm_t35/Makefile b/board/cm_t35/Makefile index 894fa09..bde56e6 100644 --- a/board/cm_t35/Makefile +++ b/board/cm_t35/Makefile @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(BOARD).o COBJS-$(CONFIG_DRIVER_OMAP34XX_I2C) += eeprom.o +COBJS-$(CONFIG_LCD) += display.o COBJS := cm_t35.o leds.o $(COBJS-y) diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c index edbb941..8f3d735 100644 --- a/board/cm_t35/cm_t35.c +++ b/board/cm_t35/cm_t35.c @@ -216,6 +216,9 @@ static void cm_t3x_set_common_muxconf(void) /* SB-T35 Ethernet */ MUX_VAL(CP(GPMC_NCS4), (IEN | PTU | EN | M0)); /*GPMC_nCS4*/ + /* DVI enable */ + MUX_VAL(CP(GPMC_NCS3), (IDIS | PTU | DIS | M4));/*GPMC_nCS3*/ + /* CM-T3x Ethernet */ MUX_VAL(CP(GPMC_NCS5), (IDIS | PTU | DIS | M0)); /*GPMC_nCS5*/ MUX_VAL(CP(GPMC_CLK), (IEN | PTD | DIS | M4)); /*GPIO_59*/ diff --git a/board/cm_t35/display.c b/board/cm_t35/display.c new file mode 100644 index 0000000..11b8ed9 --- /dev/null +++ b/board/cm_t35/display.c @@ -0,0 +1,213 @@ +/* + * (C) Copyright 2012 CompuLab, Ltd. <www.compulab.co.il> + * + * Authors: Nikita Kiryanov <nikita@compulab.co.il> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc. + */ +#include <common.h> +#include <asm/gpio.h> +#include <asm/io.h> +#include <stdio_dev.h> +#include <asm/arch/dss.h> +#include <lcd.h> +#include <asm/arch-omap3/dss.h> + +DECLARE_GLOBAL_DATA_PTR; + +enum display_type { + NONE, + DVI, +}; + +/* + * The frame buffer is allocated before we have the chance to parse user input. + * To make sure enough memory is allocated for all resolutions, we define + * vl_{col | row} to the maximal resolution supported by OMAP3. + */ +vidinfo_t panel_info = { + .vl_col = 1400, + .vl_row = 1050, + .vl_bpix = 4, /* 16-bits pixel data */ + .cmap = (ushort *)0x80100000, +}; + +static struct panel_config panel_cfg; +static enum display_type lcd_def; + +static const struct panel_config preset_dvi_640X480 = { + .lcd_size = PANEL_LCD_SIZE(640, 480), + .timing_h = DSS_HBP(48) | DSS_HFP(16) | DSS_HSW(96), + .timing_v = DSS_VBP(33) | DSS_VFP(10) | DSS_VSW(2), + .divisor = 12 | (1 << 16), + .data_lines = LCD_INTERFACE_24_BIT, + .panel_type = ACTIVE_DISPLAY, + .load_mode = 2, +}; + +static const struct panel_config preset_dvi_800X600 = { + .lcd_size = PANEL_LCD_SIZE(800, 600), + .timing_h = DSS_HBP(88) | DSS_HFP(40) | DSS_HSW(128), + .timing_v = DSS_VBP(23) | DSS_VFP(1) | DSS_VSW(4), + .divisor = 8 | (1 << 16), + .data_lines = LCD_INTERFACE_24_BIT, + .panel_type = ACTIVE_DISPLAY, + .load_mode = 2, +}; + +static const struct panel_config preset_dvi_1024X768 = { + .lcd_size = PANEL_LCD_SIZE(1024, 768), + .timing_h = DSS_HBP(160) | DSS_HFP(24) | DSS_HSW(136), + .timing_v = DSS_VBP(29) | DSS_VFP(3) | DSS_VSW(6), + .divisor = 5 | (1 << 16), + .data_lines = LCD_INTERFACE_24_BIT, + .panel_type = ACTIVE_DISPLAY, + .load_mode = 2, +}; + +static const struct panel_config preset_dvi_1152X864 = { + .lcd_size = PANEL_LCD_SIZE(1152, 864), + .timing_h = DSS_HBP(256) | DSS_HFP(64) | DSS_HSW(128), + .timing_v = DSS_VBP(32) | DSS_VFP(1) | DSS_VSW(3), + .divisor = 3 | (1 << 16), + .data_lines = LCD_INTERFACE_24_BIT, + .panel_type = ACTIVE_DISPLAY, + .load_mode = 2, +}; + +static const struct panel_config preset_dvi_1280X960 = { + .lcd_size = PANEL_LCD_SIZE(1280, 960), + .timing_h = DSS_HBP(312) | DSS_HFP(96) | DSS_HSW(112), + .timing_v = DSS_VBP(36) | DSS_VFP(1) | DSS_VSW(3), + .divisor = 3 | (1 << 16), + .data_lines = LCD_INTERFACE_24_BIT, + .panel_type = ACTIVE_DISPLAY, + .load_mode = 2, +}; + +static const struct panel_config preset_dvi_1280X1024 = { + .lcd_size = PANEL_LCD_SIZE(1280, 1024), + .timing_h = DSS_HBP(248) | DSS_HFP(48) | DSS_HSW(112), + .timing_v = DSS_VBP(38) | DSS_VFP(1) | DSS_VSW(3), + .divisor = 3 | (1 << 16), + .data_lines = LCD_INTERFACE_24_BIT, + .panel_type = ACTIVE_DISPLAY, + .load_mode = 2, +}; + +/* + * set_resolution_params() + * + * Due to usage of multiple display related APIs resolution data is located in + * more than one place. This function updates them all. + */ +static void set_resolution_params(int x, int y) +{ + panel_cfg.lcd_size = PANEL_LCD_SIZE(x, y); + panel_info.vl_col = x; + panel_info.vl_row = y; + lcd_line_length = (panel_info.vl_col * NBITS(panel_info.vl_bpix)) / 8; +} + +static void set_preset(const struct panel_config preset, int x_res, int y_res) +{ + panel_cfg = preset; + set_resolution_params(x_res, y_res); +} + +static enum display_type set_dvi_preset(const struct panel_config preset, + int x_res, int y_res) +{ + set_preset(preset, x_res, y_res); + return DVI; +} + +/* + * env_parse_displaytype() - parse display type. + * + * Parses the environment variable "displaytype", which contains the + * name of the display type or preset, in which case it applies its + * configurations. + * + * Returns the type of display that was specified. + */ +static enum display_type env_parse_displaytype(char *displaytype) +{ + if (!strncmp(displaytype, "dvi640x480", 10)) + return set_dvi_preset(preset_dvi_640X480, 640, 480); + else if (!strncmp(displaytype, "dvi800x600", 10)) + return set_dvi_preset(preset_dvi_800X600, 800, 600); + else if (!strncmp(displaytype, "dvi1024x768", 11)) + return set_dvi_preset(preset_dvi_1024X768, 1024, 768); + else if (!strncmp(displaytype, "dvi1152x864", 11)) + return set_dvi_preset(preset_dvi_1152X864, 1152, 864); + else if (!strncmp(displaytype, "dvi1280x960", 11)) + return set_dvi_preset(preset_dvi_1280X960, 1280, 960); + else if (!strncmp(displaytype, "dvi1280x1024", 12)) + return set_dvi_preset(preset_dvi_1280X1024, 1280, 1024); + + return NONE; +} + +int lcd_line_length; +int lcd_color_fg; +int lcd_color_bg; +void *lcd_base; +short console_col; +short console_row; +void *lcd_console_address; + +void lcd_ctrl_init(void *lcdbase) +{ + struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE; + struct prcm *prcm = (struct prcm *)PRCM_BASE; + char *displaytype = getenv("displaytype"); + + if (displaytype == NULL) + return; + + lcd_def = env_parse_displaytype(displaytype); + if (lcd_def == NONE) + return; + + panel_cfg.frame_buffer = lcdbase; + omap3_dss_panel_config(&panel_cfg); + /* + * Pixel clock is defined with many divisions and only few + * multiplications of the system clock. Since DSS FCLK divisor is set + * to 16 by default, we need to set it to a smaller value, like 3 + * (chosen via trial and error). + */ + clrsetbits_le32(&prcm->clksel_dss, 0xF, 3); + /* + * Some of what the omap3_dss_panel_config() function did, needs to be + * adjusted, otherwise the image will be messed up/not appear at all. + */ + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16 << 1); + clrsetbits_le32(&dispc->gfx_attributes, 0x3 << 6, GFXBURSTSIZE16 << 6); +} + +void lcd_enable(void) +{ + if (lcd_def == DVI) { + gpio_direction_output(54, 0); /* Turn on DVI */ + omap3_dss_enable(); + } +} + +void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort blue) {} diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h index fa6eb4e..8544b15 100644 --- a/include/configs/cm_t35.h +++ b/include/configs/cm_t35.h @@ -339,4 +339,10 @@ #define CONFIG_OMAP3_GPIO_6 /* GPIO186 is in GPIO bank 6 */ #endif +/* Display Configuration */ +#define CONFIG_OMAP3_GPIO_2 +#define CONFIG_VIDEO_OMAP3 + +#define CONFIG_LCD + #endif /* __CONFIG_H */