From: Jeroen Hofstee <jeroen@myspectrum.nl>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays
Date: Wed, 23 Jan 2013 22:39:39 +0100 [thread overview]
Message-ID: <5100589B.6080308@myspectrum.nl> (raw)
In-Reply-To: <50FCF860.1010706@compulab.co.il>
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
next prev parent reply other threads:[~2013-01-23 21:39 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-23 7:03 [U-Boot] [PATCH 0/5] Add splash screen for CM-T35 Nikita Kiryanov
2012-12-23 7:03 ` [U-Boot] [PATCH 1/5] omap3: add useful dss defines Nikita Kiryanov
2013-01-20 21:42 ` Jeroen Hofstee
2013-01-21 7:53 ` Nikita Kiryanov
2013-01-21 18:38 ` Jeroen Hofstee
2013-01-23 8:23 ` Nikita Kiryanov
2012-12-23 7:03 ` [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation Nikita Kiryanov
2013-01-20 20:34 ` Jeroen Hofstee
2013-01-21 7:51 ` Nikita Kiryanov
2013-01-21 19:14 ` Jeroen Hofstee
2013-01-23 8:31 ` Nikita Kiryanov
2013-01-23 22:13 ` Jeroen Hofstee
2013-01-24 8:35 ` Igor Grinberg
2013-01-24 22:34 ` Jeroen Hofstee
2013-01-25 6:45 ` Igor Grinberg
2013-01-26 13:33 ` Jeroen Hofstee
2012-12-23 7:03 ` [U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays Nikita Kiryanov
2013-01-20 20:59 ` Jeroen Hofstee
2013-01-21 8:12 ` Nikita Kiryanov
2013-01-23 21:39 ` Jeroen Hofstee [this message]
2013-01-24 9:02 ` Igor Grinberg
2012-12-23 7:03 ` [U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters Nikita Kiryanov
2013-01-20 21:08 ` Jeroen Hofstee
2013-01-21 8:25 ` Nikita Kiryanov
2013-01-23 22:36 ` Jeroen Hofstee
2013-01-24 9:12 ` Igor Grinberg
2012-12-23 7:03 ` [U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND Nikita Kiryanov
2012-12-24 8:55 ` Jeroen Hofstee
2012-12-25 8:56 ` Nikita Kiryanov
2012-12-26 14:27 ` Jeroen Hofstee
2012-12-30 14:39 ` Nikita Kiryanov
2013-01-22 7:37 ` Albert ARIBAUD
2013-01-23 10:47 ` Nikita Kiryanov
2013-01-23 11:07 ` Nikita Kiryanov
2013-03-26 14:51 ` [U-Boot] [U-Boot, " Tom Rini
2013-01-20 12:25 ` [U-Boot] [PATCH 0/5] Add splash screen for CM-T35 Nikita Kiryanov
2013-01-20 20:31 ` Jeroen Hofstee
2013-01-21 14:10 ` Tom Rini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5100589B.6080308@myspectrum.nl \
--to=jeroen@myspectrum.nl \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.