RFC: Enable user-configurable settings including Fn Lock, Debounce, etc

[meklort]

• Feature Name: user-configuration
• Start Date: 2020-05-18
• Relevant Issues: system76/firmware-open#78 and PRs #62 and #80

Summary

The change add support for users to configure the EC behavior without having to build and flash a custom firmware image. A new flash API has been added, allowing for arbitrary read/write access to flash. A new configuration API has been added, persistently storing saved settings into an unused region of flash.

Motivation

While I am able to customize and rebuild the EC firmware, not all users are able or willing to do so, especially with the risks towards bricking their device. Additionally, any time a firmware update is release, a new customized EC binary must be generated to contain the user customization.

This change will allow users to customize the EC without the above mentioned risks, while also allowing those customization to persists across firmware updates.

Guide-level explanation

A user may list all configurable options within the EC by using the ectool.

meklort@lemur ~/g/ec (config-pull)> ./ectool.sh config
    Finished release [optimized] target(s) in 0.00s
Fn Lock: When enabled, F1-F12 will activate their default function
Min: 1
Max: 0
Current Value: 1

Keyboard Debounce Time: Debounce time in milliseconds
Min: 1000
Max: 1
Current Value: 15

If a user wishes to customize a setting, they can also do so using the ectool.

meklort@lemur ~/g/ec (config-pull)> ./ectool.sh config "Keyboard Debounce Time" 20
    Finished release [optimized] target(s) in 0.01s
meklort@lemur ~/g/ec (config-pull)> ./ectool.sh config "Keyboard Debounce Time"
    Finished release [optimized] target(s) in 0.00s
Dump "Keyboard Debounce Time"
Keyboard Debounce Time: Debounce time in milliseconds
Min: 1000
Max: 1
Current Value: 20

Reference-level explanation

Two new modules are provided with this change:

Flash API

A new flash API is added, allowing code to read/write arbitrary locations in flash. Due to the XIP nature of the EC, this code must run out of RAM. As such, it is modeled and based off of the scratch ROM code used for firmware updates.

• A new flash application with a single entry point is added.
• The flash API is configured to use no globals and to place all locals on the stack This ensures that no memory in the standard application is mangled.
• During early init, the flash API is copied to RAM using SCAR0/SCAR2 Note: This conflicts with the scratch ROM, however this is not an issue as firmware flashing requires a full system reboot.

Config API

A new configuration API is added, allowing existing code to register a configuration entry with the system. Each configuration entry contains a short name, description, default, min, and, max values. Additionally, an option callback can bet set, allowing a function to be called when a configuration value is updated.

Using SMFI, all configuration entries are exported to the host. Additionally, the host is able to specify a new value, within the limits, for the configuration entry. All configuration settings are stored in the EC, allowing for the host to auto detect available parameters.

When a configuration entry is registered with the system, the flash APIs are used to update the configuration value with any user-customized settings. If no user customization is found, the default value is used instead.

When a configuration entry is updated by the user, the flash API is used to store the new value persistently. In order to allow quick updates to the settings, old values in flash are invalidated before a new setting is added. This allows new settings to be stored without having to first erase flash.

Existing Modules

Existing modules can easily be updated to support user configurations. As an example, the keyboard module has been updated to allow configurable function lock and default state in #80 .

Drawbacks

This code not yet complete, and it does add a couple of risks:

• ~~Currently, no code handles a full flash. If the full 4KB are used with configuration entries, no more settings can be saved. In this case, the firmware should be updated to (1) erase the sector and (2) store all known settings back into flash.~~
• This code calls between two applications. The flash API is a separate image which has no knowledge of XSEG / DSEG usage. As a result, the a developer modifying the must take special not to mangle existing registers.
• Any mistakes in the flash writing code could accidentally erase or overwrite the main EC binary, bricking the device. Special care need to be taken to ensure that a device is not bricked. Note: Additional checking could be added to the flash API to limit writes to a small window.
• The configuration APIs only support a single 32bit value. In some cases, a larger value may be desired. As an example, the keyboard layout could be stored in flash using a similar mechanism. This would allow custom keymaps. At present, this use case is not supported.
• The code currently does a linear search in flash. As the ~~4KB~~ 1KB configuration space fills up, read/write access time can be impacted. This can slow down the initial boot, as well as reconfiguration time by the user.

