Attention is currently required from: Martin L Roth, Sorin Pop.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69493 )
Change subject: packeteer 6500 ......................................................................
Patch Set 1:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/69493/comment/574a262e_f7ad56c6 PS1, Line 7: packeteer 6500
At coreboot, we prefer to start the subject line with a path, and to make sure we have a verb in the […]
It's a good idea to list what is tested and works, what does not work, and what is not tested.
https://review.coreboot.org/c/coreboot/+/69493/comment/b79052cc_3eb1e629 PS1, Line 8:
Needs a developer's certificate of origin sign-off saying that this is code that you have the rights […]
You can amend the sign-off into your commit: `git commit -s --amend`
Patchset:
PS1:
Hey, welcome to coreboot! You might want to look through some of the documentation: https://corebo […]
Welcome!
File src/arch/x86/pirq_routing.c:
https://review.coreboot.org/c/coreboot/+/69493/comment/4e3afdf3_76e19b4a PS1, Line 143: if (link > 0x5f) { : link -= 0x5f; : }
Split this int a separate patch and explain why its needed please.
+1
File src/cpu/intel/Kconfig:
https://review.coreboot.org/c/coreboot/+/69493/comment/640f823b_a956252e PS1, Line 19: source "src/cpu/intel/socket_FC_PGA370/Kconfig"
Please split all of the cpu/intel files into a separate commit.
Adding the socket can be its own commit
File src/mainboard/packeteer/6500/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/69493/comment/ea751cfc_1bcf7c76 PS1, Line 35: #include <acpi/dsdt_top.asl> This should probably be the first include
https://review.coreboot.org/c/coreboot/+/69493/comment/4ce6082e_9f319f76 PS1, Line 89: Method(OSFL, 0){ : : if(LNotEqual(OSVR, Ones)) {Return(OSVR)} /* OS version was already detected */ : : if(CondRefOf(_OSI)) : { : Store(1, OSVR) /* Assume some form of XP */ : if (_OSI("Windows 2006")) /* Vista */ : { : Store(2, OSVR) : } : } else { : If(WCMP(_OS,"Linux")) { : Store(3, OSVR) /* Linux */ : } Else { : Store(4, OSVR) /* Gotta be WinCE */ : } : } : Return(OSVR) : } Please use tabs for indentation. Also, do you think some of this could be factored out? It seems like some of this is common with asus/p2b
File src/mainboard/packeteer/6500/dsdt.asl2:
PS1: Is this used?
File src/mainboard/packeteer/6500/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/69493/comment/4ff9e3f9_4c166edf PS1, Line 27: PIRQ_SIGNATURE, /* u32 signature */
code indent should use tabs where possible
Please fix.
File src/mainboard/packeteer/6500/superio.asl:
https://review.coreboot.org/c/coreboot/+/69493/comment/9b254287_206d9065 PS1, Line 5: * expose the W83977TF/EF SuperIO and some of its functionality. Is this correct? The mainboard seems to have a SMSC Super I/O