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

[Michael Bendzick]

I would agree that some kind of commonality should be introduced for
framebuffer functions.  The work that I've been doing on a framebuffer
driver for the Innovator 1510 iss heavily based on the PPC lcd.c code as
well.  Naturally, I've stripped out the PPC memory map structure, but
haven't really touched functions that do things like setting the palette,
drawing letters, etc.  I'd have to examine the code a little more closely
when I don't have a different project taking up my time to recommend a good
split between CPU specific and Framebuffer specific code.

-Michael Bendzick

-----Original Message-----
From: himba [mailto:himba at siol.net]
Sent: Thursday, August 05, 2004 4:25 PM
To: Wolfgang Denk
Cc: u-boot-users
Subject: Re: [U-Boot-Users] PATCH: bootsplash for pxa


Wolfgang Denk wrote:
> Dear Himba,
> 
> in message <41125EF9.40607 at 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


-------------------------------------------------------
This SF.Net email is sponsored by OSTG. Have you noticed the changes on
Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now,
one more big change to announce. We are now OSTG- Open Source Technology
Group. Come see the changes on the new OSTG site. www.ostg.com
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users