[U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR.

[Stefan Roese <sr@denx.de>]

Hi Leonid,

sorry for being so insistent but there are still some issues that need
to be cleaned up in your patch. Please see below...

> --- u-boot-1.1.4-orig/common/cmd_dcr.c	2006-10-12 11:01:32.000000000 -0700
> +++ u-boot-1.1.4/common/cmd_dcr.c	2006-10-12 11:29:15.000000000 -0700
> @@ -106,6 +106,122 @@ int do_setdcr (cmd_tbl_t * cmdtp, int fl
>  	return 0;
>  }
>
> +/* =======================================================================
> + * Interpreter command to retrieve an register value through AMCC PPC 4xx
> + * Device Control Register inderect addressing.
> + * =======================================================================
> + */
> +int do_getidcr ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[] )
> +{
> +  unsigned long  get_dcr (unsigned short);

Please use tabs for indentation. Indentation size is 8 by the way.
This is a problem in the complete patch.

> +  unsigned long  set_dcr (unsigned short, unsigned long);
> +  unsigned short adr_dcrn;	/* Device Control Register Num for Address */
> +  unsigned short dat_dcrn;	/* Device Control Register Num for Data */
> +  unsigned short offset;      /* Register's offset */
> +  unsigned long  value;	/* register's value */
> +  unsigned char *ptr=NULL;

Space before and after the "=".

> +  unsigned char  buf[80];
> +
> +  /* Validate arguments */
> +  if (argc < 3) {
> +    printf ("Usage:\n%s\n", cmdtp->usage);
> +    return 1;
> +  }
> +
> +  /* Find out whether ther is '.' (dot) symbol in the first parameter. */
> +  strncpy(buf, argv[1], sizeof(buf)-1);
> +  buf[sizeof(buf)-1]=0; /* will guarantee zero-end string */
> +  ptr  = strchr(buf, '.');

Why are here 2 spaces before the "="? It's not aligned to another
line of code. Please use just one space.

> +
> +  if(ptr != NULL) {

A space after the "if" please.

> +    /* first parameter has format adr_dcrn.dat_dcrn */
> +    * ptr = 0; /* erase '.', create zero-end string */

	*ptr = 0;

> +    ptr++; /* will point to dat_dcr */

And why not:

	*ptr++ = 0;

> +    adr_dcrn = (unsigned short) simple_strtoul (buf, NULL, 16);
> +    dat_dcrn = (unsigned short) simple_strtoul (ptr, NULL, 16);
> +  } else {
> +    /* first parameter has format adr_dcrn; dat_dcrn will be
> +       calculated as adr_dcrn+1. */
> +    adr_dcrn = (unsigned short) simple_strtoul (buf, NULL, 16);
> +    dat_dcrn = adr_dcrn+1;
> +  }
> +
> +  /* Register's offset */
> +  offset = (unsigned short) simple_strtoul (argv[2], NULL, 16);
> +
> +  /* Disable interrupts */
> +  disable_interrupts();
> +  /* Set offset */
> +  set_dcr(adr_dcrn, offset);
> +  /* get data */
> +  value = get_dcr(dat_dcrn);
> +  /* Enable interrupts */
> +  enable_interrupts();
> +
> +  printf ("%04x.%04x-%04x Read  %08lx\n", adr_dcrn, dat_dcrn, offset, value);
> + 
> +  return 0;
> +}
> +
> +/* =======================================================================
> + * Interpreter command to update an register value through AMCC PPC 4xx
> + * Device Control Register inderect addressing.
> + * =======================================================================
> + */
> +int do_setidcr (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
> +{
> +  unsigned long  get_dcr (unsigned short);
> +  unsigned long  set_dcr (unsigned short, unsigned long);
> +  unsigned short adr_dcrn;	/* Device Control Register Num for Address */
> +  unsigned short dat_dcrn;	/* Device Control Register Num for Data */
> +  unsigned short offset;      /* Register's offset */
> +  unsigned long  value;	/* register's value */
> +  unsigned char *ptr=NULL;
> +  unsigned char  buf[80];
> +
> +  /* Validate arguments */
> +  if (argc < 4) {
> +    printf ("Usage:\n%s\n", cmdtp->usage);
> +    return 1;
> +  }
> +
> +  /* Find out whether ther is '.' (dot) symbol in the first parameter. */
> +  strncpy(buf, argv[1], sizeof(buf)-1);
> +  buf[sizeof(buf)-1]=0; /* will guarantee zero-end string */
> +  ptr  = strchr(buf, '.');
> +
> +  if(ptr != NULL) {

Space after if again.

> +    /* first parameter has format adr_dcrn.dat_dcrn */
> +    * ptr = 0; /* erase '.', create zero-end string */
> +    ptr++; /* will point to dat_dcr */

	*ptr++ = 0;

> +    adr_dcrn = (unsigned short) simple_strtoul (buf, NULL, 16);
> +    dat_dcrn = (unsigned short) simple_strtoul (ptr, NULL, 16);
> +  } else {
> +    /* first parameter has format adr_dcrn; dat_dcrn will be
> +       calculated as adr_dcrn+1. */
> +    adr_dcrn = (unsigned short) simple_strtoul (buf, NULL, 16);
> +    dat_dcrn = adr_dcrn+1;
> +  }
> +
> +  /* Register's offset */
> +  offset = (unsigned short) simple_strtoul (argv[2], NULL, 16);
> +  /* New value */
> +  value  = (unsigned  long) simple_strtoul (argv[3], NULL, 16);
> +
> +  /* Disable interrupts */
> +  disable_interrupts();
> +  /* Set offset */
> +  set_dcr(adr_dcrn, offset);
> +  /* set data */
> +  set_dcr(dat_dcrn, value);
> +  /* Enable interrupts */
> +  enable_interrupts();
> +
> +  printf ("%04x.%04x-%04x Write %08lx\n", adr_dcrn, dat_dcrn, offset, value);
> + 
> +  return 0;
> +}
> +
>  /***************************************************/
>
>  U_BOOT_CMD(
> @@ -119,4 +235,16 @@ U_BOOT_CMD(
>  	"dcrn - set a DCR's value.\n"
>  );
>
> +U_BOOT_CMD(
> +	getidcr,	3,	1,	do_getidcr,
> +	"getidcr - Get a register value via indirect DCR addressing\n",
> +	"adr_dcrn[.dat_dcrn] offset - write offset to adr_dcrn, read value from
> dat_dcrn.\n" +);
> +
> +U_BOOT_CMD(
> +	setidcr,	4,	1,	do_setidcr,
> +	"setidcr - Set a register value via indirect DCR addressing\n",
> +	"adr_dcrn[.dat_dcrn] offset value - write offset to adr_dcrn, write value
> to dat_dcrn.\n" +);
> +
>  #endif /* CONFIG_4xx & CFG_CMD_SETGETDCR */

Please fix the issues mentioned above and resubmit your patch. Thanks.

Best regards,
Stefan