* [U-Boot-Users] PATCH: bootsplash for pxa
@ 2004-08-05 22:57 Michael Bendzick
0 siblings, 0 replies; 5+ messages in thread
From: Michael Bendzick @ 2004-08-05 22:57 UTC (permalink / raw)
To: u-boot
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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot-Users] PATCH: bootsplash for pxa
2004-08-05 21:24 ` himba
@ 2004-08-05 21:55 ` Wolfgang Denk
0 siblings, 0 replies; 5+ messages in thread
From: Wolfgang Denk @ 2004-08-05 21:55 UTC (permalink / raw)
To: u-boot
Dear Himba,
in message <4112A5A7.8050609@siol.net> you wrote:
>
> > - /* 7168 = 256*4*56/8 - actually 2 pages (8192 bytes) are allocated */
> > - params->u.videolfb.lfb_size = 7168;
...
> > + params->u.videolfb.lfb_size = 80896;
...
> I speculated that it was just too small for any framebuffer out there
> nowadays, so I took liberty in increasing it.
You are wrong. There are 4 VFD's on the TRAB board, and this requires
only 7 kB of framebuffer.
> > [I recommend to make the actual value configurable in the board's
> > config file.]
>
> Fine. That would introduce another CONFIG_xxx variable, right?
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...
That's fine with me, too. It just has to work :-)
> > 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.
You are right. But with the other functions there was at least some
minimal difference ;-)
> OK, I'll try. Will put out RFC here in a while.
Thanks a lot!
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-4596-87 Fax: (+49)-8142-4596-88 Email: wd at denx.de
Too much of anything, even love, isn't necessarily a good thing.
-- Kirk, "The Trouble with Tribbles", stardate 4525.6
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot-Users] PATCH: bootsplash for pxa
2004-08-05 17:09 ` Wolfgang Denk
@ 2004-08-05 21:24 ` himba
2004-08-05 21:55 ` Wolfgang Denk
0 siblings, 1 reply; 5+ messages in thread
From: himba @ 2004-08-05 21:24 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot-Users] PATCH: bootsplash for pxa
2004-08-05 16:23 himba
@ 2004-08-05 17:09 ` Wolfgang Denk
2004-08-05 21:24 ` himba
0 siblings, 1 reply; 5+ messages in thread
From: Wolfgang Denk @ 2004-08-05 17:09 UTC (permalink / raw)
To: u-boot
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 recommend to make the actual value configurable in the board's
config file.]
--- 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.
--- 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!!!
+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.
Please fix these issues and resubmit.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-4596-87 Fax: (+49)-8142-4596-88 Email: wd at denx.de
Is a computer language with goto's totally Wirth-less?
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot-Users] PATCH: bootsplash for pxa
@ 2004-08-05 16:23 himba
2004-08-05 17:09 ` Wolfgang Denk
0 siblings, 1 reply; 5+ messages in thread
From: himba @ 2004-08-05 16:23 UTC (permalink / raw)
To: u-boot
Hi,
I'm attaching patch against latest CVS for pxa FB driver which enables
use of bootsplash feature in u-boot.
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
regards,
himba
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pxa-lcd.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20040805/df383d82/attachment.txt
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2004-08-05 22:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-08-05 22:57 [U-Boot-Users] PATCH: bootsplash for pxa Michael Bendzick
-- strict thread matches above, loose matches on Subject: below --
2004-08-05 16:23 himba
2004-08-05 17:09 ` Wolfgang Denk
2004-08-05 21:24 ` himba
2004-08-05 21:55 ` Wolfgang Denk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.