Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39133 )
Change subject: mb/kontron: Add Kontron mAL10 COMe module support ......................................................................
Patch Set 57:
(9 comments)
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... File src/mainboard/kontron/mal10/Kconfig:
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 51: config MAINBOARD_SMBIOS_MANUFACTURER : string : default "Kontron"
Redundant?
Yes, removed
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 55: config MAINBOARD_SMBIOS_PRODUCT_NAME : string : default "COMe-mAL10"
Redundant?
Yes, removed
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 59: config INTEL_GMA_VBT_FILE : string : default "src/mainboard/$(MAINBOARDDIR)/data.vbt"
Please mention this is necessary to override default variants behavior: […]
Removed
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... File src/mainboard/kontron/mal10/acpi/cpld.asl:
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 3: Device (CPLD)
Why is this not under kontron/kempld?
Done in CB:47774
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 13: 0x0A80
This magic number matches that of bootblock. […]
Done in CB:47774
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... File src/mainboard/kontron/mal10/ramstage.c:
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 8: /* Configure GPIO */ : size_t num = 0; : const struct pad_config *gpio_table = get_gpio_table(&num); : gpio_configure_pads(gpio_table, num);
Isn't there another place to do this?
now inside a function carrier_gpio_configure()
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 14: * CPU Power Management Configuration from AMI UEFI BIOS v112: : * [AMI BIOS] -> [Advanced] -> [Cpu Configuration] : * : * | EIST | [Enabled] | : * | Turbo Mode | [Enabled] | : * | Boot performance mode | [Max Performance] | : * | C-States | [Enabled] | : * | Enhanced C-states | [Enabled] | : * | Max Package C State | C0 | : * | Max Core C State | [Core C6] | : * | C-State Auto Demoti | [C1] | : * | C-State Un-demotion | [C1] | : *
Is this info here for any particular reason?
added comment instead this
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 31: PkgCStateLimit
Why?
added comment here in the patchset:57
/* * Attention! Do not change PkgCStateLimit! This causes spikes in the power * consumption of the SoC when the system comes out of power saving mode, and * voltage sagging at the output of DC-DC converters on the COMe module. In the * AMI BIOS Setup shows this parameter, but does not allow changing it. */
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 32: MaxCoreCState
No enum for this either?
I marked this as TODO I would like to move these paramters to devicetree.cb the same patch will add enum
-
Maxim Polyakov (Code Review)