| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<VI2PR04MB11147D35A22A7B29384468B21E8D8A@VI2PR04MB11147.eurprd04.prod.outlook.com>
Date: Tue, 2 Dec 2025 07:00:58 +0000
From: Carlos Song <carlos.song@....com>
To: Frank Li <frank.li@....com>
CC: "broonie@...nel.org" <broonie@...nel.org>, "hawnguo@...nel.org"
<hawnguo@...nel.org>, "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>, "festevam@...il.com"
<festevam@...il.com>, "linux-spi@...r.kernel.org"
<linux-spi@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 6/6] spi: imx: enable DMA mode for target operation
> -----Original Message-----
> From: Frank Li <frank.li@....com>
> Sent: Wednesday, November 26, 2025 12:06 AM
> To: Carlos Song <carlos.song@....com>
> Cc: broonie@...nel.org; hawnguo@...nel.org; s.hauer@...gutronix.de;
> kernel@...gutronix.de; festevam@...il.com; linux-spi@...r.kernel.org;
> imx@...ts.linux.dev; linux-arm-kernel@...ts.infradead.org;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH 6/6] spi: imx: enable DMA mode for target operation
>
> On Tue, Nov 25, 2025 at 06:06:18PM +0800, Carlos Song wrote:
> > Enable DMA mode for SPI IMX in target mode.
> > Disable the word delay feature for target mode, because target mode
> > should always keep high performance to make sure it can follow the
> > master. Target mode continues to operate in dynamic burst mode.
>
> If two paragraph, need extra empty line. If one parapraph, move to previous
> line.
>
> >
> > Signed-off-by: Carlos Song <carlos.song@....com>
> > ---
> > drivers/spi/spi-imx.c | 78
> > +++++++++++++++++++++++++++++++------------
> > 1 file changed, 56 insertions(+), 22 deletions(-)
> >
> ...
> >
> > @@ -1756,23 +1753,51 @@ static int spi_imx_dma_submit(struct
> > spi_imx_data *spi_imx,
> >
> > transfer_timeout = spi_imx_calculate_timeout(spi_imx,
> > transfer->len);
> >
> > - /* Wait SDMA to finish the data transfer.*/
> > - time_left = wait_for_completion_timeout(&spi_imx->dma_tx_completion,
> > - transfer_timeout);
> > - if (!time_left) {
> > - dev_err(spi_imx->dev, "I/O Error in DMA TX\n");
> > - dmaengine_terminate_all(controller->dma_tx);
> > - dmaengine_terminate_all(controller->dma_rx);
> > - return -ETIMEDOUT;
> > - }
> > + if (!spi_imx->target_mode) {
> > + /* Wait SDMA to finish the data transfer.*/
> > + time_left =
> wait_for_completion_timeout(&spi_imx->dma_tx_completion,
> > + transfer_timeout);
> > + if (!time_left) {
> > + dev_err(spi_imx->dev, "I/O Error in DMA TX\n");
> > + dmaengine_terminate_all(controller->dma_tx);
> > + dmaengine_terminate_all(controller->dma_rx);
> > + return -ETIMEDOUT;
> > + }
> >
> > - time_left = wait_for_completion_timeout(&spi_imx->dma_rx_completion,
> > - transfer_timeout);
> > - if (!time_left) {
> > - dev_err(&controller->dev, "I/O Error in DMA RX\n");
> > - spi_imx->devtype_data->reset(spi_imx);
> > - dmaengine_terminate_all(controller->dma_rx);
> > - return -ETIMEDOUT;
> > + time_left =
> wait_for_completion_timeout(&spi_imx->dma_rx_completion,
> > + transfer_timeout);
> > + if (!time_left) {
> > + dev_err(&controller->dev, "I/O Error in DMA RX\n");
> > + spi_imx->devtype_data->reset(spi_imx);
> > + dmaengine_terminate_all(controller->dma_rx);
> > + return -ETIMEDOUT;
> > + }
> > + } else {
> > + spi_imx->target_aborted = false;
> > +
> > + if (wait_for_completion_interruptible(&spi_imx->dma_tx_completion)
> ||
> > + spi_imx->target_aborted) {
>
Hi, Frank
Sorry missing these questions:
> Suppose somewhere set target_aborted to true. I think here should use
> READ_ONCE() to make sure read from memory.
>
Will do this in V2.
> Not sure why here use wait_for_completion_interruptible() but at master mode
> use wait_for_completion_timeout().
>
Because target should support ctrl+c to stop this transfer(need interruptible) and target will keep waiting until the master clocks coming to shift out/in data(don't need timeout).
So use wait_for_completion_interruptible() for target mode
> > + dev_dbg(spi_imx->dev, "I/O Error in DMA TX interrupted\n");
> > + dmaengine_terminate_all(controller->dma_tx);
> > + dmaengine_terminate_all(controller->dma_rx);
> > + return -EINTR;
> > + }
> > +
> > + if (wait_for_completion_interruptible(&spi_imx->dma_rx_completion)
> ||
> > + spi_imx->target_aborted) {
> > + dev_dbg(spi_imx->dev, "I/O Error in DMA RX interrupted\n");
> > + dmaengine_terminate_all(controller->dma_rx);
> > + return -EINTR;
> > + }
> > +
> > + /*
> > + * ECSPI has a HW issue when works in Target mode, after 64 words
> > + * writtern to TXFIFO, even TXFIFO becomes empty, ECSPI_TXDATA
> keeps
> > + * shift out the last word data, so we have to disable ECSPI when in
> > + * target mode after the transfer completes.
> > + */
> > + if (spi_imx->devtype_data->disable)
> > + spi_imx->devtype_data->disable(spi_imx);
> > }
> >
> > return 0;
> > @@ -1895,10 +1920,16 @@ static int spi_imx_dma_package_transfer(struct
> > spi_imx_data *spi_imx, static int spi_imx_dma_transfer(struct spi_imx_data
> *spi_imx,
> > struct spi_transfer *transfer)
> > {
> > - bool word_delay = transfer->word_delay.value != 0;
> > + bool word_delay = transfer->word_delay.value != 0 &&
> > +!spi_imx->target_mode;
> > int ret;
> > int i;
> >
> > + if (transfer->len > MX53_MAX_TRANSFER_BYTES &&
> spi_imx->target_mode)
> > +{
>
> why only target have len limiation?
>
> Frank
This is a errata:
Due to Freescale errata ERR003775 "eCSPI: Burst completion by Chip Select (SS) signal in Slave mode is not functional" burst size must be set exactly to the size of the transfer.
This limit SPI transaction with maximum 2^12 bits.
So I add a limit for this. I will comment at V2.
Carlos
> > + dev_err(spi_imx->dev, "Transaction too big, max size is %d bytes\n",
> > + MX53_MAX_TRANSFER_BYTES);
> > + return -EMSGSIZE;
> > + }
> > +
> > ret = spi_imx_dma_data_prepare(spi_imx, transfer, word_delay);
> > if (ret < 0) {
> > transfer->error |= SPI_TRANS_FAIL_NO_START; @@ -2104,7 +2135,7
> @@
> > static int spi_imx_transfer_one(struct spi_controller *controller,
> > while (spi_imx->devtype_data->rx_available(spi_imx))
> > readl(spi_imx->base + MXC_CSPIRXDATA);
> >
> > - if (spi_imx->target_mode)
> > + if (spi_imx->target_mode && !spi_imx->usedma)
> > return spi_imx_pio_transfer_target(spi, transfer);
> >
> > /*
> > @@ -2116,7 +2147,10 @@ static int spi_imx_transfer_one(struct
> spi_controller *controller,
> > ret = spi_imx_dma_transfer(spi_imx, transfer);
> > if (transfer->error & SPI_TRANS_FAIL_NO_START) {
> > spi_imx->usedma = false;
> > - return spi_imx_pio_transfer(spi, transfer);
> > + if (spi_imx->target_mode)
> > + return spi_imx_pio_transfer_target(spi, transfer);
> > + else
> > + return spi_imx_pio_transfer(spi, transfer);
> > }
> > return ret;
> > }
> > --
> > 2.34.1
> >
Powered by blists - more mailing lists