[M] Change in coreboot[master]: soc/intel/jasperlake: Add per-SKU power limits

[Reka Norman (Code Review)]

Attention is currently required from: Arthur Heymans, ChiaLing, Reka Norman, Ryan Lin, Zhuohao Lee.

Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75395?usp=email )

Change subject: soc/intel/jasperlake: Add per-SKU power limits
......................................................................

Patch Set 5:

(9 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/75395/comment/0d52e37a_5bf4e623 :
PS5, Line 12: check psys on DUT
Please add details of how you tested this specific CL since it's not related to PSYS. How did you check the power limits are being set correctly?

Patchset:

PS5: 
The build is failing. You can look at the logs which the Jenkins bot linked: https://qa.coreboot.org/job/coreboot-gerrit/237558/ It looks like the PCI ID variable names are wrong.

In general please make sure all your CLs build when you upload them. You should get a verified+1 from the bot.

File src/mainboard/google/dedede/variants/blipper/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/75395/comment/49b5a5b6_b243bf62 :
PS5, Line 62: 	register "power_limits_config[JSL_N4500_6W_CORE]" = "{
            : 			.tdp_pl1_override = 6,
            : 			.tdp_pl2_override = 20,
            : 			.tdp_pl4 = 60,
            : 	}"
            : 
            : 	register "power_limits_config[JSL_N5100_6W_CORE]" = "{
            : 			.tdp_pl1_override = 6,
            : 			.tdp_pl2_override = 20,
            : 			.tdp_pl4 = 60,
            : 	}"
For the ones which are the same as the baseboard, you can just delete them instead.

https://review.coreboot.org/c/coreboot/+/75395/comment/95af07a8_b9e2803b :
PS5, Line 73: 
Why not override `JSL_N6000_6W_CORE` too? Are you sure there are no dedede variants using this SKU?

File src/soc/intel/jasperlake/chip.h:

https://review.coreboot.org/c/coreboot/+/75395/comment/b43f05b1_9fb5d63b :
PS5, Line 7: #include <device/pci_ids.h>
up one line

File src/soc/intel/jasperlake/chip.h:

https://review.coreboot.org/c/coreboot/+/75395/comment/3891c0dd_e89ff3db :
PS3, Line 45: cpu_id
> need to be cpu_id

Why?

File src/soc/intel/jasperlake/systemagent.c:

https://review.coreboot.org/c/coreboot/+/75395/comment/539a7c29_e205d400 :
PS3, Line 66: /* Get System Agent PCI ID */
            : 	sa = pcidev_path_on_root(SA_DEVFN_ROOT);
            : 	sa_pci_id = sa ? pci_read_config16(sa, PCI_DEVICE_ID) : 0xFFFF;
> Acknowledged

He means `sa` and `dev` are the same device, so you don't need the `sa = pcidev_path_on_root(SA_DEVFN_ROOT);` line, you can just use `dev` instead.

(And in general please don't mark comments as resolved until you've addressed them, or given an explanation for why you won't address them.)

https://review.coreboot.org/c/coreboot/+/75395/comment/04182ee9_b112b0c3 :
PS3, Line 84: printk(BIOS_ERR, "unknown SA ID: 0x%4x, "
            : 			"skipped power limits configuration.\n",
> There's no SA ID, so can't search corresponding TDP

He means print the value of `tdp`. We're trying to find an entry which matches both the SA ID and TDP. If we can't find a matching entry, printing both will help us figure out why.

File src/soc/intel/jasperlake/systemagent.c:

https://review.coreboot.org/c/coreboot/+/75395/comment/b64d8e96_7355b4a8 :
PS5, Line 4: #include <chip.h>
up one line