[coreboot-gerrit] [M] Change in coreboot[master]: mb/google/skyrim: Use CMOS bit to toggle ABL WA for Hynix DRAM

[Martin Roth (Code Review)]

Attention is currently required from: Felix Held, Jason Glenesk, Jason Nien, Karthik Ramasubramanian, Matt DeVillier, Paul Menzel.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75698?usp=email )

Change subject: mb/google/skyrim: Use CMOS bit to toggle ABL WA for Hynix DRAM
......................................................................

Patch Set 3:

(6 comments)

File src/mainboard/google/skyrim/bootblock.c:

https://review.coreboot.org/c/coreboot/+/75698/comment/8412a6a3_bbfd6d1b :
PS2, Line 17: 18
> I'm not sure that matters for strncmp

Agreed - I was more thinking about creating the cbi_part_number array though.

https://review.coreboot.org/c/coreboot/+/75698/comment/788154f9_d0a6d28d :
PS2, Line 41: strncmp
> we don't seem to have the case-insensitive version in coreboot currently

Acknowledged

https://review.coreboot.org/c/coreboot/+/75698/comment/00521551_8a41fb3e :
PS2, Line 43: 	/* If problematic Hynix module, ensure bit set */
> Most of the comments could be dropped, as they are redundant to the code or the same thing is set by […]

I think if we're worried about it, we should get rid of most of the log messages.  If we want to know the DRAM part number, this isn't the right place to print it - that'd be in something that's run on every chromeos platform that stores the part number in CBI.

Other than that, the only log message we really need is when the bit is set or cleared - the no-change messages are implied if you don't see one of the others.

https://review.coreboot.org/c/coreboot/+/75698/comment/32fb858a_973d7f09 :
PS2, Line 94: hynix_dram_cmos_check();
> Karthik requested that we not restrict to FF, in case another variant picked up the same memory part […]

YAGNI.

So let's add a Kconfig value stating whether to check or not. It's a waste of time to check whether the system is using that memory on boards that simply don't use it. We can use that same Kconfig to see which RAMID that value is on if it's supported.

-1 = don't check.
0-15 = ram ID to check

Something like this example code:

```
config CHECK_FOR_H9JCNNNCP3MLYR-N6E
       int
       default 2 if BOARD_FROSTFLOW
       default -1
       help
         Enable checking for H9JCNNNCP3MLYR-N6E DRAM
```

If there's still concern, we can add a makefile function that greps through each boards' memory config for the faulty memory and throws a build-time error if it's there, but this value is set to -1.

File src/mainboard/google/skyrim/bootblock.c:

https://review.coreboot.org/c/coreboot/+/75698/comment/8a7de190_d7af776f :
PS3, Line 32: google_chromeec_cbi_get_dram_part_num
Part of the reason I was asking for the termination byte for the string is just because I don't have a good idea of what this function does. Does this truncate the part number, or return an error if the supplied buffer isn't large enough? Is the termination part of the size or not?

This seems like a function where a pointer should be returned instead of having the calling function make a buffer.  It's very ambiguous this way.

https://review.coreboot.org/c/coreboot/+/75698/comment/c3031c59_c71cf37e :
PS3, Line 35: return;
Should we clear the cmos bit in this case? Can we assume that if there's an error here, the bad hynix memory is not being used?  Clearing it seems better than leaving it in an unknown state.