[U-Boot] [PATCH v3 10/10] arm, davinci: add cam_enc_4xx support

[Heiko Schocher <hs@denx.de>]

Hello Igor,

Igor Grinberg wrote:
> On 10/21/2011 08:32 AM, Heiko Schocher wrote:
>> - DM368 SOC
>> - booting with spl not with UBL from TI
>> - before loading u-boot from NAND into RAM, test
>>   the RAM with the post memory test. If error
>>   is found, switch all LEDs on and halt system.
>> - SPI Flash
>>   Dataflash Typ: M25PE80
>> - Ethernet DM9161BI
>> - MMC
>> - USB
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> ---
[...]
>> diff --git a/board/ait/cam_enc_4xx/Makefile b/board/ait/cam_enc_4xx/Makefile
>> new file mode 100644
>> index 0000000..4804597
>> --- /dev/null
>> +++ b/board/ait/cam_enc_4xx/Makefile
[...]
> I don't think you should be adding this.
> Please, see the commit 464c79207c89f247f97b344495924eabb0c9738e
> (punt unused clean/distclean targets) by Mike.

Yep, you are right, done.

>> diff --git a/board/ait/cam_enc_4xx/cam_enc_4xx.c b/board/ait/cam_enc_4xx/cam_enc_4xx.c
>> new file mode 100644
>> index 0000000..059a08a
>> --- /dev/null
>> +++ b/board/ait/cam_enc_4xx/cam_enc_4xx.c
[...]
>> +int board_init(void)
>> +{
>> +	gd->bd->bi_arch_number = MACH_TYPE_DAVINCI_DM365_EVM;
> 
> You should be using the new standard for specifying the machine type.
> Please, read the README file (CONFIG_MACH_TYPE option).

Changed, thanks!

[...]
>> +static void davinci_std_write_page_syndrome(struct mtd_info *mtd,
>> +				    struct nand_chip *chip, const uint8_t *buf)
>> +{
>> +	unsigned char davinci_ecc_buf[NAND_MAX_OOBSIZE];
>> +	struct nand_chip *this = mtd->priv;
>> +	int i, eccsize = chip->ecc.size;
>> +	int eccbytes = chip->ecc.bytes;
>> +	int eccsteps = chip->ecc.steps;
>> +	int chunk = chip->ecc.bytes + chip->ecc.prepad + chip->ecc.postpad;
>> +	int offset = 0;
>> +	const uint8_t *p = buf;
>> +	uint8_t *oob = chip->oob_poi;
>> +
>> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
>> +		chip->ecc.hwctl(mtd, NAND_ECC_WRITE);
>> +		chip->write_buf(mtd, p, eccsize);
>> +
>> +		/* Calculate ECC without prepad */
>> +		chip->ecc.calculate(mtd, p, oob + chip->ecc.prepad);
>> +
>> +		if (chip->ecc.prepad) {
>> +			offset = ((chip->ecc.steps - eccsteps) * chunk);
>> +			memcpy(&davinci_ecc_buf[offset], oob, chip->ecc.prepad);
>> +			oob += chip->ecc.prepad;
>> +		}
>> +
>> +		offset = (((chip->ecc.steps - eccsteps) * chunk) +
>> +				chip->ecc.prepad);
> 
> 2 sets of parenthesis is enough.
> I don't see any good in having the whole expression wrapped.

Changed.

>> +		memcpy(&davinci_ecc_buf[offset], oob, eccbytes);
>> +		oob += eccbytes;
>> +
>> +		if (chip->ecc.postpad) {
>> +			offset = (((chip->ecc.steps - eccsteps) * chunk) +
>> +					(chip->ecc.prepad + eccbytes));
> 
> same here

Changed.

>> +			memcpy(&davinci_ecc_buf[offset], oob,
>> +				chip->ecc.postpad);
>> +			oob += chip->ecc.postpad;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Write the sparebytes into the page once
>> +	 * all eccsteps have been covered
>> +	 */
>> +	for (i = 0; i < mtd->oobsize; i++)
>> +		writeb(davinci_ecc_buf[i], this->IO_ADDR_W);
>> +
>> +	/* Calculate remaining oob bytes */
>> +	i = mtd->oobsize - (oob - chip->oob_poi);
> 
> This one looks good, I think the previous should be the same.

Yep.

[...]
>> +void enable_vbus(void)
>> +{
>> +	/*
>> +	 * nothing to do, but this function is needed from
>> +	 * drivers/usb/musb/davinci.c
>> +	 */
>> +}
> 
> I think the common way would be to define a "weak" implementation in
> drivers/usb/musb/davinci.c?

Yes, that would be cleaner, changed.

>> +
>> +int board_late_init(void)
>> +{
>> +	struct davinci_gpio *gpio = davinci_gpio_bank45;
>> +
>> +	/* 24MHz InputClock / 15 prediv -> 1.6 MHz timer running */
>> +	while (get_timer_val() < 0x186a00)
>> +		;
>> +
>> +	/* 1 sec reached -> stop timer, clear all LED */
>> +	stop_timer();
>> +	clrbits_le32(&gpio->out_data, CONFIG_CAM_ENC_LED_MASK);
>> +	return 0;
>> +}
>> +
>> +void reset_phy(void)
>> +{
>> +	char *name = "GENERIC @ 0x00";
>> +
>> +	/* reset the phy */
>> +	miiphy_reset(name, 0x0);
>> +}
>> +
>> +#else
> 
> I think, adding a comment to which #if that #else belongs,
> will improve the understanding/readability.

Done.

[...]

Thanks for the review!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany