Julius Werner 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:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.h... PS2, Line 198: u32
This file already used both styles (see google_chromeec_i2c_xfer vs. google_chromeec_set_sku_id). […]
It's okay to use either but consistency within the same file is appreciated.
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 504: cmd;
Can you please initialize all the command/request & response with either either zero or some safe va […]
This follows existing practice in this file but I agree that it's a bit dicey. Maybe we should just fix the whole file to use static initializer notation rather than assigning members individually? That guarantees that unspecified fields will be 0.
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 720: EC_LPC_HOST_PACKET_SIZE
You can't check an LPC-specific constant in common code. […]
Hmm... yeah, I guess that's more of a tangential problem with the I2C and SPI drivers. The real problem here is that you have to prepend 8 bytes of data in front of the transfer buffer and the API doesn't really let you do that. (The way you're using crosec_get_buffer() here is really wrong btw, that would blow up for SPI and I2C. create_proto3_request() expects to be able to use that buffer for the whole packet, including command header.)
I just don't like adding extra static buffers in pre-RAM environments when we can avoid it. Maybe a simple option for now would be to alloca() the packet buffer on the stack (enforcing a reasonable max limit, like 1K)? 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. (Also, why does the LPC driver not use crosec_proto.c? It's just reimplementing the same protocol again, would be really useful if we could merge that somehow...)
The depthcharge version...
Understood, will add support for that.
So thinking about this a bit more, one of the problems in coreboot is always that we can't (easily) persist global variables across stage transitions. This means that if we were to query the transfer buffer size dynamically, we would have to query it again in every stage, which is a bunch of unnecessary overhead. Really, other than flashing I don't think we do any EC transfers larger than 256 bytes (which is the default that should work for all ECs).
So I think we should implement support for GET_PROTOCOL_INFO (because it should make flashing faster), but I think we should only use it for flashing and we should stick with the default transfer size in all other cases. We should not always run it immediately on initialization like depthcharge.