[U-Boot-Users] [PATCH V7] ARM: Add support for Lyrtech SFF-SDR board (ARM926EJS)

[Wolfgang Denk]

In message <1212760465-16024-1-git-send-email-hugo.villeneuve at lyrtech.com> you wrote:
> ARM: This patch adds support for the Lyrtech SFF-SDR
> board, based on the TI DaVinci architecture (ARM926EJS).

...
> +int read_mac_address(uint8_t *buf)
> +{
> +	u_int32_t value, mac[2], address;
> +
> +	/* Read Integrity data structure checkword. */
> +	if (i2c_read(CFG_I2C_EEPROM_ADDR, INTEGRITY_CHECKWORD_OFFSET,
> +		     CFG_I2C_EEPROM_ADDR_LEN, (uint8_t *) &value, 4)) {
> +		printf("Read from EEPROM @ 0x%02x failed\n",
> +		       CFG_I2C_EEPROM_ADDR);
> +		return 1;

# 1

> +	}
> +	if (value != INTEGRITY_CHECKWORD_VALUE)
> +		return 1;
> +
> +	/* Read SYSCFG structure offset. */
> +	if (i2c_read(CFG_I2C_EEPROM_ADDR, INTEGRITY_SYSCFG_OFFSET,
> +		     CFG_I2C_EEPROM_ADDR_LEN, (uint8_t *) &value, 4)) {
> +		printf("Read from EEPROM @ 0x%02x failed\n",
> +		       CFG_I2C_EEPROM_ADDR);
> +		return 1;

# 2

> +	}
> +	address = 0x800 + (int) value; /* Address where SYSCFG structure
> +					* is located. */
> +
> +	/* Read NET CONFIG structure offset. */
> +	if (i2c_read(CFG_I2C_EEPROM_ADDR, address,
> +		     CFG_I2C_EEPROM_ADDR_LEN, (uint8_t *) &value, 4)) {
> +		printf("Read from EEPROM @ 0x%02x failed\n",
> +		       CFG_I2C_EEPROM_ADDR);
> +		return 1;

# 3

> +	}
> +	address = 0x800 + (int) value; /* Address where NET CONFIG structure
> +					* is located. */
> +	address += 12; /* Address where NET INTERFACE CONFIG structure
> +			* is located. */
> +
> +	/* Read NET INTERFACE CONFIG 2 structure offset. */
> +	if (i2c_read(CFG_I2C_EEPROM_ADDR, address,
> +		     CFG_I2C_EEPROM_ADDR_LEN, (uint8_t *) &value, 4)) {
> +		printf("Read from EEPROM @ 0x%02x failed\n",
> +		       CFG_I2C_EEPROM_ADDR);
> +		return 1;

# 4

> +	}
> +	address = 0x800 + 16 + (int) value;/* Address where NET INTERFACE
> +					    * CONFIG 2 structure is located. */
> +
> +	/* Read MAC address. */
> +	if (i2c_read(CFG_I2C_EEPROM_ADDR, address,
> +		     CFG_I2C_EEPROM_ADDR_LEN, (uint8_t *) &mac[0], 8)) {
> +		printf("Read from EEPROM @ 0x%02x failed\n",
> +		       CFG_I2C_EEPROM_ADDR);
> +		return 1;

# 5

> +	}
> +
> +	buf[0] = mac[0] >> 24;
> +	buf[1] = mac[0] >> 16;
> +	buf[2] = mac[0] >> 8;
> +	buf[3] = mac[0];
> +	buf[4] = mac[1] >> 24;
> +	buf[5] = mac[1] >> 16;
> +
> +	return 0;

Instead of repeating the same code 5 times, replace it by a "goto err;"
and add here:

err:
	printf("Read from EEPROM @ 0x%02x failed\n", CFG_I2C_EEPROM_ADDR);
	return 1;


That's MUCH easier to read, and especially easier to maintain.


> +int misc_init_r(void)
> +{
> +	u_int8_t tmp[20], buf[10];
> +	int clk = 0;
> +
> +	/* EMIF-A CS3 configuration for FPGA. */
> +	REG(DAVINCI_A3CR) = DAVINCI_A3CR_VAL;
> +
> +	clk = ((REG(PLL2_PLLM) + 1) * 27) / ((REG(PLL2_DIV2) & 0x1f) + 1);
> +
> +	printf("ARM Clock: %dMHz\n", ((REG(PLL1_PLLM) + 1) * 27) / 2);
> +	printf("DDR Clock: %dMHz\n", (clk / 2));
> +
> +	/* Configure I2C switch (PCA9543) to enable channel 0. */
> +	tmp[0] = CFG_I2C_PCA9543_ENABLE_CH0;
> +	if (i2c_write(CFG_I2C_PCA9543_ADDR, 0,
> +		      CFG_I2C_PCA9543_ADDR_LEN, tmp, 1))
> +		printf("Write to MUX @ 0x%02x failed\n", CFG_I2C_PCA9543_ADDR);

And this is not an error that needs to be handled?

> +	/* Set Ethernet MAC address from EEPROM if available. */
> +	if (read_mac_address(buf) == 0) {
> +		if ((buf[0] != 0xff) && (getenv("ethaddr") == NULL)) {
> +			sprintf((char *)&tmp[0],
> +				"%02x:%02x:%02x:%02x:%02x:%02x",
> +				buf[0], buf[1], buf[2], buf[3],
> +				buf[4], buf[5]);
> +			setenv("ethaddr", (char *)&tmp[0]);
> +		}
> +	}

Hm. I always thought this was documented somewhere (probably it's in the
CHANGELOG ?), but I didn't find it right now. The intended "standard"
behaviour should be:

If there is a MAC address in  the  environment,  and  if  it  is  not
identical  to  the  MAC  address  in the ROM, then a warning shall be
printed, and the MAC address from the environment be used.

If there is no MAC address in  the  environment,  then  it  shall  be
initialized (silently) from the value in the ROM.

See for example drivers/net/cs8900.c (which is the first driver where
we implemented this, IIRC).

> +	/* For most DaVinci boards, U-Boot is copied in RAM by the UBL,
> +	 * so we are always in the relocated state. This is
> +	 * necessary for command history to work. */
> +	gd->flags |= GD_FLG_RELOC;

"most" != "all". What happens for the N (N = all - most) remaining
boards?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Many aligators will be slain, but the swamp will remain.