New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 795689 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ec: spi_transaction_async() has questionable implementation details on stm32

Reported by vpalatin@chromium.org, Dec 18 2017

Issue description

While implementation SPI transactions for the new SPI controller in STM32H7, I have noticed a couple of weird details in spi_transaction_async() in  chip/stm32/spi_master.c.
Writing them down to check them later:
1. the API being calling 'spi_transaction_async' then 'spi_transaction_flush' (effectively doing spi_dma_wait)
  it seems that we might want to call the 'spi_dma_wait' in spi_transaction_async only if we are starting a second transaction.
  so moving it inside the 'if (rxlen) {' clause ?
2. when we are releasing the shared memory, the SPI DMA transfer from/to it might still be on-going ?

Both of them even if they are real issues, should be harmless in the current code.


int spi_transaction_async(const struct spi_device_t *spi_device,
                          const uint8_t *txdata, int txlen,
                          uint8_t *rxdata, int rxlen)
{
        int rv = EC_SUCCESS;
        int port = spi_device->port;
        int full_readback = 0;

        stm32_spi_regs_t *spi = SPI_REGS[port];
        char *buf = NULL;

#ifndef CONFIG_SPI_HALFDUPLEX
        if (rxlen == SPI_READBACK_ALL) {
                buf = rxdata;
                full_readback = 1;
        } else {
                rv = shared_mem_acquire(MAX(txlen, rxlen), &buf);
                if (rv != EC_SUCCESS)
                        return rv;
        }
#endif

        /* Drive SS low */
        gpio_set_level(spi_device->gpio_cs, 0);

        /* Clear out the FIFO. */
        while (spi->sr & (STM32_SPI_SR_FRLVL | STM32_SPI_SR_RXNE))
                (void) (uint8_t) spi->dr;

#ifdef CONFIG_SPI_HALFDUPLEX
        /* Enable bidirection mode and select output direction  */
        spi->cr1 |= STM32_SPI_CR1_BIDIMODE | STM32_SPI_CR1_BIDIOE;
#endif
        rv = spi_dma_start(port, txdata, buf, txlen);
        if (rv != EC_SUCCESS)
                goto err_free;

        if (full_readback)
                return EC_SUCCESS;

        rv = spi_dma_wait(port);                  <<<<<<<<<<<<< (1)
        if (rv != EC_SUCCESS)
                goto err_free;

        if (rxlen) {
#ifdef CONFIG_SPI_HALFDUPLEX
                /* Select input direction  */
                spi->cr1 &= ~STM32_SPI_CR1_BIDIOE;
#endif
                rv = spi_dma_start(port, buf, rxdata, rxlen);
                if (rv != EC_SUCCESS)
                        goto err_free;
        }

err_free:
#ifndef CONFIG_SPI_HALFDUPLEX
        if (!full_readback)
                shared_mem_release(buf);        <<<<<<<<<<<<<< (2)
#endif
        return rv;
}

 

Sign in to add a comment