RE: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
Hi Brian, On Mon, Mar 15, 2010 at 21:20:13, Kevin Hilman wrote: Brian Niebuhr bnieb...@efjohnson.com writes: This patch is a significant overhaul of the davinci spi controller driver that corrects multiple errors: - Eliminate a race condition that exists for slow SPI devices - Fix DMA transfer length error - Fix limitations preventing multiple SPI devices on the same controller Signed-off-by: Brian Niebuhr bnieb...@efjohnson.com The verbose description of the issues addressed from PATCH 0/2 should go here is well so it makes it into the permanent git history. I can certainly do that. That being said, I think for the sake of reviewing, you're going to need to break this up into reviewable pieces, each having a verbose description of the issues being solved. There is also a mixture of fixes, enhancements, renames etc. These should be done as separate patches. I know that it's more work to break it up like this, but that's the only way to make a large change like this reviewable by others. I guess I was hoping that this could be reviewed as if it were a new driver submission. I ended up more or less rewriting all of the functional parts of the driver (txrx_bufs(), chipselect(), IRQs and DMA callbacks), so it's very difficult to show this as a series of changes. I do understand the problem from your perspective, though. My thought was that if the TI folks were willing to look the driver over and they gave their blessing, that you would look at it as if it were a replacement driver and accept or deny it on that basis. I'm OK with the approach of considering it as a brand new driver. The changelog made me think it was a bunch of fixes/enhancements and not a re-write. It should then be made more clear in the changelog that this is essentially a re-write, and why it is not done in a series of small changes. Whichever approach, this will need to worked out between you and the origial TI authors (Sandeep, Sudahkar) who will need to review/signoff this replacement. I tried testing this patch on some DA8xx platforms I have but could not do it as the patch did not apply cleanly to Kevin's tree. You might have to rebase this patch so that it applies on Kevin's tree. Also, I would still prefer if you can breakdown this patch as per the various fixes you have done because the current SPI driver is already accepted in mainline and is working, of course with some limitations. But if you want to treat it as complete overhaul, then it is better to split the patch such that, one patch removes the existing driver and other patch adds the new driver. In this way it is easy to review the patch. Regards, Sudhakar ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
This patch is a significant overhaul of the davinci spi controller driver that corrects multiple errors: - Eliminate a race condition that exists for slow SPI devices - Fix DMA transfer length error - Fix limitations preventing multiple SPI devices on the same controller Signed-off-by: Brian Niebuhr bnieb...@efjohnson.com The verbose description of the issues addressed from PATCH 0/2 should go here is well so it makes it into the permanent git history. I can certainly do that. That being said, I think for the sake of reviewing, you're going to need to break this up into reviewable pieces, each having a verbose description of the issues being solved. There is also a mixture of fixes, enhancements, renames etc. These should be done as separate patches. I know that it's more work to break it up like this, but that's the only way to make a large change like this reviewable by others. I guess I was hoping that this could be reviewed as if it were a new driver submission. I ended up more or less rewriting all of the functional parts of the driver (txrx_bufs(), chipselect(), IRQs and DMA callbacks), so it's very difficult to show this as a series of changes. I do understand the problem from your perspective, though. My thought was that if the TI folks were willing to look the driver over and they gave their blessing, that you would look at it as if it were a replacement driver and accept or deny it on that basis. Thanks for your consideration, Brian ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
Brian Niebuhr bnieb...@efjohnson.com writes: This patch is a significant overhaul of the davinci spi controller driver that corrects multiple errors: - Eliminate a race condition that exists for slow SPI devices - Fix DMA transfer length error - Fix limitations preventing multiple SPI devices on the same controller Signed-off-by: Brian Niebuhr bnieb...@efjohnson.com The verbose description of the issues addressed from PATCH 0/2 should go here is well so it makes it into the permanent git history. I can certainly do that. That being said, I think for the sake of reviewing, you're going to need to break this up into reviewable pieces, each having a verbose description of the issues being solved. There is also a mixture of fixes, enhancements, renames etc. These should be done as separate patches. I know that it's more work to break it up like this, but that's the only way to make a large change like this reviewable by others. I guess I was hoping that this could be reviewed as if it were a new driver submission. I ended up more or less rewriting all of the functional parts of the driver (txrx_bufs(), chipselect(), IRQs and DMA callbacks), so it's very difficult to show this as a series of changes. I do understand the problem from your perspective, though. My thought was that if the TI folks were willing to look the driver over and they gave their blessing, that you would look at it as if it were a replacement driver and accept or deny it on that basis. I'm OK with the approach of considering it as a brand new driver. The changelog made me think it was a bunch of fixes/enhancements and not a re-write. It should then be made more clear in the changelog that this is essentially a re-write, and why it is not done in a series of small changes. Whichever approach, this will need to worked out between you and the origial TI authors (Sandeep, Sudahkar) who will need to review/signoff this replacement. Kevin ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
I've applied the patch to the latest git kernel (I believe it was against arago), and tested it on the DM355EVM, in the meantime in polling mode only. I intend to make more tests in the near future. Several issues I've encountered were indeed solved (such as the extra writes to the bus after the chip-select turns idle). ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
This patch is a significant overhaul of the davinci spi controller driver
that corrects multiple errors:
- Eliminate a race condition that exists for slow SPI devices
- Fix DMA transfer length error
- Fix limitations preventing multiple SPI devices on the same controller
Signed-off-by: Brian Niebuhr bnieb...@efjohnson.com
---
arch/arm/mach-davinci/include/mach/spi.h | 30 +-
drivers/spi/davinci_spi.c| 990 --
drivers/spi/davinci_spi.h| 53 +-
3 files changed, 437 insertions(+), 636 deletions(-)
diff --git a/arch/arm/mach-davinci/include/mach/spi.h
b/arch/arm/mach-davinci/include/mach/spi.h
index 7b54926..10c39d8 100644
--- a/arch/arm/mach-davinci/include/mach/spi.h
+++ b/arch/arm/mach-davinci/include/mach/spi.h
@@ -34,18 +34,24 @@ enum {
struct davinci_spi_platform_data {
u8 version;
u16 num_chipselect;
- u32 wdelay;
- u32 odd_parity;
- u32 parity_enable;
- u32 wait_enable;
- u32 timer_disable;
- u32 clk_internal;
- u32 cs_hold;
- u32 intr_level;
- u32 poll_mode;
- u32 use_dma;
- u8 c2tdelay;
- u8 t2cdelay;
+ u8 *chip_sel;
+};
+
+struct davinci_spi_config {
+ u32 odd_parity:1;
+ u32 parity_enable:1;
+ u32 intr_level:1;
+ u32 io_type:2;
+#define SPI_IO_TYPE_INTR0
+#define SPI_IO_TYPE_POLL1
+#define SPI_IO_TYPE_DMA 2
+ u32 bytes_per_word:2;
+ u32 wdelay:6;
+ u32 timer_disable:1;
+ u8 c2t_delay;
+ u8 t2c_delay;
+ u8 t2e_delay;
+ u8 c2e_delay;
};
#endif /* __ARCH_ARM_DAVINCI_SPI_H */
diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
index 956f617..0bed840 100644
--- a/drivers/spi/davinci_spi.c
+++ b/drivers/spi/davinci_spi.c
@@ -1,5 +1,6 @@
/*
* Copyright (C) 2009 Texas Instruments.
+ * Copyright (C) 2010 EF Johnson Technologies
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -33,43 +34,45 @@
#include davinci_spi.h
-static unsigned use_dma;
-
#defineDAVINCI_SPI_NO_RESOURCE ((resource_size_t)-1)
static void davinci_spi_rx_buf_u8(u32 data, struct davinci_spi *davinci_spi)
{
- u8 *rx = davinci_spi-rx;
-
- *rx++ = (u8)data;
- davinci_spi-rx = rx;
+ if (davinci_spi-rx) {
+ u8 *rx = davinci_spi-rx;
+ *rx++ = (u8)data;
+ davinci_spi-rx = rx;
+ }
}
static void davinci_spi_rx_buf_u16(u32 data, struct davinci_spi *davinci_spi)
{
- u16 *rx = davinci_spi-rx;
-
- *rx++ = (u16)data;
- davinci_spi-rx = rx;
+ if (davinci_spi-rx) {
+ u16 *rx = davinci_spi-rx;
+ *rx++ = (u16)data;
+ davinci_spi-rx = rx;
+ }
}
static u32 davinci_spi_tx_buf_u8(struct davinci_spi *davinci_spi)
{
- u32 data;
- const u8 *tx = davinci_spi-tx;
-
- data = *tx++;
- davinci_spi-tx = tx;
+ u32 data = 0;
+ if (davinci_spi-tx) {
+ const u8 *tx = davinci_spi-tx;
+ data = *tx++;
+ davinci_spi-tx = tx;
+ }
return data;
}
static u32 davinci_spi_tx_buf_u16(struct davinci_spi *davinci_spi)
{
- u32 data;
- const u16 *tx = davinci_spi-tx;
-
- data = *tx++;
- davinci_spi-tx = tx;
+ u32 data = 0;
+ if (davinci_spi-tx) {
+ const u16 *tx = davinci_spi-tx;
+ data = *tx++;
+ davinci_spi-tx = tx;
+ }
return data;
}
@@ -89,26 +92,6 @@ static inline void clear_io_bits(void __iomem *addr, u32
bits)
iowrite32(v, addr);
}
-static inline void set_fmt_bits(void __iomem *addr, u32 bits, int cs_num)
-{
- set_io_bits(addr + SPIFMT0 + (0x4 * cs_num), bits);
-}
-
-static inline void clear_fmt_bits(void __iomem *addr, u32 bits, int cs_num)
-{
- clear_io_bits(addr + SPIFMT0 + (0x4 * cs_num), bits);
-}
-
-static void davinci_spi_set_dma_req(const struct spi_device *spi, int enable)
-{
- struct davinci_spi *davinci_spi = spi_master_get_devdata(spi-master);
-
- if (enable)
- set_io_bits(davinci_spi-base + SPIINT, SPIINT_DMA_REQ_EN);
- else
- clear_io_bits(davinci_spi-base + SPIINT, SPIINT_DMA_REQ_EN);
-}
-
/*
* Interface to control the chip select signal
*/
@@ -116,25 +99,54 @@ static void davinci_spi_chipselect(struct spi_device *spi,
int value)
{
struct davinci_spi *davinci_spi;
struct davinci_spi_platform_data *pdata;
- u32 data1_reg_val = 0;
+ u8 i, chip_sel = spi-chip_select;
+ u32 spidat1;
+ u16 spidat1_cfg;
davinci_spi = spi_master_get_devdata(spi-master);
pdata = davinci_spi-pdata;
- /*
-*
Re: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
Brian Niebuhr bniebu...@gmail.com writes: This patch is a significant overhaul of the davinci spi controller driver that corrects multiple errors: - Eliminate a race condition that exists for slow SPI devices - Fix DMA transfer length error - Fix limitations preventing multiple SPI devices on the same controller Signed-off-by: Brian Niebuhr bnieb...@efjohnson.com The verbose description of the issues addressed from PATCH 0/2 should go here is well so it makes it into the permanent git history. That being said, I think for the sake of reviewing, you're going to need to break this up into reviewable pieces, each having a verbose description of the issues being solved. There is also a mixture of fixes, enhancements, renames etc. These should be done as separate patches. I know that it's more work to break it up like this, but that's the only way to make a large change like this reviewable by others. Thanks, Kevin ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
