Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32405 )
Change subject: kohaku: Update overridetree.cb ......................................................................
Patch Set 1:
(10 comments)
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 3: Let's keep this consisten with rest of the entries i.e. use space only.
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 21: PcdSdDetectChk I don't see this defined for cannonlake. Is that required?
https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/cannonlake/...
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 22: sd_acpi_mode Same with this.
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 28: | extra space?
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 42: .rise_time_ns = 152, : .fall_time_ns = 30, These will have to be updated after running tests on the board.
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 73: ACPI_IRQ_WAKE_EDGE_LOW Wake using same gpio as interrupt will not work until the ITSS configuration is fixed in FSP. b/123967687
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 90: 09 0x9 to make it consistent with 0x2c above.
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 94: drivers/i2c/da7219 This will require appropriate config to be selected in Kconfig for hatch.
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 109: 1a 0x1a
https://review.coreboot.org/#/c/32405/1/src/mainboard/google/hatch/variants/... PS1, Line 116: # no native SD support : register "sdcard_cd_gpio" = "" Can you please put this up with rest of the registers?