Hello Iru Cai,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34358
to review the following change.
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
haswell: reinitialize EHCI debug hardware after raminit
Change-Id: I54b0c9dbb64819f0f502783b632470d27ed0b2b1 Signed-off-by: Iru Cai mytbk920423@gmail.com --- M src/northbridge/intel/haswell/raminit.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/34358/1
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 9beb23c..46566be 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -14,6 +14,7 @@ */
#include <console/console.h> +#include <console/usb.h> #include <string.h> #include <cbmem.h> #include <arch/cbfs.h> @@ -173,6 +174,10 @@ die("UEFI PEI System Agent not found.\n"); }
+ /* mrc.bin reconfigures USB, so reinit it to have debug */ + if (CONFIG(USBDEBUG_IN_PRE_RAM)) + usbdebug_hw_init(true); + /* For reference print the System Agent version * after executing the UEFI PEI stage. */
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34358 )
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
Patch Set 1:
was this tested?
Hello Patrick Rudolph, Iru Cai, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34358
to look at the new patch set (#2).
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
haswell: reinitialize EHCI debug hardware after raminit
Tested on Lenovo ThinkPad T440p.
Change-Id: I54b0c9dbb64819f0f502783b632470d27ed0b2b1 Signed-off-by: Iru Cai mytbk920423@gmail.com --- M src/northbridge/intel/haswell/raminit.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/34358/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34358 )
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34358/2/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/34358/2/src/northbridge/intel/haswe... PS2, Line 159: if (rv) { : switch (rv) { : case -1: : printk(BIOS_ERR, "PEI version mismatch.\n"); : break; : case -2: : printk(BIOS_ERR, "Invalid memory frequency.\n"); : break; : default: : printk(BIOS_ERR, "MRC returned %x.\n", rv); : } : die_with_post_code(POST_INVALID_VENDOR_BINARY, : "Nonzero MRC return value.\n"); : } It would be nice to ensure this (which is rather important IMHO) gets printed out to the usbdebug gadget.
https://review.coreboot.org/c/coreboot/+/34358/2/src/northbridge/intel/haswe... PS2, Line 174: die("UEFI PEI System Agent not found.\n"); This one gets printed out fine, as mrc has not run if we're on this path.
Iru Cai (vimacs) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34358 )
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34358/2/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/34358/2/src/northbridge/intel/haswe... PS2, Line 159: if (rv) { : switch (rv) { : case -1: : printk(BIOS_ERR, "PEI version mismatch.\n"); : break; : case -2: : printk(BIOS_ERR, "Invalid memory frequency.\n"); : break; : default: : printk(BIOS_ERR, "MRC returned %x.\n", rv); : } : die_with_post_code(POST_INVALID_VENDOR_BINARY, : "Nonzero MRC return value.\n"); : }
It would be nice to ensure this (which is rather important IMHO) gets printed out to the usbdebug ga […]
Hmm, it looks like the added code can be put before the ``if (rv)`` line.
Hello Patrick Rudolph, Iru Cai, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34358
to look at the new patch set (#4).
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
haswell: reinitialize EHCI debug hardware after raminit
Tested on Lenovo ThinkPad T440p.
Change-Id: I54b0c9dbb64819f0f502783b632470d27ed0b2b1 Signed-off-by: Iru Cai mytbk920423@gmail.com --- M src/northbridge/intel/haswell/raminit.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/34358/4
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34358 )
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34358 )
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34358/2/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/34358/2/src/northbridge/intel/haswe... PS2, Line 159: if (rv) { : switch (rv) { : case -1: : printk(BIOS_ERR, "PEI version mismatch.\n"); : break; : case -2: : printk(BIOS_ERR, "Invalid memory frequency.\n"); : break; : default: : printk(BIOS_ERR, "MRC returned %x.\n", rv); : } : die_with_post_code(POST_INVALID_VENDOR_BINARY, : "Nonzero MRC return value.\n"); : }
Hmm, it looks like the added code can be put before the ``if (rv)`` line.
Done
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34358 )
Change subject: haswell: reinitialize EHCI debug hardware after raminit ......................................................................
haswell: reinitialize EHCI debug hardware after raminit
Tested on Lenovo ThinkPad T440p.
Change-Id: I54b0c9dbb64819f0f502783b632470d27ed0b2b1 Signed-off-by: Iru Cai mytbk920423@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34358 Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/haswell/raminit.c 1 file changed, 6 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 9beb23c..2fdbe07 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -14,6 +14,7 @@ */
#include <console/console.h> +#include <console/usb.h> #include <string.h> #include <cbmem.h> #include <arch/cbfs.h> @@ -155,6 +156,11 @@ asm volatile ( "call *%%ecx\n\t" :"=a" (rv) : "c" (entry), "a" (pei_data)); + + /* mrc.bin reconfigures USB, so reinit it to have debug */ + if (CONFIG(USBDEBUG_IN_PRE_RAM)) + usbdebug_hw_init(true); + if (rv) { switch (rv) { case -1:
-
Angel Pons (Code Review) -
Iru Cai (vimacs) (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)