[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