Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-2
Patch Set 1: Code-Review+2
Patch Set 1: Code-Review-1 Ideally amd/block/spi would work with hudson too.
Why would soc/amd code apply to southbridge/amd chips? Isn't it ok to draw a line and say that the soc blocks will only support chips in the soc directory?
My -1 is because of not using the introduced to SPI subsystem that tells command byte will not consume FIFO. Giving this -2 and hoping Furquan will comment as he probably nows the reasoning best why the flag was added.
-------/* Deduct the command length from the spi_crop_chunk() calculation for ------- sizing a transaction. */ -------SPI_CNTRLR_DEDUCT_CMD_LEN = 1 << 0, -------/* Remove the opcode size from the command length used in the ------- spi_crop_chunk() calculation. Controllers which have a dedicated ------- register for the command byte would set this flag which would ------- allow the use of the maximum transfer size. */ -------SPI_CNTRLR_DEDUCT_OPCODE_LEN = 1 << 1,
The flags are there for describing the underlying controller hardware. The spi transactions currently add length and opcode to the buffer. These flags indicate that there are other registers aside from the fifo to hold that information. The hudson code has the following:
static const struct spi_ctrlr spi_ctrlr = {
-------.xfer_vector = xfer_vectors, -------.max_xfer_size = AMD_SB_SPI_TX_LEN, -------.flags = SPI_CNTRLR_DEDUCT_CMD_LEN,
};
And it also implements xfer_vector() which is what the majority of the code takes advantage of in the code base. What I think Kyösti is looking for is the spi controller code here be more conforming with the way the rest of the code is doing things.
That said, when there's a flash_probe() callback in the spi_controller object then the spi flash API is used for probing (and should then every other spi flash command). This code seems to be taking the latter path, but the code is a bit of a hybrid it seems. e.g. fch_spi_flash_probe() is sending commands through its own internal API. I didn't go through all the paths in the driver, but it looks like this code is duplicating a lot of things that are done in the generic spi code. i.e. there seems to be a lot of duplication in this code and the generic stuff. I think the code needs a broader refactoring to fit better into the current API.