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

[Marek Vasut <marex@denx.de>]

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.