Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42450 )
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
Patch Set 2: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 132: ENABLE_RC6 Make it clear that it's for the iGPU?
GMA_ENABLE_RC6
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 138: ENABLE_RC6P GMA_ENABLE_RC6P
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 144: This should only be selected on Ivy Bridge. We can check that it's Ivy Bridge at runtime, see comments on the code. Maybe say:
Select this if you want to enable RC6p (Deep Render Standby) for Ivy Bridge.
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 455: #if CONFIG(ENABLE_RC6) Why not make this code flexible enough so that RC6p can be enabled correctly without RC6? It's just separating the checks and changing a mask. We can control the dependency in Kconfig. Also, there's no need for preprocessor (compiler will optimize the checks):
reg32 = 0;
if (CONFIG(GMA_ENABLE_RC6)) reg32 |= 0x88040000;
if (CONFIG(GMA_ENABLE_RC6P) && (bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB) reg32 |= 0x88020000;
gtt_write(0xa090, 0x88040000); /* HW RC Control */
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 461: gtt_write(0xa090, reg32); Where did the comment go?
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 520: #if CONFIG(ENABLE_RC6) || CONFIG(ENABLE_RC6P) How about:
if (CONFIG(ENABLE_RC6) || (CONFIG(GMA_ENABLE_RC6P) && (bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB)) gtt_write(0xa094, 0x00060000);