Re: [coreboot] epia-cn patch

[Uwe Hermann]

On Thu, May 15, 2008 at 12:05:02PM +0800, aaron lwe wrote:
> I've added these headers and did some clean ups. Thanks for your comments.
> Signed-off-by: Aaron Lwe

Great, thanks. I build-tested it here and it works fine, some more
comments inline.
> Index: src/mainboard/via/epia-cn/Config.lb
>
> --- src/mainboard/via/epia-cn/Config.lb	(revision 0)
> +++ src/mainboard/via/epia-cn/Config.lb	(revision 0)
> @@ -0,0 +1,216 @@
> +##
> +## This file is part of the coreboot project.
> +## 
> +## Copyright (C) 2008 VIA Technologies, Inc.

Feel free to also add the

(Written by Aaron Lwe  for VIA)

in each file (but that's optional I guess).
> +chip northbridge/via/cn700
> device pci_domain 0 on
> device pci 0.0 on end # AGP Bridge
> device pci 0.1 on end # Error Reporting
> device pci 0.2 on end # Host Bus Control
> device pci 0.3 on end # Memory Controller
> device pci 0.4 on end # Power Management
> device pci 0.7 on end # V-Link Controller
> device pci 1.0 on # PCI Bridge
> end # PCI Bridge

This looks confusing, please put the "end" on the same line.
> Index: src/mainboard/via/epia-cn/irq_tables.c
>
> --- src/mainboard/via/epia-cn/irq_tables.c	(revision 0)
> +++ src/mainboard/via/epia-cn/irq_tables.c	(revision 0)
> @@ -0,0 +1,53 @@
> +/*
> This file is part of the coreboot project.

Please add

Copyright (C) 2008 VIA Technologies, Inc.
  (Written by Aaron Lwe  for VIA)
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
> Index: src/mainboard/via/epia-cn/Options.lb
>
> --- src/mainboard/via/epia-cn/Options.lb	(revision 0)
> +++ src/mainboard/via/epia-cn/Options.lb	(revision 0)
> @@ -0,0 +1,172 @@
> +##
> +## This file is part of the coreboot project.
> +## 
> +## Copyright (C) 2008 VIA Technologies, Inc.

See above.
> +###
> +### coreboot layout values
> +###
>
> +## ROM_IMAGE_SIZE is the amount of space to allow coreboot to occupy.
> +default ROM_IMAGE_SIZE = 0x20000

Change this to 0x10000, or rather:

default ROM_IMAGE_SIZE = 64 * 1024

to make the actual coreboot code only half the size. I verified locally
that the epia-cn target will still build fine with the change.
>   /*
> If I enbale sata, filo will not find the ide disk, so I'll disable sata here
> To not conflict with pci sepc, I'll move ide device from 00:0f.1 to 00:0f.0
> */
> dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VT8237R_SATA), 0);
> if (dev != PCI_DEV_INVALID) {
> /* enable backdoor */
> reg = pci_read_config8(dev, 0xd1);
> reg |= 0x08;
> pci_write_config8(dev, 0xd1, reg);

Can you add a more detailed comment about what exactly this
does and why? It sure sounds scary for someone who hasn't read
the datasheets (i.e. someone like me).
> do_ram_command(dev, RAM_COMMAND_MRS, 0x0011d8);
> read32(rank_address + 0x002258);
> read32(rank_address + 0x121c20);
> read32(rank_address + 0x120020);

Stuff like this could use a comment as it's highly non-obvious what is
going on, especially without datasheets.
> Index: targets/via/epia-cn/Config.lb
>
> --- targets/via/epia-cn/Config.lb	(revision 0)
> +++ targets/via/epia-cn/Config.lb	(revision 0)
> @@ -0,0 +1,45 @@
> +##
> +## This file is part of the coreboot project.
> +## 
> +## Copyright (C) 2008 VIA Technologies, Inc.

See above.
> +##
> +## This program is free software; you can redistribute it and/or modify
> +## it under the terms of the GNU General Public License as published by
> +## the Free Software Foundation; either version 2 of the License, or
> +## (at your option) any later version.
> +##
> +## This program is distributed in the hope that it will be useful,
> +## but WITHOUT ANY WARRANTY; without even the implied warranty of
> +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +## GNU General Public License for more details.
> +##
> +## You should have received a copy of the GNU General Public License
> +## along with this program; if not, write to the Free Software
> +## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> +##
>
> +target via_epia_cn
>
> +mainboard via/epia-cn
> +option  CONFIG_CONSOLE_SERIAL8250=1

This should also go into Options.lb.
> +#
> +# generate the final rom like this: cat vgabios bochsbios coreboot.rom > coreboot.rom.final
> +#
> +option ROM_SIZE=512*1024 - 64 * 1024 - 64 * 1024
>
> +#
> +###
> +### Compute the start location and size size of
> +### The coreboot bootloader.
> +###
>
> +#
> +# EPIA-CN
> +#
> +romimage "image"
> option COREBOOT_EXTRA_VERSION="-epiacn"
> payload ../payload.elf
>
> +end
>
> +buildrom ./coreboot.rom ROM_SIZE "image"

Very nice stuff otherwise. I cannot test on actual hardware though, I'll
leave that to others.

Cheers, Uwe.