[U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays

[Nikita Kiryanov <nikita@compulab.co.il>]

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
>


-- 
Regards,
Nikita.