Rationale and alternatives

A couple of additional possibilities were considered including:

• Using the UEFI payload to store setting in nvram. This would allow the EC firmware to never touch flash, and instead could use the existing nvram APIs in UEFI. This would require (1) UEFI to scan nvram for all EC related variables (using a new GUID) and passing these to the EC. (2) Adding a mechanism to set the UEFI variables by first reading possible entries from the EC, then writing these to NVRAM. This means the change cannot be standalone, and so it was not really considered.
• Using a more compact format on flash could have been done. The flash usage could be minimized by only storing a single 32bit value. This compresses the used space, however means that the location in flash cannot change. As a result, firmware updates must maintain past locations and the flash must be erased to update each value. Due to these limitations, a method supporting auto-detection and arbitrary locations in flash is supported instead. Also due to the large 128KB flash vs the (current) 32KB firmware size, the larger size seems acceptable.

Prior art

N/A

Unresolved questions

• What is the best way for the user to interact with this? The firmware-setup UEFI app could be updated to expose these to the user. The Pop! OS settings page could be updated instead to provide a gui for these settings instead.

Future possibilities

This can be further enhanced in the future (as mentioned above) to support other configuration value types (strings, blobs, etc) instead of just 32bit integers. A number of additional settings make sense to be implemented (I have done some, but have not cleaned them up and included them here yet). These include:

