From mboxrd@z Thu Jan 1 00:00:00 1970 From: himba Date: Thu, 05 Aug 2004 23:24:55 +0200 Subject: [U-Boot-Users] PATCH: bootsplash for pxa In-Reply-To: <20040805170915.1A0C2C109F@atlas.denx.de> References: <20040805170915.1A0C2C109F@atlas.denx.de> Message-ID: <4112A5A7.8050609@siol.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Wolfgang Denk wrote: > Dear Himba, > > in message <41125EF9.40607@siol.net> you wrote: > >>CHANGELOG: >>Patch by Hinko Kocevar, 05 Aug 2004: >> Add support for bmp command and bootsplash feature under pxa >> Add support for ATAG_VIDEOLFB usage when LCD is used > > > I'm sorry, but I have to reject this patch. > > --- u-boot/lib_arm/armlinux.c~xcep-lcd 2004-08-04 22:40:27.000000000 +0200 > +++ u-boot/lib_arm/armlinux.c 2004-08-04 23:10:20.000000000 +0200 > ... > @@ -365,8 +365,16 @@ > params->hdr.size = tag_size (tag_videolfb); > > params->u.videolfb.lfb_base = (u32) gd->fb_base; > - /* 7168 = 256*4*56/8 - actually 2 pages (8192 bytes) are allocated */ > - params->u.videolfb.lfb_size = 7168; > + /* 80896 = (320*240) + PAGE_SIZE; for 8bpp and not page aligned > + * UPDATE: lcd_setmem is not needed to get fb working, because > + * we are not relocating the code properly. 80896b includes > + * palette also - see pxafb_init_mem() for (al)location! > + * layout of the mem block: > + * FB > + * DMA descriptors > + * palette > + */ > + params->u.videolfb.lfb_size = 80896; > > I cannot accept this. So far, on the TRAB board only 7 kB of memory > were allocated for the VFD buffer, and now you hard-wire 79 kB > instead - more than 10 times more than before! > > I agree that it's bad to have a hardwired value, but it's even worse > to simply overwrite it. Your modification breaks U-Boot on the TRAB > board. Please fix this. > I speculated that it was just too small for any framebuffer out there nowadays, so I took liberty in increasing it. > [I recommend to make the actual value configurable in the board's > config file.] Fine. That would introduce another CONFIG_xxx variable, right? Or maybe we could provide FB driver with simple function that calculates the value upon configured LCD type and returns size on-the-fly instead of hardcoding it again somewhere... > > --- u-boot/common/cmd_bmp.c~xcep-lcd 2004-08-04 22:40:55.000000000 +0200 > +++ u-boot/common/cmd_bmp.c 2004-08-04 23:35:37.000000000 +0200 > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > > You should NEVER include linux/byteorder/little_endian.h!!! Always > include only. Anything else is broken. > Acked. > > --- u-boot/cpu/pxa/pxafb.c~xcep-lcd 2004-08-04 22:40:27.000000000 +0200 > +++ u-boot/cpu/pxa/pxafb.c 2004-08-04 23:34:49.000000000 +0200 > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include > > > Same here. > > @@ -604,7 +643,7 @@ > lcd_line_length = (panel_info.vl_col * NBITS (panel_info.vl_bpix)) / 8; > > lcd_init (lcd_base); /* LCD initialization */ > - > + > > PLEASE DO NOT ADD TRAILING WHITE SPACE!!! > And I thought my double check was enough... > +int lcd_display_bitmap(ulong bmp_image, int x, int y) > +{ > + ushort *cmap; > + ushort i, j; > + uchar *fb; > + bmp_image_t *bmp=(bmp_image_t *)bmp_image; > + uchar *bmap; > + ushort padded_line; > + unsigned long width, height; > + unsigned colors,bpix; > + unsigned long compression; > + struct pxafb_info *fbi = &panel_info.pxa; > + > + > + if (!((bmp->header.signature[0]=='B') && > + (bmp->header.signature[1]=='M'))) { > + printf ("Error: no valid bmp image at %lx\n", bmp_image); > + return 1; > + } > + > + width = le32_to_cpu (bmp->header.width); > + height = le32_to_cpu (bmp->header.height); > + colors = 1<header.bit_count); > + compression = le32_to_cpu (bmp->header.compression); > ... > > > This code looks like a 1:1 copy of the same function in > "cpu/mpc8xx/lcd.c". Instead of duplicating the code, we should factor > it out into a common source file. > It is from "cpu/mpc8xx/lcd.c". This could be put in common/ or drivers/ I presume - all common FB functions could be there. I haven't really compared the mpc8xx and pxa FB drivers in detail, but I think pxa FB is driven out of mpc8xx FB. > > Please fix these issues and resubmit. > OK, I'll try. Will put out RFC here in a while. regards, himba