Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Serge Semin <fancer.lancer@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Linus Walleij <linus.walleij@stericsson.com>,
	Vinod Koul <vkoul@kernel.org>, Feng Tang <feng.tang@intel.com>,
	Grant Likely <grant.likely@secretlab.ca>,
	Alan Cox <alan@linux.intel.com>,
	Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>,
	Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Paul Burton <paulburton@kernel.org>,
	Ralf Baechle <ralf@linux-mips.org>, Arnd Bergmann <arnd@arndb.de>,
	Rob Herring <robh+dt@kernel.org>,
	linux-mips@vger.kernel.org, devicetree@vger.kernel.org,
	Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jarkko Nikula <jarkko.nikula@linux.intel.com>,
	"wuxu.wu" <wuxu.wu@huawei.com>, Clement Leger <cleger@kalray.eu>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
Date: Sat, 23 May 2020 11:34:10 +0300
Message-ID: <20200523083410.3qarkfkwsmodvxwk@mobilestation> (raw)
In-Reply-To: <20200522152241.GK5801@sirena.org.uk>

On Fri, May 22, 2020 at 04:22:41PM +0100, Mark Brown wrote:
> On Fri, May 22, 2020 at 05:45:42PM +0300, Serge Semin wrote:
> > On Fri, May 22, 2020 at 05:36:39PM +0300, Andy Shevchenko wrote:
> 
> > > My point is: let's warn and see if anybody comes with a bug report. We will
> > > solve an issue when it appears.
> 
> > In my environment the stack trace happened (strictly speaking it has been a
> > BUG() invoked due to the sleep_range() called within the tasklet) when SPI bus
> > had been enabled to work with !8MHz! clock. It's quite normal bus speed.
> > So we'll get the bug report pretty soon.)
> 
> Right, that definitely needs to be fixed then - 8MHz is indeed a totally
> normal clock rate for SPI so people will hit it.  I guess if there's a
> noticable performance hit to defer to thread then we could implement
> both and look at how long the delay is going to be to decide which to
> use, that's annoyingly complicated though so if the overhead is small
> enough we could just not bother.

As I suggested before we can implement a solution without performance drop.
Just wait for the DMA completion locally in the dw_spi_dma_transfer() method and
return 0 instead of 1 from the transfer_one() callback. In that function we'll
wait while DMA finishes its business, after that we can check the Tx/Rx FIFO
emptiness and wait for the data to be completely transferred with delays or
sleeps or whatever.

There are several drawback of the solution:
1) We need to alter the dw_spi_transfer_one() method in a way one would return
0 instead of 1 (for DMA) so the generic spi_transfer_one_message() method would
be aware that the transfer has been finished and it doesn't need to wait by
calling the spi_transfer_wait() method.
2) Locally in the dw_spi_dma_transfer() I have to implement a method similar
to the spi_transfer_wait(). It won't be that similar though. We can predict a
completion timeout better in here due to using a more exact SPI bus frequency.
Anyway in the rest of aspects the functions will be nearly the same. 
3) Not using spi_transfer_wait() means we also have to locally add the SPI
timeout statistics incremental.

So to speak the local wait method will be like this:

+static int dw_spi_dma_wait(struct dw_spi *dws, struct spi_transfer *xfer)
+{
+ 	struct spi_statistics *statm = &dws->master->statistics;
+	struct spi_statistics *stats = &dws->master->cur_msg->spi->statistics;
+	unsigned long ms = 1;
+
+	ms = xfer->len * MSEC_PER_SEC * BITS_PER_BYTE;
+	ms /= xfer->effective_speed_hz;
+	ms += ms + 200;
+
+	ms = wait_for_completion_timeout(&dws->xfer_completion,
+					msecs_to_jiffies(ms));
+
+	if (ms == 0) {
+		SPI_STATISTICS_INCREMENT_FIELD(statm, timedout);
+		SPI_STATISTICS_INCREMENT_FIELD(stats, timedout);
+		dev_err(&dws->master->cur_msg->spi->dev,
+			"SPI transfer timed out\n");
+			return -ETIMEDOUT;
+	}
+}

NOTE Currently the DW APB SSI driver doesn't set xfer->effective_speed_hz, though as
far as I can see that field exists there to be initialized by the SPI controller
driver, right? If so, strange it isn't done in any SPI drivers...

Then we can use that method to wait for the DMA transfers completion:

