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

[Pantelis Antoniou]

On 29 Νοε 2006, at 11:55 ΜΜ, Wolfgang Denk wrote:

> 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.
>

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 at denx.de
> "The great question... which I have not been able  to  answer...  is,
> `What does woman want?'"                              - Sigmund Freud

Regards

Pantelis