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.

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

[Stefan Reinauer]

* Uwe Hermann  [070628 23:32]:
> 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.

Yes, definitely!
> >  Mainboard specific PCI subsystem device ID.
>
> Are the subsystem IDs correct for this board or copy+pasted?

They are copy and pasted for now :-(
> > +$(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.

Potentially mainboard specific. Maybe we should drop this at some point
and choose a different approach. I am not really sure what to do yet.

We have not been using it anywhere so far.
> 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).

One of the plans for v3 should be to drop these files completely and
generate pirq from the dts.
This will need some more work though

[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 Fri, Jun 29, 2007 at 12:07:18AM +0200, Stefan Reinauer wrote:
> > 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).
>
> One of the plans for v3 should be to drop these files completely and
> generate pirq from the dts.

Now _this_ would be really great! Still needs to be created manually I
guess (I doubt we can automate this as demonstrated by getpir's failure)
but putting it in dts is a very good thing.

Uwe.

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

[Carl-Daniel Hailfinger]

On 28.06.2007 23:32, Uwe Hermann wrote:
> On Thu, Jun 28, 2007 at 06:21:38PM +0200, [e-mail] wrote:
> > ===================================================================
> > --- 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)
> > @@ -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).

The URL is wrong anyway. http://www.microsoft.com/whdc/archive/pciirq.mspx
is the current URL of that spec (for how long?).

Regards,
Carl-Daniel

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

[ron minnich]

Uwe, I hesitate to ask this, since I know we've been pushing it, but
could you do some of those 'too many spaces' and such fixes? Also yank
the URLs from those files that are wrong? I could use the help. I'll
review and ack as fast as I can.

If you can do that, and we get it committed, I will change the
mainboard-part-number to mainboard-name -- I just took that name from
the existing mainboard.c files. I am not worried about the name too
much.

irq_tables.c needs to die, but that's going to be done later.

Marc, if you can fix up the DBE61 code, I'm fine with whatever you
would like to do.

Can someone review my mods for stage1.c etc.? I realize I screwed up
and committed by mistake, but it would be nice to get this acked so I
can get stage1 mods in for our other 3 boards. I need to get the
feedback and get your corrections in.

Thanks!

ron, he of the tired hands.