Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36452 )
Change subject: hatch: Create puff variant ......................................................................
Patch Set 4:
(14 comments)
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/gpio.c:
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 16: #include <arch/acpi.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 25: override_gpio_table nit: This is not required since there is already a weak implementation provided by baseboard.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 41: /* E23 : TP1 */ : PAD_NC(GPP_E23, NONE), : /* H17 : TP2 */ : PAD_NC(GPP_H17, NONE), Do you need to configure the TPs in bootblock?
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 65: /* F2 : MEM_CH_SEL */ : PAD_CFG_GPI(GPP_F2, NONE, PLTRST), This is not required.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/include/variant/ec.h:
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 17: baseboard/ec.h Not everything from baseboard/ec.h applies to Puff. You can fix it later, but it would be good to have a bug to capture that.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 19: baseboard/gpio.h I am assuming you ensured everything in baseboard/gpio.h applies to Puff.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 4: PchSerialIoPci PchSerialIoDisabled? since it looks like you are disabling I2C0 below.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 5: PchSerialIoPci PchSerialIoDisabled? since it looks like you are disabling I2C1 below.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 18: # VR Slew rate setting : register "AcousticNoiseMitigation" = "1" : register "SlowSlewRateForIa" = "2" : register "SlowSlewRateForGt" = "2" : register "SlowSlewRateForSa" = "2" : register "FastPkgCRampDisableIa" = "1" : register "FastPkgCRampDisableGt" = "1" : register "FastPkgCRampDisableSa" = "1" These can be skipped for now. Those can be added if really required.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 47: .rise_time_ns = 0, : .fall_time_ns = 0, nit: You can set these later when/if required rather than setting to 0 explicitly.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 76: chip drivers/i2c/generic hid is a required field: https://review.coreboot.org/cgit/coreboot.git/tree/src/drivers/i2c/generic/c...
If you are unsure of the hid right now, you can skip adding this node for now and re-visit it later when support for some kernel driver needs to be enabled. For now, you can leave it as:
device pci 15.2 on end
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 82: # ????? Was there supposed to be a comment?
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 85: 15.3 Same comment as above.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 107: chip drivers/spi/acpi : register "name" = ""CRFP"" : register "hid" = "ACPI_DT_NAMESPACE_HID" : register "uid" = "1" : register "compat_string" = ""google,cros-ec-spi"" : register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A23_IRQ)" : register "wake" = "GPE0_DW0_23" : device spi 1 on end : end # FPMCU I don't think this is correct for Puff as per schematics.