[LinuxBIOS] PATCH: add the pcengines ALIX1
ron minnich
rminnich at gmail.com
Sun Sep 9 07:12:11 CEST 2007
Partial diff attached, .... here are comments.
Still builds.
On 9/8/07, Uwe Hermann <uwe at hermann-uwe.de> wrote:
> Add a license header, please.
done
> As it's commented out (and likely copied from some other target), please
> drop the file completely for now. We can add a working version (or
> preferrably use a global/common file if possible) if and when we need it.
> I don't like adding dummy files which aren't really necessary.
removed.
> > + 32+16*9, /* There can be total 9 devices on the bus */
>
> Is the 9 (and the rest of this file) correct and tested?
fixed constant in options.lb and replace 9 with the IRQ_SLOT_COUNT.
> Add a license header, please.
done
>
> NACK. Not yet another copy of this file, please. Move it into lib/ or
> somewhere globally, and use that in all targets. We should stop
> replicating there files again and again.
patch 2 as attached.
>
> NACK, see above. This is common code just about every board duplicateѕ
> again and again. I have a patch which adds a global failover.c into
> lib/ (which my recent i810 board patch already uses, btw).
>
> I'll post the patch ASAP.
I'll wait for your failover.c patch, but beware: they are not ALWAYS
totally identical.
>
>
> > Index: src/mainboard/pcengines/alix1c/chip.h
> > ===================================================================
> > --- src/mainboard/pcengines/alix1c/chip.h (revision 0)
> > +++ src/mainboard/pcengines/alix1c/chip.h (revision 0)
>
> Trivial, but please add a license header.
done.
> > +++ src/mainboard/pcengines/alix1c/cmos.layout (revision 0)
>
> This should be dropped for now (needs a small fix in Config.lb to make
> it still compile), as you use
not, I want it here as we will want CMOS support on this board at some point.
> Add a license header, please.
done.
> > +
> > +/* Print the platform configuration */
> > +void print_conf(void) {
>
> Is this really needed here? Should it go in some common lib/ file?
> Doesn't look board-specific to me.
let's unify it but later. It's not always non-board-specific.
> > + printk_debug("ALIX1C ENTER %s\n", __FUNCTION__);
> > +
> > + printk_debug("ALIX1C EXIT %s\n", __FUNCTION__);
>
> Drop both printks? Are they useful?
They are useful for debug, so that people can see what's getting run
and when. I would prefer to leave them in.
>
> CHIP_NAME("PC Engines ALIX1.C Mainboard")
done I think.
> > +++ src/mainboard/pcengines/alix1c/cache_as_ram_auto.c (revision 0)
>
> Add a license header, please.
done
> > +#define POST_CODE(x) outb(x, 0x80)
>
> We have a global implementation already.
cache as ram is weird, I want to test this before I use the library.
The library can do lots more than outb, what with serial post and
timers etc.
> > +
> > +}
>
> Is this board-specific or chipset-specific? If the latter, it should not
> be here.
I am not sure. Marc?
> > + /* Check all of memory */
>
> Only checks _some_ memory.
:-)
Will fix in next go round, I forgot to.
> > +
> > + We use method 1 on Norwich and on this board too.
> > + */
>
> This comment is in some other file, too. Maybe it should go in the wiki
> or in the generic CAR code somewhere? No need to duplicate it in every
> LX board...
I want this comment in, since not all users of LX read all files that use LX ;-)
>
>
> > + POST_CODE(0x02);
> > + print_err("POST 02\n");
> > + __asm__("wbinvd\n");
>
> Don't we have a wbinvd() function? Or is that in v3 only?
v3 only AFAIK.
> > +++ targets/pcengines/alix1c/Config.lb (revision 0)
>
> Add a license header, please.
done
So, try 2.
ron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcengines.diff
Type: text/x-patch
Size: 8421 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070908/fa6ebae3/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: removedebug.diff
Type: text/x-patch
Size: 1856 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070908/fa6ebae3/attachment-0001.diff>
More information about the coreboot
mailing list