[U-Boot] TI:OMAP: [PATCH 4/4] Minimal Display driver for OMAP3
From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001
From: Syed Mohammed Khasim khasim@ti.com Date: Fri, 8 Jan 2010 21:01:44 +0530 Subject: [PATCH] Minimal Display driver for OMAP3
Supports dynamic configuration of Panel and Video Encoder Supports Background color on DVID Supports Color bar on S-Video
Signed-off-by: Syed Mohammed Khasim khasim@ti.com --- board/ti/beagle/beagle.c | 13 +++ board/ti/beagle/beagle.h | 73 ++++++++++++++ drivers/video/Makefile | 1 + drivers/video/omap3_dss.c | 128 +++++++++++++++++++++++++ include/asm-arm/arch-omap3/dss.h | 193 ++++++++++++++++++++++++++++++++++++++ include/configs/omap3_beagle.h | 1 + 6 files changed, 409 insertions(+), 0 deletions(-) create mode 100644 drivers/video/omap3_dss.c create mode 100644 include/asm-arm/arch-omap3/dss.h
diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c index 7985ee9..29e47c8 100644 --- a/board/ti/beagle/beagle.c +++ b/board/ti/beagle/beagle.c @@ -114,6 +114,17 @@ void beagle_identify(void) }
/* + * Configure DSS to display background color on DVID + * Configure VENC to display color bar on S-Video + */ +void display_init(void) +{ + omap3_dss_venc_config(&venc_config_std_tv); + omap3_dss_panel_config(&dvid_cfg); + omap3_dss_set_background_col(DVI_BEAGLE_ORANGE_COL); +} + +/* * Routine: misc_init_r * Description: Configure board specific parts */ @@ -122,6 +133,7 @@ int misc_init_r(void) struct gpio *gpio5_base = (struct gpio *)OMAP34XX_GPIO5_BASE; struct gpio *gpio6_base = (struct gpio *)OMAP34XX_GPIO6_BASE;
+ display_init(); beagle_identify();
twl4030_power_init(); @@ -154,6 +166,7 @@ int misc_init_r(void) writel(GPIO31 | GPIO30 | GPIO29 | GPIO28 | GPIO22 | GPIO21 | GPIO15 | GPIO14 | GPIO13 | GPIO12, &gpio5_base->setdataout);
+ omap3_dss_enable(); dieid_num_r();
return 0; diff --git a/board/ti/beagle/beagle.h b/board/ti/beagle/beagle.h index b1720c9..7f6769f 100644 --- a/board/ti/beagle/beagle.h +++ b/board/ti/beagle/beagle.h @@ -23,6 +23,8 @@ #ifndef _BEAGLE_H_ #define _BEAGLE_H_
+#include <asm/arch/dss.h> + const omap3_sysinfo sysinfo = { DDR_STACKED, "OMAP3 Beagle board", @@ -385,4 +387,75 @@ const omap3_sysinfo sysinfo = { MUX_VAL(CP(UART2_RTS), (IDIS | PTD | DIS | M0)) /*UART2_RTS*/\ MUX_VAL(CP(UART2_TX), (IDIS | PTD | DIS | M0)) /*UART2_TX*/
+/* + * Display Configuration + */ + +#define DVI_BEAGLE_ORANGE_COL 0x00FF8000 + +/* + * Configure VENC in DSS for Beagle to generate Color Bar + * + * Kindly refer to OMAP TRM for definition of these values. + */ +static const struct venc_config venc_config_std_tv = { + .status = 0x0000001B, + .f_control = 0x00000040, + .vidout_ctrl = 0x00000000, + .sync_ctrl = 0x00008000, + .llen = 0x00008359, + .flens = 0x0000020C, + .hfltr_ctrl = 0x00000000, + .cc_carr_wss_carr = 0x043F2631, + .c_phase = 0x00000024, + .gain_u = 0x00000130, + .gain_v = 0x00000198, + .gain_y = 0x000001C0, + .black_level = 0x0000006A, + .blank_level = 0x0000005C, + .x_color = 0x00000000, + .m_control = 0x00000001, + .bstamp_wss_data = 0x0000003F, + .s_carr = 0x21F07C1F, + .line21 = 0x00000000, + .ln_sel = 0x00000015, + .l21__wc_ctl = 0x00001400, + .htrigger_vtrigger = 0x00000000, + .savid__eavid = 0x069300F4, + .flen__fal = 0x0016020C, + .lal__phase_reset = 0x00060107, + .hs_int_start_stop_x = 0x008D034E, + .hs_ext_start_stop_x = 0x000F0359, + .vs_int_start_x = 0x01A00000, + .vs_int_stop_x__vs_int_start_y = 0x020501A0, + .vs_int_stop_y__vs_ext_start_x = 0x01AC0024, + .vs_ext_stop_x__vs_ext_start_y = 0x020D01AC, + .vs_ext_stop_y = 0x00000006, + .avid_start_stop_x = 0x03480079, + .avid_start_stop_y = 0x02040024, + .fid_int_start_x__fid_int_start_y = 0x0001008A, + .fid_int_offset_y__fid_ext_start_x = 0x01AC0106, + .fid_ext_start_y__fid_ext_offset_y = 0x01060006, + .tvdetgp_int_start_stop_x = 0x00140001, + .tvdetgp_int_start_stop_y = 0x00010001, + .gen_ctrl = 0x00FF0000, + .output_control = 0x0000000D, + .dac_b__dac_c = 0x00000000, + .height_width = 0x00ef027f +}; + +/* + * Configure Timings for DVI D + */ +static const struct panel_config dvid_cfg = { + .timing_h = 0x0ff03f31, /* Horizantal timing */ + .timing_v = 0x01400504, /* Vertical timing */ + .pol_freq = 0x00007028, /* Pol Freq */ + .divisor = 0x00010006, /* 72Mhz Pixel Clock */ + .lcd_size = 0x02ff03ff, /* 1024x768 */ + .panel_type = 0x01, /* TFT */ + .data_lines = 0x03, /* 24 Bit RGB */ + .load_mode = 0x02 /* Frame Mode */ +}; + #endif diff --git a/drivers/video/Makefile b/drivers/video/Makefile index bb6b5a0..cb15dc2 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -37,6 +37,7 @@ COBJS-$(CONFIG_SED156X) += sed156x.o COBJS-$(CONFIG_VIDEO_SM501) += sm501.o COBJS-$(CONFIG_VIDEO_SMI_LYNXEM) += smiLynxEM.o COBJS-$(CONFIG_VIDEO_VCXK) += bus_vcxk.o +COBJS-$(CONFIG_VIDEO_OMAP3) += omap3_dss.o COBJS-y += videomodes.o
COBJS := $(COBJS-y) diff --git a/drivers/video/omap3_dss.c b/drivers/video/omap3_dss.c new file mode 100644 index 0000000..2ead7b9 --- /dev/null +++ b/drivers/video/omap3_dss.c @@ -0,0 +1,128 @@ +/* + * (C) Copyright 2010 + * Texas Instruments, <www.ti.com> + * Syed Mohammed Khasim khasim@ti.com + * + * Referred to Linux DSS driver files for OMAP3 + * + * 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's version 2 of + * the License. + * + * 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., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <asm/io.h> +#include <asm/arch/dss.h> + +/* + * VENC configuration + */ +void omap3_dss_venc_config(const struct venc_config *venc_cfg) +{ + dss_write_reg(VENC_STATUS, venc_cfg->status); + dss_write_reg(VENC_F_CONTROL, venc_cfg->f_control); + dss_write_reg(VENC_VIDOUT_CTRL, venc_cfg->vidout_ctrl); + dss_write_reg(VENC_SYNC_CTRL, venc_cfg->sync_ctrl); + dss_write_reg(VENC_LLEN, venc_cfg->llen); + dss_write_reg(VENC_FLENS, venc_cfg->flens); + dss_write_reg(VENC_HFLTR_CTRL, venc_cfg->hfltr_ctrl); + dss_write_reg(VENC_CC_CARR_WSS_CARR, venc_cfg->cc_carr_wss_carr); + dss_write_reg(VENC_C_PHASE, venc_cfg->c_phase); + dss_write_reg(VENC_GAIN_U, venc_cfg->gain_u); + dss_write_reg(VENC_GAIN_V, venc_cfg->gain_v); + dss_write_reg(VENC_GAIN_Y, venc_cfg->gain_y); + dss_write_reg(VENC_BLACK_LEVEL, venc_cfg->black_level); + dss_write_reg(VENC_BLANK_LEVEL, venc_cfg->blank_level); + dss_write_reg(VENC_X_COLOR, venc_cfg->x_color); + dss_write_reg(VENC_M_CONTROL, venc_cfg->m_control); + dss_write_reg(VENC_BSTAMP_WSS_DATA, venc_cfg->bstamp_wss_data); + dss_write_reg(VENC_S_CARR, venc_cfg->s_carr); + dss_write_reg(VENC_LINE21, venc_cfg->line21); + dss_write_reg(VENC_LN_SEL, venc_cfg->ln_sel); + dss_write_reg(VENC_L21__WC_CTL, venc_cfg->l21__wc_ctl); + dss_write_reg(VENC_HTRIGGER_VTRIGGER, venc_cfg->htrigger_vtrigger); + dss_write_reg(VENC_SAVID__EAVID, venc_cfg->savid__eavid); + dss_write_reg(VENC_FLEN__FAL, venc_cfg->flen__fal); + dss_write_reg(VENC_LAL__PHASE_RESET, venc_cfg->lal__phase_reset); + dss_write_reg(VENC_HS_INT_START_STOP_X, + venc_cfg->hs_int_start_stop_x); + dss_write_reg(VENC_HS_EXT_START_STOP_X, + venc_cfg->hs_ext_start_stop_x); + dss_write_reg(VENC_VS_INT_START_X, venc_cfg->vs_int_start_x); + dss_write_reg(VENC_VS_INT_STOP_X__VS_INT_START_Y, + venc_cfg->vs_int_stop_x__vs_int_start_y); + dss_write_reg(VENC_VS_INT_STOP_Y__VS_EXT_START_X, + venc_cfg->vs_int_stop_y__vs_ext_start_x); + dss_write_reg(VENC_VS_EXT_STOP_X__VS_EXT_START_Y, + venc_cfg->vs_ext_stop_x__vs_ext_start_y); + dss_write_reg(VENC_VS_EXT_STOP_Y, venc_cfg->vs_ext_stop_y); + dss_write_reg(VENC_AVID_START_STOP_X, venc_cfg->avid_start_stop_x); + dss_write_reg(VENC_AVID_START_STOP_Y, venc_cfg->avid_start_stop_y); + dss_write_reg(VENC_FID_INT_START_X__FID_INT_START_Y, + venc_cfg->fid_int_start_x__fid_int_start_y); + dss_write_reg(VENC_FID_INT_OFFSET_Y__FID_EXT_START_X, + venc_cfg->fid_int_offset_y__fid_ext_start_x); + dss_write_reg(VENC_FID_EXT_START_Y__FID_EXT_OFFSET_Y, + venc_cfg->fid_ext_start_y__fid_ext_offset_y); + dss_write_reg(VENC_TVDETGP_INT_START_STOP_X, + venc_cfg->tvdetgp_int_start_stop_x); + dss_write_reg(VENC_TVDETGP_INT_START_STOP_Y, + venc_cfg->tvdetgp_int_start_stop_y); + dss_write_reg(VENC_GEN_CTRL, venc_cfg->gen_ctrl); + dss_write_reg(VENC_OUTPUT_CONTROL, venc_cfg->output_control); + dss_write_reg(VENC_DAC_B__DAC_C, venc_cfg->dac_b__dac_c); + dss_write_reg(DISPC_SIZE_DIG, venc_cfg->height_width); + dss_write_reg(DSS_CONTROL, VENC_DSS_CONFIG); +} + +/* + * Configure Panel Specific parameters + */ +void omap3_dss_panel_config(const struct panel_config *panel_cfg) +{ + dss_write_reg(DISPC_TIMING_H, panel_cfg->timing_h); + dss_write_reg(DISPC_TIMING_V, panel_cfg->timing_v); + dss_write_reg(DISPC_POL_FREQ, panel_cfg->pol_freq); + dss_write_reg(DISPC_DIVISOR, panel_cfg->divisor); + dss_write_reg(DISPC_SIZE_LCD, panel_cfg->lcd_size); + dss_write_reg(DISPC_CONFIG, + (panel_cfg->load_mode << FRAME_MODE_OFFSET)); + dss_write_reg(DISPC_CONTROL, + ((panel_cfg->panel_type << TFTSTN_OFFSET) | + (panel_cfg->data_lines << DATALINES_OFFSET))); +} + +/* + * Enable LCD and DIGITAL OUT in DSS + */ +void omap3_dss_enable(void) +{ + u32 l = 0; + + l = dss_read_reg(DISPC_CONTROL); + l |= DISPC_ENABLE; + + dss_write_reg(DISPC_CONTROL, l); +} + +/* + * Set Background Color in DISPC + */ +void omap3_dss_set_background_col(u32 color) +{ + dss_write_reg(DISPC_DEFAULT_COLOR0, color); +} diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h new file mode 100644 index 0000000..08c7d8d --- /dev/null +++ b/include/asm-arm/arch-omap3/dss.h @@ -0,0 +1,193 @@ +/* + * (C) Copyright 2010 + * Texas Instruments, <www.ti.com> + * Syed Mohammed Khasim khasim@ti.com + * + * Referred to Linux DSS driver files for OMAP3 + * + * 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's version 2 of + * the License. + * + * 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., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef DSS_H +#define DSS_H + +/* VENC Register address */ +#define VENC_REV_ID 0x48050C00 +#define VENC_STATUS 0x48050C04 +#define VENC_F_CONTROL 0x48050C08 +#define VENC_VIDOUT_CTRL 0x48050C10 +#define VENC_SYNC_CTRL 0x48050C14 +#define VENC_LLEN 0x48050C1C +#define VENC_FLENS 0x48050C20 +#define VENC_HFLTR_CTRL 0x48050C24 +#define VENC_CC_CARR_WSS_CARR 0x48050C28 +#define VENC_C_PHASE 0x48050C2C +#define VENC_GAIN_U 0x48050C30 +#define VENC_GAIN_V 0x48050C34 +#define VENC_GAIN_Y 0x48050C38 +#define VENC_BLACK_LEVEL 0x48050C3C +#define VENC_BLANK_LEVEL 0x48050C40 +#define VENC_X_COLOR 0x48050C44 +#define VENC_M_CONTROL 0x48050C48 +#define VENC_BSTAMP_WSS_DATA 0x48050C4C +#define VENC_S_CARR 0x48050C50 +#define VENC_LINE21 0x48050C54 +#define VENC_LN_SEL 0x48050C58 +#define VENC_L21__WC_CTL 0x48050C5C +#define VENC_HTRIGGER_VTRIGGER 0x48050C60 +#define VENC_SAVID__EAVID 0x48050C64 +#define VENC_FLEN__FAL 0x48050C68 +#define VENC_LAL__PHASE_RESET 0x48050C6C +#define VENC_HS_INT_START_STOP_X 0x48050C70 +#define VENC_HS_EXT_START_STOP_X 0x48050C74 +#define VENC_VS_INT_START_X 0x48050C78 +#define VENC_VS_INT_STOP_X__VS_INT_START_Y 0x48050C7C +#define VENC_VS_INT_STOP_Y__VS_EXT_START_X 0x48050C80 +#define VENC_VS_EXT_STOP_X__VS_EXT_START_Y 0x48050C84 +#define VENC_VS_EXT_STOP_Y 0x48050C88 +#define VENC_AVID_START_STOP_X 0x48050C90 +#define VENC_AVID_START_STOP_Y 0x48050C94 +#define VENC_FID_INT_START_X__FID_INT_START_Y 0x48050CA0 +#define VENC_FID_INT_OFFSET_Y__FID_EXT_START_X 0x48050CA4 +#define VENC_FID_EXT_START_Y__FID_EXT_OFFSET_Y 0x48050CA8 +#define VENC_TVDETGP_INT_START_STOP_X 0x48050CB0 +#define VENC_TVDETGP_INT_START_STOP_Y 0x48050CB4 +#define VENC_GEN_CTRL 0x48050CB8 +#define VENC_OUTPUT_CONTROL 0x48050CC4 +#define VENC_DAC_B__DAC_C 0x48050CC8 + +/* DSS register addresses */ +#define DSS_SYSCONFIG 0x48050010 +#define DSS_CONTROL 0x48050040 + +/* DISPC register addresses */ +#define DISPC_SYSCONFIG 0x48050410 +#define DISPC_SYSSTATUS 0x48050414 +#define DISPC_CONTROL 0x48050440 +#define DISPC_CONFIG 0x48050444 +#define DISPC_DEFAULT_COLOR0 0x4805044c +#define DISPC_DEFAULT_COLOR1 0x48050450 +#define DISPC_TRANS_COLOR0 0x48050454 +#define DISPC_TRANS_COLOR1 0x48050458 +#define DISPC_TIMING_H 0x48050464 +#define DISPC_TIMING_V 0x48050468 +#define DISPC_POL_FREQ 0x4805046c +#define DISPC_DIVISOR 0x48050470 +#define DISPC_SIZE_DIG 0x48050478 +#define DISPC_SIZE_LCD 0x4805047c + +/* Few Register Offsets */ +#define FRAME_MODE_OFFSET 1 +#define TFTSTN_OFFSET 3 +#define DATALINES_OFFSET 8 + +/* Enabling Display controller */ +#define LCD_ENABLE 1 +#define DIG_ENABLE (1 << 1) +#define GO_LCD (1 << 5) +#define GO_DIG (1 << 6) +#define GP_OUT0 (1 << 15) +#define GP_OUT1 (1 << 16) + +#define DISPC_ENABLE (LCD_ENABLE | \ + DIG_ENABLE | \ + GO_LCD | \ + GO_DIG | \ + GP_OUT0| \ + GP_OUT1) +/* Configure VENC DSS Params */ +#define VENC_CLK_ENABLE (1 << 3) +#define DAC_DEMEN (1 << 4) +#define DAC_POWERDN (1 << 5) +#define VENC_OUT_SEL (1 << 6) + +#define VENC_DSS_CONFIG (VENC_CLK_ENABLE | \ + DAC_DEMEN | \ + DAC_POWERDN | \ + VENC_OUT_SEL) + +struct venc_config { + u32 status; + u32 f_control; + u32 vidout_ctrl; + u32 sync_ctrl; + u32 llen; + u32 flens; + u32 hfltr_ctrl; + u32 cc_carr_wss_carr; + u32 c_phase; + u32 gain_u; + u32 gain_v; + u32 gain_y; + u32 black_level; + u32 blank_level; + u32 x_color; + u32 m_control; + u32 bstamp_wss_data; + u32 s_carr; + u32 line21; + u32 ln_sel; + u32 l21__wc_ctl; + u32 htrigger_vtrigger; + u32 savid__eavid; + u32 flen__fal; + u32 lal__phase_reset; + u32 hs_int_start_stop_x; + u32 hs_ext_start_stop_x; + u32 vs_int_start_x; + u32 vs_int_stop_x__vs_int_start_y; + u32 vs_int_stop_y__vs_ext_start_x; + u32 vs_ext_stop_x__vs_ext_start_y; + u32 vs_ext_stop_y; + u32 avid_start_stop_x; + u32 avid_start_stop_y; + u32 fid_int_start_x__fid_int_start_y; + u32 fid_int_offset_y__fid_ext_start_x; + u32 fid_ext_start_y__fid_ext_offset_y; + u32 tvdetgp_int_start_stop_x; + u32 tvdetgp_int_start_stop_y; + u32 gen_ctrl; + u32 output_control; + u32 dac_b__dac_c; + u32 height_width; +}; + +struct panel_config { + u32 timing_h; + u32 timing_v; + u32 pol_freq; + u32 divisor; + u32 lcd_size; + u32 panel_type; + u32 data_lines; + u32 load_mode; +}; + +static inline void dss_write_reg(int reg, u32 val) +{ + __raw_writel(val, reg); +} + +static inline u32 dss_read_reg(int reg) +{ + u32 l = __raw_readl(reg); + return l; +} + +#endif /* DSS_H */ diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h index ff6d432..2c15df9 100644 --- a/include/configs/omap3_beagle.h +++ b/include/configs/omap3_beagle.h @@ -120,6 +120,7 @@ #define CONFIG_CMD_I2C /* I2C serial bus support */ #define CONFIG_CMD_MMC /* MMC support */ #define CONFIG_CMD_NAND /* NAND support */ +#define CONFIG_VIDEO_OMAP3 /* DSS Support */
#undef CONFIG_CMD_FLASH /* flinfo, erase, protect */ #undef CONFIG_CMD_FPGA /* FPGA configuration Support */
On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 From: Syed Mohammed Khasim khasim@ti.com Date: Fri, 8 Jan 2010 21:01:44 +0530 Subject: [PATCH] Minimal Display driver for OMAP3
Supports dynamic configuration of Panel and Video Encoder Supports Background color on DVID Supports Color bar on S-Video
We are getting there.. thanks a bunch. if you can split this series into two sets: a) introducing DSS layer b) introduce beagle support for the same it will be better.
but NAK to this patch.
Signed-off-by: Syed Mohammed Khasim khasim@ti.com
board/ti/beagle/beagle.c | 13 +++ board/ti/beagle/beagle.h | 73 ++++++++++++++ drivers/video/Makefile | 1 + drivers/video/omap3_dss.c | 128 +++++++++++++++++++++++++ include/asm-arm/arch-omap3/dss.h | 193 ++++++++++++++++++++++++++++++++++++++ include/configs/omap3_beagle.h | 1 + 6 files changed, 409 insertions(+), 0 deletions(-) create mode 100644 drivers/video/omap3_dss.c create mode 100644 include/asm-arm/arch-omap3/dss.h
[...]
diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h new file mode 100644 index 0000000..08c7d8d --- /dev/null +++ b/include/asm-arm/arch-omap3/dss.h @@ -0,0 +1,193 @@ +/*
- (C) Copyright 2010
- Texas Instruments, <www.ti.com>
- Syed Mohammed Khasim khasim@ti.com
- Referred to Linux DSS driver files for OMAP3
- 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's version 2 of
- the License.
- 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., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#ifndef DSS_H +#define DSS_H
+/* VENC Register address */ +#define VENC_REV_ID 0x48050C00
NAK. why do you need this if you have a struct?
here is what I think: venc_config { }
if it is organized as the register definitions,
configure_venc(struct venc_config *values) struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; writel(values->regx, &d->regx);
refer: drivers/mtd/nand/omap_gpmc.c
+#define VENC_STATUS 0x48050C04 +#define VENC_F_CONTROL 0x48050C08
[...] Regards, Nishanth Menon
On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon menon.nishanth@gmail.com wrote:
On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 From: Syed Mohammed Khasim khasim@ti.com Date: Fri, 8 Jan 2010 21:01:44 +0530 Subject: [PATCH] Minimal Display driver for OMAP3
Supports dynamic configuration of Panel and Video Encoder Supports Background color on DVID Supports Color bar on S-Video
We are getting there.. thanks a bunch. if you can split this series into two sets: a) introducing DSS layer b) introduce beagle support for the same it will be better.
Can you complete your review review comments here, I will take up this when in-corporate all review comments together.
Kindly remember these patches will be tested on B4, C2, C3, C4, EVM before releasing.
but NAK to this patch.
Signed-off-by: Syed Mohammed Khasim khasim@ti.com
board/ti/beagle/beagle.c | 13 +++ board/ti/beagle/beagle.h | 73 ++++++++++++++ drivers/video/Makefile | 1 + drivers/video/omap3_dss.c | 128 +++++++++++++++++++++++++ include/asm-arm/arch-omap3/dss.h | 193 ++++++++++++++++++++++++++++++++++++++ include/configs/omap3_beagle.h | 1 + 6 files changed, 409 insertions(+), 0 deletions(-) create mode 100644 drivers/video/omap3_dss.c create mode 100644 include/asm-arm/arch-omap3/dss.h
[...]
diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h new file mode 100644 index 0000000..08c7d8d --- /dev/null +++ b/include/asm-arm/arch-omap3/dss.h @@ -0,0 +1,193 @@ +/*
- (C) Copyright 2010
- Texas Instruments, <www.ti.com>
- Syed Mohammed Khasim khasim@ti.com
- Referred to Linux DSS driver files for OMAP3
- 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's version 2 of
- the License.
- 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., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#ifndef DSS_H +#define DSS_H
+/* VENC Register address */ +#define VENC_REV_ID 0x48050C00
NAK. why do you need this if you have a struct?
You need to read patch more carefully before giving NAK.
Structure is used to give multiple instance of Panels or different VENC settings like NTSC or PAL
You can add a new panel or a new tv standard with these structures easily. Structures are not used for register accesses.
here is what I think: venc_config { }
if it is organized as the register definitions,
configure_venc(struct venc_config *values) struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; writel(values->regx, &d->regx);
refer: drivers/mtd/nand/omap_gpmc.c
GPIO, GPMC and other controllers have multiple instances in OMAP, it makes sense to organize such register set in structure mode. I did start with that but found no use for DSS as it is just one instance. Structures don't give any value here.
More over I am introducing minimal DSS driver with minimal register set. It doesn't help any to give structure based register access for single instance drivers.
Give more reasons for NAK.
Regards, Khasim
Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM:
On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon menon.nishanth@gmail.com wrote:
On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 From: Syed Mohammed Khasim khasim@ti.com Date: Fri, 8 Jan 2010 21:01:44 +0530 Subject: [PATCH] Minimal Display driver for OMAP3
Supports dynamic configuration of Panel and Video Encoder Supports Background color on DVID Supports Color bar on S-Video
We are getting there.. thanks a bunch. if you can split this series into two sets: a) introducing DSS layer b) introduce beagle support for the same it will be better.
Can you complete your review review comments here, I will take up this when in-corporate all review comments together.
if you can give it a week before posting again, you could probably collate comments from various quarters. I just am a part time reviewer ;)..
Kindly remember these patches will be tested on B4, C2, C3, C4, EVM before releasing.
great. good to know.
but NAK to this patch.
Signed-off-by: Syed Mohammed Khasim khasim@ti.com
board/ti/beagle/beagle.c | 13 +++ board/ti/beagle/beagle.h | 73 ++++++++++++++ drivers/video/Makefile | 1 + drivers/video/omap3_dss.c | 128 +++++++++++++++++++++++++ include/asm-arm/arch-omap3/dss.h | 193 ++++++++++++++++++++++++++++++++++++++ include/configs/omap3_beagle.h | 1 + 6 files changed, 409 insertions(+), 0 deletions(-) create mode 100644 drivers/video/omap3_dss.c create mode 100644 include/asm-arm/arch-omap3/dss.h
[...]
diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h new file mode 100644 index 0000000..08c7d8d --- /dev/null +++ b/include/asm-arm/arch-omap3/dss.h @@ -0,0 +1,193 @@ +/*
- (C) Copyright 2010
- Texas Instruments, <www.ti.com>
- Syed Mohammed Khasim khasim@ti.com
- Referred to Linux DSS driver files for OMAP3
- 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's version 2 of
- the License.
- 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., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#ifndef DSS_H +#define DSS_H
+/* VENC Register address */ +#define VENC_REV_ID 0x48050C00
NAK. why do you need this if you have a struct?
You need to read patch more carefully before giving NAK.
Structure is used to give multiple instance of Panels or different VENC settings like NTSC or PAL
Thanks for clarifying That is the crux of the problem. if you use struct to describe the venc -> then you'd be staying with how the rest of u-boot accesses are: using struct dereference. if you look at nand.c as I pointed out, Dirk had done a great job of converting register offsets to struct and referencing off it. This was required to get the driver mainlined. The recomendation here is to move from #defines to struct based register usage. I am ok with the rest(except for need to split).
You can add a new panel or a new tv standard with these structures easily. Structures are not used for register accesses.
here is what I think: venc_config { }
if it is organized as the register definitions,
configure_venc(struct venc_config *values) struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; writel(values->regx, &d->regx);
refer: drivers/mtd/nand/omap_gpmc.c
GPIO, GPMC and other controllers have multiple instances in OMAP, it makes sense to organize such register set in structure mode. I did start with that but found no use for DSS as it is just one instance. Structures don't give any value here.
there were other reasons mentioned when nand got split -> one of them had to do with the compiler or something. Dirk might remember - unfortunately, this was more than a year back.. if I recollect right..
More over I am introducing minimal DSS driver with minimal register set. It doesn't help any to give structure based register access for single instance drivers.
moving to struct based is easy and done once and improves your chance of your driver getting upstreamed :).
Give more reasons for NAK.
This is the only reason I have other than the need to split this patch up into logical chunks. my NAK still stands unfortunately for the following generic reasons: a) split the patch as mentioned above. b) move away from #defines to struct based reg access.
Regards, Khasim
Regards, Nishanth Menon
On Sat, Jan 9, 2010 at 8:18 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM:
On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon menon.nishanth@gmail.com wrote:
On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 From: Syed Mohammed Khasim khasim@ti.com Date: Fri, 8 Jan 2010 21:01:44 +0530 Subject: [PATCH] Minimal Display driver for OMAP3
Supports dynamic configuration of Panel and Video Encoder Supports Background color on DVID Supports Color bar on S-Video
We are getting there.. thanks a bunch. if you can split this series into two sets: a) introducing DSS layer b) introduce beagle support for the same it will be better.
Can you complete your review review comments here, I will take up this when in-corporate all review comments together.
if you can give it a week before posting again, you could probably collate comments from various quarters. I just am a part time reviewer ;)..
Kindly remember these patches will be tested on B4, C2, C3, C4, EVM before releasing.
great. good to know.
but NAK to this patch.
Signed-off-by: Syed Mohammed Khasim khasim@ti.com
board/ti/beagle/beagle.c | 13 +++ board/ti/beagle/beagle.h | 73 ++++++++++++++ drivers/video/Makefile | 1 + drivers/video/omap3_dss.c | 128 +++++++++++++++++++++++++ include/asm-arm/arch-omap3/dss.h | 193 ++++++++++++++++++++++++++++++++++++++ include/configs/omap3_beagle.h | 1 + 6 files changed, 409 insertions(+), 0 deletions(-) create mode 100644 drivers/video/omap3_dss.c create mode 100644 include/asm-arm/arch-omap3/dss.h
[...]
diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h new file mode 100644 index 0000000..08c7d8d --- /dev/null +++ b/include/asm-arm/arch-omap3/dss.h @@ -0,0 +1,193 @@ +/*
- (C) Copyright 2010
- Texas Instruments, <www.ti.com>
- Syed Mohammed Khasim khasim@ti.com
- Referred to Linux DSS driver files for OMAP3
- 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's version 2 of
- the License.
- 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., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#ifndef DSS_H +#define DSS_H
+/* VENC Register address */ +#define VENC_REV_ID 0x48050C00
NAK. why do you need this if you have a struct?
You need to read patch more carefully before giving NAK.
Structure is used to give multiple instance of Panels or different VENC settings like NTSC or PAL
Thanks for clarifying That is the crux of the problem. if you use struct to describe the venc -> then you'd be staying with how the rest of u-boot accesses are: using struct dereference. if you look at nand.c as I pointed out, Dirk had done a great job of converting register offsets to struct and referencing off it. This was required to get the driver mainlined.
As said before, I agree. NAND and GPIO needs struct based development as it has multiple instances. It will look odd to implement in #defines.
The recomendation here is to move from #defines to struct based register usage. I am ok with the rest(except for need to split).
Split is done, posted yesterday.
Struct based register needs more comments, not that I am lazy to implement that. I need to know the reason for doing the same when no multiple instances are used.
You can add a new panel or a new tv standard with these structures easily. Structures are not used for register accesses.
here is what I think: venc_config { }
if it is organized as the register definitions,
configure_venc(struct venc_config *values) struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; writel(values->regx, &d->regx);
refer: drivers/mtd/nand/omap_gpmc.c
GPIO, GPMC and other controllers have multiple instances in OMAP, it makes sense to organize such register set in structure mode. I did start with that but found no use for DSS as it is just one instance. Structures don't give any value here.
there were other reasons mentioned when nand got split -> one of them had to do with the compiler or something. Dirk might remember - unfortunately, this was more than a year back.. if I recollect right..
Will try doing a google. May be some one can point me to that decision. It would help developing drivers which have single instance of controller being used.
More over I am introducing minimal DSS driver with minimal register set. It doesn't help any to give structure based register access for single instance drivers.
moving to struct based is easy and done once and improves your chance of your driver getting upstreamed :).
DSS in OMAP3 has following register domains.
DSI Protocol Engine 0x4804 FC00 512 bytes DSI_PHY 0x4804 FE00 64 bytes DSI PLL Controller 0x4804 FF00 32 bytes Display Subsystem 0x4805 0000 512 bytes Display Controller 0x4805 0400 1K byte Display Controller VID1 0x4805 0400 1K byte Display Controller VID2 0x4805 0400 1K byte RFBI 0x4805 0800 256 bytes Video Encode 0x4805 0C00 256 bytes
I am not sure why one would ask me to give struct definitions for these 500 (approx) registers when only 50 of these are required to implement background and color bar. As I am saying all the way, DSS is not multiple instance module like GPMC (NAND) and GPIO it is just one module.
Will wait for others in list to comment on these.
Give more reasons for NAK.
This is the only reason I have other than the need to split this patch up into logical chunks. my NAK still stands unfortunately for the following generic reasons: a) split the patch as mentioned above.
Done.
b) move away from #defines to struct based reg access.
Reasons given above. Will wait for others in community to comment on the above implementation.
Many thanks for the review.
Regards, Khasim
Khasim Syed Mohammed said the following on 01/09/2010 09:16 PM:
On Sat, Jan 9, 2010 at 8:18 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM:
On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon menon.nishanth@gmail.com wrote:
On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 From: Syed Mohammed Khasim khasim@ti.com
[...]
The recomendation here is to move from #defines to struct based register usage. I am ok with the rest(except for need to split).
Split is done, posted yesterday.
Struct based register needs more comments, not that I am lazy to implement that. I need to know the reason for doing the same when no multiple instances are used.
You can add a new panel or a new tv standard with these structures easily. Structures are not used for register accesses.
here is what I think: venc_config { }
if it is organized as the register definitions,
configure_venc(struct venc_config *values) struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; writel(values->regx, &d->regx);
refer: drivers/mtd/nand/omap_gpmc.c
GPIO, GPMC and other controllers have multiple instances in OMAP, it makes sense to organize such register set in structure mode. I did start with that but found no use for DSS as it is just one instance. Structures don't give any value here.
there were other reasons mentioned when nand got split -> one of them had to do with the compiler or something. Dirk might remember - unfortunately, this was more than a year back.. if I recollect right..
Will try doing a google. May be some one can point me to that decision. It would help developing drivers which have single instance of controller being used.
the reference I got: http://old.nabble.com/-U-Boot---PATCH-08-13-v4--ARM%3A-OMAP3%3A-Add-NAND-sup...
V5 became: http://old.nabble.com/-U-Boot---PATCH-07-13-v5--ARM%3A-OMAP3%3A-Add-NAND-sup...
similar changes happend for GPMC etc..
Quote:
Is GPMC_BASE an integer or a pointer?
Nothing. A macro:
#define OMAP34XX_GPMC_BASE (0x6E000000) #define GPMC_BASE (OMAP34XX_GPMC_BASE)
So it's an integer.
It's then casted to volatile pointer by ARM's readx/writex.
The cast should be done by the driver, or you'll get warnings if readx/writex ever become inline functions (as they are on other arches).
might help explain..
More over I am introducing minimal DSS driver with minimal register set. It doesn't help any to give structure based register access for single instance drivers.
moving to struct based is easy and done once and improves your chance of your driver getting upstreamed :).
DSS in OMAP3 has following register domains.
DSI Protocol Engine 0x4804 FC00 512 bytes DSI_PHY 0x4804 FE00 64 bytes DSI PLL Controller 0x4804 FF00 32 bytes Display Subsystem 0x4805 0000 512 bytes Display Controller 0x4805 0400 1K byte Display Controller VID1 0x4805 0400 1K byte Display Controller VID2 0x4805 0400 1K byte RFBI 0x4805 0800 256 bytes Video Encode 0x4805 0C00 256 bytes
I am not sure why one would ask me to give struct definitions for these 500 (approx) registers when only 50 of these are required to implement background and color bar. As I am saying all the way, DSS is not multiple instance module like GPMC (NAND) and GPIO it is just one module.
Aren't you extrapolating this a bit out of scope? DSI,RFBI etc.. is not relevant to your patch. you may need DSS, controller and VID1(and VID2 is the same). I think your complaint is about having to define the reg structs when multiple instances dont exist - how about OMAP4? wont these structs get reused there(once we get around to it)?
Regards, Nishanth Menon
On Sun, Jan 10, 2010 at 9:11 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/09/2010 09:16 PM:
On Sat, Jan 9, 2010 at 8:18 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM:
On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon menon.nishanth@gmail.com wrote:
On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 From: Syed Mohammed Khasim khasim@ti.com
[...]
The recomendation here is to move from #defines to struct based register usage. I am ok with the rest(except for need to split).
Split is done, posted yesterday.
Struct based register needs more comments, not that I am lazy to implement that. I need to know the reason for doing the same when no multiple instances are used.
You can add a new panel or a new tv standard with these structures easily. Structures are not used for register accesses.
here is what I think: venc_config { }
if it is organized as the register definitions,
configure_venc(struct venc_config *values) struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; writel(values->regx, &d->regx);
refer: drivers/mtd/nand/omap_gpmc.c
GPIO, GPMC and other controllers have multiple instances in OMAP, it makes sense to organize such register set in structure mode. I did start with that but found no use for DSS as it is just one instance. Structures don't give any value here.
there were other reasons mentioned when nand got split -> one of them had to do with the compiler or something. Dirk might remember - unfortunately, this was more than a year back.. if I recollect right..
Will try doing a google. May be some one can point me to that decision. It would help developing drivers which have single instance of controller being used.
the reference I got: http://old.nabble.com/-U-Boot---PATCH-08-13-v4--ARM%3A-OMAP3%3A-Add-NAND-sup...
V5 became: http://old.nabble.com/-U-Boot---PATCH-07-13-v5--ARM%3A-OMAP3%3A-Add-NAND-sup...
similar changes happend for GPMC etc..
Quote:
Is GPMC_BASE an integer or a pointer?
Nothing. A macro:
#define OMAP34XX_GPMC_BASE (0x6E000000) #define GPMC_BASE (OMAP34XX_GPMC_BASE)
So it's an integer.
It's then casted to volatile pointer by ARM's readx/writex.
The cast should be done by the driver, or you'll get warnings if readx/writex ever become inline functions (as they are on other arches).
might help explain..
This is a valid comment, many thanks for digging this out. Considering this comment, my dss_read_reg and dss_write_reg should become as shown below
+static inline void dss_write_reg(int reg, u32 val) +{ + __raw_writel(val, (uint32_t *) reg); +} + +static inline u32 dss_read_reg(int reg) +{ + u32 l = __raw_readl((uint32_t *) reg); + return l; +}
I will do the above changes and re-submit the patch.
But Kindly NOTE: This still doesn't give me a reason to implement the register definition as structures when we have single instance of register set. I am still not considering the structure based read/write currently.
More over I am introducing minimal DSS driver with minimal register set. It doesn't help any to give structure based register access for single instance drivers.
moving to struct based is easy and done once and improves your chance of your driver getting upstreamed :).
DSS in OMAP3 has following register domains.
DSI Protocol Engine 0x4804 FC00 512 bytes DSI_PHY 0x4804 FE00 64 bytes DSI PLL Controller 0x4804 FF00 32 bytes Display Subsystem 0x4805 0000 512 bytes Display Controller 0x4805 0400 1K byte Display Controller VID1 0x4805 0400 1K byte Display Controller VID2 0x4805 0400 1K byte RFBI 0x4805 0800 256 bytes Video Encode 0x4805 0C00 256 bytes
I am not sure why one would ask me to give struct definitions for these 500 (approx) registers when only 50 of these are required to implement background and color bar. As I am saying all the way, DSS is not multiple instance module like GPMC (NAND) and GPIO it is just one module.
Aren't you extrapolating this a bit out of scope? DSI,RFBI etc.. is not relevant to your patch.
For Beagle it is not, but other boards will have to use DSI, RFBI etc. We have boards that use these modules today.
you may need DSS, controller and VID1(and VID2 is the same). I think your complaint is about having to define the reg structs when multiple instances dont exist - how about OMAP4? wont these structs get reused there(once we get around to it)?
OMAP4 DSS is completely different from that of 3. So it won't be re-used.
Thanks,
Regards, Khasim
On Sun, Jan 10, 2010 at 7:46 PM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
On Sun, Jan 10, 2010 at 9:11 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/09/2010 09:16 PM:
On Sat, Jan 9, 2010 at 8:18 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM:
On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon menon.nishanth@gmail.com wrote:
On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 > From: Syed Mohammed Khasim khasim@ti.com
[...]
The recomendation here is to move from #defines to struct based register usage. I am ok with the rest(except for need to split).
Split is done, posted yesterday.
Struct based register needs more comments, not that I am lazy to implement that. I need to know the reason for doing the same when no multiple instances are used.
You can add a new panel or a new tv standard with these structures easily. Structures are not used for register accesses.
here is what I think: venc_config { }
if it is organized as the register definitions,
configure_venc(struct venc_config *values) struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; writel(values->regx, &d->regx);
refer: drivers/mtd/nand/omap_gpmc.c
GPIO, GPMC and other controllers have multiple instances in OMAP, it makes sense to organize such register set in structure mode. I did start with that but found no use for DSS as it is just one instance. Structures don't give any value here.
there were other reasons mentioned when nand got split -> one of them had to do with the compiler or something. Dirk might remember - unfortunately, this was more than a year back.. if I recollect right..
Will try doing a google. May be some one can point me to that decision. It would help developing drivers which have single instance of controller being used.
the reference I got: http://old.nabble.com/-U-Boot---PATCH-08-13-v4--ARM%3A-OMAP3%3A-Add-NAND-sup...
V5 became: http://old.nabble.com/-U-Boot---PATCH-07-13-v5--ARM%3A-OMAP3%3A-Add-NAND-sup...
similar changes happend for GPMC etc..
Quote:
Is GPMC_BASE an integer or a pointer?
Nothing. A macro:
#define OMAP34XX_GPMC_BASE (0x6E000000) #define GPMC_BASE (OMAP34XX_GPMC_BASE)
So it's an integer.
It's then casted to volatile pointer by ARM's readx/writex.
The cast should be done by the driver, or you'll get warnings if readx/writex ever become inline functions (as they are on other arches).
might help explain..
This is a valid comment, many thanks for digging this out. Considering this comment, my dss_read_reg and dss_write_reg should become as shown below
+static inline void dss_write_reg(int reg, u32 val) +{
- __raw_writel(val, (uint32_t *) reg);
+}
+static inline u32 dss_read_reg(int reg) +{
- u32 l = __raw_readl((uint32_t *) reg);
- return l;
+}
I will do the above changes and re-submit the patch.
But Kindly NOTE: This still doesn't give me a reason to implement the register definition as structures when we have single instance of register set. I am still not considering the structure based read/write currently.
IIRC the main reason was that Wolfgang would refuse to merge initial OMAP3 support unless _all_ register accesses were structure based, single instance or not. He gave his reasons but they didn't look convincing to me (personal humble opinion only), CCed him for a possible comments or reminder :)
More over I am introducing minimal DSS driver with minimal register set. It doesn't help any to give structure based register access for single instance drivers.
moving to struct based is easy and done once and improves your chance of your driver getting upstreamed :).
DSS in OMAP3 has following register domains.
DSI Protocol Engine 0x4804 FC00 512 bytes DSI_PHY 0x4804 FE00 64 bytes DSI PLL Controller 0x4804 FF00 32 bytes Display Subsystem 0x4805 0000 512 bytes Display Controller 0x4805 0400 1K byte Display Controller VID1 0x4805 0400 1K byte Display Controller VID2 0x4805 0400 1K byte RFBI 0x4805 0800 256 bytes Video Encode 0x4805 0C00 256 bytes
I am not sure why one would ask me to give struct definitions for these 500 (approx) registers when only 50 of these are required to implement background and color bar. As I am saying all the way, DSS is not multiple instance module like GPMC (NAND) and GPIO it is just one module.
Aren't you extrapolating this a bit out of scope? DSI,RFBI etc.. is not relevant to your patch.
For Beagle it is not, but other boards will have to use DSI, RFBI etc. We have boards that use these modules today.
you may need DSS, controller and VID1(and VID2 is the same). I think your complaint is about having to define the reg structs when multiple instances dont exist - how about OMAP4? wont these structs get reused there(once we get around to it)?
OMAP4 DSS is completely different from that of 3. So it won't be re-used.
Thanks,
Regards, Khasim _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Mon, Jan 11, 2010 at 6:39 PM, Grazvydas Ignotas notasas@gmail.com wrote:
On Sun, Jan 10, 2010 at 7:46 PM, Khasim Syed Mohammed khasim@beagleboard.org wrote:
On Sun, Jan 10, 2010 at 9:11 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/09/2010 09:16 PM:
On Sat, Jan 9, 2010 at 8:18 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM:
On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon menon.nishanth@gmail.com wrote:
> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed > khasim@beagleboard.org wrote: > >> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 >> From: Syed Mohammed Khasim khasim@ti.com
[...]
The recomendation here is to move from #defines to struct based register usage. I am ok with the rest(except for need to split).
Split is done, posted yesterday.
Struct based register needs more comments, not that I am lazy to implement that. I need to know the reason for doing the same when no multiple instances are used.
You can add a new panel or a new tv standard with these structures easily. Structures are not used for register accesses.
> here is what I think: > venc_config { > } > > if it is organized as the register definitions, > > configure_venc(struct venc_config *values) > struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; > writel(values->regx, &d->regx); > > refer: drivers/mtd/nand/omap_gpmc.c > > GPIO, GPMC and other controllers have multiple instances in OMAP, it makes sense to organize such register set in structure mode. I did start with that but found no use for DSS as it is just one instance. Structures don't give any value here.
there were other reasons mentioned when nand got split -> one of them had to do with the compiler or something. Dirk might remember - unfortunately, this was more than a year back.. if I recollect right..
Will try doing a google. May be some one can point me to that decision. It would help developing drivers which have single instance of controller being used.
the reference I got: http://old.nabble.com/-U-Boot---PATCH-08-13-v4--ARM%3A-OMAP3%3A-Add-NAND-sup...
V5 became: http://old.nabble.com/-U-Boot---PATCH-07-13-v5--ARM%3A-OMAP3%3A-Add-NAND-sup...
similar changes happend for GPMC etc..
Quote:
Is GPMC_BASE an integer or a pointer?
Nothing. A macro:
#define OMAP34XX_GPMC_BASE (0x6E000000) #define GPMC_BASE (OMAP34XX_GPMC_BASE)
So it's an integer.
It's then casted to volatile pointer by ARM's readx/writex.
The cast should be done by the driver, or you'll get warnings if readx/writex ever become inline functions (as they are on other arches).
might help explain..
This is a valid comment, many thanks for digging this out. Considering this comment, my dss_read_reg and dss_write_reg should become as shown below
+static inline void dss_write_reg(int reg, u32 val) +{
- __raw_writel(val, (uint32_t *) reg);
+}
+static inline u32 dss_read_reg(int reg) +{
- u32 l = __raw_readl((uint32_t *) reg);
- return l;
+}
I will do the above changes and re-submit the patch.
But Kindly NOTE: This still doesn't give me a reason to implement the register definition as structures when we have single instance of register set. I am still not considering the structure based read/write currently.
IIRC the main reason was that Wolfgang would refuse to merge initial OMAP3 support unless _all_ register accesses were structure based, single instance or not. He gave his reasons but they didn't look convincing to me (personal humble opinion only), CCed him for a possible comments or reminder :)
oh ok, then I don't want to waste time waiting. I will change them to structures and repost.
Regards, Khasim
More over I am introducing minimal DSS driver with minimal register set. It doesn't help any to give structure based register access for single instance drivers.
moving to struct based is easy and done once and improves your chance of your driver getting upstreamed :).
DSS in OMAP3 has following register domains.
DSI Protocol Engine 0x4804 FC00 512 bytes DSI_PHY 0x4804 FE00 64 bytes DSI PLL Controller 0x4804 FF00 32 bytes Display Subsystem 0x4805 0000 512 bytes Display Controller 0x4805 0400 1K byte Display Controller VID1 0x4805 0400 1K byte Display Controller VID2 0x4805 0400 1K byte RFBI 0x4805 0800 256 bytes Video Encode 0x4805 0C00 256 bytes
I am not sure why one would ask me to give struct definitions for these 500 (approx) registers when only 50 of these are required to implement background and color bar. As I am saying all the way, DSS is not multiple instance module like GPMC (NAND) and GPIO it is just one module.
Aren't you extrapolating this a bit out of scope? DSI,RFBI etc.. is not relevant to your patch.
For Beagle it is not, but other boards will have to use DSI, RFBI etc. We have boards that use these modules today.
you may need DSS, controller and VID1(and VID2 is the same). I think your complaint is about having to define the reg structs when multiple instances dont exist - how about OMAP4? wont these structs get reused there(once we get around to it)?
OMAP4 DSS is completely different from that of 3. So it won't be re-used.
Thanks,
Regards, Khasim _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
participants (3)
-
Grazvydas Ignotas -
Khasim Syed Mohammed -
Nishanth Menon