Fix and enable SPI for pit

I've simplified the SPI module, since we only ever use SPI1 (and there
were already a number of places which assumed this was true).
Somewhere along the way I fixed a number of problems keeping the code
from compiling and working on STM32L.  The code isn't currently used
anywhere else, but should still work there (that is, I don't think I
broke it working on STM32F if you re-enable it on some STM32F
platform).

BUG=chrome-os-partner:19073
BRANCH=none
TEST=from u-boot console, sspi 2:0 64 9f0000
     u-boot prints: FDFDFDFDFDFDFDFD
     ec prints: [193.740912 HC 0x9f][193.741141 HC err 1]

[sjg: gpio optimization back in for now]
[dianders: add comment as rspangler requested; update SOBs]

Change-Id: Ib9419403e4e44dadc1f17681e48401882cb49175
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/49684

board/pit/board.c

@@ -28,6 +28,8 @@ const struct gpio_info gpio_list[GPIO_COUNT] = {
 	{"CHARGER_INT", GPIO_C, (1<<6),  GPIO_INT_RISING, pmu_irq_handler},
 	{"LID_OPEN",    GPIO_C, (1<<13), GPIO_INT_BOTH, lid_interrupt},
 	{"SUSPEND_L",   GPIO_C, (1<<7),  GPIO_INT_BOTH, gaia_suspend_event},
+	{"SPI1_NSS",    GPIO_A, (1<<4),  GPIO_INT_BOTH | GPIO_PULL_UP,
+	 spi_event},
 	{"KB_IN00",     GPIO_C, (1<<8),  GPIO_KB_INPUT,
 	 keyboard_raw_gpio_interrupt},
 	{"KB_IN01",     GPIO_C, (1<<9),  GPIO_KB_INPUT,
@@ -65,7 +67,6 @@ const struct gpio_info gpio_list[GPIO_COUNT] = {
 	{"PMIC_RESET",  GPIO_A, (1<<15), GPIO_OUT_LOW, NULL},
 #ifndef CONFIG_SPI
 	{"SPI1_MISO",   GPIO_A, (1<<6),  GPIO_OUT_HIGH, NULL},
-	{"SPI1_NSS",    GPIO_A, (1<<4),  GPIO_PULL_UP, NULL},
 #endif
  	{"KB_OUT00",    GPIO_B, (1<<0),  GPIO_KB_OUTPUT, NULL},
 	{"KB_OUT01",    GPIO_B, (1<<8),  GPIO_KB_OUTPUT, NULL},
@@ -102,14 +103,14 @@ void board_config_post_gpio_init(void)
 	gpio_set_alternate_function(GPIO_B, (1 << 3), GPIO_ALT_TIM2);
 
 #ifdef CONFIG_SPI
-	/* SPI1 on pins PA4-7 (alt. function push-pull, 10MHz) */
-	val = STM32_GPIO_CRL(GPIO_A) & ~0xffff0000;
-	val |= 0x99990000;
-	STM32_GPIO_CRL(GPIO_A) = val;
-
-	gpio_set_flags(GPIO_SPI1_NSS, GPIO_INT_BOTH);
+	/* SPI1 on pins PA4-7 */
+	gpio_set_alternate_function(GPIO_A,
+				    (1 << 4) | (1 << 5) | (1 << 6) | (1 << 7),
+				    GPIO_ALT_SPI);
+	/* 10 MHz pin speed */
+	STM32_GPIO_OSPEEDR(GPIO_A) = (STM32_GPIO_OSPEEDR(GPIO_A) & ~0xff00) |
+		0xaa00;
 #endif
-
 }
 
 #ifdef CONFIG_PMU_BOARD_INIT

board/pit/board.h

@@ -39,16 +39,14 @@
 #define CONFIG_PMU_HARD_RESET
 #define CONFIG_PMU_TPS65090
 #define CONFIG_SMART_BATTERY
-
+#define CONFIG_SPI
 
 #ifdef PORT_TO_PIT
 /* TODO(rspangler): enable these features when they compile */
 #define CONFIG_LOW_POWER_IDLE
-#define CONFIG_SPI
 #define CONFIG_WATCHDOG_HELP
 #endif
 
-
 #ifndef __ASSEMBLER__
 
 /* By default, enable all console messages except keyboard */
@@ -57,7 +55,17 @@
 /* Keyboard output port list */
 #define KB_OUT_PORT_LIST GPIO_A, GPIO_B, GPIO_C
 
-/* Charging */
+/*
+ * Charging.
+ *
+ * "HOST" means the port where the EC is the master, which has the battery,
+ * charger and PMU.
+ *
+ * "SLAVE" means the port where the EC is the slave, which has the AP (host
+ * processor).
+ *
+ * TODO: In this context, "host" is badly overloaded and confusing.
+ */
 #define I2C_PORT_HOST 0
 #define I2C_PORT_BATTERY I2C_PORT_HOST
 #define I2C_PORT_CHARGER I2C_PORT_HOST
@@ -77,6 +85,7 @@ enum gpio_signal {
 	GPIO_CHARGER_INT,
 	GPIO_LID_OPEN,
 	GPIO_SUSPEND_L,
+	GPIO_SPI1_NSS,
 	/* Keyboard inputs */
 	GPIO_KB_IN00,
 	GPIO_KB_IN01,
@@ -107,7 +116,6 @@ enum gpio_signal {
 	GPIO_PMIC_RESET,
 #ifndef CONFIG_SPI
 	GPIO_SPI1_MISO,
-	GPIO_SPI1_NSS,
 #endif
 	GPIO_KB_OUT00,
 	GPIO_KB_OUT01,

chip/stm32/gpio-stm32l15x.c

@@ -178,6 +178,12 @@ test_mockable int gpio_get_level(enum gpio_signal signal)
 		  gpio_list[signal].mask);
 }
 
+uint16_t *gpio_get_level_reg(enum gpio_signal signal, uint32_t *mask)
+{
+	*mask = gpio_list[signal].mask;
+	return (uint16_t *)&STM32_GPIO_IDR(gpio_list[signal].port);
+}
+
 void gpio_set_level(enum gpio_signal signal, int value)
 {
 	STM32_GPIO_BSRR(gpio_list[signal].port) =

chip/stm32/registers.h

@@ -193,6 +193,8 @@ struct timer_ctlr {
 
 	unsigned or;
 };
+/* Must be volatile, or compiler optimizes out repeated accesses */
+typedef volatile struct timer_ctlr timer_ctlr_t;
 
 /* --- GPIO --- */
 
@@ -437,29 +439,24 @@ struct timer_ctlr {
 #define STM32_SPI2_PORT             1
 
 /* The SPI controller registers */
-struct spi_ctlr {
-	unsigned ctrl1;
-	unsigned ctrl2;
+struct stm32_spi_regs {
+	uint16_t ctrl1;
+	uint16_t _pad0;
+	uint16_t ctrl2;
+	uint16_t _pad1;
 	unsigned stat;
-	unsigned data;
+	uint16_t data;
+	uint16_t _pad2;
 	unsigned crcp;
 	unsigned rxcrc;
 	unsigned txcrc;
 	unsigned i2scfgr;	/* STM32F10x only */
 	unsigned i2spr;		/* STM32F10x only */
 };
+/* Must be volatile, or compiler optimizes out repeated accesses */
+typedef volatile struct stm32_spi_regs stm32_spi_regs_t;
 
-/*
- * TODO(vpalatin):
- * For whatever reason, our toolchain is substandard and generate a
- * function every time you are using this inline function.
- *
- * That's why I have not used inline stuff in the registers definition.
- */
-#define stm32_spi_addr(port) \
-	((struct spi_ctlr *)(port == 0 ? STM32_SPI1_BASE : STM32_SPI2_BASE))
-#define stm32_spi_port(addr) \
-	((addr) == STM32_SPI1_BASE ? 0 : 1)
+#define STM32_SPI1_REGS ((stm32_spi_regs_t *)STM32_SPI1_BASE)
 
 /* --- Debug --- */
 

chip/stm32/spi.c

@@ -23,18 +23,14 @@
 #define CPRINTF(format, args...) cprintf(CC_SPI, format, ## args)
 
 /* DMA channel option */
-static const struct dma_option dma_tx_option[2] = {
-	{DMA_SPI1_TX, (void *)&stm32_spi_addr(STM32_SPI1_PORT)->data,
-	 DMA_MSIZE_BYTE | DMA_PSIZE_HALF_WORD},
-	{DMA_SPI2_TX, (void *)&stm32_spi_addr(STM32_SPI2_PORT)->data,
-	 DMA_MSIZE_BYTE | DMA_PSIZE_HALF_WORD},
+static const struct dma_option dma_tx_option = {
+	DMAC_SPI1_TX, (void *)&STM32_SPI1_REGS->data,
+	DMA_MSIZE_BYTE | DMA_PSIZE_HALF_WORD
 };
 
-static const struct dma_option dma_rx_option[2] = {
-	{DMA_SPI1_RX, (void *)&stm32_spi_addr(STM32_SPI1_PORT)->data,
-	 DMA_MSIZE_BYTE | DMA_PSIZE_HALF_WORD},
-	{DMA_SPI2_RX, (void *)&stm32_spi_addr(STM32_SPI2_PORT)->data,
-	 DMA_MSIZE_BYTE | DMA_PSIZE_HALF_WORD},
+static const struct dma_option dma_rx_option = {
+	DMAC_SPI1_RX, (void *)&STM32_SPI1_REGS->data,
+	DMA_MSIZE_BYTE | DMA_PSIZE_HALF_WORD
 };
 
 /* Status register flags that we use */
@@ -155,14 +151,13 @@ static int wait_for_bytes(struct dma_channel *rxdma, int needed,
  * We keep an eye on the NSS line - if this goes high then the transaction is
  * over so there is no point in trying to send the reply.
  *
- * @param spi		SPI controller to send data on
  * @param txdma		TX DMA channel to send on
  * @param status	Status result to send
  * @param msg_ptr	Message payload to send, which normally starts
  *			SPI_MSG_HEADER_LEN bytes into out_msg
  * @param msg_len	Number of message bytes to send
  */
-static void reply(struct spi_ctlr *spi, struct dma_channel *txdma,
+static void reply(struct dma_channel *txdma,
 		  enum ec_status status, char *msg_ptr, int msg_len)
 {
 	char *msg;
@@ -194,7 +189,7 @@ static void reply(struct spi_ctlr *spi, struct dma_channel *txdma,
 	/* Add the checksum and get ready to send */
 	msg[msg_len - 2] = sum & 0xff;
 	msg[msg_len - 1] = SPI_MSG_PREAMBLE_BYTE;
-	dma_prepare_tx(dma_tx_option[stm32_spi_port(spi)], msg_len, msg);
+	dma_prepare_tx(&dma_tx_option, msg_len, msg);
 
 	/* Kick off the DMA to send the data */
 	dma_go(txdma);
@@ -206,23 +201,22 @@ static void reply(struct spi_ctlr *spi, struct dma_channel *txdma,
  * Set up our RX DMA and disable our TX DMA. Set up the data output so that
  * we will send preamble bytes.
  */
-static void setup_for_transaction(struct spi_ctlr *spi)
+static void setup_for_transaction(void)
 {
-	int dmac;
+	stm32_spi_regs_t *spi = STM32_SPI1_REGS;
+	int dmac __attribute__((unused));
 
 	/* We are no longer actively processing a transaction */
 	active = 0;
 
 	/* write 0xfd which will be our default output value */
-	REG16(&spi->data) = 0xfd;
-	dma_disable(DMA_CHANNEL_FOR_SPI_TX(spi));
+	spi->data = 0xfd;
+	dma_disable(DMAC_SPI1_TX);
 	*in_msg = 0xff;
 
 	/* read a byte in case there is one, and the rx dma gets it */
-	dmac = REG16(&spi->data);
-	dmac = DMA_CHANNEL_FOR_SPI_RX(spi);
-	dma_start_rx(dma_rx_option[stm32_spi_port(spi)],
-		     sizeof(in_msg), in_msg);
+	dmac = spi->data;
+	dma_start_rx(&dma_rx_option, sizeof(in_msg), in_msg);
 }
 
 /**
@@ -234,7 +228,6 @@ static void setup_for_transaction(struct spi_ctlr *spi)
  */
 static void spi_send_response(struct host_cmd_handler_args *args)
 {
-	struct spi_ctlr *spi;
 	enum ec_status result = args->result;
 	struct dma_channel *txdma;
 
@@ -242,7 +235,6 @@ static void spi_send_response(struct host_cmd_handler_args *args)
 	if (!active)
 		return;
 
-	spi = (struct spi_ctlr *)stm32_spi_addr(SPI_PORT_HOST);
 	if (args->response_size > EC_HOST_PARAM_SIZE)
 		result = EC_RES_INVALID_RESPONSE;
 
@@ -251,8 +243,8 @@ static void spi_send_response(struct host_cmd_handler_args *args)
 		ASSERT(args->response == out_msg + SPI_MSG_HEADER_LEN);
 
 	/* Transmit the reply */
-	txdma = dma_get_channel(DMA_CHANNEL_FOR_SPI_TX(spi));
-	reply(spi, txdma, result, args->response, args->response_size);
+	txdma = dma_get_channel(DMAC_SPI1_TX);
+	reply(txdma, result, args->response, args->response_size);
 }
 
 /**
@@ -266,28 +258,26 @@ static void spi_send_response(struct host_cmd_handler_args *args)
 void spi_event(enum gpio_signal signal)
 {
 	struct dma_channel *rxdma;
-	struct spi_ctlr *spi;
 	uint16_t *nss_reg;
 	uint32_t nss_mask;
 
-	spi = (struct spi_ctlr *)stm32_spi_addr(SPI_PORT_HOST);
-
 	/*
 	 * If NSS is rising, we have finished the transaction, so prepare
 	 * for the next.
 	 */
 	nss_reg = gpio_get_level_reg(GPIO_SPI1_NSS, &nss_mask);
 	if (REG16(nss_reg) & nss_mask) {
-		setup_for_transaction(spi);
+		setup_for_transaction();
 		return;
 	}
 
+	/* Otherwise, NSS is low and we're now inside a transaction */
 	active = 1;
-	rxdma = dma_get_channel(DMA_CHANNEL_FOR_SPI_RX(spi));
+	rxdma = dma_get_channel(DMAC_SPI1_RX);
 
 	/* Wait for version, command, length bytes */
 	if (wait_for_bytes(rxdma, 3, nss_reg, nss_mask)) {
-		setup_for_transaction(spi);
+		setup_for_transaction();
 		return;
 	}
 
@@ -306,7 +296,7 @@ void spi_event(enum gpio_signal signal)
 
 	/* Wait for parameters */
 	if (wait_for_bytes(rxdma, 3 + args.params_size, nss_reg, nss_mask)) {
-		setup_for_transaction(spi);
+		setup_for_transaction();
 		return;
 	}
 
@@ -322,33 +312,21 @@ void spi_event(enum gpio_signal signal)
 	host_command_received(&args);
 }
 
-static int spi_init(void)
+static void spi_init(void)
 {
-	struct spi_ctlr *spi;
+	stm32_spi_regs_t *spi = STM32_SPI1_REGS;
 
-	/* Enable clocks to SPI module */
+	/* Enable clocks to SPI1 module */
 	STM32_RCC_APB2ENR |= 1 << 12;
 
-	/**
-	 * SPI1
-	 * PA7: SPI1_MOSI
-	 * PA6: SPI1_MISO
-	 * PA5: SPI1_SCK
-	 * PA4: SPI1_NSS
-	 *
-	 * 8-bit data, master mode, full-duplex, clock is fpclk / 2
-	 */
-	spi = (struct spi_ctlr *)stm32_spi_addr(SPI_PORT_HOST);
-
 	/* Enable rx DMA and get ready to receive our first transaction */
-	REG16(&spi->ctrl2) = CR2_RXDMAEN | CR2_TXDMAEN;
+	spi->ctrl2 = CR2_RXDMAEN | CR2_TXDMAEN;
 
-	/* enable the SPI peripheral */
-	REG16(&spi->ctrl1) |= CR1_SPE;
+	/* Enable the SPI peripheral */
+	spi->ctrl1 |= CR1_SPE;
 
-	setup_for_transaction(spi);
+	setup_for_transaction();
 
 	gpio_enable_interrupt(GPIO_SPI1_NSS);
-	return EC_SUCCESS;
 }
 DECLARE_HOOK(HOOK_INIT, spi_init, HOOK_PRIO_DEFAULT);