Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Are there any plans to rebase the FW EC code against the more up to date google EC code ? #57

Open
jcdutton opened this issue Dec 21, 2024 · 2 comments

Comments

@jcdutton
Copy link

Are there any plans to rebase the FW EC code against the more up to date google EC code ?
I have looked at some of the FW EC code, and some of it is pretty low quality to be honest.
So, my first thought is maybe go through it and improve things but I then looked at the google EC code, and it seems to have already fixed most of the problems.
I compared it to this:
git clone https://chromium.googlesource.com/chromiumos/platform/ec

Now, that is not to say the google code is super high quality. For example, it also has no re-entrant locking around the port80 history ring buffer. it also will not display all port80 values on the console. For example, if 4 port80 values are set in quick succession, it only outputs one of those to the console, thus missing/dropping some.

@DHowett
Copy link
Contributor

DHowett commented Dec 21, 2024

For what it's worth, it appears that Google operates the same way. When a program is shipped it gets a branch, and that branch does not typically take updates from main.

There was an effort early on to move hx20 (the Intel 11th Gen) to the new Zephyr-based EC over on Google's Gerrit instance, but it did not work properly and I suspect re-validating the entire program on a new EC would have been prohibitively expensive.

I am sure Framework would appreciate some pull requests that fix the code you've identified as "low quality".

But just a quick note...

not ... super high quality
no re-entrant locking

The embedded controller is single-threaded and cooperatively multitasked. I believe there is no reentrancy in this case because the incoming port80 signals through eSPI trigger an interrupt and subsequent task queuing of the one task that handles port80 IO.

@jcdutton
Copy link
Author

jcdutton commented Dec 21, 2024

Looking at the code it looks like port80 writes happen from multiple sources. the port80 value is treated in some places as a u8, other places a u16, u32 and int. Not particularly consistent. I have not checked the code, but some might be from normal code, and some might be from interrupt context, so that sort of potential problem does not look like it is addressed in the code yet.
Do you happen to know how many bits are used for port80 values coming from the BIOS, as opposed to other sources?
My view of the port80 stuff, is really that I wish to come up with a lookup table I can apply offline that takes any PORT80: XXXX value in the console logs and converts it into a meaningful message. I say offline, because I doubt there is enough space in the flash to contain all the meaningful message texts.
Another part I am looking to add is to add some message in the console log or panic log that shows:

  1. Why the system shutdown (to catch some hint as to why some people are seeing unexpected shutdowns). I don't believe the laptop can shut down without the EC helping.
  2. Why the system woke. I.e. was it a key press, lid open, WoL packet, or some other source.
  3. Provide some way for the ec console log to appear in the Linux syslog and maybe increase the console log buffer size to be able to handle all the logs messages during a laptop power cycle.
  4. implement the CCD "chan" command on ectool side. It is useful for filtering log messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants