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

[Igor Grinberg <grinberg@compulab.co.il>]

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.

-- 
Regards,
Igor.