Re: [LinuxBIOS] r394 - in LinuxBIOSv3: mainboard mainboard/adl/msm800sev mainboard/amd/norwich mainboard/artecgroup mainboard/artecgroup/dbe61 mainboard/emulation/qemu-x86 util/dtc

[Uwe Hermann]

On Thu, Jun 28, 2007 at 06:21:38PM +0200, [e-mail] wrote:
>   mainboard-vendor = "AMD";
>   mainboard-part-number = "Norwich";

Wouldn't

mainboard-name = "Norwich";

make more sense? It's not really a part number, it's a name.
> }
>
> statictree.h will have:
>  extern const char *mainboard_vendor, *mainboard_part_number;
>
> and statictree.c will have:
>  const char *mainboard_vendor = "AMD";
>  const char *mainboard_part_number = "Norwich";
>
> It is an error to NOT have the vendor and part number in the top
>  level dts.

Great! Needs to be documentated somewhere, though.
> Modified: LinuxBIOSv3/mainboard/Kconfig
>
> --- LinuxBIOSv3/mainboard/Kconfig	2007-06-28 15:19:30 UTC (rev 393)
> +++ LinuxBIOSv3/mainboard/Kconfig	2007-06-28 16:21:38 UTC (rev 394)
> @@ -38,6 +38,12 @@
>      Select this option for various systems from 
>      Advanced Micro Devices, Inc
>
> +config VENDOR_ARTECGROUP
> bool "Artec Group"
> help
>  Select this option for various systems from 
>  the Artec Group

Cosmetic:
Should end with a full stop, as every other sentence, and please write
it into one line (it won't exceed 79 characters, I think).
> +choice
> prompt "Mainboard model"
> depends on VENDOR_ARTECGROUP
>
> +config BOARD_ARTECGROUP_DBE61
> bool "dbe61"

Should be "DBE61" as it's the user-visible name in menuconfig.
> +config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID
> hex
> default 0x1022
> depends BOARD_ARTECGROUP_DBE61
> help
>  Mainboard specific PCI subsystem vendor ID.
>
> +config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID
> hex
> default 0x2323
> depends BOARD_ARTECGROUP_DBE61
> help
>  Mainboard specific PCI subsystem device ID.

Are the subsystem IDs correct for this board or copy+pasted?
> +$(obj)/linuxbios.vpd:
> $(Q)printf "  BUILD   DUMMY VPD\n"
> $(Q)dd if=/dev/zero of=$(obj)/linuxbios.vpd bs=256 count=1 $(SILENT)

Mainboard-specific? If not it should be in some common Makefile, not in
every single mainboard Makefile.
> Added: LinuxBIOSv3/mainboard/artecgroup/dbe61/cmos.layout
>
> --- LinuxBIOSv3/mainboard/artecgroup/dbe61/cmos.layout	                        (rev 0)
> +++ LinuxBIOSv3/mainboard/artecgroup/dbe61/cmos.layout	2007-06-28 16:21:38 UTC (rev 394)

Missing license header.
> msr.lo =
> 	(6<<28) |		// cas_lat
> 	(10<<24)|		// ref2act
> 	(7<<20)|		// act2pre
> 	(3<<16)|		// pre2act
> 	(3<<12)|		// act2cmd
> 	(2<<8)|			// act2act
> 	(2<<6)|			// dplwr
> 	(2<<4)|			// dplrd
> 	(3);			// dal

Maybe make the hardcoded values defines which have the name of the
comments to the right?
> Added: LinuxBIOSv3/mainboard/artecgroup/dbe61/irq_tables.c
>
> --- LinuxBIOSv3/mainboard/artecgroup/dbe61/irq_tables.c	                        (rev 0)
> +++ LinuxBIOSv3/mainboard/artecgroup/dbe61/irq_tables.c	2007-06-28 16:21:38 UTC (rev 394)

Missing license header.
> @@ -0,0 +1,60 @@
> +/* This file was generated by getpir.c, do not modify!
> (but if you do, please run checkpir on it to verify)
> Contains the IRQ Routing Table dumped directly from your memory, which BIOS sets up
> Documentation at : http://www.microsoft.com/hwdev/busbios/PCIIRQ.HTM
>
> +*/

Can we drop this? A link to the documentation should be in the wiki, not
every single file. As far as I understand the getpir output is not
reliable anyway, so we should drop the above note (also from getpir output).
> +#include <arch/pirq_routing.h>
>
> +#define ID_SLOT_PCI_NET		1			// ThinCan ethernet
> +#define ID_SLOT_PCI_RSVD1	2           // reserved entry 1
> +#define ID_SLOT_PCI_RSVD3	3           // reserved entry 2
> +#define ID_SLOT_PCI_RSVD2	4			// reserved entry 3
> +#define ID_EMBED_PCI		0xff		// onboard PCI device
>
> +// CS5535 PCI INT[A-D] Interrupt Routing lines.
> +#define NO_CONNECT			0			// not used
> +#define CS_PCI_INTA			1			// PCI INTA
> +#define CS_PCI_INTB			2			// PCI INTB
> +#define CS_PCI_INTC			3			// PCI INTC
> +#define CS_PCI_INTD			4			// PCI INTD
>
> +// IRQ bitmap reference line	FEDCBA9876543210
> +//								0000110000100000b
> +#define PCI_IRQ					0xc20	// PCI allowed IRQs here
>
> +const struct irq_routing_table intel_irq_routing_table = 
> +{
> PIRQ_SIGNATURE,  /* u32 signature */
> PIRQ_VERSION,    /* u16 version   */
> 32+16*6,		/* there can be total 2 devices on the bus */

^                                       ^
   One of them is wrong (2 or 6)
> /* special for the root. Emit the names for the mainboard vendor and part # */
> for_each_property(bi->dt, prop) {
> if (streq(prop->name, "mainboard-vendor")){
> 	found_mainboard_vendor = 1;
> 	fprintf(f, "const char *mainboard_vendor = \"%s\";\n", prop->val.val);
> }
> if (streq(prop->name, "mainboard-part-number")){
> 	found_mainboard_partnumber = 1;
> 	fprintf(f, "const char *mainboard_part_number = \"%s\";\n", prop->val.val);
> }
> }
> if (! 	found_mainboard_vendor){

^^^
       Drop the spaces.
> die("There is no mainboard-vendor property in the root. Please add one."
> 	"(and make sure there is a mainboard-part-number property too");
> }
> if (! 	found_mainboard_partnumber){

Ditto
> die("There is no mainboard-part-number property in the root. "
> 	"Please add one."
> 	"(and make sure there is a mainboard-vendor property too");
> }
> /* emit the code, if any */
>  if (code)
>   fprintf(f, "%s\n", code);
>
> @@ -1323,6 +1350,7 @@
>    fix_next(bi->dt);
>    /* emit any includes that we need  -- TODO: ONLY ONCE PER TYPE*/
>    fprintf(f, "#include <device/device.h>\n#include <device/pci.h>\n");
>
> fprintf(f, "extern const char *mainboard_vendor, *mainboard_part_number;\n");
> flatten_tree_emit_includes(bi->dt, &linuxbios_emitter, f, &strbuf, vi);
>
> flatten_tree_emit_structdecls(bi->dt, &linuxbios_emitter, f, &strbuf, vi);

Uwe.