<div dir="ltr">On Wed, Oct 15, 2008 at 6:06 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:<br><div class="gmail_quote">
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="Wj3C7c">On 15.10.2008 08:36, Corey Osgood wrote:<br>
> I'm reposting the patch with the changes, just to make sure I got<br>
> everything, and I've made a few more little changes. It once again builds<br>
> without warning and passes a ram_check().<br>
><br>
<br>
</div></div>Wow, thanks for going through this again. You patch can go in without<br>
any more changes.<br>
<br>
Acked-by: Carl-Daniel Hailfinger <<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>><br>
<br>
I just spotted a few minor oddnesses (not necessary to fix for this<br>
commit). Please take them with a grain of salt.</blockquote><div><br>I'll commit this afternoon, but comments inline.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> +static void do_twr_trfc(struct board_info *dev, int i, int ram_cycle)<br><div class="Ih2E3d">
> +{<br>
</div><div class="Ih2E3d">> +     u8 reg8, j;<br>
> +     u16 spd_data;<br>
> +<br>
> +     printk(BIOS_SPEW, "Calculating tWR and tRFC\n");<br>
> +     /* tWR (bits 7:6) */<br>
> +     /* JEDEC spec allows for decimal values, but coreboot doesn't.<br>
> +      * Convert the decimal value to an int (done more below). */<br>
><br>
<br>
</div>I think I know what you mean here. From your code, it seems the JEDEC<br>
value is not decimal either, but simply scaled with a factor of 4 (or<br>
you could say it is fixed-point with 2 fractional bits).</blockquote><div><br>Right.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<div class="Ih2E3d"><br>
> +static void do_tras_cas(struct board_info *dev, int i, int ram_cycle)<br>
> +{<br>
> +     u8 reg8;<br>
> +     int spd_data, j;<br>
> +<br>
> +     printk(BIOS_SPEW, "Calculating tRAS and CAS\n");<br>
> +<br>
> +     /* tRAS */<br>
> +     spd_data = spd_read_byte(dev->spd_channel0[i], 30);<br>
> +     spd_data = (spd_data / ram_cycle);<br>
> +     spd_data = check_timing(spd_data, 5, 20);<br>
> +<br>
> +     reg8 = pci_conf1_read_config8(dev->d0f3, 0x62);<br>
> +     if ((spd_data - 10) > (reg8 >> 4))<br>
><br>
<br>
</div>If spd_data can ever become smaller than 10, this if clause will not do<br>
what you want because of unsigned arithmetic (unless I just confused C<br>
arithmetic rules in my head).</blockquote><div><br>Yep, I'll take another look at this.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
> +     {<br>
<div class="Ih2E3d">> +             reg8 &= 0x0f;<br>
> +             reg8 |= ((spd_data -10) << 4);<br>
> +     }<br>
> +<br>
> +     /* CAS Latency */<br>
> +     spd_data = spd_read_byte(dev->spd_channel0[i],<br>
> +                     SPD_ACCEPTABLE_CAS_LATENCIES);<br>
> +<br>
> +     j = 2;<br>
> +     while(!((spd_data >> j) & 1))<br>
> +     {<br>
> +             j++;<br>
> +     }<br>
><br>
<br>
</div>The loop above counts the number of contiguous zero bits from bit 2<br>
upwards. A small comment explaining this would be appreciated.<br>
You could probably replace the loop with the following instruction (not<br>
correct if (spd_data>>2)==0, but that case is broken in the current code<br>
anyway.)<br>
j += fls(spd_data >> 2);</blockquote><div><br>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?<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<div class="Ih2E3d"><br>
> +     /* j should now be the CAS latency,<br>
> +      * in T for the module's rated speed */<br>
> +     j = check_timing(j, 2, 5);<br>
> +     if ((j - 2) > (reg8 & 0x7))<br>
> +     {<br>
> +             reg8 &= ~0x7;<br>
> +             reg8 |= j;<br>
> +     }<br>
> +     pci_conf1_write_config8(dev->d0f3, 0x62, reg8);<br>
> +}<br>
><br>
<br>
> +static void do_trrd_trtp_twtr(struct board_info *dev, int i, int ram_cycle)<br>
> +{<br>
> +     u8 reg8, j;<br>
> +     u16 spd_data;<br>
> +<br>
> +     printk(BIOS_SPEW, "Calculating tRRD, tRTP, and tWTR\n");<br>
> +     /* tRRD */<br>
> +     reg8 = pci_conf1_read_config8(dev->d0f3, 0x63);<br>
> +     j = spd_read_byte(dev->spd_channel0[i], SPD_tRRD);<br>
> +     spd_data = ((j >> 2) * 100);<br>
> +     spd_data |= ((j & 0x3) * 25);<br>
> +     spd_data = spd_data / (ram_cycle * 100);<br>
> +     spd_data = check_timing(spd_data, 2, 5);<br>
> +     if ((spd_data - 2) > (reg8 >> 6))<br>
><br>
<br>
</div>Unsigned arithmetic. Will explode if spd_data < 2.</blockquote><div><br>check_timing() makes sure spd_data is never less than 2.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
> +     {<br>
<div class="Ih2E3d">> +             reg8 &= 0x3f;<br>
> +             reg8 |= (spd_data - 2) << 6;<br>
> +     }<br>
> +<br>
> +     /* tRTP */<br>
> +     j = spd_read_byte(dev->spd_channel0[i], SPD_tRTP);<br>
> +     spd_data = ((j >> 2) * 100);<br>
> +     spd_data |= ((j & 0x3) * 25);<br>
> +     spd_data = spd_data / (ram_cycle * 100);<br>
> +     spd_data = check_timing(spd_data, 2, 3);<br>
> +     if (spd_data - 2)<br>
><br>
<br>
</div>Are you sure this if-clause is complete? It is equivalent to the clause<br>
below:<br>
<div class="Ih2E3d">if (spd_data != 2)</div></blockquote><div><br>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?<br>
<br>Thanks,<br>Corey<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br>
<br>
<br>
> +     {<br>
> +             reg8 |= 0x8;<br>
> +     }<br>
> +<br>
> +     /* tWTR */<br>
> +     j = spd_read_byte(dev->spd_channel0[i], SPD_tWTR);<br>
> +     spd_data = ((j >> 2) * 100);<br>
> +     spd_data |= ((j & 0x3) * 25);<br>
> +     spd_data = spd_data / (ram_cycle * 100);<br>
> +     spd_data = check_timing(spd_data, 2, 3);<br>
> +     if (spd_data - 2)<br>
><br>
<br>
</div>Same as above.<br>
<br>
> +     {<br>
> +             reg8 |= 0x2;<br>
> +     }<br>
> +     pci_conf1_write_config8(dev->d0f3, 0x63, reg8);<br>
> +}<br>
<div><div></div><div class="Wj3C7c">><br>
><br>
<br>
Regards,<br>
Carl-Daniel<br>
<br>
--<br>
<a href="http://www.hailfinger.org/" target="_blank">http://www.hailfinger.org/</a><br>
<br>
</div></div></blockquote></div><br></div>