• Battery charge thresholds
• Startup keyboard back light value
• (possibly, if hardware supported) Start on lid open
• Additional keyboard customization (remap pgup/pgdwn to left/right,
• USB port settings (disable left usb port when in a sleep power state)

[meklort]

I've updated the code to handle a full configuration space - the flash block is now erased and the configuration is written back, re-compacted.

With that change, it should be functional without any issues, at least on lemp9.

[meklort]

I've added the SPDX identifiers. Note that per the SPDX specification, it's recommended to include the standard license header in addition to the SPDX specified, so I've left it in. If you prefer it removed, I can do so.

I've also added KEYMAP_FN_LOCK to the remaining system76/ boards and made sure everything compiles (I have not tested the Arduino builds).

[crawfxrd]

If you prefer it removed, I can do so.

Yes. SPDX should replace the boilerplate license block with easy to understand identifiers. The "This file is part of..." lines should also be removed (adds no value).

[meklort]

As requested, I've removed the standard license header.

[crawfxrd]

Testing on a galp3-c:

• When configured for release, system does not boot (doesn't reach first power event; LEDs are off)
• When parport debugging is enabled, system boots, but system76_ectool config reports bad values

Min: -1 æ÷      æ÷      æ÷ìp!LÀÀÀÀ1ÐÐÐÐ
Max: -11114494
Current Value: -1

: 
Min: 0
Max: 0
Current Value: 2637

[meklort]

Testing on a galp3-c:

• When configured for release, system does not boot (doesn't reach first power event; LEDs are off)

Is there anything special that needs to configure for release, or is it the default? I've been using the following: make BOARD=system76/lemp9 and make BOARD=system76/lemp9 flash_internal

If there's a different way to test release builds, let me know (-DLEVEL to a value other than 4?). I can try to reproduce it on the lemp9, once I verify that I can debrick with the RT809F. It'd also be useful to know if this only happens on galp3-c or if it can be reproduced on lemp9 on your side.

• When parport debugging is enabled, system boots, but system76_ectool config reports bad values

Min: -1 æ÷      æ÷      æ÷ìp!LÀÀÀÀ1ÐÐÐÐ
Max: -11114494
Current Value: -1

: 
Min: 0
Max: 0
Current Value: 2637

That's interesting. It implies that the command executed and tried to read the two configuration parameters (fn lock, camera en). This implies that the handshaking (smfi_cmd[1] is ok) is working as expected between the host + ec, just with bad data in smfi_cmd[2] and beyond.

I've also been testing with a modified system76_ectool set to only erase the first 64K, ensuring that my current configuration isn't erased. I'll see if that's a possible reason. (erased flash causing bootup issues vs valid) Lat time I tried there was no issue with an erased config area.

Other possible reasons might be related to the different ITE devices on the two machines. Note that I do not have access to a galp3-c, and so I can't easily test this. If you are able to trace it to a specific commit, that would be useful (all commits should be build-able / run-able.

(this is all of course ignoring the part where the code uses more stack space, and if it was already tight on galp3-c, then it could overflow / cause problems now).

[crawfxrd]

Is there anything special that needs to configure for release, or is it the default?

It's the default. That is, there's no modifications to board.mk. As a "debug" build, I enable parport logging.

EC reset hangs at flash_init(). Moving it after gpio_init() allows reset to complete (system boots), but I don't know what effect that has.

[meklort]

EC reset hangs at flash_init(). Moving it after gpio_init() allows reset to complete (system boots), but I don't know what effect that has.

Ah, makes sense. I based the flash_init code off of the lemp9 scratch.c file using SCAR0. Looks like galp3-c uses SCAR1 instead. I'll add something to update it to match on the galp3-c

[meklort]

I've added a quick change to use SCAR1 instead of SCAR0 for the galp3-c. I don't know if it'll cause any other issue (comment in scratch.c said SCAR1 is in xram at 0x800-0xC00) but feel free to test it. If it works I can either create a separate flash_init function per board, or make the EC switch a bit more generic (move the IT defines to a common include).

Note that with the current code, placing flash_init after gpio_init should not have an impact, and so the fact that it does implies SCAR0 might be doing something I don't expect on the it8587e

(for reference, I specifically use the same configuration as scratch.c for lemp to ensure that I didn't brick anything on lemp9. Code was tested conditionally as well originally such that the flash access was only triggered via an smfi command).

[crawfxrd]

system76_ectool config when using SCAR1 works:

Default Camera State: When set, the camera is enabled on system reset
Min: 1
Max: 0
Current Value: 1

Fn Lock: When enabled, F1-F12 will activate their default function
Min: 1
Max: 0
Current Value: 0

[crawfxrd]

Suppose the format changes to handle a new config. How will the EC and tool detect and handle this?

I've previously used a simple incrementing version number to detect such a case. It doesn't need to be implemented now, but depending on ideas on how this will be handled (if at all), we may want to reserve some some bytes at the start of the config space.

[meklort]

Suppose the format changes to handle a new config. How will the EC and tool detect and handle this?

I've previously used a simple incrementing version number to detect such a case. It doesn't need to be implemented now, but depending on ideas on how this will be handled (if at all), we may want to reserve some some bytes at the start of the config space.

One option could be using a different magic number to differentiate between the two.

I can also see where it might be good to add a "config type" byte in the config entry itself. On older version of the firmware, unknown types can by skipped as the size is known.

Also note that currently the code probably won't handle downgrading the most gracefully - it'd effectively try to overwrite the magic number w/o erasing, silently fail, and then try to use the following data anyway. I'll clean it up a bit and add something to future proof it.

Do note that I'll be offline for about a week, so you won't see any updates from me for a bit.

[crawfxrd]

Going over the drawbacks section:

• This code calls between two applications. The flash API is a separate image which has no knowledge of XSEG / DSEG usage. As a result, the a developer modifying the must take special not to mangle existing registers.

This one will require documentation on. Maybe a block comment at the top of one of the files, or a section in a new doc/ file on this feature. (This definitely sounds like something I will screw up.)

• Any mistakes in the flash writing code could accidentally erase or overwrite the main EC binary, bricking the device. Special care need to be taken to ensure that a device is not bricked. Note: Additional checking could be added to the flash API to limit writes to a small window.

What checks would these be? It looks like the checks in place prevent getting an address outside the config space. But it's probably reasonable to add them.

• The configuration APIs only support a single 32bit value. In some cases, a larger value may be desired. As an example, the keyboard layout could be stored in flash using a similar mechanism. This would allow custom keymaps. At present, this use case is not supported.

This sounds like a good "version 2" project.

• The code currently does a linear search in flash. As the ~4KB~ 1KB configuration space fills up, read/write access time can be impacted. This can slow down the initial boot, as well as reconfiguration time by the user.

I think this can be punted. A linear search seems sufficient for now.

But thinking of what we do for coreboot's smmstore [1]: A "compact" command could be implemented to erase the space and write back the valid entries.

[meklort]

Going over the drawbacks section:

• This code calls between two applications. The flash API is a separate image which has no knowledge of XSEG / DSEG usage. As a result, the a developer modifying the must take special not to mangle existing registers.

This one will require documentation on. Maybe a block comment at the top of one of the files, or a section in a new doc/ file on this feature. (This definitely sounds like something I will screw up.)

I think part of the main PR comments could be added to a new docs file. I'll look into doing that in addition to comments as needed in the flash code.

• Any mistakes in the flash writing code could accidentally erase or overwrite the main EC binary, bricking the device. Special care need to be taken to ensure that a device is not bricked. Note: Additional checking could be added to the flash API to limit writes to a small window.

What checks would these be? It looks like the checks in place prevent getting an address outside the config space. But it's probably reasonable to add them.

My main thought would be to add checks for the config space in the flash main.c file itself . The thought would be that if there's a bug due to communication with the two different binaries, the EC would still be bootable as the only thing that could be erased / written is the config space. The other thought is it'd add a second layer of checks in case if there's a bug in the config code. It's not strictly necessary, but I want to be extra careful when doing anything that has to do with erasing or writing to flash that's shared with the main executable to mitigate risks.

• The configuration APIs only support a single 32bit value. In some cases, a larger value may be desired. As an example, the keyboard layout could be stored in flash using a similar mechanism. This would allow custom keymaps. At present, this use case is not supported.

This sounds like a good "version 2" project.

Agreed, that doesn't need to be included in this pull request.

• The code currently does a linear search in flash. As the ~4KB~ 1KB configuration space fills up, read/write access time can be impacted. This can slow down the initial boot, as well as reconfiguration time by the user.

I think this can be punted. A linear search seems sufficient for now.

Agreed, The flash size is small enough that it shouldn't be noticeable.

But thinking of what we do for coreboot's smmstore [1]: A "compact" command could be implemented to erase the space and write back the valid entries.

This actually automatically happens right now any time the flash fills up - it compacts. The main reason is to limit flash writes (and minimizes slowdowns due to erasing), but as a side effect I can easily add a compact command that could when needed. I should probably add a check to ignore the command if it's already as compact as possible, though.

[crawfxrd]

LGTM. I think this is nearly done. @jackpot51 this seems like a good point to get some feedback from you.

[meklort]

I've rebased this and added the compact command as requested earlier.

[crawfxrd]

Keymap changes need to be added for the oryp6.

This commit looks unneeded: 744b85b6fb235 ("common: Enable preprocessor #ifs based on __EC__")

[meklort]

Keymap changes need to be added for the oryp6.

Done. I've also added changes for the new keymaps as well.

This commit looks unneeded: 744b85b ("common: Enable preprocessor #ifs based on __EC__")

This is needed for the compiler defines based on __EC__ to work properly in 215343f45872465476c208739e45227ced6610c0

Previously, __EC__ was defined to an undefined value. As a result, any preprocessor conditionals based on __EC__ could not be evaluated (always evaluated as false). Commit 744b85b allows the preprocessor to evaluate __EC__ to a valid value, and so conditionals now evaluate properly.

[meklort]

I've removed the two commits that enable configuring the function lock and CCD_EN default state as requested. Those are now in the separate pull request #80 (I think this is what you wanted, but please let me know if it's not the case.)

[rellimmot]

Is this a dead effort? I'm wondering if y'all ever plan on fielding this feature or if I should seek alternate means of Fn Lock/Invert.

[leviport]

@rellimmot Thank you for so thoroughly expressing interest in this feature. If the PR is open, the effort is not dead.