Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Monday 29 October 2012 08:47 PM, Stephen Warren wrote: On 10/26/2012 12:49 PM, Laxman Dewangan wrote: Why not just always set SLINK_FIFO_ERROR; does it have to be set in the write only if the status was previously asserted? If that is true, how do you avoid a race condition where the bit gets set in SLINK_STATUS after you read it but before you write to clear it? Status gets updated together. There is no steps of updating status. Sorry, I don't understand this answer. The status should be updated once by HW and so there is no race condition. HW behavior is that if the tx or Rx error occurs, it updates the status, generates interrupt and still continue transfer and later on, it generates the ready. In first isr, we read status, error status found and so in isr thread, we reset controller to stop the transfer. So in good state, only ready bit will be set and hence writing 1 to clear it. In error state, clearing error first in ISR and in isr thread resetting the controller to stop the controller engine. Is there a way to support the reset of controller. We will need this functionality. Why do we need to reset the controller at all; can't we simply program all the (few) configuration registers? Are there HW bugs that hang the controller and require a reset or something? HW generates error, then interrupt and still continue transfer and later point of time it generates the transfer done. We want to stop the transfer once error get detected. For this we need to reset controller. I did disabling rx and tx but still controller shows as busy. -- The Windows 8 Center - In partnership with Sourceforge Your idea - your app - 30 days. Get started! http://windows8center.sourceforge.net/ what-html-developers-need-to-know-about-coding-windows-8-metro-style-apps/ ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/29/2012 10:18 AM, Laxman Dewangan wrote: On Monday 29 October 2012 08:47 PM, Stephen Warren wrote: On 10/26/2012 12:49 PM, Laxman Dewangan wrote: Why not just always set SLINK_FIFO_ERROR; does it have to be set in the write only if the status was previously asserted? If that is true, how do you avoid a race condition where the bit gets set in SLINK_STATUS after you read it but before you write to clear it? Status gets updated together. There is no steps of updating status. Sorry, I don't understand this answer. The status should be updated once by HW and so there is no race condition. HW behavior is that if the tx or Rx error occurs, it updates the status, generates interrupt and still continue transfer and later on, it generates the ready. In first isr, we read status, error status found and so in isr thread, we reset controller to stop the transfer. So in good state, only ready bit will be set and hence writing 1 to clear it. In error state, clearing error first in ISR and in isr thread resetting the controller to stop the controller engine. OK, I see why there's no race. It still seems simply to me if tegra_slink_clear_status() just always writes all the status bits, but I suppose it isn't a correctness issue. Is there a way to support the reset of controller. We will need this functionality. Why do we need to reset the controller at all; can't we simply program all the (few) configuration registers? Are there HW bugs that hang the controller and require a reset or something? HW generates error, then interrupt and still continue transfer and later point of time it generates the transfer done. We want to stop the transfer once error get detected. For this we need to reset controller. I did disabling rx and tx but still controller shows as busy. Oh dear. Well, I guess it'll have to be OK then; we'll just have to find a way of decoupling this API from the mach-tegra directory later:-( -- The Windows 8 Center - In partnership with Sourceforge Your idea - your app - 30 days. Get started! http://windows8center.sourceforge.net/ what-html-developers-need-to-know-about-coding-windows-8-metro-style-apps/ ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
Thanks Stephen for review.
I have taken care of almost all feedback. Some of having my below comments.
On Tuesday 23 October 2012 01:32 AM, Stephen Warren wrote:
On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
+static inline void tegra_slink_writel(struct tegra_slink_data *tspi,
+ unsigned long val, unsigned long reg)
+{
+ writel(val, tspi-base + reg);
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != SLINK_TX_FIFO)
+ readl(tspi-base + SLINK_MAS_DATA);
Is that really necessary on every single write, or only for certain
specific operations? This seems rather heavy-weight.
We do write register very less (as we shadow the register locally), I
do not see any perf degradation here.
+ val_write = SLINK_RDY;
+ val_write |= (val SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
Status gets updated together. There is no steps of updating status.
+ if (bits_per_word == 8 || bits_per_word == 16) {
+ tspi-is_packed = 1;
+ tspi-words_per_32bit = 32/bits_per_word;
Spaces required around all operators. Does this pass checkpatch? No:
total: 1405 errors, 11 warnings, 1418 lines checked
I saw the issue by my checkpatch is not caching the issue. Let me
recheck my command.
+ bits_per_word = t-bits_per_word ? t-bits_per_word :
+ tspi-cur_spi-bits_per_word;
That logic is repeated a few times. Shouldn't there be a macro to avoid
cut/paste. Perhaps even in the SPI core rather than this driver.
I will post the change for moving the code to core later. I will keep
this for now.
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
I think it is possible by checking some flags, I will explore and will
post the fix later.
+static int tegra_slink_unprepare_transfer(struct spi_master *master)
+{
+ struct tegra_slink_data *tspi = spi_master_get_devdata(master);
+
+ pm_runtime_put_sync(tspi-dev);
+ return 0;
+}
Does this driver actually implement any runtime PM ops? I certainly
don't see any in struct dev_pm_ops tegra_slink_dev_pm_ops. Related, why
do tegra_slink_clk_{en,dis}able exist; shouldn't those functions be the
implementation of the runtime PM ops? Related, why are
tegra_slink_clk_{en,dis}able called all over the place, rather than
relying on runtime PM API calls, or tegra_slink_{,un}prepare_transfer
having been called?
OK, I move the clock control in runtime callbacks.
So that all implies that we wait for the very first interrupt from the
SPI peripheral for a transfer, and then wait for the DMA controller to
complete all DMA (which would probably entail actually waiting for DMA
to drain the RX FIFO in the RX case). Does it make sense to simply
return from the ISR if not all conditions that we will wait on have
occurred, and so avoid blocking this ISR thread? I suppose since this
thread gets blocked rather than spinning, it's not a big deal though.
In this case, we can get rid of isr thread and move the wait/status
check to caller thread.
I need to do some more stress on this and if it is fine then will post
the patch later for this, not as part of this patch.
+ spin_lock_irqsave(tspi-lock, flags);
+ if (err) {
+ dev_err(tspi-dev,
+ DmaXfer: ERROR bit set 0x%x\n, tspi-status_reg);
+ dev_err(tspi-dev,
+ DmaXfer 0x%08x:0x%08x:0x%08x\n, tspi-command_reg,
+ tspi-command2_reg, tspi-dma_control_reg);
+ tegra_periph_reset_assert(tspi-clk);
+ udelay(2);
+ tegra_periph_reset_deassert(tspi-clk);
Do we /have/ to reset the SPI block; can't we just disable it in the
control register, clear all status, and re-program it from scratch?
If at all possible, I would like to avoid introducing any new use of
tegra_periph_reset_{,de}assert, since that API has no standard subsystem
equivalent (or if it does, isn't hooked into the standard subsystem
yet), and hence means this driver relies on a header file currently in
arch/arm/mach-tegra/include/mach, and we need to move or delete all such
headers in order to support single zImage.
Is there a way to support the reset of controller. We will need this
functionality.
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
+static inline void tegra_slink_writel(struct tegra_slink_data *tspi,
+ unsigned long val, unsigned long reg)
+{
+ writel(val, tspi-base + reg);
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != SLINK_TX_FIFO)
+ readl(tspi-base + SLINK_MAS_DATA);
Is that really necessary on every single write, or only for certain
specific operations? This seems rather heavy-weight.
+static void tegra_slink_clear_status(struct tegra_slink_data *tspi)
+{
+ unsigned long val;
+ unsigned long val_write = 0;
+
+ val = tegra_slink_readl(tspi, SLINK_STATUS);
+
+ /* Write 1 to clear status register */
+ val_write = SLINK_RDY;
+ val_write |= (val SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
+ tegra_slink_writel(tspi, val_write, SLINK_STATUS);
+}
+static unsigned tegra_slink_calculate_curr_xfer_param(
+ if (bits_per_word == 8 || bits_per_word == 16) {
+ tspi-is_packed = 1;
+ tspi-words_per_32bit = 32/bits_per_word;
Spaces required around all operators. Does this pass checkpatch? No:
total: 1405 errors, 11 warnings, 1418 lines checked
+static unsigned tegra_slink_fill_tx_fifo_from_client_txbuf(
+ if (tspi-is_packed) {
+ nbytes = tspi-curr_dma_words * tspi-bytes_per_word;
+ max_n_32bit = (min(nbytes, tx_empty_count*4) - 1)/4 + 1;
+ for (count = 0; count max_n_32bit; ++count) {
Very minor almost irrelevant nit: count++ would be much more typical here.
+ x = 0;
+ for (i = 0; (i 4) nbytes; i++, nbytes--)
+ x |= (*tx_buf++) (i*8);
+ tegra_slink_writel(tspi, x, SLINK_TX_FIFO);
+ }
+ written_words = min(max_n_32bit * tspi-words_per_32bit,
+ tspi-curr_dma_words);
The calculations here seem a little over-complex. Why not calculate the
FIFO space in words above, and just set written words based on that.
Something like:
fifo_words_left = tx_empty_count * spi-words_per_32bit;
written_words = min(fifo_words_left, tspi-curr_dma_words);
nbytes = written_words * spi-bytes_per_word;
max_n_32bit = (nbytes + 3) / 4;
Most likely a similar comment applies to all the other similar functions
for filling FIFOs and buffers.
In fact, I suspect you can completely get rid of the if (is_packed)
statement here by appropriately parameterising the code using variables
in *spi...
+static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf(
+ bits_per_word = t-bits_per_word ? t-bits_per_word :
+ tspi-cur_spi-bits_per_word;
That logic is repeated a few times. Shouldn't there be a macro to avoid
cut/paste. Perhaps even in the SPI core rather than this driver.
+ rx_mask = (1 bits_per_word) - 1;
+ for (count = 0; count rx_full_count; ++count) {
+ x = tegra_slink_readl(tspi, SLINK_RX_FIFO);
+ x = rx_mask;
I don't think you need that mask; the loop below only extracts bytes
that actually exist in the FIFO right?
+ for (i = 0; (i tspi-bytes_per_word); ++i)
+ *rx_buf++ = (x (i*8)) 0xFF;
+static void tegra_slink_copy_client_txbuf_to_spi_txbuf(
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
+static int tegra_slink_start_dma_based_transfer(
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
+ /* Make sure that Rx and Tx fifo are empty */
+ status = tegra_slink_readl(tspi, SLINK_STATUS);
+ if ((status SLINK_FIFO_EMPTY) != SLINK_FIFO_EMPTY)
+ dev_err(tspi-dev,
+ Rx/Tx fifo are not empty status 0x%08lx\n, status);
Shouldn't this return an error, or drain the FIFO, or something?
+static int tegra_slink_init_dma_param(struct tegra_slink_data *tspi,
+ if (dma_to_memory) {
+ dma_sconfig.src_addr = tspi-phys + SLINK_RX_FIFO;
+ dma_sconfig.dst_addr = tspi-phys + SLINK_RX_FIFO;
+ } else {
+ dma_sconfig.src_addr = tspi-phys + SLINK_TX_FIFO;
+ dma_sconfig.dst_addr = tspi-phys + SLINK_TX_FIFO;
+ }
[PATCH] spi: tegra: add spi driver for SLINK controller
Tegra20/Tegra30 supports the spi interface through its SLINK controller. Add spi driver for SLINK controller. Signed-off-by: Laxman Dewangan ldewan...@nvidia.com --- drivers/spi/Kconfig |6 + drivers/spi/Makefile|2 +- drivers/spi/spi-tegra20-slink.c | 1356 +++ include/linux/spi/spi-tegra.h | 40 ++ 4 files changed, 1403 insertions(+), 1 deletions(-) create mode 100644 drivers/spi/spi-tegra20-slink.c create mode 100644 include/linux/spi/spi-tegra.h diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 1acae35..25290d9 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -385,6 +385,12 @@ config SPI_MXS help SPI driver for Freescale MXS devices. +config SPI_TEGRA20_SLINK + tristate Nvidia Tegra20/Tegra30 SLINK Controller + depends on ARCH_TEGRA TEGRA20_APB_DMA + help + SPI driver for Nvidia Tegra20/Tegra30 SLINK Controller interface. + config SPI_TI_SSP tristate TI Sequencer Serial Port - SPI Support depends on MFD_TI_SSP diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index c48df47..f87c0f1 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -60,10 +60,10 @@ obj-$(CONFIG_SPI_SH_MSIOF) += spi-sh-msiof.o obj-$(CONFIG_SPI_SH_SCI) += spi-sh-sci.o obj-$(CONFIG_SPI_SIRF) += spi-sirf.o obj-$(CONFIG_SPI_STMP3XXX) += spi-stmp.o +obj-$(CONFIG_SPI_TEGRA20_SLINK)+= spi-tegra20-slink.o obj-$(CONFIG_SPI_TI_SSP) += spi-ti-ssp.o obj-$(CONFIG_SPI_TLE62X0) += spi-tle62x0.o obj-$(CONFIG_SPI_TOPCLIFF_PCH) += spi-topcliff-pch.o obj-$(CONFIG_SPI_TXX9) += spi-txx9.o obj-$(CONFIG_SPI_XCOMM)+= spi-xcomm.o obj-$(CONFIG_SPI_XILINX) += spi-xilinx.o - diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c new file mode 100644 index 000..c8705d9 --- /dev/null +++ b/drivers/spi/spi-tegra20-slink.c @@ -0,0 +1,1356 @@ +/* + * SPI driver for Nvidia's Tegra20/Tegra30 SLINK Controller. + * + * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. + */ + +#include linux/clk.h +#include linux/completion.h +#include linux/delay.h +#include linux/dmaengine.h +#include linux/dma-mapping.h +#include linux/dmapool.h +#include linux/err.h +#include linux/init.h +#include linux/interrupt.h +#include linux/io.h +#include linux/kernel.h +#include linux/kthread.h +#include linux/module.h +#include linux/platform_device.h +#include linux/pm_runtime.h +#include linux/of.h +#include linux/of_device.h +#include linux/spi/spi.h +#include linux/spi/spi-tegra.h +#include mach/clk.h + +#define SLINK_COMMAND 0x000 +#define SLINK_BIT_LENGTH(x)(((x) 0x1f) 0) +#define SLINK_WORD_SIZE(x) (((x) 0x1f) 5) +#define SLINK_BOTH_EN (1 10) +#define SLINK_CS_SW(1 11) +#define SLINK_CS_VALUE (1 12) +#define SLINK_CS_POLARITY (1 13) +#define SLINK_IDLE_SDA_DRIVE_LOW (0 16) +#define SLINK_IDLE_SDA_DRIVE_HIGH (1 16) +#define SLINK_IDLE_SDA_PULL_LOW(2 16) +#define SLINK_IDLE_SDA_PULL_HIGH (3 16) +#define SLINK_IDLE_SDA_MASK(3 16) +#define SLINK_CS_POLARITY1 (1 20) +#define SLINK_CK_SDA (1 21) +#define SLINK_CS_POLARITY2 (1 22) +#define SLINK_CS_POLARITY3 (1 23) +#define SLINK_IDLE_SCLK_DRIVE_LOW (0 24) +#define SLINK_IDLE_SCLK_DRIVE_HIGH (1 24) +#define SLINK_IDLE_SCLK_PULL_LOW (2 24) +#define SLINK_IDLE_SCLK_PULL_HIGH (3 24) +#define SLINK_IDLE_SCLK_MASK (3 24) +#define SLINK_M_S (1 28) +#define SLINK_WAIT (1 29) +#define SLINK_GO (1 30) +#define SLINK_ENB (1 31) + +#define SLINK_MODES(SLINK_IDLE_SCLK_MASK | SLINK_CK_SDA) + +#define SLINK_COMMAND2 0x004 +#define SLINK_LSBFE(1 0) +#define SLINK_SSOE (1 1) +#define SLINK_SPIE (1 4) +#define SLINK_BIDIROE (1 6) +#define SLINK_MODFEN (1 7) +#define SLINK_INT_SIZE(x) (((x) 0x1f) 8) +#define
