v3 Tyan s2892

[Myles Watson]

This patch adds initial support for mainboard/tyan/s2892.  It compiles, but is
untested in hardware.

Signed-off-by: Myles Watson

Thanks,
Myles

[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

[Myles Watson]

On Mon, Jan 5, 2009 at 4:07 PM, Peter Stuge  wrote:
> 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.

Thanks for the help.  I'll resubmit tomorrow.  I've got a few questions inlined.
> > +++ 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.

The board gets enumerated as a device in the tree, and I find it
helpful to have it show up with a type, vendor, and device ID instead
of Unknown Device.  Do you think we should create a new type for
mainboards?

I appreciate the reviews.

Thanks,
Myles

[Myles Watson]

On Mon, Jan 5, 2009 at 4:07 PM, Peter Stuge  wrote:
> 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.
> 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 don't know how to do that.  It would affect all vendors currently in
v3, not just the one I'm adding, though.
> > +++ svn/mainboard/tyan/s2892/stage1.c
>
> ..
> > +void ck804_enable_rom(void);
>
> This prototype shouldn't be needed here right?

It won't compile without it.  It's possible that the sources could be
rearranged so that it doesn't need to be there.
> What ROM is it anyway?

The flashROM where coreboot is stored.  Sorry I'm not sure of the
correct terminology there.
> > +#define SERIAL_DEV W83627HF_SP1
> > +#define SERIAL_IOBASE 0x3f8
>
> Isn't at least the iobase in a CONFIG_ already?

No.  It's in the dts only, which isn't available yet.
> > +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?

This function is only used for geode right now, but in geode it's used
to disable caching of the ROM.  I think that's up in the air right
now.
> > +++ 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.

Yes.  I was hoping to unify the code when we had two (maybe three)
working k8 boards in v3.  I still haven't figured out v2's execution
path.  cache_as_ram_auto.c vs. auto.c vs. ...

I'd love help here.
> > +/**
> > 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.

Me too.  See above.

Thanks again,
Myles