+static int dw_spi_dma_transfer(struct dw_spi *dws, struct spi_transfer *xfer)
+{
+	...
+	/* DMA channels/buffers preparation and the transfers execution */
+	...
+
+	ret = dw_spi_dma_wait(dws, xfer);
+	if (ret)
+		return ret;
+
+	ret = dw_spi_dma_wait_tx_done(dws);
+	if (ret)
+		return ret;
+
+	ret = dw_spi_dma_wait_rx_done(dws);
+	if (ret)
+		return ret;
+
+	return 0;
+}

What do think about this?

If you don't mind I'll send this fixup separately from the patchset we discuss
here, since it's going to be a series of patches. What would be better for you:
implement it based on the current DW APB SSI driver, or on top of this
patchset "spi: dw: Add generic DW DMA controller support" (it's being under
review in this email thread) ? Anyway, if the fixup is getting to be that
complicated, will it have to be backported to another stable kernels?

-Sergey

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22  0:07 [PATCH v4 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
2020-05-22  0:07 ` [PATCH v4 02/16] spi: dw: Enable interrupts in accordance with DMA xfer mode Serge Semin
2020-05-22  0:07 ` [PATCH v4 04/16] spi: dw: Discard unused void priv pointer Serge Semin
2020-05-22  0:07 ` [PATCH v4 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length Serge Semin
2020-05-22 11:06   ` Andy Shevchenko
2020-05-22  0:07 ` [PATCH v4 07/16] spi: dw: Use DMA max burst to set the request thresholds Serge Semin
2020-05-22  0:08 ` [PATCH v4 11/16] spi: dw: Remove DW DMA code dependency from DW_DMAC_PCI Serge Semin
2020-05-22  0:08 ` [PATCH v4 12/16] spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core Serge Semin
2020-05-22 11:07   ` Andy Shevchenko
2020-05-22 11:08     ` Andy Shevchenko
2020-05-22  0:08 ` [PATCH v4 14/16] spi: dw: Add DMA support to the DW SPI MMIO driver Serge Semin
2020-05-22  0:08 ` [PATCH v4 16/16] dt-bindings: spi: Convert DW SPI binding to DT schema Serge Semin
2020-05-28 23:28   ` Rob Herring
     [not found] ` <20200522000806.7381-2-Sergey.Semin@baikalelectronics.ru>
2020-05-22 11:13   ` [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA Andy Shevchenko
     [not found]     ` <20200522115235.rt3ay7lveimrgooa@mobilestation>
2020-05-22 12:10       ` Mark Brown
2020-05-22 12:12       ` Andy Shevchenko
2020-05-22 12:18         ` Mark Brown
2020-05-22 12:34           ` Andy Shevchenko
     [not found]             ` <20200522124406.co7gmteojfsooerc@mobilestation>
     [not found]               ` <20200522131013.GH5801@sirena.org.uk>
     [not found]                 ` <20200522132742.taf2ixfjpyd5u3dt@mobilestation>
     [not found]                   ` <20200522140025.bmd6bhpjjk5msvsm@mobilestation>
2020-05-22 14:36                     ` Andy Shevchenko
     [not found]                       ` <20200522144542.brhibh453wid2d6v@mobilestation>
2020-05-22 15:22                         ` Mark Brown
2020-05-23  8:34                           ` Serge Semin [this message]
2020-05-25 11:41                             ` Mark Brown
2020-05-25 21:36                               ` Serge Semin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200523083410.3qarkfkwsmodvxwk@mobilestation \
    --to=fancer.lancer@gmail.com \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Georgy.Vlasov@baikalelectronics.ru \
    --cc=Ramil.Zaripov@baikalelectronics.ru \
    --cc=Sergey.Semin@baikalelectronics.ru \
    --cc=alan@linux.intel.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=arnd@arndb.de \
    --cc=broonie@kernel.org \
    --cc=cleger@kalray.eu \
    --cc=devicetree@vger.kernel.org \
    --cc=feng.tang@intel.com \
    --cc=grant.likely@secretlab.ca \
    --cc=jarkko.nikula@linux.intel.com \
    --cc=linus.walleij@linaro.org \
    --cc=linus.walleij@stericsson.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=paulburton@kernel.org \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=tsbogend@alpha.franken.de \
    --cc=vkoul@kernel.org \
    --cc=wan.ahmad.zainie.wan.mohamad@intel.com \
    --cc=wuxu.wu@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git