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