Attention is currently required from: Sean Rhodes, Paul Menzel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 )
Change subject: ec/starlabs: Add standardised ITE EC support ......................................................................
Patch Set 19:
(29 comments)
File src/ec/starlabs/merlin/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/d9c33e0b_6d5a4dea PS19, Line 41: 0x0a 10
https://review.coreboot.org/c/coreboot/+/58343/comment/28a32d02_6e2bd84e PS19, Line 42: 0x64 100
https://review.coreboot.org/c/coreboot/+/58343/comment/ad601849_4ebf9ac3 PS19, Line 43: 0x64 100
File src/ec/starlabs/merlin/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/767f8750_41e75582 PS19, Line 56: \LIDS = 0x03 This seems weird
https://review.coreboot.org/c/coreboot/+/58343/comment/57552d3f_1e16f81a PS19, Line 67: Embedded nit: `Embedded Controller`?
https://review.coreboot.org/c/coreboot/+/58343/comment/536ca3d2_d21fae7f PS19, Line 101: Local1 Local1 is not always initialized
https://review.coreboot.org/c/coreboot/+/58343/comment/5a2d005b_73e50010 PS19, Line 104: // ECWT (Embedded Write Method) What exactly does this method do? Is it used anywhere?
https://review.coreboot.org/c/coreboot/+/58343/comment/d742ffea_0df58e1f PS19, Line 135: Embedded nit: `Embedded Controller`?
https://review.coreboot.org/c/coreboot/+/58343/comment/060dcc71_377cc10d PS19, Line 170: Method (ECMD, 0, Serialized) : { : Return (0x00) : } : : Method (ECM1, 0, Serialized) : { : Return (0x00) : } : : Method (ECNT, 0, Serialized) : { : Return (0x00) : } Are these methods needed?
File src/ec/starlabs/merlin/acpi/hid.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/ab866d13_4504c125 PS19, Line 109: 0xFA 250
https://review.coreboot.org/c/coreboot/+/58343/comment/2bf6337c_e93b68c3 PS19, Line 111: 0x04 4
https://review.coreboot.org/c/coreboot/+/58343/comment/5ca0e78d_c9794dc9 PS19, Line 244: Its It's
https://review.coreboot.org/c/coreboot/+/58343/comment/40b4458a_7a076e0b PS19, Line 248: Its It's
https://review.coreboot.org/c/coreboot/+/58343/comment/7ee4c6d2_4f2769b5 PS19, Line 255: Its It's
File src/ec/starlabs/merlin/acpi/keyboard.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/bec4164d_b2418840 PS19, Line 23: _Q80 Copy-paste error
File src/ec/starlabs/merlin/acpi/thermal.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/65036123_a1f98f75 PS19, Line 5: Notify (_TZ.TZ01, 0x80 Syntax error. Looks like the file is unused, I'd remove it.
File src/ec/starlabs/merlin/acpi/typec.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/8a6b0415_a593bc93 PS19, Line 5: 0x32 50
https://review.coreboot.org/c/coreboot/+/58343/comment/e6ab44d7_ee181678 PS19, Line 35: 0x32 50
https://review.coreboot.org/c/coreboot/+/58343/comment/c64af2fe_10d60683 PS19, Line 60: 0x32 50
File src/ec/starlabs/merlin/acpi/ubtc.asl:
PS19: Is this generated via acpigen?
File src/ec/starlabs/merlin/ec.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/0f2cfb72_59d267f9 PS19, Line 41: /* Keyboard Backlight Timeout */ : #define SEC_30 0x00 : #define MIN_1 0x01 : #define MIN_3 0x02 : #define MIN_5 0x03 : #define NEVER 0x04 : : /* Fn Ctrl Swap */ : #define FN_CTRL 0x00 : #define CTRL_FN 0x01 : : /* Max Charge Setting */ : #define CHARGE_100 0x00 : #define CHARGE_80 0xbb : #define CHARGE_60 0xaa : : /* Fan Mode Setting */ : #define FAN_NORMAL 0x00 : #define FAN_AGGRESSIVE 0xbb : #define FAN_QUIET 0xaa : : /* Fn Lock State */ : #define UNLOCKED 0x00 : #define LOCKED 0x01 : : /* Trackpad State */ : #define TRACKPAD_ENABLED 0x00 : #define TRACKPAD_DISABLED 0x22 : : /* Keyboard Brightness Levels */ : #define KBL_ON 0xdd : #define KBL_OFF 0xcc : #define KBL_LOW 0xbb : #define KBL_HIGH 0xaa : : /* Keyboard Backlight State */ : #define KBL_DISABLED 0x00 : #define KBL_ENABLED 0xaa I'd use actual enums for these instead of macros.
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/d2d4d40b_65ef1ebd PS19, Line 68: 0 I'd use a named value here
https://review.coreboot.org/c/coreboot/+/58343/comment/2b1cd191_cf7ef9ae PS19, Line 81: default: nit: for completeness, I'd explicitly handle `SEC_30`:
case SEC_30: default: ec_write(ECRAM_KBL_TIMEOUT, SEC_30); break;
https://review.coreboot.org/c/coreboot/+/58343/comment/254653f5_aa5a9436 PS19, Line 59: /* : * Keyboard Backlight Timeout : * : * Setting: kbl_timeout : * : * Values: 30 Seconds, 1 Minute, 3 Minutes, 5 Minutes, Never : * Default: 30 Seconds : * : */ : switch (get_uint_option("kbl_timeout", 0)) { : case MIN_1: : ec_write(ECRAM_KBL_TIMEOUT, MIN_1); : break; : case MIN_3: : ec_write(ECRAM_KBL_TIMEOUT, MIN_3); : break; : case MIN_5: : ec_write(ECRAM_KBL_TIMEOUT, MIN_5); : break; : case NEVER: : ec_write(ECRAM_KBL_TIMEOUT, NEVER); : break; : default: : ec_write(ECRAM_KBL_TIMEOUT, SEC_30); : break; : } Idea: Encapsulate these into helper functions?
https://review.coreboot.org/c/coreboot/+/58343/comment/6103c8d4_44b50934 PS19, Line 113: switch (get_uint_option("max_charge", 0)) { : case CHARGE_80: : ec_write(ECRAM_MAX_CHARGE, CHARGE_80); : break; : case CHARGE_60: : ec_write(ECRAM_MAX_CHARGE, CHARGE_60); : break; : default: : ec_write(ECRAM_MAX_CHARGE, CHARGE_100); : break; : } cmos.layout of CB:56088 won't work with this, the default case will always be taken. The case values need to match cmos.layout enumeration values.
Same issue applies to the other options, although some happen to work because CMOS and EC use the same enumeration values.
Another idea: The current cmos.layout uses 0-based sequential values for enumerations. The switch statement can be replaced with an array as look-up table, using the CMOS value as index to obtain the corresponding value to write to the EC.
https://review.coreboot.org/c/coreboot/+/58343/comment/db1c3918_4e5a3c16 PS19, Line 134: #if CONFIG(EC_STARLABS_FAN) : switch (get_uint_option("fan_mode", 0)) { : case FAN_AGGRESSIVE: : ec_write(ECRAM_FAN_MODE, FAN_AGGRESSIVE); : break; : case FAN_QUIET: : ec_write(ECRAM_FAN_MODE, FAN_QUIET); : break; : default: : ec_write(ECRAM_FAN_MODE, FAN_NORMAL); : break; : } : #endif Why not use a regular C check?
https://review.coreboot.org/c/coreboot/+/58343/comment/7bbedbab_59c34cea PS19, Line 193: #if CONFIG(EC_STARLABS_KBL_LEVELS) : switch (get_uint_option("kbl_brightness", 0)) { : case KBL_OFF: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_OFF); : break; : case KBL_HIGH: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_HIGH); : break; : default: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_LOW); : break; : } : #else : switch (get_uint_option("kbl_brightness", 0)) { : case KBL_OFF: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_OFF); : break; : default: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_ON); : break; : } : #endif Same as before
https://review.coreboot.org/c/coreboot/+/58343/comment/4b5cbfdb_f318d90c PS19, Line 237: .read_resources = noop_read_resources, : .set_resources = noop_set_resources, : }; ITE ECs have a Super I/O part, and the LDNs' resources should be reported. See the code for other ITE ECs and Super I/Os on how to do it.
https://review.coreboot.org/c/coreboot/+/58343/comment/28811687_0170e90a PS19, Line 251: ITE ITE One "ITE" is enough?
-
Angel Pons (Code Review)