[coreboot] [PATCH] v3: CN700 initram support

[Corey Osgood]

On Wed, Oct 15, 2008 at 6:06 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> 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.


I'll commit this afternoon, but comments inline.


> > +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).


Right.


>
>
> > +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).


Yep, I'll take another look at this.


>
>
> > +     {
> > +             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?


>
>
> > +     /* 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.


>
>
> > +     {
> > +             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?

Thanks,
Corey


>
>
> > +     {
> > +             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/
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081015/c989faf9/attachment.html>