Re: [coreboot] latest cumulative kontron patch

[ron minnich]

On Fri, Nov 14, 2008 at 7:17 PM, Peter Stuge  wrote:
> ron minnich wrote:
> > Cumulative set of changes. If there are no objections I'm going to
> > commit.
>
> I have some. :p
> > +++ southbridge/intel/i82801gx/lpc.c  (working copy)
> > @@ -128,6 +128,9 @@
> >  {
> >       u8 reg8;
> >       u16 reg16;
> > +#ifndef MAINBOARD_POWER_ON_AFTER_POWER_FAIL
> > +#define MAINBOARD_POWER_ON_AFTER_POWER_FAIL 1
> > +#endif
> >   int pwr_on=MAINBOARD_POWER_ON_AFTER_POWER_FAIL;
> >   int nmi_option;
>
> Could this go somewhere outside the function? Eventually I'd like
> this particular setting to go into Kconfig and of course be
> changeable at runtime. :)

I welcome a patch. But having users set that setting could be rally
dangerous. Stefan?
> > +++ northbridge/intel/i945/raminit.c  (working copy)
>
> Big changes moving all the magic values around here and I don't have
> the energy to look them all over. Maybe rework the changes to this
> file and make a separate patch with just whitespace changes?
> > +static const u32 dq2030[] = {
> > 0x08070706, 0x0a090908, 0x0d0c0b0a, 0x12100f0e,
> > 0x1a181614, 0x22201e1c, 0x2a282624, 0x3934302d,
> > 0x0a090908, 0x0c0b0b0a, 0x0e0d0d0c, 0x1211100f,
> > 0x19171513, 0x211f1d1b, 0x2d292623, 0x3f393531
> >
> > +};
>
> These tables.. So ugly. sigh..

but what else can we do? this is dram training.
> > +static const u32 * map(u32 *table, unsigned int i){
> > const u32 *p = NULL;
> > switch(table[i]) {
> >         case NC: p = nc; break;
> >         case DQ2030: p = dq2030; break;
> >         case CMD3210: p = cmd3210; break;
> >         case CTL3215: p = ctl3215; break;
> >         case CLK2030: p = clk2030; break;
> >         case CMD2710: p = cmd2710; break;
> >         case DQ2330: p = dq2330; break;
> >         default: printk(BIOS_ERR, "Missing table entry %p[0x%x]\n", table, i); die("fix raminit.c");
> > }
> > return p;
> >
> > +}
>
> ..
> >   /* Channel 0 */
> > sdram_write_slew_rates(G1SRPUT, slew_group_lookup[idx * 8 + 0]);
> > sdram_write_slew_rates(G2SRPUT, slew_group_lookup[idx * 8 + 1]);
> > if ((slew_group_lookup[idx * 8 + 2] != nc) && (sysinfo->package == SYSINFO_PACKAGE_STACKED)) {
> > sdram_write_slew_rates(G1SRPUT, map(slew_group_lookup, idx * 8 + 0));
> > sdram_write_slew_rates(G2SRPUT, map(slew_group_lookup, idx * 8 + 1));
> > if ((map(slew_group_lookup, idx * 8 + 2) != nc) && (sysinfo->package == SYSINFO_PACKAGE_STACKED)) {
>
> What's this? Is this adding functionality?

This resolves the problem of having pointers in arrays in initram.
> > +static void i945_detect_chipset(void)
>
> And here's a bunch of code moved from stage1 to raminit? Why?

because it has to be in raminit. It is raminit code. It is the result
of splitting auto.c between stage1 and raminit.
> > -static inline __attribute__ ((always_inline))
> > -void pcie_write_config32(device_t dev, unsigned int where, u32 value)
> > +++ northbridge/intel/i945/pcie_config.h      (working copy)
> >  static inline __attribute__ ((always_inline))
> > -u8 pcie_read_config8(device_t dev, unsigned int where)
> > +u8 pcie_read_config8(u32 dev, unsigned int where)
> >  {
> >       unsigned long addr;
> >       addr = DEFAULT_PCIEXBAR | dev | where;
> > return read8(addr);
> > return readb((void *)addr);
> >
> > }
>
> Was something said about functions in .h ?

note the attribute: static inline __attribute__ ((always_inline))

so there is not much choice here.

ron