[U-Boot] [PATCH 05/11] ARM: mxs: Add Creative ZEN XFi3 board

Marek Vasut
2013-07-31T15:09:25Z

Dear Stefano Babic,

[...]

> > @@ -0,0 +1,47 @@
> > +#
> > +# (C) Copyright 2000-2006
> > +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > +#
> > +# See file CREDITS for list of people who contributed to this
> > +# project.
> > +#
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation; either version 2 of
> > +# the License, or (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > +# MA 02111-1307 USA
> > +#
>
> Please use new licence style with SPDX identifier. Fix it globally.

Fixed in my tree already, thanks.

[...]

> > +void mxsfb_system_setup(void)
> > +{
> > + struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE;
> > +
> > + /* Switch the LCDIF into System-Mode */
> > + writel(LCDIF_CTRL_LCDIF_MASTER | LCDIF_CTRL_DOTCLK_MODE |
> > + LCDIF_CTRL_BYPASS_COUNT, &regs->hw_lcdif_ctrl_clr);
> > +
> > + /* Restart the SmartLCD controller */
> > + mdelay(50);
>
> Nothing again, but 50mS is a lot of time and this sums up in the whole
> function making a big delay. Really needed ?

Yeah, I had these smartLCDs (just for the record).

> > + writel(1, &regs->hw_lcdif_ctrl1_set);
> > + mdelay(50);
> > + writel(1, &regs->hw_lcdif_ctrl1_clr);
> > + mdelay(50);
> > + writel(1, &regs->hw_lcdif_ctrl1_set);
> > + mdelay(50);
> > +
> > + /* Program the SmartLCD controller */
> > + writel(LCDIF_CTRL1_RECOVER_ON_UNDERFLOW, &regs->hw_lcdif_ctrl1_set);
> > +
> > + writel(0x03030202, &regs->hw_lcdif_timing);
> > +
> > + mxsfb_write_register(1, 0x01c);
> > +
> > + mxsfb_write_register(2, 0x100);
> > + /* 0x30 flips the LCD */
> > + mxsfb_write_register(3, 0x1038);
> > + mxsfb_write_register(8, 0x808);
> > + /* This can possibly contain 0x111 */
> > + mxsfb_write_register(0xc, 0x0);
> > + mxsfb_write_register(0xf, 0xc01);
> > + mxsfb_write_register(0x20, 0);
> > + mxsfb_write_register(0x21, 0);
>
> There is a kind of magic here. Any way to get #defines or some
> explanation ? I admit that the comment "This can possibly contain 0x111
> *" does not help me a lot ;-)

No, I won't transcript the datasheet here, I see no added value. I added comment
that this stuff is pulled from the OTM2201A datasheet so the reader can look the
meaning of the registers up.

> > + mdelay(30);
> > + mxsfb_write_register(0x10, 0xa00);
> > + mxsfb_write_register(0x11, 0x1038);
> > + mdelay(30);
> > + mxsfb_write_register(0x12, 0x1010);
> > + mxsfb_write_register(0x13, 0x50);
> > + mxsfb_write_register(0x14, 0x4f58);
> > + mxsfb_write_register(0x30, 0);
> > + mxsfb_write_register(0x31, 0xdb);
> > + mxsfb_write_register(0x32, 0);
> > + mxsfb_write_register(0x33, 0);
> > + mxsfb_write_register(0x34, 0xdb);
> > + mxsfb_write_register(0x35, 0);
> > + mxsfb_write_register(0x36, 0xaf);
> > + mxsfb_write_register(0x37, 0);
> > + mxsfb_write_register(0x38, 0xdb);
> > + mxsfb_write_register(0x39, 0);
> > + mxsfb_write_register(0x50, 0);
> > + mxsfb_write_register(0x51, 0x705);
> > + mxsfb_write_register(0x52, 0xe0a);
> > + mxsfb_write_register(0x53, 0x300);
> > + mxsfb_write_register(0x54, 0xa0e);
> > + mxsfb_write_register(0x55, 0x507);
> > + mxsfb_write_register(0x56, 0);
> > + mxsfb_write_register(0x57, 3);
> > + mxsfb_write_register(0x58, 0x90a);
> > + mxsfb_write_register(0x59, 0xa09);
> > + mdelay(30);
> > + mxsfb_write_register(7, 0x1017);
> > + mdelay(40);
> > + mxsfb_write_register(0x36, 0xaf);
> > + mxsfb_write_register(0x37, 0);
> > + mxsfb_write_register(0x38, 0xdb);
> > + mxsfb_write_register(0x39, 0);
> > + mxsfb_write_register(0x20, 0);
> > + mxsfb_write_register(0x21, 0);
> > + /* Turn on Framebuffer Upload Mode */
> > + mxsfb_write_byte(0x22, 0);
> > +
> > + writel(LCDIF_CTRL_LCDIF_MASTER | LCDIF_CTRL_DATA_SELECT,
> > + &regs->hw_lcdif_ctrl_set);
> > +}
> > +#endif
> > +
> > +int board_init(void)
> > +{
> > + /* Adress of boot parameters */
> > + gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
>
> Complete OT: these two lines are in each board and often a board_init()
> is written only for them. Maybe we can drop them putting the code
> global. I have not seen a board where bi_boot_params is not set to RAM
> address + 0x100.

Very valid point, feel free to submit a patch and CC me on that!

> > diff --git a/boards.cfg b/boards.cfg
> > index f919c53..85e0c35 100644
> > --- a/boards.cfg
> > +++ b/boards.cfg
> > @@ -198,6 +198,7 @@ zmx25 arm arm926ejs
> > zmx25 syteco
> >
> > imx27lite arm arm926ejs imx27lite
> > logicpd mx27 magnesium arm arm926ejs
> > imx27lite logicpd mx27 mx23_olinuxino
> > arm arm926ejs mx23_olinuxino olimex
> > mxs mx23_olinuxino
> >
> > +xfi3 arm arm926ejs xfi3
> > creative mxs xfi3
>
> Entries here are not sorted. Can you fix it, please ? Thanks !

AARGH, they got unsorted by mx23evk already :-E~ Will add a patch to fix this
and then keep this sorted in these patches here.