[LinuxBIOS] PATCH: add the pcengines ALIX1

[ron minnich]

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>