* [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR. @ 2006-10-05 23:17 Leonid 2006-10-07 10:46 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Leonid @ 2006-10-05 23:17 UTC (permalink / raw) To: u-boot DESCRIPTION: Existing getdcr and setdcr monitor functions don't allow indirect access to PPC440 registers in one step. It's still possible theoretically use setdcr to write offset into correct address DCRN and then read/write from data DCRN, but that doesn't work if address got updated in the middle. For instance, I never could read value of SDR0_PINSTP (address/data DCRNs 0xE/0xF, offset 0x40) - immediately after me somebody changed address DCRN to 0x20. The solution is to implement new special functions getidcr/setidcr for indirect access with following format: getidcr adr_dcrn[.dat_dcrn] offset setidcr adr_dcrn[.dat_dcrn] offset value dat_dcrn will be set adr_dcrn+1 if omitted. Examples (Yosemite PPC440EP board has been used): 1. Read SDR0_PINSTP: => getidcr e 40 000e.000f-0040 Read e0000000 2. Read SDR0_CUST0, update it and read back: => getidcr e 4000 000e.000f-4000 Read 40082350 => setidcr e.f 4000 50082350 000e.000f-4000 Write 50082350 => getidcr e 4000 000e.000f-4000 Read 50082350 CHANGELOG entry: * Add monitor functions for indirect access to PPC440 registers via Data Control Register (DCR). PATCH: (diff file ppc440dcr.txt attached). Signed-off-by: Leonid Baryudin <leonid@a-k-a.net> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: ppc440dcr.txt Url: http://lists.denx.de/pipermail/u-boot/attachments/20061005/8696d0a0/attachment.txt ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR. 2006-10-05 23:17 [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR Leonid @ 2006-10-07 10:46 ` Stefan Roese 2006-10-12 18:38 ` Leonid 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2006-10-07 10:46 UTC (permalink / raw) To: u-boot Hi Leonid, On Friday 06 October 2006 01:17, Leonid wrote: > * Add monitor functions for indirect access to PPC440 registers > via Data Control Register (DCR). > > PATCH: (diff file ppc440dcr.txt attached). Please find some comments below... > ==== //depot/Alchemy/ppc/uboot/u-boot-1.1.4/common/cmd_dcr.c#1 - /home/leonid/pd/ppc/uboot/u-boot-1.1.4/common/cmd_dcr.c ==== > @@ -104,10 +104,126 @@ Please generate the patch as described in the README: diff -purN OLD NEW Or create a git patch. > } while (nbytes); > > 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); > + 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, buf[80]; Only tabs for indentation. > + > + /* 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); Indentation. > + buf[sizeof(buf)-1]=0; /* will guarantee zero-end string */ > + ptr = strchr(buf, '.'); > + > + if(ptr != NULL) > + {/* first parameter has fromat adr_dcrn.dat_dcrn */ Opening brace in the line of the if statement please. > + * ptr = 0; /* erase '.', create zero-end string */ > + ptr++; /* will point to dat_dcr */ > + adr_dcrn = (unsigned short) simple_strtoul (buf, NULL, 16); > + dat_dcrn = (unsigned short) simple_strtoul (ptr, NULL, 16); > + } > + else > + { /* first parameter has fromat adr_dcrn; dat_dcrn will be > + calculated as adr_dcrn+1. */ Please use: } else { I'm stopping here. Please recheck you patch regarding the coding style. You could of course use something like "Lindent" to help you here. <snip> Please fix the above mentioned issues and resubmit a new patch. Thanks. Best regards, Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR. 2006-10-07 10:46 ` Stefan Roese @ 2006-10-12 18:38 ` Leonid 2006-10-12 18:58 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Leonid @ 2006-10-12 18:38 UTC (permalink / raw) To: u-boot On Saturday, October 07, 2006 3:46 AM Stefan Roese wrote: > Please generate the patch as described in the README: > diff -purN OLD NEW Done, attached (ppc440dcr_diff.txt). > Only tabs for indentation. Done. > Opening brace in the line of the if statement please. Done. > Please use: > } else { Done. > Please fix the above mentioned issues and resubmit a new patch. Thanks. Done; see below, after asterisks. Best regards, Leonid. ************************************************************************ *** DESCRIPTION: Existing getdcr and setdcr monitor functions don't allow indirect access to PPC440 registers in one step. It's still possible theoretically use setdcr to write offset into correct address DCRN and then read/write from data DCRN, but that doesn't work if address got updated in the middle. For instance, I never could read value of SDR0_PINSTP (address/data DCRNs 0xE/0xF, offset 0x40) - immediately after me somebody changed address DCRN to 0x20. The solution is to implement new special functions getidcr/setidcr for indirect access with following format: getidcr adr_dcrn[.dat_dcrn] offset setidcr adr_dcrn[.dat_dcrn] offset value dat_dcrn will be set adr_dcrn+1 if omitted. Examples (Yosemite PPC440EP board has been used): 1. Read SDR0_PINSTP: => getidcr e 40 000e.000f-0040 Read e0000000 2. Read SDR0_CUST0, update it and read back: => getidcr e 4000 000e.000f-4000 Read 40082350 => setidcr e.f 4000 50082350 000e.000f-4000 Write 50082350 => getidcr e 4000 000e.000f-4000 Read 50082350 CHANGELOG entry: * Add monitor functions for indirect access to PPC440 registers via Data Control Register (DCR). PATCH: (diff file ppc440dcr_diff.txt attached). Signed-off-by: Leonid Baryudin <leonid@a-k-a.net> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: ppc440dcr_diff.txt Url: http://lists.denx.de/pipermail/u-boot/attachments/20061012/76efea57/attachment.txt ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR. 2006-10-12 18:38 ` Leonid @ 2006-10-12 18:58 ` Stefan Roese 2006-10-12 20:55 ` Leonid 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2006-10-12 18:58 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR. 2006-10-12 18:58 ` Stefan Roese @ 2006-10-12 20:55 ` Leonid 2006-10-25 8:13 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Leonid @ 2006-10-12 20:55 UTC (permalink / raw) To: u-boot On Thursday, October 12, 2006 11:58 AM Stefan Roese wrote: > sorry for being so insistent but there are still some issues that need > to be cleaned up in your patch. Please see below... No worries, quite opposite, I appreciate your comments. > Please use tabs for indentation. Indentation size is 8 by the way. > This is a problem in the complete patch. Well, I kind of did, but looks like xemacs, I'm using has different tab configuration. I've inserted tabs using vi, now they are 8 spaces each. > Space before and after the "=". Done. > Why are here 2 spaces before the "="? It's not aligned to another > line of code. Please use just one space. Done. > *ptr = 0; > > + ptr++; /* will point to dat_dcr */ > And why not: > *ptr++ = 0; Done. > Space after if again. Done. ************************************************************************ *** DESCRIPTION: Existing getdcr and setdcr monitor functions don't allow indirect access to PPC440 registers in one step. It's still possible theoretically use setdcr to write offset into correct address DCRN and then read/write from data DCRN, but that doesn't work if address got updated in the middle. For instance, I never could read value of SDR0_PINSTP (address/data DCRNs 0xE/0xF, offset 0x40) - immediately after me somebody changed address DCRN to 0x20. The solution is to implement new special functions getidcr/setidcr for indirect access with following format: getidcr adr_dcrn[.dat_dcrn] offset setidcr adr_dcrn[.dat_dcrn] offset value dat_dcrn will be set adr_dcrn+1 if omitted. Examples (Yosemite PPC440EP board has been used): 1. Read SDR0_PINSTP: => getidcr e 40 000e.000f-0040 Read e0000000 2. Read SDR0_CUST0, update it and read back: => getidcr e 4000 000e.000f-4000 Read 40082350 => setidcr e.f 4000 50082350 000e.000f-4000 Write 50082350 => getidcr e 4000 000e.000f-4000 Read 50082350 CHANGELOG entry: * Add monitor functions for indirect access to PPC440 registers via Data Control Register (DCR). PATCH: (diff file ppc440dcr_diff.txt attached). Signed-off-by: Leonid Baryudin <leonid@a-k-a.net> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: ppc440dcr_diff.txt Url: http://lists.denx.de/pipermail/u-boot/attachments/20061012/7da2b3a6/attachment.txt ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR. 2006-10-12 20:55 ` Leonid @ 2006-10-25 8:13 ` Stefan Roese 0 siblings, 0 replies; 6+ messages in thread From: Stefan Roese @ 2006-10-25 8:13 UTC (permalink / raw) To: u-boot On Thursday 12 October 2006 22:55, Leonid wrote: > * Add monitor functions for indirect access to PPC440 registers > via Data Control Register (DCR). Applied. Thanks. Viele Gr??e, Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-10-25 8:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-10-05 23:17 [U-Boot-Users] [PATCH] Indirect register access through PPC440 DCR Leonid 2006-10-07 10:46 ` Stefan Roese 2006-10-12 18:38 ` Leonid 2006-10-12 18:58 ` Stefan Roese 2006-10-12 20:55 ` Leonid 2006-10-25 8:13 ` Stefan Roese
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox