[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow PL1/PL2 configuration

[Angel Pons (Code Review)]

Attention is currently required from: Anastasios Koutian, Nico Huber, Patrick Rudolph.

Angel Pons 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:

(8 comments)

File src/cpu/intel/model_206ax/chip.h:

https://review.coreboot.org/c/coreboot/+/83269/comment/77813df1_b9d86afc?usp... :
PS1, Line 46: int pl1; /* Long-term power limit*/
            : 	int pl2; /* Short-term power limit*/
nit: space before comment end

Also, which units are these values in? I would at least make these unsigned, and possibly suffix the units:

```suggestion
    unsigned int pl1; /* Long-term power limit */
    unsigned int pl2; /* Short-term power limit */
```

File src/cpu/intel/model_206ax/model_206ax.h:

https://review.coreboot.org/c/coreboot/+/83269/comment/79ed9a71_039e0706?usp... :
PS1, Line 132: void set_power_limits(u8 power_limit_1_time, struct device *);
```suggestion
void set_power_limits(u8 power_limit_1_time, struct device *dev);
```

File src/cpu/intel/model_206ax/model_206ax_init.c:

https://review.coreboot.org/c/coreboot/+/83269/comment/b4d6c47d_e78c8ae0?usp... :
PS1, Line 99: dev->upstream->dev->upstream->children->chip_info;
I'm not too fond of walking the hierarchy like this but I don't know of a better way to handle it

https://review.coreboot.org/c/coreboot/+/83269/comment/6ddccf02_78ffccad?usp... :
PS1, Line 137: 	if(conf->pl1) {
```suggestion
    if (conf->pl1) {
```

https://review.coreboot.org/c/coreboot/+/83269/comment/f533289d_47fc89f5?usp... :
PS1, Line 138: dev_path(dev)
This is a bit confusing because it would print the northbridge's path from CPU code.

https://review.coreboot.org/c/coreboot/+/83269/comment/e158f1b0_2d406066?usp... :
PS1, Line 139: 		limit.lo |= ((conf->pl1 * power_unit) / 1000000) & PKG_POWER_LIMIT_MASK;
This could theoretically overflow, and the resulting value would then be truncated. Not sure what happens if power limits are too low (e.g. zero or one).

https://review.coreboot.org/c/coreboot/+/83269/comment/916879ff_a21941a6?usp... :
PS1, Line 149: 	if(conf->pl2){
```suggestion
    if (conf->pl2) {
```

File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/83269/comment/d92cf5f5_e2433a5e?usp... :
PS1, Line 354: 28
Would it make sense to turn this into another devicetree setting? Doesn't have to be in this change.