From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Thu, 12 Oct 2006 20:58:05 +0200 Subject: [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR. In-Reply-To: <406A31B117F2734987636D6CCC93EE3C441FC0@ehost011-3.exch011.intermedia.net> References: <406A31B117F2734987636D6CCC93EE3C441FC0@ehost011-3.exch011.intermedia.net> Message-ID: <200610122058.05574.sr@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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