From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jeroen Hofstee <jeroen@myspectrum.nl>
Date: Wed, 23 Jan 2013 22:39:39 +0100
Subject: [U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays
In-Reply-To: <50FCF860.1010706@compulab.co.il>
References: <1356246228-26732-1-git-send-email-nikita@compulab.co.il>
	<1356246228-26732-4-git-send-email-nikita@compulab.co.il>
	<50FC5AA6.2050406@myspectrum.nl> <50FCF860.1010706@compulab.co.il>
Message-ID: <5100589B.6080308@myspectrum.nl>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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