Re: [U-Boot-Users] [PATCH 2/5] Support LATTICE FPGA parts using JTAG programming.
Dear Pantelis,
in message 20061129172539.24638.52486.stgit@pantathon.hol.gr you wrote:
Add support for Lattice FPGA parts programmed using JTAG.
Again, the previous comments on necessary coding style cleanup apply.
Also:
common/Makefile | 4 common/ec.c | 473 ++++++
^^^^^^^^^^^^^^
Huh? What's "ec"? Please use a more descriptive name.
--- /dev/null +++ b/common/lattice_ivm_core.c
...
+* JTAG state machine transistion tragetory table.
Please fix typos ;-)
+struct {
- unsigned char CurState; /* From this state */
- unsigned char NextState; /* Step to this state */
- unsigned char Pattern; /* The tragetory of TMS */
- unsigned char Pulses; /* The number of steps */
+} g_JTAGTransistions[24] = {
- {
- RESET, RESET, 0xFC, 6}, /* Transitions from RESET */
- {
- RESET, IDLE, 0x00, 1}, {
- RESET, DRPAUSE, 0x50, 5}, {
- RESET, IRPAUSE, 0x68, 6}, {
- IDLE, RESET, 0xE0, 3}, /* Transitions from IDLE */
- {
- IDLE, DRPAUSE, 0xA0, 4}, {
- IDLE, IRPAUSE, 0xD0, 5}, {
- DRPAUSE, RESET, 0xF8, 5}, /* Transitions from DRPAUSE */
- {
- DRPAUSE, IDLE, 0xC0, 3}, {
- DRPAUSE, IRPAUSE, 0xF4, 7}, {
- IRPAUSE, RESET, 0xF8, 5}, /* Transitions from IRPAUSE */
- {
- IRPAUSE, IDLE, 0xC0, 3}, {
- IRPAUSE, DRPAUSE, 0xE8, 6}, {
- DRPAUSE, SHIFTDR, 0x80, 2}, /* Extra transitions using SHIFTDR */
- {
- IRPAUSE, SHIFTDR, 0xE0, 5}, {
- SHIFTDR, DRPAUSE, 0x80, 2}, {
- SHIFTDR, IDLE, 0xC0, 3}, {
- IRPAUSE, SHIFTIR, 0x80, 2}, /* Extra transitions using SHIFTIR */
- {
- SHIFTIR, IRPAUSE, 0x80, 2}, {
- SHIFTIR, IDLE, 0xC0, 3}, {
- DRPAUSE, DRCAPTURE, 0xE0, 4}, /* 11/15/05 Nguyen changed to support DRCAPTURE */
- {
- DRCAPTURE, DRPAUSE, 0x80, 2}, {
- IDLE, DRCAPTURE, 0x80, 2}, {
- IRPAUSE, DRCAPTURE, 0xE0, 4}
+};
Please fix indentation.
+#ifdef VME_DEBUG +/*************************************************************** +* +* vme_print +* +* Prints VME to SVF file or terminal, depending on the +* pre-compiler definitions. This function may be compiler or OS +* specific. Please see your compiler's documentation regarding +* va_list, va_arg, and va_end. +* +***************************************************************/
Is this really needed?
+void vme_print(const char *a_pszString, ...) +{
- va_list list;
- unsigned short usStringIndex;
- static char szString[5000] = { 0 };
- static char szTempString[5000] = { 0 };
Do you really need static stings here?
- va_start(list, a_pszString);
- for (usStringIndex = 0; usStringIndex < strlen(a_pszString);
usStringIndex++) {if (a_pszString[usStringIndex] == '%') {szString[strlen(szString)] = a_pszString[usStringIndex];usStringIndex++;
Please note that the variable names are basicly a violation of the Coding Style (see Chapter 4: Naming). This code, even though it's not really complicated, is more or less unreadable as is.
...
+signed char ispVMRead(unsigned short a_usiDataSize)
...
- for (usDataSizeIndex = 0; usDataSizeIndex < a_usiDataSize;
usDataSizeIndex++) {if (cByteIndex == 0) {/***************************************************************** Grab byte from TDO buffer.****************************************************************/if (g_usDataType & TDO_DATA) {cDataByte = g_pucOutData[usBufferIndex];}/***************************************************************** Grab byte from MASK buffer.****************************************************************/
Comments don't line up here and elsewhere.
+/*************************************************************** +* +* internal_udelay +* +* Users must devise their own timing procedure to ensure the +* specified minimum delay is observed when using different +* platform. The timing function used here is for PC only by +* hocking the clock chip. +* +***************************************************************/
+void internal_udelay(unsigned short a_usMicroSecondDelay) +{
- int i, msecs;
- if (a_usMicroSecondDelay & 0x8000) {
msecs = a_usMicroSecondDelay & ~0x8000;for (i = 0; i < msecs; i++)udelay(1000);return;- }
- udelay(a_usMicroSecondDelay);
+}
Comment and implementation don't match. Also: are you sure this is needed in U-Boot?
Best regards,
Wolfgang Denk
On 29 Νοε 2006, at 11:55 ΜΜ, Wolfgang Denk wrote:
Dear Pantelis,
in message 20061129172539.24638.52486.stgit@pantathon.hol.gr you wrote:
Add support for Lattice FPGA parts programmed using JTAG.Again, the previous comments on necessary coding style cleanup apply.
Also:
common/Makefile | 4 common/ec.c | 473 ++++++
^^^^^^^^^^^^^^
Huh? What's "ec"? Please use a more descriptive name.
EC is the family name for the parts - follows the convention by the virtex & spartan families.
--- /dev/null +++ b/common/lattice_ivm_core.c
...
+* JTAG state machine transistion tragetory table.
Please fix typos ;-)
+struct {
- unsigned char CurState; /* From this state */
- unsigned char NextState; /* Step to this state */
- unsigned char Pattern; /* The tragetory of TMS */
- unsigned char Pulses; /* The number of steps */
+} g_JTAGTransistions[24] = {
- {
- RESET, RESET, 0xFC, 6}, /* Transitions from RESET */
- {
- RESET, IDLE, 0x00, 1}, {
- RESET, DRPAUSE, 0x50, 5}, {
- RESET, IRPAUSE, 0x68, 6}, {
- IDLE, RESET, 0xE0, 3}, /* Transitions from IDLE */
- {
- IDLE, DRPAUSE, 0xA0, 4}, {
- IDLE, IRPAUSE, 0xD0, 5}, {
- DRPAUSE, RESET, 0xF8, 5}, /* Transitions from DRPAUSE */
- {
- DRPAUSE, IDLE, 0xC0, 3}, {
- DRPAUSE, IRPAUSE, 0xF4, 7}, {
- IRPAUSE, RESET, 0xF8, 5}, /* Transitions from IRPAUSE */
- {
- IRPAUSE, IDLE, 0xC0, 3}, {
- IRPAUSE, DRPAUSE, 0xE8, 6}, {
- DRPAUSE, SHIFTDR, 0x80, 2}, /* Extra transitions using SHIFTDR */
- {
- IRPAUSE, SHIFTDR, 0xE0, 5}, {
- SHIFTDR, DRPAUSE, 0x80, 2}, {
- SHIFTDR, IDLE, 0xC0, 3}, {
- IRPAUSE, SHIFTIR, 0x80, 2}, /* Extra transitions using SHIFTIR */
- {
- SHIFTIR, IRPAUSE, 0x80, 2}, {
- SHIFTIR, IDLE, 0xC0, 3}, {
- DRPAUSE, DRCAPTURE, 0xE0, 4}, /* 11/15/05 Nguyen changed to
support DRCAPTURE */
- {
- DRCAPTURE, DRPAUSE, 0x80, 2}, {
- IDLE, DRCAPTURE, 0x80, 2}, {
- IRPAUSE, DRCAPTURE, 0xE0, 4}
+};
Please fix indentation.
+#ifdef VME_DEBUG +/*************************************************************** +* +* vme_print +* +* Prints VME to SVF file or terminal, depending on the +* pre-compiler definitions. This function may be compiler or OS +* specific. Please see your compiler's documentation regarding +* va_list, va_arg, and va_end. +* +***************************************************************/
Is this really needed?
+void vme_print(const char *a_pszString, ...) +{
- va_list list;
- unsigned short usStringIndex;
- static char szString[5000] = { 0 };
- static char szTempString[5000] = { 0 };
Do you really need static stings here?
- va_start(list, a_pszString);
- for (usStringIndex = 0; usStringIndex < strlen(a_pszString);
usStringIndex++) {if (a_pszString[usStringIndex] == '%') {szString[strlen(szString)] = a_pszString[usStringIndex];usStringIndex++;Please note that the variable names are basicly a violation of the Coding Style (see Chapter 4: Naming). This code, even though it's not really complicated, is more or less unreadable as is.
...
+signed char ispVMRead(unsigned short a_usiDataSize)
...
- for (usDataSizeIndex = 0; usDataSizeIndex < a_usiDataSize;
usDataSizeIndex++) {if (cByteIndex == 0) {/
** Grab byte from TDO buffer.****************************************************************/
if (g_usDataType & TDO_DATA) {cDataByte = g_pucOutData[usBufferIndex];}/
** Grab byte from MASK buffer.****************************************************************/
Comments don't line up here and elsewhere.
+/*************************************************************** +* +* internal_udelay +* +* Users must devise their own timing procedure to ensure the +* specified minimum delay is observed when using different +* platform. The timing function used here is for PC only by +* hocking the clock chip. +* +***************************************************************/
+void internal_udelay(unsigned short a_usMicroSecondDelay) +{
- int i, msecs;
- if (a_usMicroSecondDelay & 0x8000) {
msecs = a_usMicroSecondDelay & ~0x8000;for (i = 0; i < msecs; i++)udelay(1000);return;- }
- udelay(a_usMicroSecondDelay);
+}
Comment and implementation don't match. Also: are you sure this is needed in U-Boot?
As a general comment, this is the same file that RedBoot uses - it's lattice code, so I didn't do much with it. Yes the formatting is pretty horrible. And the comments are pretty misleading.
It is needed, the kernel expects the FPGA to be already programmed & ready to go, there is no code to handle FPGA loading in the kernel.
The whole point of this is to have u-boot to be a drop in replacement for RedBoot, providing backwards compatibility with kernels & images created for RedBoot.
Best regards,
Wolfgang Denk
-- Software Engineering: Embedded and Realtime Systems, Embedded Linux Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "The great question... which I have not been able to answer... is, `What does woman want?'" - Sigmund Freud
Regards
Pantelis
As a general comment, this is the same file that RedBoot uses - it's lattice code, so I didn't do much with it. Yes the formatting is pretty horrible. And the comments are pretty misleading.
It is needed, the kernel expects the FPGA to be already programmed & ready to go, there is no code to handle FPGA loading in the kernel.
The whole point of this is to have u-boot to be a drop in replacement for RedBoot, providing backwards compatibility with kernels & images created for RedBoot.
Best regards,
Wolfgang Denk
-- Software Engineering: Embedded and Realtime Systems, Embedded Linux Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "The great question... which I have not been able to answer... is, `What does woman want?'" - Sigmund Freud
Regards
Pantelis
If it's any consolation, I have the same problem getting Xilinx-derived code accepted into U-Boot. :)
In message OFD0E1F0C1.FEF8E56C-ON07257235.0079CCE9-07257235.0079F533@mck.us.ray.com you wrote:
If it's any consolation, I have the same problem getting Xilinx-derived code accepted into U-Boot. :)
So maybe we can try together to find a way how to handle this in a clean way. Please try to understand my position: if we accept such alien code ungrudgingly, it will be very hard to maintain any level of common style of the sources. On the other hand, I am well aware that it makes little sense to "beautify" such code for example when there is need to update or re-sync it against some other project's source tree.
I really have no good solution for this problem. Any ideas are welcome.
Best regards,
Wolfgang Denk
In message 035A383E-D0CC-4A02-A49F-B98EA7D676EA@embeddedalley.com you wrote:
common/ec.c | 473 ++++++
^^^^^^^^^^^^^^ Huh? What's "ec"? Please use a more descriptive name.
EC is the family name for the parts - follows the convention by the virtex & spartan families.
lattice_ec ?
... As a general comment, this is the same file that RedBoot uses - it's lattice code, so I didn't do much with it. Yes the formatting is pretty horrible. And the comments are pretty misleading.
Do you see a chance of cleaning this up / re-implementing the needed parts in a clean way in some foreseeable future?
Best regards,
Wolfgang Denk
participants (3)
-
Keith J Outwater -
Pantelis Antoniou -
Wolfgang Denk