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

Stefan Roese
2006-10-12T18:58:05Z

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