16 comments:
File src/mainboard/google/hatch/variants/puff/gpio.c:
Patch Set #4, Line 16: #include <arch/acpi.h>
Is this required?
Done
Patch Set #4, Line 25: override_gpio_table
nit: This is not required since there is already a weak implementation provided by baseboard.
Done
/* 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?
/* 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.
File src/mainboard/google/hatch/variants/puff/include/variant/acpi/dptf.asl:
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 :)
File src/mainboard/google/hatch/variants/puff/include/variant/ec.h:
Same here
Done
Patch Set #4, Line 17: baseboard/ec.h
Not everything from baseboard/ec.h applies to Puff. […]
Ack
File src/mainboard/google/hatch/variants/puff/include/variant/gpio.h:
Patch Set #4, Line 19: baseboard/gpio.h
I am assuming you ensured everything in baseboard/gpio.h applies to Puff.
For the moment yes.
File src/mainboard/google/hatch/variants/puff/overridetree.cb:
Patch Set #4, Line 4: PchSerialIoPci
PchSerialIoDisabled? since it looks like you are disabling I2C0 below.
Ah I see, Done
Patch Set #4, Line 5: PchSerialIoPci
PchSerialIoDisabled? since it looks like you are disabling I2C1 below.
Done
# 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.
.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?
Patch Set #4, 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..
Patch Set #4, Line 82: # ?????
Was there supposed to be a comment?
Done
Same comment as above.
ditto, Done
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.
To view, visit change 36452. To unsubscribe, or for help writing mail filters, visit settings.