[U-Boot-Users] [PATCH 2/5] Support LATTICE FPGA parts using JTAG programming.

[Wolfgang Denk]

Dear Pantelis,

in message <20061129172539.24638.52486.stgit at 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

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The great question... which I have not been able  to  answer...  is,
`What does woman want?'"                              - Sigmund Freud