Re: [U-Boot] PATCH kup4-boards: minor configuration changes

Dear Heydeck, Klaus-Jürgen,

In message DC2C564DA65CFA4AACC2C8CBF9129CCC0441403195@EXMBX1.kiebackpeter.kup you wrote:

also preparation for using hwconfig and device tree support

Signed-off-by: Klaus Heydeck heydeck@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