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

[Jens Rottmann]

Hi,

>> + * (C)2008 LiPPERT Embedded Computers GmbH, released under the GNU GPLv2
>> + *
>> + * Based on cache_as_ram_auto.c from AMD's DB800 and DBM690T mainboards.
> 
> Please use the usual license header format for further patches, see:
> http://www.coreboot.org/Development_Guidelines#Common_License_Header

No offense, this probably wasn't your idea, but my personal opinion is
that these extensive license headers are a complete waste of disk space
and screen lines, without any legal significance. It is more than enough
to say that a file is GPLed. We don't need to explain _in every file_
what the GPL contains, that's what the COPYING file in the top level dir
is for. Another 5 copies of the GPL can be found in various other dirs,
so it's kind of funny we put in every file "... if not, write to the FSF
...". (And what if their address ever changes? ;-)
AFAIK the Linux kernel doesn't do this. If I look at some of the most
essential files of Linux, kernel/sched.c, mm/memory.c, I only see Linus'
(C), no more. If lawyers find self-fulfilment in adding lengthy legal
blurbs to everything they write, fine - but please let's not follow
them, that's where insanity lies.

However, I'm surely not the first to start this discussion, and obviosly
all others have given up unsuccessful, too. And I want my stuff to be
committed, so I comply and added legal blurbs to all my files. Sigh. The
lawyers have won yet again, looks like a good day to become insane ...

>> static const unsigned short sio_init_table[] = { // hi=data, lo=index
>> 	0x0707,		// select LDN 7 (GPIO, SPI, watchdog, ...)
>> 	0x1E2C,		// disable ATXPowerGood
>> 	0x0423,		// don't delay POWerOK1/2
> 
> 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.

>> +	it8712f_enter_conf();
>> +	for (i=0; i<ARRAY_SIZE(sio_init_table); i++) {
>> +		...
>> +	}
>> +	//it8712f_exit_conf() is included in sio_init_table
> 
> Why that? I'd call it8712f_exit_conf() here instead, makes more sense
> and is more readable, IMHO.

I saw no reason to add lots of additional code when this could be
handled in the loop like every other index-data-pair, costing only 2
bytes. (I think it8712f_exit_conf() even needlessly switches to LDN 0
before exiting.) I thought with the comment it would be easy to understand.

But you're probably right. Some dozen bytes extra code won't hurt
anyone. I changed it.

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

>> +++ src/mainboard/lippert/spacerunner-lx/chip.h	(working copy)
> 
>> + * Based on chip.h from AMD's DB800 mainboard.
> 
> This line is not needed, way too trivial file. The license header should
> stay though.

I copied chip.h, and although almost nothing of the original file
survived, I still _do_ want to pay credit, because without its example
I would not have known what chip.h was supposed to look like.

>> +++ src/mainboard/lippert/spacerunner-lx/cmos.layout	(working copy)
> 
> You can drop this file completely if you don't want to use it at all,
> see below.

Done.

>> +	makerule ./cache_as_ram_auto.inc
>> +			depends "$(MAINBOARD)/cache_as_ram_auto.c option_table.h"
> 
> Remove the "option_table.h" here and the board should compile fine
> if you remove cmos.layout.

Thanks!

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

>> +++ src/mainboard/lippert/spacerunner-lx/Options.lb	(working copy)
> 
>> +# Compile extra debugging code
>> +default DEBUG=1
> 
> Not sure about this, I'd rather drop it. We already have 9 (!)
> loglevels for DEFAULT_CONSOLE_LOGLEVEL/MAXIMUM_CONSOLE_LOGLEVEL,
> no need to add yet another debug option.

I didn't add this. It is defined in the global src/config/Options.lb,
where DEBUG defaults to 1, and it is also used by other code than mine.

I only repeated it here to show the user that this option exists, and
that it can be turned off (probably together with
CONFIG_CONSOLE_SERIAL8250=1).

This is also not quite the same as a loglevel. This could enable
additional sanity checks that do more than a simple printf.

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

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

Well, at least it works fine, what else can I ask for. I don't need to
understand everything ... :-)

By the way, I changed this, too:

>> +	option COREBOOT_EXTRA_VERSION=".0Fallback"


Thanks a lot for your help!

Jens Rottmann

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: spacerunner-lx.diff
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081030/17072d6b/attachment.ksh>