View Change

14 comments:

• File src/mainboard/google/hatch/variants/puff/gpio.c:

  • Patch Set #4, Line 16: #include <arch/acpi.h>

    Is this required?

  • Patch Set #4, Line 25: override_gpio_table

    nit: This is not required since there is already a weak implementation provided by baseboard.

  • Patch Set #4, 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?

  • Patch Set #4, Line 65:

    /* F2  : MEM_CH_SEL */
    	PAD_CFG_GPI(GPP_F2, NONE, PLTRST),
    

    This is not required.

• File src/mainboard/google/hatch/variants/puff/include/variant/ec.h:

  • Patch Set #4, 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.

• 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.

• 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.

  • Patch Set #4, Line 5: PchSerialIoPci

    PchSerialIoDisabled? since it looks like you are disabling I2C1 below.

  • Patch Set #4, 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.

  • Patch Set #4, 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.

  • Patch Set #4, Line 76: chip drivers/i2c/generic

    hid is a required field: https://review.coreboot.org/cgit/coreboot.git/tree/src/drivers/i2c/generic/chip.h?id=refs/heads/master#n23

    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

  • Patch Set #4, Line 82: # ?????

    Was there supposed to be a comment?

  • Patch Set #4, Line 85: 15.3

    Same comment as above.

  • Patch Set #4, 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.

To view, visit change 36452. To unsubscribe, or for help writing mail filters, visit settings.