On Thursday 24 September 2009 11:23:23 Niklaus Giger wrote: > Adds a command 440epx_r to dump over 140 internal register which > define the HW configuration. Command name shortened from > 440epx_regdump to 440epx_r to align nicely with the help command.
Did you see my last mail about the naming of the command (and its files)? I really thing it makes sense to change this into a more generic name, so that other 4xx variants may use it as well. More comments below. > Handy for documentation and verifying changes. > > Signed-off-by: Niklaus Giger <[email protected]> > --- > cpu/ppc4xx/440epx_regdump.c | 282 > +++++++++++++++++++++++++++++++++++++++++++ cpu/ppc4xx/Makefile | > 4 + > 2 files changed, 286 insertions(+), 0 deletions(-) > create mode 100644 cpu/ppc4xx/440epx_regdump.c > > diff --git a/cpu/ppc4xx/440epx_regdump.c b/cpu/ppc4xx/440epx_regdump.c > new file mode 100644 > index 0000000..0e0478d > --- /dev/null > +++ b/cpu/ppc4xx/440epx_regdump.c > @@ -0,0 +1,282 @@ > +/* > + *(C) Copyright 2005-2009 Netstal Maschinen AG > + * Bruno Hars ([email protected]) > + * > + * This source code is free software; you can redistribute it > + * and/or modify it in source code form under the terms of the GNU > + * General Public License as published by the Free Software > + * Foundation; either version 2 of the License, or (at your option) > + * any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA > 02111-1307, USA + */ > + > +/* > + * cmd_440epx_regdump.c - CPU Register Dump for HCU5 board with PPC440EPx This comment does not fit any more. > + */ > + > +#include <common.h> > +#include <command.h> > +#include <asm/processor.h> > + > +enum REGISTER_TYPE { > + DCR, /* Directly Accessed DCR's */ > + IDCR1, /* Indirectly Accessed DCR via SDRAM0_CFGADDR/SDRAM0_CFGDATA */ > + IDCR2, /* Indirectly Accessed DCR via EBC0_CFGADDR/EBC0_CFGDATA */ > + IDCR3, /* Indirectly Accessed DCR via EBM0_CFGADDR/EBM0_CFGDATA */ > + IDCR4, /* Indirectly Accessed DCR via PPM0_CFGADDR/PPM0_CFGDATA */ > + IDCR5, /* Indirectly Accessed DCR via CPR0_CFGADDR/CPR0_CFGDATA */ > + IDCR6, /* Indirectly Accessed DCR via SDR0_CFGADDR/SDR0_CFGDATA */ > + MM /* Directly Accessed MMIO Register */ > +}; > + > +struct cpu_register { > + char *name; > + enum REGISTER_TYPE type; > + u32 address; > +}; > + > +/* > + * PPC440EPx registers ordered for output > + * name type addr size > + * ------------------------------------------- > + */ > +const struct cpu_register ppc440epx_reg[] = { > + {"EBC0_B0CR", IDCR2, PB0CR}, > + {"EBC0_B1CR", IDCR2, PB1CR}, > + {"EBC0_B2CR", IDCR2, PB2CR}, > + {"EBC0_B3CR", IDCR2, PB3CR}, > + {"EBC0_B4CR", IDCR2, PB4CR}, > + {"EBC0_B5CR", IDCR2, PB5CR}, > + {"EBC0_B0AP", IDCR2, PB0AP}, > + {"EBC0_B1AP", IDCR2, PB1AP}, > + {"EBC0_B2AP", IDCR2, PB2AP}, > + {"EBC0_B3AP", IDCR2, PB3AP}, > + {"EBC0_B4AP", IDCR2, PB4AP}, > + {"EBC0_B5AP", IDCR2, PB5AP}, > + {"EBC0_CFG", IDCR2, 0x23}, EBC0_CFG is defined. Please use it instead of 0x23. > + {"SDR0_SDSTP0", IDCR6, SDR0_SDSTP0}, > + {"SDR0_SDSTP1", IDCR6, SDR0_SDSTP1}, > + {"SDR0_SDSTP2", IDCR6, 0x4001}, > + {"SDR0_SDSTP3", IDCR6, 0x4003}, Those regs should be defines as well. If not please add them to the headers instead. > + {"SDR0_CUST0", IDCR6, SDR0_CUST0}, > + {"SDR0_CUST1", IDCR6, SDR0_CUST1}, > + {"SDR0_EBC0", IDCR6, 0x0100}, > + {"SDR0_AMP0", IDCR6, 0x0240}, > + {"SDR0_AMP1", IDCR6, 0x0241}, Again. Please do this for all regs used in this array. Thanks. <snip> > +/* > + * CPU Register dump of PPC440EPx > + * Output in order of struct ppc440epx_reg > + */ > +int do_440epx_regdump (cmd_tbl_t * cmdtp, int flag, int argc, char > *argv[]) +{ Name change? > + unsigned int i; > + unsigned int n; > + u32 value; > + enum REGISTER_TYPE type; > + > + printf > + ("\nDump PPC440EPx HW configuration registers\n\n"); Is this line split really necessary? Doesn't look too long for me. > + n = sizeof (ppc440epx_reg) / sizeof (ppc440epx_reg[0]); > + for (i = 0; i < n; i++) { > + value = 0; > + type = ppc440epx_reg[i].type; > + switch (type) { > + case DCR: /* Directly Accessed DCR's */ > + switch (ppc440epx_reg[i].address) { > + case 0x00b0: > + value = mfdcr ( 0x00b0 ); Please drop the spaces around the braces: value = mfdcr(0x00b0); Also in the other cases below. > + break; > + case 0x00f0: > + value = mfdcr ( 0x00f0 ); > + break; > + case 0x0180: > + value = mfdcr ( 0x0180 ); > + break; > + case 0x0081: > + value = mfdcr ( 0x0081 ); > + break; > + case 0x0089: > + value = mfdcr ( 0x0089 ); > + break; > + case 0x0077: > + value = mfdcr ( 0x0077 ); > + break; > + case 0x0350: > + value = mfdcr ( 0x0350 ); > + break; > + case 0x0026: > + value = mfdcr ( 0x0026 ); > + break; > + default: > + printf ("\nERROR: unknown DCR address: 0x%x\n", > + ppc440epx_reg[i].address); I would prefer the style func(), without a space before the "(". Most of the code uses this code, so its more consistent. > + break; > + } > + break; > + case IDCR1: /* Indirect via SDRAM0_CFGADDR/DDR0_CFGDATA */ > + mtdcr (SDRAM0_CFGADDR, ppc440epx_reg[i].address); > + value = mfdcr (SDRAM0_CFGDATA); > + break; > + case IDCR2: /* Indirect via EBC0_CFGADDR/EBC0_CFGDATA */ > + mtdcr (EBC0_CFGADDR, ppc440epx_reg[i].address); > + value = mfdcr (EBC0_CFGDATA); > + break; > + case IDCR5: /* Indirect via CPR0_CFGADDR/CPR0_CFGDATA */ > + mtdcr (CPR0_CFGADDR, ppc440epx_reg[i].address); > + value = mfdcr (CPR0_CFGDATA); > + break; > + case IDCR6: /* Indirect via SDR0_CFGADDR/SDR0_CFGDATA */ > + mtdcr (SDR0_CFGADDR, ppc440epx_reg[i].address); > + value = mfdcr (SDR0_CFGDATA); > + break; > + case MM: /* Directly Accessed MMIO Register */ > + value = *(volatile unsigned long*) > ppc440epx_reg[i].address; Please use io accessor function here: in_be32() > + break; > + default: > + printf > + ("\nERROR: struct entry %d: unknown register > type\n", > + i); I don't like this linesplit. Most likely introduced by lindent. Please use something like: printf("\nERROR: struct entry %d: unknown " "register type\n", i); > + break; > + } > + printf ("0x%08x %-16s: 0x%08x\n", ppc440epx_reg[i].address, > + ppc440epx_reg[i].name, value); > + } > + return 0; > +} > + > +/* define do_440epx_regdump as u-boot command */ > +U_BOOT_CMD (440epx_r, 2, 1, do_440epx_regdump, > + "print register information for PPC440EPX processor", > + ""); > diff --git a/cpu/ppc4xx/Makefile b/cpu/ppc4xx/Makefile > index 2050b17..2d623a9 100644 > --- a/cpu/ppc4xx/Makefile > +++ b/cpu/ppc4xx/Makefile > @@ -33,6 +33,10 @@ SOBJS += dcr.o > SOBJS += kgdb.o > > COBJS := 40x_spd_sdram.o > +ifdef CONFIG_440EPX > +COBJS += 440epx_regdump.o > +endif Please don't add this commend unconditionally. We need a config option/switch for this. How about CONFIG_CMD_REGDUMP? And btw, please keep me always on Cc on ppc4xx related patches. Thanks. Cheers, Stefan -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: [email protected] _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
