Dear Heydeck, Klaus-Jürgen,
In message <dc2c564da65cfa4aacc2c8cbf9129ccc0441403...@exmbx1.kiebackpeter.kup>
you wrote:
> also preparation for using hwconfig and device tree support
>
> Signed-off-by: Klaus Heydeck <heyd...@kieback-peter.de>
Please consider using git-format-patch / git-send-email for submitting
patches.
> diff -purN u-boot.git/board/kup/common/kup.c u-boot/board/kup/common/kup.c
> --- u-boot.git/board/kup/common/kup.c 2010-02-16 09:02:08.000000000 +0100
> +++ u-boot/board/kup/common/kup.c 2010-02-17 13:03:51.000000000 +0100
> @@ -24,6 +24,24 @@
> #include <common.h>
> #include <mpc8xx.h>
> #include "kup.h"
> +#ifdef CONFIG_KUP4_LOGO
> + #include "s1d13706.h"
> +#endif
> +
> +#undef DEBUG
> +#ifdef DEBUG
> +# define debugk(fmt,args...) printf(fmt ,##args)
> +#else
> +# define debugk(fmt,args...)
> +#endif
Please drop this. Use the standard debug() macro instead which does
the same.
> +#define PRINTF(fmt,args...) printf(fmt ,##args)
This seems bogus to me. Please drop it.
> @@ -31,7 +49,7 @@ int misc_init_f (void)
> volatile sysconf8xx_t *siu = &immap->im_siu_conf;
>
> while (siu->sc_sipend & 0x20000000) {
> - /* printf("waiting for 5V VCC\n"); */
> + debugk("waiting for 5V VCC\n");
Please use TABs only for indentation.
> @@ -40,6 +58,14 @@ int misc_init_f (void)
> immap->im_ioport.iop_papar &= ~(PA_RS485);
> immap->im_ioport.iop_paodr &= ~(PA_RS485);
> immap->im_ioport.iop_padir |= (PA_RS485);
> +
> + /* IO Reset min 1 msec */
> + immap->im_ioport.iop_padat |= (PA_RESET_IO_01 | PA_RESET_IO_02);
> + immap->im_ioport.iop_papar &= ~(PA_RESET_IO_01 | PA_RESET_IO_02);
> + immap->im_ioport.iop_paodr &= ~(PA_RESET_IO_01 | PA_RESET_IO_02);
> + immap->im_ioport.iop_padir |= (PA_RESET_IO_01 | PA_RESET_IO_02);
> + udelay(1000);
> + immap->im_ioport.iop_padat &= ~(PA_RESET_IO_01 | PA_RESET_IO_02);
I am aware that the rest of the existing code is not any better, but we
should consider changing all this to using proper I/O accessors.
> +unsigned char swapbyte(unsigned char c)
> +{
> + unsigned char result=0;
> + int i=0;
> + for(i=0;i<8;++i){
> + result=result<<1;
> + result|=(c&1);
> + c=c>>1;
> + }
> + return result;
> +}
Please make this and similar functions "static".
> +/*------------------------------------------------------------------------>
> ----- */
> +/* Initialize the chip and the frame buffer driver. */
> +/*------------------------------------------------------------------------>
> ----- */
Incorrect multiline comment style.
> + /*
> + * Init ChipSelect #5 (S1D13768)
> + */
> + memctl->memc_or5 = CONFIG_SYS_OR5;
> + memctl->memc_br5 = CONFIG_SYS_BR5;
> + __asm__ ("eieio");
Please use I/O accessors instead.
> + fb_info.VmemAddr = (unsigned char *) (S1D_PHYSICAL_VMEM_ADDR);
> + fb_info.RegAddr = (unsigned char *) (S1D_PHYSICAL_REG_ADDR);
> +
> + if ((((S1D_VALUE *) fb_info.RegAddr)[0] != 0x28)
> + || (((S1D_VALUE *) fb_info.RegAddr)[1] != 0x14)) {
Please use I/O accessors instead. Please fix globally.
> + ((S1D_VALUE*)fb_info.RegAddr)[0xA8] = 0x00;
> + ((S1D_VALUE*)fb_info.RegAddr)[0xA9] = 0x80;
> + unsigned char s1d1gpio = ((S1D_VALUE*)fb_info.RegAddr)[0xAC];
Please do not declare variables in the middle of the code.
> + /* printf("s1d1gpio:0x%02X\n",s1d1gpio); */
Please do not add dead code. Drop this.
> + switch (s1d1gpio){
> +
> + case 0x02: /* STN */
> + for (i = 0; i < sizeof (aS1DRegs_stn) / sizeof (aS1DRegs_>
> stn[0]); i++)
> + {
> + s1dReg = aS1DRegs_stn[i].Index;
> + s1dValue = aS1DRegs_stn[i].Value;
> + ((S1D_VALUE *) fb_info.RegAddr)[s1dReg / sizeof (S1>
> D_VALUE)] = s1dValue;
> + }
Incorrect brace style / indentation. Please fix globally.
Lines too long. Please fix globally.
> + switch (bd->bi_busfreq){
> + case 40000000:
> + ((S1D_VALUE *) fb_info.RegAddr)[0x05] = 0x32;
> + ((S1D_VALUE *) fb_info.RegAddr)[0x12] = 0x41;
> + break;
> + case 48000000:
> + ((S1D_VALUE *) fb_info.RegAddr)[0x05] = 0x22;
> + ((S1D_VALUE *) fb_info.RegAddr)[0x12] = 0x34;
> + break;
What happens in case of rounding errors or the like?
> + default:
> + printf ("KUP4 S1D1: unknown busfrequency: %ld assum>
> ing 64 MHz\n", bd->bi_busfreq);
> + case 64000000:
> + ((S1D_VALUE *) fb_info.RegAddr)[0x05] = 0x32;
> + ((S1D_VALUE *) fb_info.RegAddr)[0x12] = 0x66;
For conistency, please move the default case down.
All these magic indexes and values make not much sense to me.
Please convert the code to use a proper C struct to describe the LCD
controller, use I/O accessors to access it, and define some symbolic
values for these constants.
> + /* create and set colormap */
> + rs = 256 / (r - 1);
> + gs = 256 / (g - 1);
> + bs = 256 / (b - 1);
> + for (i = 0; i < 256; i++) {
> + r1 = (rs * ((i / (g * b)) % r)) * 255;
> + g1 = (gs * ((i / b) % g)) * 255;
> + b1 = (bs * ((i) % b)) * 255;
> + debugk ("%d %04x %04x %04x\n", i, r1 >> 4, g1 >> 4, b1 >> 4>
> );
> + S1D_WRITE_PALETTE (fb_info.RegAddr, i, (r1 >> 4), (g1 >> 4),
> + (b1 >> 4));
Reads a bit like black mgic to me. Please add some comments to explain
what you are doing.
> diff -purN u-boot.git/board/kup/common/s1d13706.h u-boot/board/kup/common/s>
> 1d13706.h
> --- u-boot.git/board/kup/common/s1d13706.h 1970-01-01 01:00:00.0000000>
> 00 +0100
> +++ u-boot/board/kup/common/s1d13706.h 2010-02-17 13:03:51.000000000 +0100
> @@ -0,0 +1,237 @@
> +/*------------------------------------------------------------------------>
> ---- */
> +/* */
> +/* File generated by S1D13706CFG.EXE */
> +/* */
> +/* Copyright (c) 2000,2001 Epson Research and Development, Inc. */
> +/* All rights reserved. */
This license is not compatible with GPL. We cannot accept this file.
> +#define S1D_WRITE_PALETTE(p,i,r,g,b) \
> +{ \
> + ((volatile S1D_VALUE*)(p))[0x0A/sizeof(S1D_VALUE)] = (S1D_VALUE)((r)>
> >>4); \
> + ((volatile S1D_VALUE*)(p))[0x09/sizeof(S1D_VALUE)] = (S1D_VALUE)((g)>
> >>4); \
> + ((volatile S1D_VALUE*)(p))[0x08/sizeof(S1D_VALUE)] = (S1D_VALUE)((b)>
> >>4); \
> + ((volatile S1D_VALUE*)(p))[0x0B/sizeof(S1D_VALUE)] = (S1D_VALUE)(i);> \
> +}
> +
> +#define S1D_READ_PALETTE(p,i,r,g,b) \
> +{ \
> + ((volatile S1D_VALUE*)(p))[0x0F/sizeof(S1D_VALUE)] = (S1D_VALUE)(i);> \
> + r = ((volatile S1D_VALUE*)(p))[0x0E/sizeof(S1D_VALUE)]; \
> + g = ((volatile S1D_VALUE*)(p))[0x0D/sizeof(S1D_VALUE)]; \
> + b = ((volatile S1D_VALUE*)(p))[0x0C/sizeof(S1D_VALUE)]; \
> +}
Please use I/O accessors instead.
> +typedef struct
> +{
> + S1D_INDEX Index;
> + S1D_VALUE Value;
> +} S1D_REGS;
> +
> +
> +static S1D_REGS aS1DRegs_stn[] =
> +{
> + {0x04,0x10}, /* BUSCLK MEMCLK Config Register */
> + {0x10,0xD0}, /* PANEL Type Register */
> + {0x11,0x00}, /* MOD Rate Register */
> + {0x14,0x27}, /* Horizontal Display Period Register */
NAK. Please use a proper C struct to describe the register layout.
> diff -purN u-boot.git/board/kup/kup4k/kup4k.c u-boot/board/kup/kup4k/kup4k.c
> --- u-boot.git/board/kup/kup4k/kup4k.c 2010-02-16 09:02:08.000000000 +0100
> +++ u-boot/board/kup/kup4k/kup4k.c 2010-02-17 13:03:51.000000000 +0100
...
> + immap->im_memctl.memc_or4 = CONFIG_SYS_OR4;
> + immap->im_memctl.memc_br4 = CONFIG_SYS_BR4;
> + __asm__ ("eieio");
> + latch = (uchar *)0x90000200;
> + tmp = swapbyte (*latch);
> + rev = (tmp & 0xF8) >> 3;
> + mod = (tmp & 0x07);
> +
> + i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> +
> + if (read_diag())
> + gd->flags &= ~GD_FLG_SILENT;
> +
> + printf ("Board: KUP4K Rev %d.%d AK:",rev,mod);
> +
> + /* TI Application report: Before using the IO as an input,
> + * a high must be written to the IO first
> + */
> + pcf = 0xFF;
> + i2c_write (0x21, 0, 0 , &pcf, 1);
> + if (i2c_read (0x21, 0, 0, &pcf, 1)) {
> + puts ("n/a\n");
> + }
> + else {
Style issues: indentation not by TAB, incorrect brace style,
incorrect multiline comment style. Please fix globally.
> + if(rev >= 7){
> + char* arguments[3]={"memtest","0x00000000","0x05FFFFFF"};
> + char *s;
> + size = 32 * 3 * 1024 * 1024;
> + memctl->memc_or1 = CONFIG_SYS_OR1_9COL;
> + memctl->memc_br1 = CONFIG_SYS_BR1_9COL;
> + memctl->memc_or2 = CONFIG_SYS_OR2_9COL;
> + memctl->memc_br2 = CONFIG_SYS_BR2_9COL;
> + memctl->memc_or3 = CONFIG_SYS_OR3_9COL;
> + memctl->memc_br3 = CONFIG_SYS_BR3_9COL;
> + s = getenv ("memtest");
> + if(s)
> + do_mem_mtest(0,0,3,arguments);
> + }
> + else{
> + char* arguments[3]={"memtest","0x00000000","0x02FFFFFF"};
> + char *s;
> + size = 16 * 3 * 1024 * 1024;
> + memctl->memc_or1 = CONFIG_SYS_OR1_8COL;
> + memctl->memc_br1 = CONFIG_SYS_BR1_8COL;
> + memctl->memc_or2 = CONFIG_SYS_OR2_8COL;
> + memctl->memc_br2 = CONFIG_SYS_BR2_8COL;
> + memctl->memc_or3 = CONFIG_SYS_OR3_8COL;
> + memctl->memc_br3 = CONFIG_SYS_BR3_8COL;
> + s = getenv ("memtest");
> + if(s)
> + do_mem_mtest(0,0,3,arguments);
> + }
This is mostly repeated code with a single parameter actually. Please
consider factoring out into a function.
> int misc_init_r (void)
> {
> -#ifdef CONFIG_STATUS_LED
> + DECLARE_GLOBAL_DATA_PTR;
This does not work. DECLARE_GLOBAL_DATA_PTR must be done on file
level, not function level.
> +#if 0
> + if ((char *p = getenv ("contrast")) != NULL) {
> + char buffer[64];
> + unsigned long contrast = simple_strtoul (p, NULL, 10) * 1> 27
> / 100;
> + sprintf(buffer,"%x",contrast);
> + char* arguments[4]={"imw","2E","0.0",buffer};
> + do_i2c_mw(0,0,4,arguments);
> + }
> +#endif
Please do not add dead code.
> + #define PC4 0x0800
> + #define PC5 0x0400
Please unindent. And please consider moving thse defines into your
header file.
> + int diag;
> +
Drop this blank line.
> + immap_t *immr = (immap_t *)CONFIG_SYS_IMMR;
And add one here (i. e. after the declarations, before the code).
> + if(immr->im_ioport.iop_pcdat & PC4){
if (...) {
Please mind spacing.
> +/*
> + * Device Tree Support
> + */
> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT)
> +int fdt_set_node_and_value (void *blob,
Copied from board/keymile/common/common.c ?
> +int fdt_del_node_name (void *blob, char *nodename) {
...
> +int fdt_del_prop_name (void *blob, char *nodename, char *propname) {
...
These fdt_* functions should be factored out and added to libfdt
instead.
> diff -purN u-boot.git/include/configs/KUP4K.h u-boot/include/configs/KUP4K.h
> --- u-boot.git/include/configs/KUP4K.h 2010-02-16 09:02:08.000000000 +0100
> +++ u-boot/include/configs/KUP4K.h 2010-02-17 13:03:51.000000000 +0100
...
> #ifdef CONFIG_POST
> #define CONFIG_CMD_DIAG
> #endif
>
> +
> +
> +
Excessive white space. Please fix globally.
> @@ -197,8 +192,10 @@
> #define CONFIG_SYS_MAXARGS 16 /* max number of command args
> */
> #define CONFIG_SYS_BARGSIZE CONFIG_SYS_CBSIZE /* Boot Argument
> Buffer Size */
>
> -#define CONFIG_SYS_MEMTEST_START 0x000400000 /* memtest works on
> */
> -#define CONFIG_SYS_MEMTEST_END 0x002C00000 /* 4 ... 44 MB in
> DRAM */
> +#define CONFIG_SYS_MEMTEST_START 0x000400000 /* memtest works on
> */
> +#define CONFIG_SYS_MEMTEST_END 0x005C00000 /* 4 ... 92 MB in
> DRAM */
Hm... this is inconsistent with your do_mem_mtest() calls above!
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
It is impractical for the standard to attempt to constrain the
behavior of code that does not obey the constraints of the standard.
- Doug Gwyn
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
