From 851dde8255efda7ecf9b37a3b7b22f4edab8881f Mon Sep 17 00:00:00 2001 From: Aaron Durbin Date: Thu, 19 Apr 2018 21:15:25 -0600 Subject: drivers/spi: reduce confusion in the API Julius brought up confusion about the current spi api in [1]. In order alleviate the confusion stemming from supporting x86 spi flash controllers: - Remove spi_xfer_two_vectors() which was fusing transactions to accomodate the limitations of the spi controllers themselves. - Add spi_flash_vector_helper() for the x86 spi flash controllers to utilize in validating driver/controller current assumptions. - Remove the xfer() callback in the x86 spi flash drivers which will trigger an error as these controllers can't support the api. [1] https://mail.coreboot.org/pipermail/coreboot/2018-April/086561.html Change-Id: Id88adc6ad5234c29a739d43521c5f344bb7d3217 Signed-off-by: Aaron Durbin Reviewed-on: https://review.coreboot.org/25745 Tested-by: build bot (Jenkins) Reviewed-by: Julius Werner --- src/drivers/spi/spi-generic.c | 53 -------------------------------- src/drivers/spi/spi_flash.c | 48 +++++++++++++++++++++++++++++ src/include/spi-generic.h | 28 +++++------------ src/include/spi_flash.h | 14 +++++++++ src/soc/amd/stoneyridge/spi.c | 9 ++++-- src/soc/intel/baytrail/spi.c | 9 ++++-- src/soc/intel/braswell/spi.c | 9 ++++-- src/soc/intel/broadwell/spi.c | 9 ++++-- src/soc/intel/fsp_baytrail/spi.c | 9 ++++-- src/soc/intel/fsp_broadwell_de/spi.c | 9 ++++-- src/southbridge/amd/agesa/hudson/spi.c | 9 ++++-- src/southbridge/amd/cimx/sb800/spi.c | 9 ++++-- src/southbridge/amd/sb700/spi.c | 9 ++++-- src/southbridge/intel/common/spi.c | 9 ++++-- src/southbridge/intel/fsp_rangeley/spi.c | 9 ++++-- 15 files changed, 147 insertions(+), 95 deletions(-) (limited to 'src') diff --git a/src/drivers/spi/spi-generic.c b/src/drivers/spi/spi-generic.c index 1fcc05d4f3..6d7fcdc021 100644 --- a/src/drivers/spi/spi-generic.c +++ b/src/drivers/spi/spi-generic.c @@ -146,56 +146,3 @@ int spi_setup_slave(unsigned int bus, unsigned int cs, struct spi_slave *slave) return 0; } - -static int spi_xfer_combine_two_vectors(const struct spi_slave *slave, - struct spi_op *v1, struct spi_op *v2) -{ - struct spi_op op = { - .dout = v1->dout, .bytesout = v1->bytesout, - .din = v2->din, .bytesin = v2->bytesin, - }; - int ret; - - /* - * Combine two vectors only if: - * v1 has non-NULL dout and NULL din and - * v2 has non-NULL din and NULL dout and - * - * In all other cases, do not combine the two vectors. - */ - if ((!v1->dout || v1->din) || (v2->dout || !v2->din)) - return -1; - - ret = spi_xfer_single_op(slave, &op); - v1->status = v2->status = op.status; - - return ret; -} - -/* - * Helper function to allow chipsets to combine two vectors if possible. This - * function can only handle upto 2 vectors. - * - * Two vectors are combined if first vector has a non-NULL dout and NULL din and - * second vector has a non-NULL din and NULL dout. Otherwise, each vector is - * operated upon one at a time. - * - * Returns 0 on success and non-zero on failure. - */ -int spi_xfer_two_vectors(const struct spi_slave *slave, - struct spi_op vectors[], size_t count) -{ - int ret; - - assert (count <= 2); - - if (count == 2) { - ret = spi_xfer_combine_two_vectors(slave, &vectors[0], - &vectors[1]); - - if (!ret || (vectors[0].status != SPI_OP_NOT_EXECUTED)) - return ret; - } - - return spi_xfer_vector_default(slave, vectors, count); -} diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index f0f119addb..9cb10855fa 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -519,3 +519,51 @@ int spi_flash_ctrlr_protect_region(const struct spi_flash *flash, return -1; } + +int spi_flash_vector_helper(const struct spi_slave *slave, + struct spi_op vectors[], size_t count, + int (*func)(const struct spi_slave *slave, const void *dout, + size_t bytesout, void *din, size_t bytesin)) +{ + int ret; + void *din; + size_t bytes_in; + + if (count < 1 || count > 2) + return -1; + + /* SPI flash commands always have a command first... */ + if (!vectors[0].dout || !vectors[0].bytesout) + return -1; + /* And not read any data during the command. */ + if (vectors[0].din || vectors[0].bytesin) + return -1; + + if (count == 2) { + /* If response bytes requested ensure the buffer is valid. */ + if (vectors[1].bytesin && !vectors[1].din) + return -1; + /* No sends can accompany a receive. */ + if (vectors[1].dout || vectors[1].bytesout) + return -1; + din = vectors[1].din; + bytes_in = vectors[1].bytesin; + } else { + din = NULL; + bytes_in = 0; + } + + ret = func(slave, vectors[0].dout, vectors[0].bytesout, din, bytes_in); + + if (ret) { + vectors[0].status = SPI_OP_FAILURE; + if (count == 2) + vectors[1].status = SPI_OP_FAILURE; + } else { + vectors[0].status = SPI_OP_SUCCESS; + if (count == 2) + vectors[1].status = SPI_OP_SUCCESS; + } + + return ret; +} diff --git a/src/include/spi-generic.h b/src/include/spi-generic.h index 20c7cc72c7..e3e7f829f7 100644 --- a/src/include/spi-generic.h +++ b/src/include/spi-generic.h @@ -109,7 +109,10 @@ enum { }; /*----------------------------------------------------------------------- - * Representation of a SPI controller. + * Representation of a SPI controller. Note the xfer() and xfer_vector() + * callbacks are meant to process full duplex transactions. If the + * controller cannot handle these transactions then return an error when + * din and dout are both set. See spi_xfer() below for more details. * * claim_bus: Claim SPI bus and prepare for communication. * release_bus: Release SPI bus. @@ -232,6 +235,10 @@ void spi_release_bus(const struct spi_slave *slave); * din: Pointer to a string of bytes that will be filled in. * bytesin: How many bytes to read. * + * Note that din and dout are transferred simulataneously in a full duplex + * transaction. The number of clocks within one transaction is calculated + * as: MAX(bytesout*8, bytesin*8). + * * Returns: 0 on success, not 0 on failure */ int spi_xfer(const struct spi_slave *slave, const void *dout, size_t bytesout, @@ -281,23 +288,4 @@ static inline int spi_w8r8(const struct spi_slave *slave, unsigned char byte) return ret < 0 ? ret : din[1]; } -/* - * Helper function to allow chipsets to combine two vectors if possible. It can - * only handle upto 2 vectors. - * - * This function is provided to support command-response kind of transactions - * expected by users like flash. Some special SPI flash controllers can handle - * such command-response operations in a single transaction. For these special - * controllers, separate command and response vectors can be combined into a - * single operation. - * - * Two vectors are combined if first vector has a non-NULL dout and NULL din and - * second vector has a non-NULL din and NULL dout. Otherwise, each vector is - * operated upon one at a time. - * - * Returns 0 on success and non-zero on failure. - */ -int spi_xfer_two_vectors(const struct spi_slave *slave, - struct spi_op vectors[], size_t count); - #endif /* _SPI_GENERIC_H_ */ diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index 3a6df9ab1f..f7f3b3dbdf 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -125,4 +125,18 @@ const struct spi_flash *boot_device_spi_flash(void); int spi_flash_ctrlr_protect_region(const struct spi_flash *flash, const struct region *region); +/* + * This function is provided to support spi flash command-response transactions. + * Only 2 vectors are supported and the 'func' is called with appropriate + * write and read buffers together. This can be used for chipsets that + * have specific spi flash controllers that don't conform to the normal + * spi xfer API because they are specialized controllers and not generic. + * + * Returns 0 on success and non-zero on failure. + */ +int spi_flash_vector_helper(const struct spi_slave *slave, + struct spi_op vectors[], size_t count, + int (*func)(const struct spi_slave *slave, const void *dout, + size_t bytesout, void *din, size_t bytesin)); + #endif /* _SPI_FLASH_H_ */ diff --git a/src/soc/amd/stoneyridge/spi.c b/src/soc/amd/stoneyridge/spi.c index c94f5e7f69..718ad94bec 100644 --- a/src/soc/amd/stoneyridge/spi.c +++ b/src/soc/amd/stoneyridge/spi.c @@ -181,9 +181,14 @@ int chipset_volatile_group_end(const struct spi_flash *flash) return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = SPI_FIFO_DEPTH, .flags = SPI_CNTRLR_DEDUCT_CMD_LEN | SPI_CNTRLR_DEDUCT_OPCODE_LEN, }; diff --git a/src/soc/intel/baytrail/spi.c b/src/soc/intel/baytrail/spi.c index 73847e9b8e..918e6d6134 100644 --- a/src/soc/intel/baytrail/spi.c +++ b/src/soc/intel/baytrail/spi.c @@ -607,9 +607,14 @@ static int spi_ctrlr_xfer(const struct spi_slave *slave, const void *dout, return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = member_size(ich9_spi_regs, fdata), }; diff --git a/src/soc/intel/braswell/spi.c b/src/soc/intel/braswell/spi.c index 3fc9ba1114..b9e1627d15 100644 --- a/src/soc/intel/braswell/spi.c +++ b/src/soc/intel/braswell/spi.c @@ -591,9 +591,14 @@ static int spi_ctrlr_xfer(const struct spi_slave *slave, const void *dout, return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = member_size(ich9_spi_regs, fdata), }; diff --git a/src/soc/intel/broadwell/spi.c b/src/soc/intel/broadwell/spi.c index 2bcbebac31..7a764f1e60 100644 --- a/src/soc/intel/broadwell/spi.c +++ b/src/soc/intel/broadwell/spi.c @@ -650,9 +650,14 @@ static int spi_flash_protect(const struct spi_flash *flash, return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = member_size(ich9_spi_regs, fdata), .flash_protect = spi_flash_protect, }; diff --git a/src/soc/intel/fsp_baytrail/spi.c b/src/soc/intel/fsp_baytrail/spi.c index 96e0671c4a..6787dcbe18 100644 --- a/src/soc/intel/fsp_baytrail/spi.c +++ b/src/soc/intel/fsp_baytrail/spi.c @@ -588,9 +588,14 @@ spi_xfer_exit: return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = member_size(ich9_spi_regs, fdata), }; diff --git a/src/soc/intel/fsp_broadwell_de/spi.c b/src/soc/intel/fsp_broadwell_de/spi.c index b87ae90bc5..26c6e65741 100644 --- a/src/soc/intel/fsp_broadwell_de/spi.c +++ b/src/soc/intel/fsp_broadwell_de/spi.c @@ -604,9 +604,14 @@ spi_xfer_exit: return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = member_size(ich9_spi_regs, fdata), }; diff --git a/src/southbridge/amd/agesa/hudson/spi.c b/src/southbridge/amd/agesa/hudson/spi.c index 379594e0a3..22951ab9f0 100644 --- a/src/southbridge/amd/agesa/hudson/spi.c +++ b/src/southbridge/amd/agesa/hudson/spi.c @@ -160,9 +160,14 @@ int chipset_volatile_group_end(const struct spi_flash *flash) return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = AMD_SB_SPI_TX_LEN, .flags = SPI_CNTRLR_DEDUCT_CMD_LEN, }; diff --git a/src/southbridge/amd/cimx/sb800/spi.c b/src/southbridge/amd/cimx/sb800/spi.c index ba3f643b73..2c541e352e 100644 --- a/src/southbridge/amd/cimx/sb800/spi.c +++ b/src/southbridge/amd/cimx/sb800/spi.c @@ -151,9 +151,14 @@ int chipset_volatile_group_end(const struct spi_flash *flash) return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = AMD_SB_SPI_TX_LEN, .flags = SPI_CNTRLR_DEDUCT_CMD_LEN, }; diff --git a/src/southbridge/amd/sb700/spi.c b/src/southbridge/amd/sb700/spi.c index 6df47fd309..d3aa29662a 100644 --- a/src/southbridge/amd/sb700/spi.c +++ b/src/southbridge/amd/sb700/spi.c @@ -108,9 +108,14 @@ static int spi_ctrlr_xfer(const struct spi_slave *slave, const void *dout, return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = AMD_SB_SPI_TX_LEN, .flags = SPI_CNTRLR_DEDUCT_CMD_LEN, }; diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index 63f6e57bcf..8cb5e627d5 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -953,9 +953,14 @@ static int spi_flash_programmer_probe(const struct spi_slave *spi, return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = member_size(ich9_spi_regs, fdata), .flash_probe = spi_flash_programmer_probe, }; diff --git a/src/southbridge/intel/fsp_rangeley/spi.c b/src/southbridge/intel/fsp_rangeley/spi.c index db84914070..933a96600c 100644 --- a/src/southbridge/intel/fsp_rangeley/spi.c +++ b/src/southbridge/intel/fsp_rangeley/spi.c @@ -719,9 +719,14 @@ spi_xfer_exit: return 0; } +static int xfer_vectors(const struct spi_slave *slave, + struct spi_op vectors[], size_t count) +{ + return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); +} + static const struct spi_ctrlr spi_ctrlr = { - .xfer = spi_ctrlr_xfer, - .xfer_vector = spi_xfer_two_vectors, + .xfer_vector = xfer_vectors, .max_xfer_size = member_size(ich10_spi_regs, fdata), }; -- cgit v1.2.3