| Message ID | 1364570381-17605-2-git-send-email-tpiepho@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Dear Trent Piepho, > In DMA mode the chip select control bits would be ORed into the CTRL0 > register without first clearing the bits. This means that after > addressing slave 1 the bit would be still be set when addressing slave > 0, resulting in slave 1 continuing to be addressed. > > The message handing function would pass the cs value to the txrx > function, which would re-program the bits on each transfer in the > message. The selected cs does not change during a message so this is > inefficient. It also means there are two different sets of code for > selecting the CS, one for PIO that worked and one for DMA that didn't. > > Change the code to set the CS bits in the message transfer function > once. Now the DMA and PIO txrx functions don't need to care about CS > at all. Ok, lemme ask this one more time -- will the DMA work with long transfers where the CTRL0 will be overwritten on each turn? Did you actually test this? > Signed-off-by: Trent Piepho <tpiepho@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Fabio Estevam <fabio.estevam@freescale.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/spi/spi-mxs.c | 40 +++++++++++++++------------------------- > 1 file changed, 15 insertions(+), 25 deletions(-) > > diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c > index aa77d96b9..5d63b21 100644 > --- a/drivers/spi/spi-mxs.c > +++ b/drivers/spi/spi-mxs.c > @@ -130,9 +130,9 @@ static int mxs_spi_setup(struct spi_device *dev) > return err; > } > > -static uint32_t mxs_spi_cs_to_reg(unsigned cs) > +static u32 mxs_spi_cs_to_reg(unsigned cs) > { > - uint32_t select = 0; > + u32 select = 0; This change is unneeded, remove it. > /* > * i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0 > @@ -150,18 +150,6 @@ static uint32_t mxs_spi_cs_to_reg(unsigned cs) > return select; > } > > -static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs) > -{ > - const uint32_t mask = > - BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ; > - uint32_t select; > - struct mxs_ssp *ssp = &spi->ssp; > - > - writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); > - select = mxs_spi_cs_to_reg(cs); > - writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); > -} > - > static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool > set) { > const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT); > @@ -199,7 +187,7 @@ static irqreturn_t mxs_ssp_irq_handler(int irq, void > *dev_id) return IRQ_HANDLED; > } > > -static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs, > +static int mxs_spi_txrx_dma(struct mxs_spi *spi, > unsigned char *buf, int len, > unsigned int flags) > { > @@ -227,10 +215,11 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int > cs, > > INIT_COMPLETION(spi->c); > > + /* Chip select was already programmed into CTRL0 */ > ctrl0 = readl(ssp->base + HW_SSP_CTRL0); > ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT & ~BM_SSP_CTRL0_IGNORE_CRC & > ~BM_SSP_CTRL0_READ; > - ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs); > + ctrl0 |= BM_SSP_CTRL0_DATA_XFER; > > if (!(flags & TXRX_WRITE)) > ctrl0 |= BM_SSP_CTRL0_READ; > @@ -332,7 +321,7 @@ err_mapped: > return ret; > } > > -static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs, > +static int mxs_spi_txrx_pio(struct mxs_spi *spi, > unsigned char *buf, int len, > unsigned int flags) > { > @@ -342,8 +331,6 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int > cs, writel(BM_SSP_CTRL0_IGNORE_CRC, > ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); > > - mxs_spi_set_cs(spi, cs); > - > while (len--) { > if (len == 0 && (flags & TXRX_DEASSERT_CS)) > writel(BM_SSP_CTRL0_IGNORE_CRC, > @@ -405,9 +392,12 @@ static int mxs_spi_transfer_one(struct spi_master > *master, struct spi_transfer *t, *tmp_t; > unsigned int flag; > int status = 0; > - int cs; > > - cs = m->spi->chip_select; > + /* Program CS register bits here, it will be used for all transfers. */ > + writel(BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ, > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); > + writel(mxs_spi_cs_to_reg(m->spi->chip_select), > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); > > list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) { > > @@ -440,11 +430,11 @@ static int mxs_spi_transfer_one(struct spi_master > *master, STMP_OFFSET_REG_CLR); > > if (t->tx_buf) > - status = mxs_spi_txrx_pio(spi, cs, > + status = mxs_spi_txrx_pio(spi, > (void *)t->tx_buf, > t->len, flag | TXRX_WRITE); > if (t->rx_buf) > - status = mxs_spi_txrx_pio(spi, cs, > + status = mxs_spi_txrx_pio(spi, > t->rx_buf, t->len, > flag); > } else { > @@ -453,11 +443,11 @@ static int mxs_spi_transfer_one(struct spi_master > *master, STMP_OFFSET_REG_SET); > > if (t->tx_buf) > - status = mxs_spi_txrx_dma(spi, cs, > + status = mxs_spi_txrx_dma(spi, > (void *)t->tx_buf, t->len, > flag | TXRX_WRITE); > if (t->rx_buf) > - status = mxs_spi_txrx_dma(spi, cs, > + status = mxs_spi_txrx_dma(spi, > t->rx_buf, t->len, > flag); > } Best regards, Marek Vasut ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
On Mon, Apr 1, 2013 at 4:13 PM, Marek Vasut <marex@denx.de> wrote: >> >> Change the code to set the CS bits in the message transfer function >> once. Now the DMA and PIO txrx functions don't need to care about CS >> at all. > > Ok, lemme ask this one more time -- will the DMA work with long transfers where > the CTRL0 will be overwritten on each turn? Did you actually test this? I don't have SPI flash, so I can't test SPI flash. I can test other spi devices and by capturing the pins with a logic analyzer. It does work after the patch, and does not work before the patch. The error should be obvious from looking at the code. ctrl0 is not overwritten, from scratch, each on transfer. The ctrl0 value in the PIO part of the DMA is based on the current value of ctrl0 with additional bits ORed in. The flaw here is that bits that should not be set are not masked out. >> >> -static uint32_t mxs_spi_cs_to_reg(unsigned cs) >> +static u32 mxs_spi_cs_to_reg(unsigned cs) >> { >> - uint32_t select = 0; >> + u32 select = 0; I'll make it a separate patch. ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
Dear Trent Piepho, > On Mon, Apr 1, 2013 at 4:13 PM, Marek Vasut <marex@denx.de> wrote: > >> Change the code to set the CS bits in the message transfer function > >> once. Now the DMA and PIO txrx functions don't need to care about CS > >> at all. > > > > Ok, lemme ask this one more time -- will the DMA work with long transfers > > where the CTRL0 will be overwritten on each turn? Did you actually test > > this? > > I don't have SPI flash, so I can't test SPI flash. I can test other > spi devices and by capturing the pins with a logic analyzer. It does > work after the patch, and does not work before the patch. The error > should be obvious from looking at the code. > > ctrl0 is not overwritten, from scratch, each on transfer. The ctrl0 > value in the PIO part of the DMA is based on the current value of > ctrl0 with additional bits ORed in. The flaw here is that bits that > should not be set are not masked out. > > >> -static uint32_t mxs_spi_cs_to_reg(unsigned cs) > >> +static u32 mxs_spi_cs_to_reg(unsigned cs) > >> > >> { > >> > >> - uint32_t select = 0; > >> + u32 select = 0; > > I'll make it a separate patch. This is completely irrelevant change, please just submit the relevant patches. Best regards, Marek Vasut ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex@denx.de> wrote: >> >> -static uint32_t mxs_spi_cs_to_reg(unsigned cs) >> >> +static u32 mxs_spi_cs_to_reg(unsigned cs) >> >> >> >> { >> >> >> >> - uint32_t select = 0; >> >> + u32 select = 0; >> >> I'll make it a separate patch. > > This is completely irrelevant change, please just submit the relevant patches. Kernel code should use u16, u32, etc. instead of the uint16_t, uint32_t types. The rest of the driver uses them. Why should this one function use a different type than the rest? It's ugly and inconsistent. And really, it's just as relevant as insisting that multiline patches use some exact format, which checkpatch.pl doesn't complain about. ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
Dear Trent Piepho, > On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex@denx.de> wrote: > >> >> -static uint32_t mxs_spi_cs_to_reg(unsigned cs) > >> >> +static u32 mxs_spi_cs_to_reg(unsigned cs) > >> >> > >> >> { > >> >> > >> >> - uint32_t select = 0; > >> >> + u32 select = 0; > >> > >> I'll make it a separate patch. > > > > This is completely irrelevant change, please just submit the relevant > > patches. > > Kernel code should use u16, u32, etc. instead of the uint16_t, > uint32_t types. The rest of the driver uses them. Why should this > one function use a different type than the rest? It's ugly and > inconsistent. Is this documented somewhere please? I see only a mention about this in chapter 5 of Documentation/CodingStyle , section (d) . Either way, separate such cosmetic change into another series so they're not in the way of relevant stuff. > And really, it's just as relevant as insisting that multiline patches > use some exact format, which checkpatch.pl doesn't complain about. The checkpatch is not almighty tool, Documentation/CodingStyle describes how code should be written/annotated/documented. Best regards, Marek Vasut ------------------------------------------------------------------------------ Own the Future-Intel(R) Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2
On Mon, Apr 1, 2013 at 5:02 PM, Marek Vasut <marex@denx.de> wrote: >> On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex@denx.de> wrote: >> >> >> -static uint32_t mxs_spi_cs_to_reg(unsigned cs) >> >> >> +static u32 mxs_spi_cs_to_reg(unsigned cs) >> >> >> >> >> >> { >> >> >> >> >> >> - uint32_t select = 0; >> >> >> + u32 select = 0; >> >> >> >> I'll make it a separate patch. >> > >> > This is completely irrelevant change, please just submit the relevant >> > patches. >> >> Kernel code should use u16, u32, etc. instead of the uint16_t, >> uint32_t types. The rest of the driver uses them. Why should this >> one function use a different type than the rest? It's ugly and >> inconsistent. > > Is this documented somewhere please? I see only a mention about this in chapter > 5 of Documentation/CodingStyle , section (d) . Either way, separate such > cosmetic change into another series so they're not in the way of relevant stuff. It's pretty clearly existing practice: $ perl -ne '@x=/\buint32_t\b/g;$x+=$#x+1;END{print "$x\n";}' drivers/spi/*.[ch] 9 $ perl -ne '@x=/\bu32\b/g;$x+=$#x+1;END{print "$x\n";}' drivers/spi/*.[ch] 501 ------------------------------------------------------------------------------ Own the Future-Intel(R) Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c index aa77d96b9..5d63b21 100644 --- a/drivers/spi/spi-mxs.c +++ b/drivers/spi/spi-mxs.c @@ -130,9 +130,9 @@ static int mxs_spi_setup(struct spi_device *dev) return err; } -static uint32_t mxs_spi_cs_to_reg(unsigned cs) +static u32 mxs_spi_cs_to_reg(unsigned cs) { - uint32_t select = 0; + u32 select = 0; /* * i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0 @@ -150,18 +150,6 @@ static uint32_t mxs_spi_cs_to_reg(unsigned cs) return select; } -static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs) -{ - const uint32_t mask = - BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ; - uint32_t select; - struct mxs_ssp *ssp = &spi->ssp; - - writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); - select = mxs_spi_cs_to_reg(cs); - writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); -} - static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set) { const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT); @@ -199,7 +187,7 @@ static irqreturn_t mxs_ssp_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; } -static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs, +static int mxs_spi_txrx_dma(struct mxs_spi *spi, unsigned char *buf, int len, unsigned int flags) { @@ -227,10 +215,11 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs, INIT_COMPLETION(spi->c); + /* Chip select was already programmed into CTRL0 */ ctrl0 = readl(ssp->base + HW_SSP_CTRL0); ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT & ~BM_SSP_CTRL0_IGNORE_CRC & ~BM_SSP_CTRL0_READ; - ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs); + ctrl0 |= BM_SSP_CTRL0_DATA_XFER; if (!(flags & TXRX_WRITE)) ctrl0 |= BM_SSP_CTRL0_READ; @@ -332,7 +321,7 @@ err_mapped: return ret; } -static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs, +static int mxs_spi_txrx_pio(struct mxs_spi *spi, unsigned char *buf, int len, unsigned int flags) { @@ -342,8 +331,6 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs, writel(BM_SSP_CTRL0_IGNORE_CRC, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); - mxs_spi_set_cs(spi, cs); - while (len--) { if (len == 0 && (flags & TXRX_DEASSERT_CS)) writel(BM_SSP_CTRL0_IGNORE_CRC, @@ -405,9 +392,12 @@ static int mxs_spi_transfer_one(struct spi_master *master, struct spi_transfer *t, *tmp_t; unsigned int flag; int status = 0; - int cs; - cs = m->spi->chip_select; + /* Program CS register bits here, it will be used for all transfers. */ + writel(BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ, + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); + writel(mxs_spi_cs_to_reg(m->spi->chip_select), + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) { @@ -440,11 +430,11 @@ static int mxs_spi_transfer_one(struct spi_master *master, STMP_OFFSET_REG_CLR); if (t->tx_buf) - status = mxs_spi_txrx_pio(spi, cs, + status = mxs_spi_txrx_pio(spi, (void *)t->tx_buf, t->len, flag | TXRX_WRITE); if (t->rx_buf) - status = mxs_spi_txrx_pio(spi, cs, + status = mxs_spi_txrx_pio(spi, t->rx_buf, t->len, flag); } else { @@ -453,11 +443,11 @@ static int mxs_spi_transfer_one(struct spi_master *master, STMP_OFFSET_REG_SET); if (t->tx_buf) - status = mxs_spi_txrx_dma(spi, cs, + status = mxs_spi_txrx_dma(spi, (void *)t->tx_buf, t->len, flag | TXRX_WRITE); if (t->rx_buf) - status = mxs_spi_txrx_dma(spi, cs, + status = mxs_spi_txrx_dma(spi, t->rx_buf, t->len, flag); }
In DMA mode the chip select control bits would be ORed into the CTRL0 register without first clearing the bits. This means that after addressing slave 1 the bit would be still be set when addressing slave 0, resulting in slave 1 continuing to be addressed. The message handing function would pass the cs value to the txrx function, which would re-program the bits on each transfer in the message. The selected cs does not change during a message so this is inefficient. It also means there are two different sets of code for selecting the CS, one for PIO that worked and one for DMA that didn't. Change the code to set the CS bits in the message transfer function once. Now the DMA and PIO txrx functions don't need to care about CS at all. Signed-off-by: Trent Piepho <tpiepho@gmail.com> Cc: Marek Vasut <marex@denx.de> Cc: Fabio Estevam <fabio.estevam@freescale.com> Cc: Shawn Guo <shawn.guo@linaro.org> --- drivers/spi/spi-mxs.c | 40 +++++++++++++++------------------------- 1 file changed, 15 insertions(+), 25 deletions(-)