libhw: hwspi: Improve comments and asserts

2 files changed, 38 insertions, 8 deletions

diff --git a/libhw_cr/rp2040_dma.h b/libhw_cr/rp2040_dma.h
index e295adf..c7f5a8f 100644
--- a/libhw_cr/rp2040_dma.h
+++ b/libhw_cr/rp2040_dma.h
@@ -11,6 +11,7 @@
 #define _LIBHW_CR_RP2040_DMA_H_
 
 #include <assert.h>
+#include <stddef.h> /* for offsetof() */
 #include <stdint.h> /* for uint32_t */
 
 #include <hardware/regs/dreq.h>   /* for DREQ_* for use with DMA_CTRL_TREQ_SEL() */
@@ -105,6 +106,8 @@ struct dma_alias3_short3                                  { READ_ADDR   ; };
 	struct dma_alias2_short3: &dma_channel_hw_addr(CH)->al2_write_addr_trig,     \
 	struct dma_alias3_short3: &dma_channel_hw_addr(CH)->al3_read_addr_trig))
 
+#define DMA_IS_TRIGGER(TYP, FIELD) (offsetof(TYP, FIELD) == 0xC)
+
 #define DMA_CHAN_WR_TRANS_COUNT(TYP) \
 	(sizeof(TYP)/4)
 
diff --git a/libhw_cr/rp2040_hwspi.c b/libhw_cr/rp2040_hwspi.c
index d4adb11..58513f4 100644
--- a/libhw_cr/rp2040_hwspi.c
+++ b/libhw_cr/rp2040_hwspi.c
@@ -133,6 +133,8 @@ void _rp2040_hwspi_init(struct rp2040_hwspi *self,
 	self->sema = (cr_sema_t){0};
 
 	/* Initialize the interrupt handler.  */
+	/* We do this on (just) the rx channel, because the way the
+	 * SSP works reads necessarily complete *after* writes.  */
 	dmairq_set_and_enable_exclusive_handler(DMAIRQ_0, self->dma_rx_data, rp2040_hwspi_intrhandler, self);
 }
 
@@ -163,23 +165,43 @@ static void rp2040_hwspi_readwritev(struct rp2040_hwspi *self, const struct dupl
 	if (!pruned_iovcnt)
 		return;
 
-	/* For tx_data_blocks, it doesn't really matter which aliases
-	 * we choose:
+	/* It doesn't *really* matter which aliases we choose:
+	 *
 	 *  - None of our fields can be NULL (so no
 	 *    false-termination).
+	 *
 	 *  - Moving const fields first so they don't have to be
-	 *    re-programmed each time isn't possible for us there need
-	 *    to be at least 2 const fields, and we only have 1
+	 *    re-programmed each time isn't possible for us; there
+	 *    need to be at least 2 const fields, and we only have 1
 	 *    (read_addr for rx_data_blocks, and write_addr for
 	 *    tx_data_blocks).
 	 *
-	 * But for rx_data_blocks, we need ctrl to be the trigger
-	 * register so that the DMA_CTRL_IRQ_QUIET flag isn't cleared
-	 * before we get to the trigger; and while for tx_data_blocks
-	 * it doesn't really matter, the inverse would be nice.
+	 * The code following this initial declaration is generic to
+	 * the alias, so changing which alias is used is easy.
+	 *
+	 * Since we have no hard requirements, here are some mild
+	 * preferences:
+	 *
+	 *  - I like the aliases being different for each channel,
+	 *    because it helps prevent alias-specific code from
+	 *    sneaking in.
+	 *
+	 *  - I like the rx channel (the channel the interrupt handler
+	 *    is wired to) having ctrl be the trigger, so that we
+	 *    don't have to worry about DMA_CTRL_IRQ_QUIET being
+	 *    cleared before the trigger, and at the end the control
+	 *    block is clean and zeroed-out.
+	 *
+	 *  - Conversely, I like the tx channel (the non-interrupt
+	 *    channel) having ctrl *not* be the trigger, so that
+	 *    DMA_CTRL_IRQ_QUIET is cleared by the time the trigger
+	 *    happens, so the IRQ machinery doesn't need to be engaged
+	 *    at all.
 	 */
 	struct dma_alias1 *tx_data_blocks = alloca(sizeof(struct dma_alias1)*(pruned_iovcnt+1));
 	struct dma_alias0 *rx_data_blocks = alloca(sizeof(struct dma_alias0)*(pruned_iovcnt+1));
+	static_assert(!DMA_IS_TRIGGER(typeof(tx_data_blocks[0]), ctrl));
+	static_assert(DMA_IS_TRIGGER(typeof(rx_data_blocks[0]), ctrl));
 
 	for (int i = 0, j = 0; i < iovcnt; i++) {
 		if (!iov[i].iov_len)
@@ -210,6 +232,11 @@ static void rp2040_hwspi_readwritev(struct rp2040_hwspi *self, const struct dupl
 	}
 	tx_data_blocks[pruned_iovcnt] = (typeof(tx_data_blocks[0])){0};
 	rx_data_blocks[pruned_iovcnt] = (typeof(rx_data_blocks[0])){0};
+	/* If ctrl isn't the trigger then we need to make sure that
+	 * DMA_CTRL_IRQ_QUIET isn't cleared before the trigger
+	 * happens.  */
+	if (!DMA_IS_TRIGGER(typeof(rx_data_blocks[0]), ctrl))
+		rx_data_blocks[pruned_iovcnt].ctrl = DMA_CTRL_IRQ_QUIET;
 
 	/* Set up ctrl.  */
 	DMA_NONTRIGGER(self->dma_tx_ctrl, read_addr) = tx_data_blocks;