Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44632 )
Change subject: mb/google/volteer/variants/eldrid: add memory.c for ddr4 support ......................................................................
Patch Set 4:
(22 comments)
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/eldrid/gpio.c:
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 22: PAD_NC(GPP_A18, NONE), This is not needed, it is already defined as a PAD_NC(GPP_A18, NONE) in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 30: PAD_NC(GPP_A22, NONE), Not needed, already defined as PAD_NC in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 37: PAD_NC(GPP_B3, NONE), : /* B5 : ISH_I2C0_CVF_SDA ==> NC */ : PAD_NC(GPP_B5, NONE), : /* B6 : ISH_I2C0_CVF_SCL ==> NC */ : PAD_NC(GPP_B6, NONE), : Not needed, already defined as PAD_NC in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 61: /* C7 : SML1DATA ==> NC */ : PAD_NC(GPP_C7, NONE), Not needed, already defined as PAD_NC in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 65: /* C13 : UART1_TXD ==> NC */ : PAD_NC(GPP_C13, NONE), Not needed, already defined as PAD_NC in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 83: /* D1 : ISH_GP1 ==> NC */ : PAD_NC(GPP_D1, NONE), : /* D2 : ISH_GP2 ==> NC */ : PAD_NC(GPP_D2, NONE), : /* D3 : ISH_GP3 ==> NC */ : PAD_NC(GPP_D3, NONE), : /* D4 : IMGCLKOUT0 ==> NC */ : PAD_NC(GPP_D4, NONE), Not needed, already defined as PAD_NC in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 99: /* D14 : ISH_UART0_TXD ==> NC */ : PAD_NC(GPP_D14, NONE) Not needed, already defined as PAD_NC in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 105: /* D18 : ISH_GP5 ==> NC */ : PAD_NC(GPP_D18, NONE), Not needed, already defined as PAD_NC in baseboard's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 109: PAD_CFG_GPO(GPP_E0, 1, DEEP), Not needed - this is already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 110: /* E1 : SPI1_IO2 ==> NC */ : PAD_NC(GPP_E1, NONE), Not needed - this is already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 125: PAD_CFG_GPO(GPP_E10, 1, DEEP) Do you want PAD_CFG_NF(GPP_E10, NONE, DEEP, NF6) here?
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 129: PAD_CFG_GPO(GPP_E13, 1, DEEP), Do you want PAD_CFG_NF(GPP_E10, NONE, DEEP, NF6) here?
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 132: /* E16 : ISH_GP7 ==> NC */ : PAD_NC(GPP_E16, NONE), : /* E17 : THC0_SPI1_INT# ==> NC */ : PAD_NC(GPP_E17, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 145: /* F6 : CNV_PA_BLANKING ==> NC */ : PAD_NC(GPP_F6, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 159: /* F14 : GSXDIN ==> NC */ : PAD_NC(GPP_F14, NONE), : /* F15 : GSXSRESET# ==> NC */ : PAD_NC(GPP_F15, NONE), : /* F16 : GSXCLK ==> NC */ : PAD_NC(GPP_F16, NONE), : /* F17 : THC1_SPI2_RST# ==> NC */ : PAD_NC(GPP_F17, NONE), : /* F18 : THC1_SPI2_INT# ==> NC */ : PAD_NC(GPP_F18, NONE), : /* F19 : SRCCLKREQ6# ==> NC */ : PAD_NC(GPP_F19, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 184: /* H6 : I2C3_SDA ==> NC */ : PAD_NC(GPP_H6, NONE), : /* H7 : I2C3_SCL ==> NC */ : PAD_NC(GPP_H7, NONE), : /* H8 : I2C4_SDA ==> NC */ : PAD_NC(GPP_H8, NONE), : /* H9 : I2C4_SCL ==> NC */ : PAD_NC(GPP_H9, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 196: /* H12 : M2_SKT2_CFG0 ==> NC */ : PAD_NC(GPP_H12, NONE), : /* H13 : M2_SKT2_CFG1 # ==> NC */ : PAD_NC(GPP_H13, NONE), : /* H14 : M2_SKT2_CFG2 # ==> NC */ : PAD_NC(GPP_H14, NONE), : /* H15 : M2_SKT2_CFG3 # ==> NC */ : PAD_NC(GPP_H15, NONE), : /* H16 : DDPB_CTRLCLK ==> NC */ : PAD_NC(GPP_H16, NONE), : /* H17 : DDPB_CTRLDATA ==> NC */ : PAD_NC(GPP_H17, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 208: /* H19 : TIME_SYNC0 ==> NC */ : PAD_NC(GPP_H19, NONE), : /* H20 : IMGCLKOUT1 ==> NC */ : PAD_NC(GPP_H20, NONE), : /* H21 : IMGCLKOUT2 ==> NC */ : PAD_NC(GPP_H21, NONE), : /* H22 : IMGCLKOUT3 ==> NC */ : PAD_NC(GPP_H22, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 225: /* R5 : HDA_SDI1 ==> NC */ : PAD_NC(GPP_R5, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 238: /* S3 : SNDW1_DATA ==> NC */ : PAD_NC(GPP_S3, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 244: /* S6 : SNDW3_CLK ==> NC */ : PAD_NC(GPP_S6, NONE), : /* S7 : SNDW3_DATA ==> NC */ : PAD_NC(GPP_S7, NONE), Not needed, already defined this way in baseboard gpio.c's gpio_table[].
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/eldrid/memory.c:
https://review.coreboot.org/c/coreboot/+/44632/3/src/mainboard/google/voltee... PS3, Line 7: static const struct mb_ddr4_cfg eldrid_memcfg = { : }; : I don't think passing an uninitialized structure to meminit_ddr4() is good. I have opened b/166127059 to get better understanding from Intel as to what is necessary.