[coreboot] [PATCH] v3: CN700 initram support
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Oct 16 01:50:44 CEST 2008
On 15.10.2008 17:04, Corey Osgood wrote:
> On Wed, Oct 15, 2008 at 6:06 AM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>
>> + {
>> + 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);
>>
>
>
> I'm not understanding how j==2 is broken in the current code. If
> (spd_data >> j) & 1 == 1, then the while loop should never run, right?
>
You start with j == 2. If (spd_data >> j) & 1 == 0 for all your values
of j, you will get an endless loop. And that will happen if spd_data < 4.
OK, to be honest, bitshifting becomes undefined for sufficiently large
values of j (j>=32) and so the loop may terminate, but I wouldn't count
on it.
Then again, this concern is moot because JEDEC 21-C for DDR2 says
(indirectly) that at least one of bit 2..7 has to be 1, so the code is
broken only if you have an invalid SPD.
>>> + /* 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.
>>
>
>
> check_timing() makes sure spd_data is never less than 2.
>
Fine. I had overlooked that. My apologies.
>>> + {
>>> + 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)
>>
>>
>
> I think so. spd_data could only ever be 2 or 3 because of check_timing(),
> which is because of chipset limitations. Should I change it or leave it
> as-is?
>
IMHO the changed version is more readable, but it's your choice.
Thanks for having patience with my review.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list