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@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@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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users