[S] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
Attention is currently required from: Subrata Banik, Paul Menzel.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73310 )
Change subject: device/pciexp_device.c: Do not enable common clock if already active ......................................................................
Patch Set 1:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/73310/comment/f2d6c10a_163a1fb7 PS1, Line 11: re-train
I would have thought it’s re-training.
Both variants can be found in the wild. But since the PCIe spec really calls it "link re-training" I will change accordingly.
https://review.coreboot.org/c/coreboot/+/73310/comment/f4bc4024_3f4c77c8 PS1, Line 14: FSP code
Which version?
For Elkhart Lake it was always there (starting with the very first versions I ever seen). So it seem more like it is platform dependent than FSP version.
https://review.coreboot.org/c/coreboot/+/73310/comment/51642413_45717caa PS1, Line 18: re-train
Ditto.
Ack
https://review.coreboot.org/c/coreboot/+/73310/comment/1f85506d_60ccacd6 PS1, Line 26: In particular, link issues were discovered : with a Pericom PCIe switch (PI7C9X2G608) on mc_ehl1 where the link has : stalled for a while after the second re-train.
Out of curiosity, are two re-trainings spec-compliant, so this is actually a bug of the Pericom PCIe […]
I have not found anything in the spec regarding the number of link re-trainings and the time between two of them. So it might be an Pericom issue, though it is not reported anywhere as a bug.
https://review.coreboot.org/c/coreboot/+/73310/comment/e7f8fbc7_2dac859f PS1, Line 33: and thus execution time
How much?
Depends how fast the link re-training is finished, which in turn depends on the root and endpoint device. Usually, somewhere in the range of a millisecond. But the timeout for the link re-training is set to 1 second in 'pciexp_retrain_link()'. So I avoided to mention dedicated time values here as the really depend on too much variables.
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/73310/comment/40e105d5_2d5fb214 PS1, Line 205: printk(BIOS_INFO, "Common Clock Configuration already enabled\n");
Maybe: […]
You mean move the CCC detection into a helper function? Since the CCC enablement is a helper function already.
-
Werner Zeh (Code Review)