[U-Boot] [PATCH 3/4] ppc4xx: Rework cmd reginfo

[Stefan Roese <sr@denx.de>]

Hi Niklaus,

On Friday 02 October 2009 20:12:10 Niklaus Giger wrote:
> The command "reginfo" got an overhaul for the ppc4xx. It dumps all the
> relevant HW configuration registers (address, symbolic name, content).
> This allows to easily detect errors in *.h files and changes in the HW
> configuration.
> 
> Signed-off-by: Niklaus Giger <niklaus.giger@member.fsf.org>
> ---

<snip>

> diff --git a/cpu/ppc4xx/reginfo.c b/cpu/ppc4xx/reginfo.c
> new file mode 100644
> index 0000000..e84d42f
> --- /dev/null
> +++ b/cpu/ppc4xx/reginfo.c
> @@ -0,0 +1,369 @@
> +/*
> + *(C) Copyright 2005-2009 Netstal Maschinen AG
> + *    Bruno Hars (Bruno.Hars at netstal.com)
> + *    Niklaus Giger (Niklaus.Giger 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
> + */

Comment does not match any more. Please remove of change.

> +#include <common.h>
> +#include <command.h>
> +#include <asm/processor.h>
> +#include <asm/io.h>
> +#include <asm/ppc4xx-uic.h>
> +#include <ppc4xx_enet.h>
> +
> +enum REGISTER_TYPE {
> +	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[] = {
> +	{"PB0CR",		IDCR2,	PB0CR},
> +	{"PB0AP",		IDCR2,	PB0AP},
> +	{"PB1CR",		IDCR2,	PB1CR},
> +	{"PB1AP",		IDCR2,	PB1AP},
> +	{"PB2CR",		IDCR2,	PB2CR},
> +	{"PB2AP",		IDCR2,	PB2AP},
> +	{"PB3CR",		IDCR2,	PB3CR},
> +	{"PB3AP",		IDCR2,	PB3AP},
> +
> +	{"PB4CR",		IDCR2,	PB4CR},
> +	{"PB4AP",		IDCR2,	PB4AP},
> +#if !defined(CONFIG_405EP)
> +	{"PB5CR",		IDCR2,	PB5CR},
> +	{"PB5AP",		IDCR2,	PB5AP},
> +	{"PB6CR",		IDCR2,	PB6CR},
> +	{"PB6AP",		IDCR2,	PB6AP},
> +	{"PB7CR",		IDCR2,	PB7CR},
> +	{"PB7AP",		IDCR2,	PB7AP},
> +#endif

I though a bit about this array with the #ifdef's for the PPC4xx variants. My 
idea to simplify this (without #ifdef in this array) was to create a macro 
which only expands into variable declaration, if the corresponding register 
macro was defined at all. So we could put all registers in this declaration 
array, and only the platforms that really have them defined (in pcc405.h etc) 
will expand them into read declarations.

Something like this:

#define REG_DEF(reg, type, addr) \
  #if defined(reg) { "reg", type, addr } #endif

But this would need an "#if" in the macro which doesn't seem to be possible. 
Not sure if there is another more elegant way to handle this.

If this is not possible at all, we need to use your solution with the 
#ifdef's.

<snip>

> +/*
> + * CPU Register dump of PPC440EPx
> + * Output: first all DCR-registers, then in order of struct ppc440epx_reg
> + */

Again, comment is not correct any more.

> +#define printDcr(dcr) 	printf("0x%08x %-16s: 0x%08x\n", dcr,#dcr,
>  mfdcr(dcr));

Macros should be upper case.

> +
> +void ppc4xx_reginfo(void)
> +{
> +	unsigned int i;
> +	unsigned int n;
> +	u32 value;
> +	enum REGISTER_TYPE type;

Please add an empty line here.

> +#if defined (CONFIG_405EP)
> +	printf("Dump PPC405EP HW configuration registers\n\n");
> +#elif CONFIG_405GP
> +	printf ("Dump 405GP HW configuration registers\n\n");
> +#elif CONFIG_440EPX
> +	printf("Dump PPC440EPx HW configuration registers\n\n");
> +#endif

I would prefer if you would add a define above:

#if defined (CONFIG_405EP)
#define PPC_VARIANT	"405EP"
#elif CONFIG_405GP
#define PPC_VARIANT	"405GP"
#elif CONFIG_440EPX
#define PPC_VARIANT	"440EPx"
#endif

and print this here:

	printf("Dump %s HW configuration registers\n\n", PPC_VARIANT);

Or we could even save the processor type upon PVR detection (cpu.c) into a 
global variable and use it here. No #ifdef needed here at all.

> +	printf("MSR: 0x%08x\n", mfmsr());
> +
> +	printf ("\nUniversal Interrupt Controller Regs\n");
> +	printDcr(UIC0SR);
> +	printDcr(UIC0ER);
> +	printDcr(UIC0CR);
> +	printDcr(UIC0PR);
> +	printDcr(UIC0TR);
> +	printDcr(UIC0MSR);
> +	printDcr(UIC0VR);
> +	printDcr(UIC0VCR);
> +
> +#if (UIC_MAX > 1)
> +	printDcr(UIC2SR);
> +	printDcr(UIC2ER);
> +	printDcr(UIC2CR);
> +	printDcr(UIC2PR);
> +	printDcr(UIC2TR);
> +	printDcr(UIC2MSR);
> +	printDcr(UIC2VR);
> +	printDcr(UIC2VCR);
> +#endif
> +
> +#if (UIC_MAX > 2)
> +	printDcr(UIC2SR);
> +	printDcr(UIC2ER);
> +	printDcr(UIC2CR);
> +	printDcr(UIC2PR);
> +	printDcr(UIC2TR);
> +	printDcr(UIC2MSR);
> +	printDcr(UIC2VR);
> +	printDcr(UIC2VCR);
> +#endif
> +
> +#if (UIC_MAX > 3)
> +	printDcr(UIC3SR);
> +	printDcr(UIC3ER);
> +	printDcr(UIC3CR);
> +	printDcr(UIC3PR);
> +	printDcr(UIC3TR);
> +	printDcr(UIC3MSR);
> +	printDcr(UIC3VR);
> +	printDcr(UIC3VCR);
> +#endif
> +
> +#if defined (CONFIG_405EP) || defined (CONFIG_405GP)
> +	printf ("\n\nDMA Channels\n");
> +	printDcr(DMASR);
> +	printDcr(DMASGC);
> +	printDcr(DMAADR);
> +
> +	printDcr(DMACR0);
> +	printDcr(DMACT0);
> +	printDcr(DMADA0);
> +	printDcr(DMASA0);
> +	printDcr(DMASB0);
> +
> +	printDcr(DMACR1);
> +	printDcr(DMACT1);
> +	printDcr(DMADA1);
> +	printDcr(DMASA1);
> +	printDcr(DMASB1);
> +
> +	printDcr(DMACR2);
> +	printDcr(DMACT2);
> +	printDcr(DMADA2);
> +	printDcr(DMASA2);
> +	printDcr(DMASB2);
> +
> +	printDcr(DMACR3);
> +	printDcr(DMACT3);
> +	printDcr(DMADA3);
> +	printDcr(DMASA3);
> +	printDcr(DMASB3);
> +#endif
> +
> +	printf ("\n\nVarious HW-Configuration registers\n");
> +#if defined (CONFIG_440EPX)
> +	printDcr(MAL0_CFG);
> +	printDcr(CPM0_ER);
> +	printDcr(CPM1_ER);
> +	printDcr(PLB4A0_ACR);
> +	printDcr(PLB4A1_ACR);
> +	printDcr(PLB3A0_ACR);
> +	printDcr(OPB2PLB40_BCTRL);
> +	printDcr(P4P3BO0_CFG);
> +#endif
> +	n = sizeof(ppc440epx_reg) / sizeof(ppc440epx_reg[0]);

ARRAY_SIZE() please.

> +	for (i = 0; i < n; i++) {
> +		value = 0;
> +		type = ppc440epx_reg[i].type;
> +		switch (type) {
> +		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 = in_be32((const volatile unsigned __iomem *)
> +					ppc440epx_reg[i].address);
> +			break;
> +		default:
> +			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);
> +	}
> +}
> 

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