Hi Otavio,
one minor point.
On 16/12/2013 23:43, Otavio Salvador wrote:
+int board_video_skip(void) +{
- int i;
- int ret;
- int detected = 0;
- char const *panel = getenv("panel");
- if (!panel) {
for (i = 0; i < ARRAY_SIZE(displays); i++) {struct display_info_t const *dev = displays+i;if (dev->detect && dev->detect(dev)) {panel = dev->mode.name;detected = 1;break;}}if (!panel) {panel = displays[0].mode.name;printf("No panel detected: default to %s\n", panel);i = 0;}- } else {
for (i = 0; i < ARRAY_SIZE(displays); i++) {if (!strcmp(panel, displays[i].mode.name))break;}- }
- if (i < ARRAY_SIZE(displays)) {
ret = ipuv3_fb_init(&displays[i].mode, 0,displays[i].pixfmt);if (!ret) {displays[i].enable(displays+i);printf("Display: %s (%ux%u) %s\n",displays[i].mode.name,displays[i].mode.xres,displays[i].mode.yres,(detected ? "[auto]" : ""));} elseprintf("LCD %s cannot be configured: %d\n",displays[i].mode.name, ret);- } else {
printf("unsupported panel %s\n", panel);return -EINVAL;- }
- return 0;
}
This is identical to the nitrogen one, and quite identical to the same function in sabresd. Or better, function in sabresd is older and has not received the fixes as nitrogen (line with dev->detect).
IMHO the function is generic to be factorized. The display_info_t structure lets us to specialize the behavior for each board, and the generic part can be put in a common part.
static void setup_display(void) { struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR; int reg;
enable_ipu_clock(); imx_setup_hdmi();
/* Turn on LDB0, LDB1, IPU,IPU DI0 clocks */
reg = __raw_readl(&mxc_ccm->CCGR3);
reg |= MXC_CCM_CCGR3_LDB_DI0_MASK | MXC_CCM_CCGR3_LDB_DI1_MASK;
writel(reg, &mxc_ccm->CCGR3);
/* set LDB0, LDB1 clk select to 011/011 */
reg = readl(&mxc_ccm->cs2cdr);
reg &= ~(MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_MASK
| MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_MASK);reg |= (3 << MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_OFFSET)
| (3 << MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_OFFSET);writel(reg, &mxc_ccm->cs2cdr);
reg = readl(&mxc_ccm->cscmr2);
reg |= MXC_CCM_CSCMR2_LDB_DI0_IPU_DIV | MXC_CCM_CSCMR2_LDB_DI1_IPU_DIV;
writel(reg, &mxc_ccm->cscmr2);
reg = readl(&mxc_ccm->chsccdr); reg |= (CHSCCDR_CLK_SEL_LDB_DI0 << MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET);
reg |= (CHSCCDR_CLK_SEL_LDB_DI0
<< MXC_CCM_CHSCCDR_IPU1_DI1_CLK_SEL_OFFSET);writel(reg, &mxc_ccm->chsccdr);
reg = IOMUXC_GPR2_BGREF_RRMODE_EXTERNAL_RES
| IOMUXC_GPR2_DI1_VS_POLARITY_ACTIVE_LOW| IOMUXC_GPR2_DI0_VS_POLARITY_ACTIVE_LOW| IOMUXC_GPR2_BIT_MAPPING_CH1_SPWG| IOMUXC_GPR2_DATA_WIDTH_CH1_18BIT| IOMUXC_GPR2_BIT_MAPPING_CH0_SPWG| IOMUXC_GPR2_DATA_WIDTH_CH0_18BIT| IOMUXC_GPR2_LVDS_CH0_MODE_DISABLED| IOMUXC_GPR2_LVDS_CH1_MODE_ENABLED_DI0;writel(reg, &iomux->gpr[2]);
reg = readl(&iomux->gpr[3]);
reg = (reg & ~(IOMUXC_GPR3_LVDS1_MUX_CTL_MASK
| IOMUXC_GPR3_HDMI_MUX_CTL_MASK))| (IOMUXC_GPR3_MUX_SRC_IPU1_DI0<< IOMUXC_GPR3_LVDS1_MUX_CTL_OFFSET);writel(reg, &iomux->gpr[3]);
/* Disable LCD backlight */
imx_iomux_v3_setup_pad(MX6_PAD_DI0_PIN4__GPIO4_IO20);
gpio_direction_input(IMX_GPIO_NR(4, 20));
} #endif /* CONFIG_VIDEO_IPUV3 */
This function is also quite identical to all boards.You add here only the disable of the LCD backlight. I can recognize some parts, that are surely common to all implementations (enabling clocks). Even if the setup of gpr[2]/[3] is identical, I can imagine that it could be board specific. Anyway, any chance to factorize the code ?
diff --git a/include/configs/wandboard.h b/include/configs/wandboard.h index e9c7e64..85f3c16 100644 --- a/include/configs/wandboard.h +++ b/include/configs/wandboard.h @@ -55,6 +55,12 @@ #define CONFIG_LOADADDR 0x12000000 #define CONFIG_SYS_TEXT_BASE 0x17800000
+/* I2C Configs */ +#define CONFIG_CMD_I2C +#define CONFIG_SYS_I2C +#define CONFIG_SYS_I2C_MXC +#define CONFIG_SYS_I2C_SPEED 100000
/* MMC Configuration */ #define CONFIG_FSL_ESDHC #define CONFIG_FSL_USDHC @@ -97,6 +103,7 @@ #define CONFIG_VIDEO_LOGO #define CONFIG_VIDEO_BMP_LOGO #define CONFIG_IPUV3_CLK 260000000 +#define CONFIG_CMD_HDMIDETECT #define CONFIG_IMX_HDMI
#if defined(CONFIG_MX6DL) || defined(CONFIG_MX6S) @@ -134,7 +141,33 @@ "fi; " \ "fi\0" \ "mmcargs=setenv bootargs console=${console},${baudrate} " \
"root=${mmcroot}\0" \
"root=${mmcroot}; run videoargs\0" \- "videoargs=" \
"setenv nextcon 0; " \"if hdmidet; then " \"setenv bootargs ${bootargs} " \"video=mxcfb${nextcon}:dev=hdmi,1280x720M@60," \"if=RGB24; " \"setenv fbmen fbmem=28M; " \"setexpr nextcon ${nextcon} + 1; " \"else " \"echo - no HDMI monitor;" \"fi; " \"i2c dev 0; " \"if i2c probe 0x10; then " \"setenv bootargs ${bootargs} " \"video=mxcfb${nextcon}:dev=lcd,800x480@60," \"if=RGB666; " \
Why do we need this ? The kernel should get the setup for display from DTS (display-timings node). Do you need to short-circuit DTS setup ?
"if test 0 -eq ${nextcon}; then " \"setenv fbmem fbmem=10M; " \"else " \"setenv fbmem ${fbmem},10M; " \"fi; " \"setexpr nextcon ${nextcon} + 1; " \"else " \"echo '- no FWBADAPT-7WVGA-LCD-F07A-0102 display';" \"fi; " \"setenv bootargs ${bootargs} ${fbmem}\0" \
I was against adding so many scripts in CONFIG_ENV_EXTRA, but as Simon's patches to move the default environment outside u-boot are not yet in mainline, I will not stop patches for this.
Best regards, Stefano Babic