Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36452 )
Change subject: hatch: Create puff variant ......................................................................
Patch Set 5:
(16 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?
Done
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.
Done
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?
I wasn't sure so I just included them however I removed them now which seems to be your advice here if I am not mistaken?
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.
I only added it as it is specified on the Puff schematic however I shall drop it for now since we don't actually use it so lets keep the initial code simple.
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/include/variant/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 3: *
While you're addressing Furquan's comments can you add […]
We should probably not even bother having this header here as I don't think we can copyright a #include declaration :)
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 3: *
Same here
Done
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. […]
Ack
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.
For the moment yes.
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.
Ah I see, Done
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.
Done
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.
Ack, removed from this patch for now.
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.
Seems harmless to be explicit and serves as a clear reminder how and were these need to be adjusted going forwards during bring up no?
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. […]
Yep ok, I commented out these blocks for now. I want to investigate once hardware is in my hands as coreboot will tell me stuff once I see the uart light up..
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 82: # ?????
Was there supposed to be a comment?
Done
https://review.coreboot.org/c/coreboot/+/36452/4/src/mainboard/google/hatch/... PS4, Line 85: 15.3
Same comment as above.
ditto, Done
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.
I dropped it. Not important for booting proto.