Attention is currently required from: Angel Pons, Nico Huber, Patrick Rudolph.
Anastasios Koutian has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83269?usp=email )
Change subject: cpu/intel/model_206ax: Allow PL1/PL2 configuration ......................................................................
Patch Set 1:
(4 comments)
File src/cpu/intel/model_206ax/chip.h:
https://review.coreboot.org/c/coreboot/+/83269/comment/86de19a3_21f010be?usp... : PS1, Line 46: int pl1; /* Long-term power limit*/ : int pl2; /* Short-term power limit*/
nit: space before comment end […]
These are in microwatts. Agree that units should be made clear. Could also add a comment as per lines 49-50 below. Also good idea about using unsigned. Perhaps we should do the same for current limits below, and the TCC offset above.
File src/cpu/intel/model_206ax/model_206ax_init.c:
https://review.coreboot.org/c/coreboot/+/83269/comment/7b1871cc_310a2766?usp... : PS1, Line 99: dev->upstream->dev->upstream->children->chip_info;
`dev->chip_info` should work as well as it's all one chip, northbridge & CPU.
Thanks, will use
https://review.coreboot.org/c/coreboot/+/83269/comment/d03b5102_cf6292f8?usp... : PS1, Line 138: dev_path(dev)
This is a bit confusing because it would print the northbridge's path from CPU code.
Ack, will remove
File src/northbridge/intel/sandybridge/northbridge.c:
https://review.coreboot.org/c/coreboot/+/83269/comment/61257c86_6b3c9d1b?usp... : PS1, Line 354: 28
Would it make sense to turn this into another devicetree setting? Doesn't have to be in this change.
Yes, although it should be made clear that the lowest of the two power limits would apply when both are specified. Can do on tip of branch.