[U-Boot] [PATCH] ppc4xx: Add command 440epx_r
Stefan Roese
sr at denx.de
Thu Sep 24 12:07:03 CEST 2009
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 <niklaus.giger at netstal.com>
> ---
> 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 (Bruno.Hars at netstal.com)
> + *
> + * 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: office at denx.de
More information about the U-Boot
mailing list