[coreboot] [PATCH] v3: CN700 initram support

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Oct 15 12:06:21 CEST 2008


On 15.10.2008 08:36, Corey Osgood wrote:
> I'm reposting the patch with the changes, just to make sure I got
> everything, and I've made a few more little changes. It once again builds
> without warning and passes a ram_check().
>   

Wow, thanks for going through this again. You patch can go in without
any more changes.

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

I just spotted a few minor oddnesses (not necessary to fix for this
commit). Please take them with a grain of salt.

> +static void do_twr_trfc(struct board_info *dev, int i, int ram_cycle)
> +{
> +	u8 reg8, j;
> +	u16 spd_data;
> +
> +	printk(BIOS_SPEW, "Calculating tWR and tRFC\n");
> +	/* tWR (bits 7:6) */
> +	/* JEDEC spec allows for decimal values, but coreboot doesn't.
> +	 * Convert the decimal value to an int (done more below). */
>   

I think I know what you mean here. From your code, it seems the JEDEC
value is not decimal either, but simply scaled with a factor of 4 (or
you could say it is fixed-point with 2 fractional bits).

> +static void do_tras_cas(struct board_info *dev, int i, int ram_cycle)
> +{
> +	u8 reg8;
> +	int spd_data, j;
> +	
> +	printk(BIOS_SPEW, "Calculating tRAS and CAS\n");
> +
> +	/* tRAS */
> +	spd_data = spd_read_byte(dev->spd_channel0[i], 30);
> +	spd_data = (spd_data / ram_cycle);
> +	spd_data = check_timing(spd_data, 5, 20);
> +
> +	reg8 = pci_conf1_read_config8(dev->d0f3, 0x62);
> +	if ((spd_data - 10) > (reg8 >> 4))
>   

If spd_data can ever become smaller than 10, this if clause will not do
what you want because of unsigned arithmetic (unless I just confused C
arithmetic rules in my head).

> +	{
> +		reg8 &= 0x0f;
> +		reg8 |= ((spd_data -10) << 4);
> +	}
> +
> +	/* CAS Latency */
> +	spd_data = spd_read_byte(dev->spd_channel0[i],
> +			SPD_ACCEPTABLE_CAS_LATENCIES);
> +
> +	j = 2;
> +	while(!((spd_data >> j) & 1))
> +	{
> +		j++;
> +	}
>   

The loop above counts the number of contiguous zero bits from bit 2
upwards. A small comment explaining this would be appreciated.
You could probably replace the loop with the following instruction (not
correct if (spd_data>>2)==0, but that case is broken in the current code
anyway.)
j += fls(spd_data >> 2);

> +	/* j should now be the CAS latency, 
> +	 * in T for the module's rated speed */
> +	j = check_timing(j, 2, 5);
> +	if ((j - 2) > (reg8 & 0x7))
> +	{
> +		reg8 &= ~0x7;
> +		reg8 |= j;
> +	}
> +	pci_conf1_write_config8(dev->d0f3, 0x62, reg8);
> +}
>   

> +static void do_trrd_trtp_twtr(struct board_info *dev, int i, int ram_cycle)
> +{
> +	u8 reg8, j;
> +	u16 spd_data;
> +
> +	printk(BIOS_SPEW, "Calculating tRRD, tRTP, and tWTR\n");
> +	/* tRRD */
> +	reg8 = pci_conf1_read_config8(dev->d0f3, 0x63);
> +	j = spd_read_byte(dev->spd_channel0[i], SPD_tRRD);
> +	spd_data = ((j >> 2) * 100);
> +	spd_data |= ((j & 0x3) * 25);
> +	spd_data = spd_data / (ram_cycle * 100);
> +	spd_data = check_timing(spd_data, 2, 5);
> +	if ((spd_data - 2) > (reg8 >> 6))
>   

Unsigned arithmetic. Will explode if spd_data < 2.

> +	{
> +		reg8 &= 0x3f;
> +		reg8 |= (spd_data - 2) << 6;
> +	}
> +
> +	/* tRTP */
> +	j = spd_read_byte(dev->spd_channel0[i], SPD_tRTP);
> +	spd_data = ((j >> 2) * 100);
> +	spd_data |= ((j & 0x3) * 25);
> +	spd_data = spd_data / (ram_cycle * 100);
> +	spd_data = check_timing(spd_data, 2, 3);
> +	if (spd_data - 2)
>   

Are you sure this if-clause is complete? It is equivalent to the clause
below:
if (spd_data != 2)


> +	{
> +		reg8 |= 0x8;
> +	}
> +
> +	/* tWTR */
> +	j = spd_read_byte(dev->spd_channel0[i], SPD_tWTR);
> +	spd_data = ((j >> 2) * 100);
> +	spd_data |= ((j & 0x3) * 25);
> +	spd_data = spd_data / (ram_cycle * 100);
> +	spd_data = check_timing(spd_data, 2, 3);
> +	if (spd_data - 2)
>   

Same as above.

> +	{
> +		reg8 |= 0x2;
> +	}
> +	pci_conf1_write_config8(dev->d0f3, 0x63, reg8);
> +}
>
>   

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list