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 <adurbin@chromium.org>
Reviewed-on: https://review.coreboot.org/25745
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
diff --git a/src/drivers/spi/spi-generic.c b/src/drivers/spi/spi-generic.c
index 1fcc05d..6d7fcdc 100644
--- a/src/drivers/spi/spi-generic.c
+++ b/src/drivers/spi/spi-generic.c
@@ -146,56 +146,3 @@
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 f0f119a..9cb1085 100644
--- a/src/drivers/spi/spi_flash.c
+++ b/src/drivers/spi/spi_flash.c
@@ -519,3 +519,51 @@
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 20c7cc7..e3e7f82 100644
--- a/src/include/spi-generic.h
+++ b/src/include/spi-generic.h
@@ -109,7 +109,10 @@
};
/*-----------------------------------------------------------------------
- * 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 @@
* 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 @@
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 3a6df9a..f7f3b3d 100644
--- a/src/include/spi_flash.h
+++ b/src/include/spi_flash.h
@@ -125,4 +125,18 @@
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 c94f5e7..718ad94 100644
--- a/src/soc/amd/stoneyridge/spi.c
+++ b/src/soc/amd/stoneyridge/spi.c
@@ -181,9 +181,14 @@
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 73847e9..918e6d6 100644
--- a/src/soc/intel/baytrail/spi.c
+++ b/src/soc/intel/baytrail/spi.c
@@ -607,9 +607,14 @@
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 3fc9ba1..b9e1627 100644
--- a/src/soc/intel/braswell/spi.c
+++ b/src/soc/intel/braswell/spi.c
@@ -591,9 +591,14 @@
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 2bcbeba..7a764f1 100644
--- a/src/soc/intel/broadwell/spi.c
+++ b/src/soc/intel/broadwell/spi.c
@@ -650,9 +650,14 @@
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 96e0671..6787dcb 100644
--- a/src/soc/intel/fsp_baytrail/spi.c
+++ b/src/soc/intel/fsp_baytrail/spi.c
@@ -588,9 +588,14 @@
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 b87ae90..26c6e65 100644
--- a/src/soc/intel/fsp_broadwell_de/spi.c
+++ b/src/soc/intel/fsp_broadwell_de/spi.c
@@ -604,9 +604,14 @@
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 379594e..22951ab 100644
--- a/src/southbridge/amd/agesa/hudson/spi.c
+++ b/src/southbridge/amd/agesa/hudson/spi.c
@@ -160,9 +160,14 @@
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 ba3f643..2c541e3 100644
--- a/src/southbridge/amd/cimx/sb800/spi.c
+++ b/src/southbridge/amd/cimx/sb800/spi.c
@@ -151,9 +151,14 @@
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 6df47fd..d3aa296 100644
--- a/src/southbridge/amd/sb700/spi.c
+++ b/src/southbridge/amd/sb700/spi.c
@@ -108,9 +108,14 @@
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 63f6e57..8cb5e62 100644
--- a/src/southbridge/intel/common/spi.c
+++ b/src/southbridge/intel/common/spi.c
@@ -953,9 +953,14 @@
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 db84914..933a966 100644
--- a/src/southbridge/intel/fsp_rangeley/spi.c
+++ b/src/southbridge/intel/fsp_rangeley/spi.c
@@ -719,9 +719,14 @@
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),
};