Re: [coreboot] [PATCH] new board: LiPPERT Cool SpaceRunner-LX

[Uwe Hermann]

Hi Jens,

On Thu, Oct 30, 2008 at 06:16:10PM +0100, Jens Rottmann wrote:
> > Is there a reason to encode this in an u16? You could also use a struct
> > like this, which is more readable IMHO:
> >
> > const struct foo {
> >           u8 data;
> >           u8 index;
> >   } foo_table[] = {
> >           {0x07, 0x07},
> >           {0x1e, 0x2c},
> >           {0x04, 0x23},
> >           [...]
> >   };
>
> In fact, I did consider this, but my personal taste was that a struct
> added lots of nested {}s, but not better readability. I found the u16
> approach plain and simple. Thanks for your suggestion, but I'd like to
> keep it.

OK.
> > > it8712f_enable_serial(0, TTYS0_BASE); // does not use its 1st parameter
> >
> > Yes, this needs some cleanups, but that's for another patch.
>
> That remark is not from me. I just copied the comment along with all
> IT8712F related code from AMD's DBM690T mainboard.

Yeah, I know, it's a TODO we should fix in the IT8712F, I think; it's
not related to your specific patch.
> > > 	register "com1_enable" = "0"
> > > 	register "com1_address" = "0x3E8"
> > > 	register "com1_irq" = "6"
> > > 	register "com2_enable" = "0"
> > > 	register "com2_address" = "0x2E8"
> > > 	register "com2_irq" = "6"
> >
> > Are your sure about these ports? Usually serial uses 0x2f8 and 0x3f8
> > (not 0x2e8/0x3e8). Or is this on purpose? Also, why are both IRQs 6?
> > And finally, these are disabled, as serial is done by the IT8712F below?
> > > 			device pnp 2e.1 on #  Com1
> > > 				io 0x60 = 0x3f8
> > > 				irq 0x70 = 4
> > > 			end
> > > 			device pnp 2e.2 on #  Com2
> > > 				io 0x60 = 0x2f8
> > > 				irq 0x70 = 3
> > > 			end
>
> Yes, this is on purpose. COM1+2 are implemented by the SIO. The two
> UARTs in the CS5536 are normally unusable, but if someone really
> wanted them, they'd have to be COM3+4 (i.e. 3E8/2E8). They must not use
> IRQ4+3, because the CS5536 (on PCI) cannot share IRQs with the SIO (on
> LPC). IRQ6 is available, because the board does not have a legacy floppy
> connector.

OK, maybe there should be a similar comment in Config.lb to clarify
this (feel free to post a patch).
> > > +romimage "fallback"
> >
> > Minor issue, but "image" is a bit more readable (instead of "fallback")
> > if you don't use the normal/fallback mechanism anyway.
> > > +buildrom ./coreboot.rom ROM_SIZE "fallback"
> >
> > This needs to be changed to "image" too, in that case.
>
> Changed.
>
> But to be honest, I don't understand the whole normal/fallback scheme. I
> copied this from AMD DB800 (which is said in the wiki to be a very good
> example for Geode-LX mainboards), but as you can see
> > > option USE_FALLBACK_IMAGE=1
>
> they actually use only a fallback image without a normal image??? Why???
> Shouldn't be USE_FALLBACK_IMAGE=0 and the romimage renamed to "normal"?

No, it's the other way around. Either you have normal+fallback, or you
have only fallback. The image names themselved are irrelevant btw, only
the 'USE_FALLBACK_IMAGE=0|1' matters.
> The wiki doesn't say much about this topic. There is a nice diagram to
> show where the images are stored and how big they are, but it doesn't
> say what they're needed for. And what the heck is the difference
> between fallback and failover??

Good question ;)

I've committed your patch in r3710 with some whitespace/cosmetic changes.

The only non-trivial change I made, was to add
 * Copyright (C) 2007 Advanced Micro Devices, Inc.
to the cache_as_ram_auto.c file, as that's (IMHO) the only non-trivial
file copied from the db800 and thus needs to preserve the (C) of AMD.

But please correct me if that should be wrong, we can still fix that
then in svn (CC'd also Marc and Jordan from AMD for input).

Thanks, Uwe.