Change in coreboot[master]: soc/amd/picasso/tsc: fix clock divisor range check

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48287 )

Change subject: soc/amd/picasso/tsc: fix clock divisor range check
......................................................................

soc/amd/picasso/tsc: fix clock divisor range check

The CPU core clock divisor ID needs to be in the range from 8 to 0x30
including both numbers.

TEST=Compared with Picasso's PPR #55570

Change-Id: Ie5ee342d22294044a68d2f4b2484c50f9e345196
Signed-off-by: Felix Held 
---
M src/soc/amd/picasso/tsc_freq.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/48287/1

diff --git a/src/soc/amd/picasso/tsc_freq.c b/src/soc/amd/picasso/tsc_freq.c
index 8a541fc..55c8665 100644
--- a/src/soc/amd/picasso/tsc_freq.c
+++ b/src/soc/amd/picasso/tsc_freq.c
@@ -33,7 +33,7 @@
    if (!cpudid) {
    	mhz = TSC_DEFAULT_FREQ_MHZ;
    	printk(BIOS_ERR, "Invalid divisor, set TSC frequency to %ldMHz\n", mhz);
-	} else if ((cpudid >= 8) && (cpudid < 0x3c)) {
+	} else if ((cpudid >= 8) && (cpudid <= 0x30)) {
    	mhz = (200 * cpufid) / cpudid;
    } else {
    	mhz = 25 * cpufid;

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48287 )

Change subject: soc/amd/picasso/tsc: fix clock divisor range check
......................................................................

Patch Set 1:

haven't looked in the reference code to see if it disagrees on this

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48287 )

Change subject: soc/amd/picasso/tsc: fix clock divisor range check
......................................................................

Patch Set 1:
> Patch Set 1:
>
> haven't looked in the reference code to see if it disagrees on this

turns out that the reference code disagrees with my change. unsure what the right number would be there

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48287 )

Change subject: soc/amd/picasso/tsc: fix clock divisor range check
......................................................................

Patch Set 1: Code-Review+2

I'm OK with the change.  However I'd convinced Nick to put it the way it was in CB:45055 in case a new did became unreserved, i.e. it looks like the reserved values are merely unused, not unsupportable.  Since we're reading what's already in the register, I'd err on the side of interpreting it if possible.  (And an error message is OK too.)

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48287 )

Change subject: soc/amd/picasso/tsc: fix clock divisor range check
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+2
>
> I'm OK with the change.  However I'd convinced Nick to put it the way it was in CB:45055 in case a new did became unreserved, i.e. it looks like the reserved values are merely unused, not unsupportable.  Since we're reading what's already in the register, I'd err on the side of interpreting it if possible.  (And an error message is OK too.)

Since the corresponding else block prints an error and then continues with a safe fallback, I don't see this change becoming an issue in the future and if it does, we'll at least get a useful error message

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48287 )

Change subject: soc/amd/picasso/tsc: fix clock divisor range check
......................................................................

soc/amd/picasso/tsc: fix clock divisor range check

The CPU core clock divisor ID needs to be in the range from 8 to 0x30
including both numbers.

TEST=Compared with Picasso's PPR #55570

Change-Id: Ie5ee342d22294044a68d2f4b2484c50f9e345196
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48287
Tested-by: build bot (Jenkins) 
Reviewed-by: Marshall Dawson 
---
M src/soc/amd/picasso/tsc_freq.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Marshall Dawson: Looks good to me, approved

diff --git a/src/soc/amd/picasso/tsc_freq.c b/src/soc/amd/picasso/tsc_freq.c
index 8a541fc..55c8665 100644
--- a/src/soc/amd/picasso/tsc_freq.c
+++ b/src/soc/amd/picasso/tsc_freq.c
@@ -33,7 +33,7 @@
    if (!cpudid) {
    	mhz = TSC_DEFAULT_FREQ_MHZ;
    	printk(BIOS_ERR, "Invalid divisor, set TSC frequency to %ldMHz\n", mhz);
-	} else if ((cpudid >= 8) && (cpudid < 0x3c)) {
+	} else if ((cpudid >= 8) && (cpudid <= 0x30)) {
    	mhz = (200 * cpufid) / cpudid;
    } else {
    	mhz = 25 * cpufid;