From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Babic <sbabic@denx.de>
Date: Wed, 24 Mar 2010 10:56:02 +0100
Subject: [U-Boot] [PATCH] SPI: added support for MX51 to mxc_spi
In-Reply-To: <3D9B2D7685E0734592024DD0F3028AE6B75C07@zch01exm23.fsl.freescale.net>
References: <1263835130-13141-1-git-send-email-sbabic@denx.de>
	<1268756501-2124-1-git-send-email-sbabic@denx.de>
	<3D9B2D7685E0734592024DD0F3028AE6B75C07@zch01exm23.fsl.freescale.net>
Message-ID: <4BA9E1B2.70104@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Liu Hui-R64343 wrote:
> Hi,  

Hi,

>>  
>>  static unsigned long spi_bases[] = {
>>  	0x43fa4000,
>>  	0x50010000,
>>  	0x53f84000,
>>  };
> Here hardcode the value in mx31, while in mx51 it use the macro. Which makes
> Code style not consistent. 

yes, agree, the driver already contains a lot of hard-coded values for
mx.31 and I changed only according to the mx.51. As I see, in the
imx-regs.h for MX.31 several defines are missing, and the drivers define
theirselves the values.
I will add the defines, at least for spi, to the mx31-regs.h in a
separate patch.

>> +static unsigned long spi_bases[] = {
>> +	CSPI1_BASE_ADDR,
>> +	CSPI2_BASE_ADDR,
>> +	CSPI3_BASE_ADDR,
>> +};
> See above comments.

Ok, but this is correct. Only the MX31 part should be changed.

>>  struct mxc_spi_slave {
>>  	struct spi_slave slave;
>>  	unsigned long	base;
>>  	u32		ctrl_reg;
>> +	u32		cfg_reg;
>>  	int		gpio;
>>  };
> Only MX51 use it, MX31 will not use it.

However, I need a general structure to support both processors. Agree
this  register is available only on the MX.51 processor, I can surround
the definition with an #ifdef CONFIG_MX51 statement.

> The function spi_cfg only used in MX51, will have compile warnings for MX31.
> Use CONFIG_MX51 to cover it.

Agree, and as reported by Tom, there are other issues regarding the
MX.31. I will check all of them globally and I will try to test on a
MX.31 board, too.

>> +				 * a single byte first, 
>> followed by 4 words.
> 
> Comments is wrong, should be "followed by 4 bytes"

Agree, it must be changed.

> 
>> +				 */
>> +				if ((cnt_blk == 0) && (bitlen % 32) &&
>> +					(j >= ((bitlen % 32) / 8))) {
>> +					continue;
>> +				}
>> +				if (pbuf)
>> +					tmpdata = *pbuf++ | 
>> (tmpdata <<  8);
>> +				n_bytes--;
>> +			}
>> +		}
>> +		debug("writing TX FIFO 0x%x\n", tmpdata);
>> +		reg_write(mxcs->base + MXC_CSPITXDATA, tmpdata);
>>  	}
> Can use word copy for the left part(cnt_blk !=0)  to get high performance.

Not sure, but it could be I have not get the real problem here.

>> +				if (din)
>> +					*pbuf++ = (tmpdata >>
>> +						((3 - 
>> spare_bytes - j) * 8))
> 
> The RX path should be logic wrong, spare_bytes not reset to zero and the data got not correct when data is not 4B alignement. 

Thanks, I will recheck the code, surely spare_bytes must be reset to zero.

Regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================