Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44457 )
Change subject: util/intelp2m: Add support for Cannonlake-LP SoCs ......................................................................
Patch Set 2: Code-Review+1
(11 comments)
It looks great. Thanks for this work.
Some notes: I recommend inheriting function from the sunrise through the interface. In this case, it is easier to understand which functions are the same as the parent (Please see platforms/lbg/macro.go)
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/parser/parser... File util/intelp2m/parser/parser.go:
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/parser/parser... PS2, Line 145: PlatformSpecific{}, PlatformSpecific{ InheritanceTemplate : snr.PlatformSpecific{}, },
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... File util/intelp2m/platforms/cnl/macro.go:
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 10: import "../snr"
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 21: type InheritanceMacro interface { Pull() GpoMacroAdd() NativeFunctionMacroAdd() NoConnMacroAdd() }
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 60: macro := common.GetMacro() : dw1 := macro.Register(PAD_CFG_DW1) : var pull = map[uint8]string{ : 0x0: "NONE", : 0x2: "DN_5K", : 0x4: "DN_20K", : 0x9: "UP_1K", : 0xa: "UP_5K", : 0xb: "UP_2K", : 0xc: "UP_20K", : 0xd: "UP_667", : 0xf: "NATIVE", : } : str, valid := pull[dw1.GetTermination()] : if !valid { : str = "INVALID" : fmt.Println("Error", : macro.PadIdGet(), : " invalid TERM value = ", : int(dw1.GetTermination())) : } : macro.Separator().Add(str) Now, if this code is the same as in Sunrise, replace with this platform.InheritanceMacro.Pull()
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 94: macro.Add("(").Id().Pull().Rstsrc().Trig().Invert().Add("),") Just for me: GPI differs from sunrise here
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 179: macro := common.GetMacro() : dw0 := macro.Register(PAD_CFG_DW0) : term := macro.Register(PAD_CFG_DW1).GetTermination() : : // #define PAD_CFG_GPO(pad, val, rst) \ : // _PAD_CFG_STRUCT(pad, \ : // PAD_FUNC(GPIO) | PAD_RESET(rst) | \ : // PAD_TRIG(OFF) | PAD_BUF(RX_DISABLE) | !!val, \ : // PAD_PULL(NONE) | PAD_IOSSTATE(TxLASTRxE)) : if dw0.GetRXLevelEdgeConfiguration() != common.TRIG_OFF { : dw0.CntrMaskFieldsClear(common.RxLevelEdgeConfigurationMask) : } : macro.Set("PAD_CFG") : if macro.IsOwnershipDriver() { : // PAD_CFG_GPO_GPIO_DRIVER(pad, val, rst, pull) : macro.Add("_GPO_GPIO_DRIVER").Add("(").Id().Val().Rstsrc().Pull().Add("),") : return : } : if term != 0 { : // e.g. PAD_CFG_TERM_GPO(GPP_B23, 1, DN_20K, DEEP), : macro.Add("_TERM") : } : macro.Add("_GPO").Add("(").Id().Val() : if term != 0 { : macro.Pull() : } : macro.Rstsrc().Add("),") platform.InheritanceMacro.GpoMacroAdd()
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 210: macro := common.GetMacro() : // e.g. PAD_CFG_NF(GPP_D23, NONE, DEEP, NF1) : macro.Set("PAD_CFG_NF") : if macro.Register(PAD_CFG_DW1).GetPadTol() != 0 { : macro.Add("_1V8") : } : macro.Add("(").Id().Pull().Rstsrc().Padfn().Add("),") platform.InheritanceMacro.NativeFunctionMacroAdd()
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 221: macro := common.GetMacro() : // #define PAD_NC(pad, pull) : // _PAD_CFG_STRUCT(pad, : // PAD_FUNC(GPIO) | PAD_RESET(DEEP) | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE), : // PAD_PULL(pull) | PAD_IOSSTATE(TxDRxE)), : dw0 := macro.Register(PAD_CFG_DW0) : : // Some fields of the configuration registers are hidden inside the macros, : // we should check them to update the corresponding bits in the control mask. : if dw0.GetRXLevelEdgeConfiguration() != common.TRIG_OFF { : dw0.CntrMaskFieldsClear(common.RxLevelEdgeConfigurationMask) : } : if dw0.GetResetConfig() != 1 { // 1 = RST_DEEP : dw0.CntrMaskFieldsClear(common.PadRstCfgMask) : } : : macro.Set("PAD_NC").Add("(").Id().Pull().Add("),") platform.InheritanceMacro.NoConnMacroAdd()
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 246: PlatformSpecific{} PlatformSpecific{InheritanceMacro : snr.PlatformSpecific{}}
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... File util/intelp2m/platforms/cnl/template.go:
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 4: type InheritanceTemplate interface { KeywordCheck(line string) bool }
https://review.coreboot.org/c/coreboot/+/44457/2/util/intelp2m/platforms/cnl... PS2, Line 27: for _, keyword := range []string{ : "GPP_", "GPD", : } { : if strings.Contains(line, keyword) { : return true : } : } : return false return platform.InheritanceTemplate.KeywordCheck(line)