Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36207 )
Change subject: ec/google/chromeec: Add EC driver support for software sync ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 36: EC_VER_MASK
Done
This is already defined in https://review.coreboot.org/cgit/coreboot.git/tree/src/ec/google/chromeec/ec.... Can we just use that directly?
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 502: int Probably okay to make this static? I believe it will be used only by the functions in this file just like cmd_version_supported()
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 544: offset It will be helpful to have comments indicating what the params mean for this and the other functions you are adding.
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 720: EC_LPC_HOST_PACKET_SIZE
Really, the right way to solve it would be to overhaul this whole API so that you can pass a sequence of at least 2 data pointers which will be sent back to back by the transfer function, but I guess that would be a bigger task
We can potentially add a google_chromeec_command_data_arr() [Sorry, couldn't think of a better name right now] that takes as input array of data_in pointers and size pointers and does the same work that crosec_proto or ec_lpc is doing i.e. package bytes correctly or write bytes to LPC port by iterating over the data pointer array. google_chromeec_command() can be moved to ec.c and that can call the same function(google_chromeec_command_data_arr) with a single element array for data_in and size_in. But yeah, that can be done separately.
I'm fine with using alloca() for the buffer. Makes me curious what the stack size is in romstage for newer archs, I'll have to look at that.
For the stack, you can find details in Kconfig for CML: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/cannonlake/...