spi: keep HW SPI module enabled longer

When the HW SPI module is disabled (i.e. SPE bit is cleared), then the
stm stops actively driving the SPI CLK signal and lets it float. This
can cause spurious communication issues or guaranteed issues if there is a
pullup on the CLK signal.

Ensure that the CLK signal is being driven (low) for the duration of a
USB SPI transaction at minimum.

Driving the CLK signal low for the duration of the SPI transaction also
seems to help with sporadic reliability issues on servo_micro

Also add a flag that enables the SPI module to be enabled for the
entire time the firmware wants to enable the SPI module opposed to
needing both the firmware and the USB host to enabled the SPI module.

BRANCH=servo
BUG=b:145314772,b:144846350
TEST=with scope verify that SPI CLK line is help low as soon at the
`enable_spi 1800` command is enter on C2D2 console and continues to stay
low in between all USB SPI traffic from host.

Change-Id: I9dbd6b3ebca8db6470d9ec70bae02ac8366d6c9e
Signed-off-by: Jett Rink <jettrink@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1995604
Reviewed-by: Brian Nemec <bnemec@chromium.org>
Reviewed-by: Diana Z <dzigterman@chromium.org>

3 files changed, 28 insertions, 13 deletions

diff --git a/chip/stm32/spi_master.c b/chip/stm32/spi_master.c
index 47c5be3979..4b48f8ea80 100644
--- a/chip/stm32/spi_master.c
+++ b/chip/stm32/spi_master.c
@@ -228,13 +228,16 @@ static int spi_master_initialize(int port)
 	spi->cr1 |= STM32_SPI_CR1_BIDIMODE | STM32_SPI_CR1_BIDIOE;
 #endif
 
+	/* Drive Chip Select high for all ports before turning on SPI module */
 	for (i = 0; i < spi_devices_used; i++) {
 		if (spi_devices[i].port != port)
 			continue;
-		/* Drive SS high */
 		gpio_set_level(spi_devices[i].gpio_cs, 1);
 	}
 
+	/* Enable SPI hardware module. This will actively drive the CLK pin */
+	spi->cr1 |= STM32_SPI_CR1_SPE;
+
 	/* Set flag */
 	spi_enabled[port] = 1;
 
@@ -257,7 +260,7 @@ static int spi_master_shutdown(int port)
 	dma_disable(dma_tx_option[port].channel);
 	dma_disable(dma_rx_option[port].channel);
 
-	/* Disable SPI */
+	/* Disable SPI. Let the CLK pin float. */
 	spi->cr1 &= ~STM32_SPI_CR1_SPE;
 
 	spi_clear_rx_fifo(spi);
@@ -349,6 +352,10 @@ int spi_transaction_async(const struct spi_device_t *spi_device,
 	stm32_spi_regs_t *spi = SPI_REGS[port];
 	char *buf = NULL;
 
+	/* We should not ever be called when disabled, but fail early if so. */
+	if (!spi_enabled[port])
+		return EC_ERROR_BUSY;
+
 #ifndef CONFIG_SPI_HALFDUPLEX
 	if (rxlen == SPI_READBACK_ALL) {
 		buf = rxdata;
@@ -372,7 +379,6 @@ int spi_transaction_async(const struct spi_device_t *spi_device,
 #ifdef CONFIG_SPI_HALFDUPLEX
 	spi->cr1 |= STM32_SPI_CR1_BIDIOE;
 #endif
-	spi->cr1 |= STM32_SPI_CR1_SPE;
 
 	if (full_readback)
 		return EC_SUCCESS;
@@ -383,8 +389,6 @@ int spi_transaction_async(const struct spi_device_t *spi_device,
 
 	spi_clear_tx_fifo(spi);
 
-	spi->cr1 &= ~STM32_SPI_CR1_SPE;
-
 	if (rxlen) {
 		rv = spi_dma_start(port, buf, rxdata, rxlen);
 		if (rv != EC_SUCCESS)
@@ -392,7 +396,6 @@ int spi_transaction_async(const struct spi_device_t *spi_device,
 #ifdef CONFIG_SPI_HALFDUPLEX
 		spi->cr1 &= ~STM32_SPI_CR1_BIDIOE;
 #endif
-		spi->cr1 |= STM32_SPI_CR1_SPE;
 	}
 
 err_free:
@@ -406,9 +409,7 @@ err_free:
 int spi_transaction_flush(const struct spi_device_t *spi_device)
 {
 	int rv = spi_dma_wait(spi_device->port);
-	stm32_spi_regs_t *spi = SPI_REGS[spi_device->port];
 
-	spi->cr1 &= ~STM32_SPI_CR1_SPE;
 	/* Drive SS high */
 	gpio_set_level(spi_device->gpio_cs, 1);
 
diff --git a/chip/stm32/usb_spi.c b/chip/stm32/usb_spi.c
index b560b55709..597d4c5df7 100644
--- a/chip/stm32/usb_spi.c
+++ b/chip/stm32/usb_spi.c
@@ -69,14 +69,19 @@ static int rx_valid(struct usb_spi_config const *config)
 
 void usb_spi_deferred(struct usb_spi_config const *config)
 {
+	int enabled;
+
+	if (config->flags & USB_SPI_CONFIG_FLAGS_IGNORE_HOST_SIDE_ENABLE)
+		enabled = config->state->enabled_device;
+	else
+		enabled = config->state->enabled_device &&
+			  config->state->enabled_host;
+
 	/*
 	 * If our overall enabled state has changed we call the board specific
 	 * enable or disable routines and save our new state.
 	 */
-	int enabled = (config->state->enabled_host &&
-		       config->state->enabled_device);
-
-	if (enabled ^ config->state->enabled) {
+	if (enabled != config->state->enabled) {
 		if (enabled) usb_spi_board_enable(config);
 		else         usb_spi_board_disable(config);
 
@@ -180,7 +185,8 @@ int usb_spi_interface(struct usb_spi_config const *config,
 	 * Our state has changed, call the deferred function to handle the
 	 * state change.
 	 */
-	hook_call_deferred(config->deferred, 0);
+	if (!(config->flags & USB_SPI_CONFIG_FLAGS_IGNORE_HOST_SIDE_ENABLE))
+		hook_call_deferred(config->deferred, 0);
 
 	btable_ep[0].tx_count = 0;
 	STM32_TOGGLE_EP(0, EP_TX_RX_MASK, EP_TX_RX_VALID, EP_STATUS_OUT);
diff --git a/chip/stm32/usb_spi.h b/chip/stm32/usb_spi.h
index fda770345d..a32febce03 100644
--- a/chip/stm32/usb_spi.h
+++ b/chip/stm32/usb_spi.h
@@ -126,6 +126,14 @@ struct usb_spi_config {
 };
 
 /*
+ * Use when you want the SPI subsystem to be enabled even when the USB SPI
+ * endpoint is not enabled by the host. This means that when this firmware
+ * enables SPI, then the HW SPI module is enabled (i.e. SPE bit is set) until
+ * this firmware disables the SPI module; it ignores the host's enables state.
+ */
+#define USB_SPI_CONFIG_FLAGS_IGNORE_HOST_SIDE_ENABLE BIT(0)
+
+/*
  * Convenience macro for defining a USB SPI bridge driver.
  *
  * NAME is used to construct the names of the trampoline functions and the