[U-Boot-Users] PATCH: bootsplash for pxa

[himba <himba@siol.net>]

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 <common.h>
>  #include <bmp_layout.h>
>  #include <command.h>
> +#include <linux/byteorder/little_endian.h>
>  
> 
> You should NEVER  include  linux/byteorder/little_endian.h!!!  Always
> include <asm/byteorder.h> 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 <linux/types.h>
>  #include <devices.h>
>  #include <asm/arch/pxa-regs.h>
> +#include <linux/byteorder/little_endian.h>
>  
> 
> 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<<le16_to_cpu (bmp->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