[coreboot] v3 Tyan s2892

[Peter Stuge]

Myles Watson wrote:
> This patch adds initial support for mainboard/tyan/s2892.  It
> compiles, but is untested in hardware.

I think this needs a little polishing.


> +++ svn/mainboard/tyan/s2892/dts
..
> +/{
> +	device_operations="s2892";
> +	mainboard_vendor = "AMD";

Tyan, not AMD, right? :)


> +++ svn/mainboard/tyan/Kconfig
..

> +choice
> +	prompt "Mainboard model"
> +	depends on VENDOR_TYAN
> +
> +config BOARD_TYAN_S2892
> +	bool "s2892"
> +	select ARCH_X86
> +	select CPU_AMD_K8
> +	select NORTHBRIDGE_AMD_K8
> +	select SOUTHBRIDGE_NVIDIA_CK804
> +	select SOUTHBRIDGE_AMD_AMD8131
> +	select SUPERIO_WINBOND_W83627HF
> +	select IOAPIC
> +	help
> +	  Tyan Thunder K8SE (s2892)
> +
> +endchoice

Does Kconfig allow choices to span multiple files? If so it would be
nice to not have to repeat the Mainboard model choice for each
vendor.

I would like this option to read "Thunder K8SE (S2892)" as Tyan uses
on the web: http://www.tyan.com/product_board_detail.aspx?pid=145

(And not just the help, but the actual user visible option name.)


> +++ svn/mainboard/tyan/s2892/mainboard.c
..
> +struct device_operations s2892 = {
> +	.id = {.type = DEVICE_ID_PCI,
> +		{.pci = {.vendor = PCI_VENDOR_ID_TYAN,
> +			 .device = 0x2892}}},
> +	.constructor = default_device_constructor,
> +};

Why is this needed? The board itself isn't a PCI device. It can
certainly have a subsystem it belonging to it, but that was already
specified in the dts so must not be needed to duplicate here.


> +++ svn/mainboard/tyan/s2892/stage1.c
..
> +void ck804_enable_rom(void);

This prototype shouldn't be needed here right? What ROM is it anyway?
The NIC? See discussion in ck804 thread.


> +#define SERIAL_DEV W83627HF_SP1
> +#define SERIAL_IOBASE 0x3f8

Isn't at least the iobase in a CONFIG_ already?


> +void mainboard_pre_payload(void)
> +{
> +	banner(BIOS_DEBUG, "mainboard_pre_payload: done");
> +}

Can we remove this function from mainboard sources when it doesn't do
anything like here?


> +++ svn/mainboard/tyan/s2892/initram.c
..
> +/* this code is very mainboard dependent, sadly. */
..
> +/**
> + * read a byte from spd.
> + * @param device device to read from
> + * @param address address in the spd ROM
> + * @return the value of the byte at that address.
> + */
> +u8 spd_read_byte(u16 device, u8 address)
> +{
> +	int smbus_read_byte(u16 device, u16 address);
> +	return smbus_read_byte(device, address);
> +}

I hope you see the irony above.


> +
> +/**
> +  * main for initram for the AMD Serengeti

No, this is for s2892. If this code is copypaste, it can not go in.
I would like us to reuse more.


//Peter