Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39888 )
Change subject: ec/google/chromeec: Emit SSDT for PS2 keyboard if needed ......................................................................
Patch Set 3:
(21 comments)
https://review.coreboot.org/c/coreboot/+/39888/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39888/1//COMMIT_MSG@13 PS1, Line 13: Add code to emit the needed SSDT for PS2 keyboard if the board : configuraton says so. So, would this be done *only* on those devices which have a special layout and not the ones using default layout going forward? Also, in the default case, is the EC still emitting scan codes for action keys instead of function keys?
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/chip... File src/ec/google/chromeec/chip.h:
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/chip... PS1, Line 55: MAX_TOP_ROW_KEYS Comment here would be helpful.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/chip... PS1, Line 58: int num_top_row_keys; : enum chromeec_top_key keys[MAX_TOP_ROW_KEYS]; Can you please add comments indicating what these are?
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/chip... PS1, Line 65: /* : * Select whether the top row (function row) is non-standard. This : * should also be set if the EC keyboard sends scancodes for action : * keys (as opposite to function keys, which is the case with older : * standard chromeos keyboards). If this is set, then *atleast* : * 10 top row keys MUST be provided (Chromebooks have atleast 10 : * top row keys). : */ : bool custom_top_row; So, IIUC, this flag is supposed to be set by the mainboard if the EC on it sends scan codes for action keys instead of function keys. This should be independent of standard or non-standard layout, correct?
I think we should add a feature bit in EC to indicate this: https://chromium.git.corp.google.com/chromiumos/platform/ec/+/e3c9d2ab775710.... This is a new feature that is being enabled on the EC. So, if the EC advertises this feature, then coreboot will have to generate the ACPI tables exposing keymap and physmap.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/chip... PS1, Line 80: /* : * Whether to generate "keymap" property (scancodes -> linux keycodes : * mapping ) for the PS2 keyboard. This can be used to provide the : * kernel a custom keymap, which can be needed if board is using new : * / non standard keys, for which new scancodes are not known to the : * kernel. Note that the keymap if generated, *replaces* entire : * kernel keymap (and not just overrides few values). Thus need to : * provide full keymap (for all the keys) if enabled. If : * "custom_top_row" is set, then this gets automatically set. : */ : bool generate_keymap; : : /* : * Whether to generate "function-row-physmap" property for the PS2 : * keyboard. This can be used to provide the kernel with physical : * locations of the function row keys. Note that this ends up telling : * the chrome that the keyboard is capable of sending action / function : * keys for the top-row as appropriate. So chrome needs to do the : * translation *only when needed* (e.g. when using chrome settings : * like "treat top row as function key"), and it wouldn't do any : * translation otherwise (For e.g. it wouldn't translate F1-F10 : * into action codes like it does traditionally on older : * keyboards). If "custom_top_row" is set, then this gets : * automatically set. : */ : bool generate_function_row_physmap; From the comments, it looks like this will have to be true any time we have an EC which generates scan codes for action keys. Or can there be other cases?
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec.h... PS1, Line 348: fill_ssdt_ps2_keyboard nit: All functions exposed here start with google_chromeec_. Maybe google_chromeec_ssdt_ps2()
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_a... File src/ec/google/chromeec/ec_acpi.c:
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_a... PS1, Line 227: fill_ssdt_ps2_keyboard(dev); This code will never execute in case google_chromeec_get_num_pd_ports() returns error. I think this function should be reorganized such that:
void google_chromeec_fill_ssdt_generator(struct device *dev) { fill_ssdt_typec(dev); fill_ssdt_ps2_keyboard(dev); }
And what was originally in this function would go under fill_ssdt_typec()
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... File src/ec/google/chromeec/ec_ps2_keyboard.c:
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 17: set1 Is this the kernel expectation? Or is it based on the assumption that EC will be using set1 only?
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 17: scancodes Just to be sure -- these are the make codes, right?
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 111: static static const
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 185: static static const
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 220: printk(BIOS_ERR, "PS2K: added %X", top_row_keys[key].scancode); This is not an error, so use of BIOS_ERR is not correct. Also, I don't think we should be printing every key. This is going to unnecessarily fill up the BIOS logs.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 249: printk(BIOS_ERR, "PS2K: added %X", keymap); Same comment as above.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 254: keymap = KEYMAP(rest_of_keys[i].scancode, rest_of_keys[i].keycode); Wouldn't it be better to do the KEYMAP calculation at compile time rather than at runtime? i.e. instead of using struct keyinfo { }, use uint32_t for the table and store all keys as KEYMAP(0x3b, KEY_F1); and for ssdt_generate_physmap(), you can use SCAN_CODE macro to extract scan code only for the top row keys.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 256: printk(BIOS_ERR, "PS2K: added %X", keymap); Same comment as above.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 264: = NULL No need to set this to NULL. It gets set in L284 before being used.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 266: config->ps2_keyboard This should be done after you have ensured config is not NULL in L269 below.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 267: struct const
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 273: ps2k->generate_keymap = true; : ps2k->generate_function_row_physmap = true; Like I mentioned in the .h file, we should use ec feature bit to decide if the SSDT nodes need to be generated.
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 283: _SB.PCI0.PS2K
Is this the required path? Shouldn't it be under the parent (EC) ? […]
Right. So, PS2K is exposed by the EC since it is the keyboard controller. It got moved out of the EC scope because I believe Windows doesn't like that. However, EC is still the keyboard controller.
Now, I think this will have to be eventually split into 2 parts: a) Generic table generation helpers b) EC-specific implementation
--> a) Generic table generation helpers: This will have to be helper routines that generate the tables for keymap and physmap. Both these helpers will allow passing in top row keys in case mainboard wants to override that.
--> b) EC-specific implementation will check the feature bit and call into the generic helpers provided above to generate the required tables.
This will allow us to use the same infrastructure beyond just Chrome EC. I believe this is something that we would also want to use for Wilco.
So, this function would be something like:
void fill_ssdt_ps2_keyboard(struct device *dev) { // Check feature bit on EC. If not set, return.
// Check if mainboard has override for top row keys.
// Generate scope.
// Call keymap generator routine with/without custom top row keys.
// Call physmap generator routine with/without custom top row keys. }
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/inpu... File src/ec/google/chromeec/input-event-codes.h:
PS1: I don't think there is anything EC specific here. This should probably go in src/include.