| Message ID | 1364905195-24286-1-git-send-email-tpiepho@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Dear Trent Piepho, > There are two bits which control the CS line in the CTRL0 register: > LOCK_CS and IGNORE_CRC. The latter would be better named DEASSERT_CS > in SPI mode. > > LOCK_CS keeps CS asserted though the entire transfer. This should > always be set. The DMA code will always set it, explicitly on the > first segment of the first transfer, and then implicitly on all the > rest by never clearing the bit from the value read from the ctrl0 > register. > > The only reason to not set LOCK_CS would be to attempt an altered > protocol where CS pulses between each word. Though don't get your > hopes up if you want to do this, as the hardware doesn't appear to do > this in any sane manner. Can you please elaborate on this part above? The description is very vague. Fabio, can you review this too please? > The code can be simplified by just setting LOCK_CS once and then not > needing to deal with it in the PIO and DMA transfer functions. > > 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 | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c > index 22a0af0..9334167 100644 > --- a/drivers/spi/spi-mxs.c > +++ b/drivers/spi/spi-mxs.c > @@ -91,6 +91,8 @@ static int mxs_spi_setup_transfer(struct spi_device *dev, > > mxs_ssp_set_clk_rate(ssp, hz); > > + writel(BM_SSP_CTRL0_LOCK_CS, > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); > writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) | > BF_SSP_CTRL1_WORD_LENGTH > (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) | > @@ -159,8 +161,6 @@ static inline void mxs_spi_enable(struct mxs_spi *spi) > { > struct mxs_ssp *ssp = &spi->ssp; > > - writel(BM_SSP_CTRL0_LOCK_CS, > - ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); > writel(BM_SSP_CTRL0_IGNORE_CRC, > ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); > } > @@ -169,8 +169,6 @@ static inline void mxs_spi_disable(struct mxs_spi *spi) > { > struct mxs_ssp *ssp = &spi->ssp; > > - writel(BM_SSP_CTRL0_LOCK_CS, > - ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); > writel(BM_SSP_CTRL0_IGNORE_CRC, > ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); > } > @@ -244,8 +242,6 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int > cs, ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT; > ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs); > > - if (*first) > - ctrl0 |= BM_SSP_CTRL0_LOCK_CS; > if (!write) > ctrl0 |= BM_SSP_CTRL0_READ; Best regards, Marek Vasut ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
On Tue, Apr 02, 2013 at 05:19:44AM -0700, Trent Piepho wrote: > There are two bits which control the CS line in the CTRL0 register: > LOCK_CS and IGNORE_CRC. The latter would be better named DEASSERT_CS > in SPI mode. > > LOCK_CS keeps CS asserted though the entire transfer. This should > always be set. The DMA code will always set it, explicitly on the > first segment of the first transfer, and then implicitly on all the > rest by never clearing the bit from the value read from the ctrl0 > register. > > The only reason to not set LOCK_CS would be to attempt an altered > protocol where CS pulses between each word. Though don't get your > hopes up if you want to do this, as the hardware doesn't appear to do > this in any sane manner. > > The code can be simplified by just setting LOCK_CS once and then not > needing to deal with it in the PIO and DMA transfer functions. > > Signed-off-by: Trent Piepho <tpiepho@gmail.com> Trent, Just FYI, Mark Brown is taking care of SPI subsystem patches recently. Please remember copy him on the series when you repost, if you want to see the series get applied eventually :) Shawn ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
Hello Trent, > There are two bits which control the CS line in the CTRL0 register: > LOCK_CS and IGNORE_CRC. The latter would be better named DEASSERT_CS > in SPI mode. > > LOCK_CS keeps CS asserted though the entire transfer. This should > always be set. The DMA code will always set it, explicitly on the > first segment of the first transfer, and then implicitly on all the > rest by never clearing the bit from the value read from the ctrl0 > register. > > The only reason to not set LOCK_CS would be to attempt an altered > protocol where CS pulses between each word. Though don't get your > hopes up if you want to do this, as the hardware doesn't appear to do > this in any sane manner. > > The code can be simplified by just setting LOCK_CS once and then not > needing to deal with it in the PIO and DMA transfer functions. > > 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> Will we see a V3 of this stuff? I don't want to see this lost. Best regards, Marek Vasut ------------------------------------------------------------------------------ Precog is a next-generation analytics platform capable of advanced analytics on semi-structured data. The platform includes APIs for building apps and a phenomenal toolset for data science. Developers can use our toolset for easy data analysis & visualization. Get a free account! http://www2.precog.com/precogplatform/slashdotnewsletter
On Tue, Apr 2, 2013 at 8:18 PM, Marek Vasut <marex@denx.de> wrote: > Dear Trent Piepho, > >> There are two bits which control the CS line in the CTRL0 register: >> LOCK_CS and IGNORE_CRC. The latter would be better named DEASSERT_CS >> in SPI mode. >> >> LOCK_CS keeps CS asserted though the entire transfer. This should >> always be set. The DMA code will always set it, explicitly on the >> first segment of the first transfer, and then implicitly on all the >> rest by never clearing the bit from the value read from the ctrl0 >> register. >> >> The only reason to not set LOCK_CS would be to attempt an altered >> protocol where CS pulses between each word. Though don't get your >> hopes up if you want to do this, as the hardware doesn't appear to do >> this in any sane manner. > > Can you please elaborate on this part above? The description is very vague. > > Fabio, can you review this too please? Sure, it would be nice if Trent could resend this series and copy the SPI maintainer, Mark Brown. ------------------------------------------------------------------------------ See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
Hi, Fabio Estevam writes: > On Tue, Apr 2, 2013 at 8:18 PM, Marek Vasut <marex@denx.de> wrote: > > Dear Trent Piepho, > > > >> There are two bits which control the CS line in the CTRL0 register: > >> LOCK_CS and IGNORE_CRC. The latter would be better named DEASSERT_CS > >> in SPI mode. > >> > >> LOCK_CS keeps CS asserted though the entire transfer. This should > >> always be set. The DMA code will always set it, explicitly on the > >> first segment of the first transfer, and then implicitly on all the > >> rest by never clearing the bit from the value read from the ctrl0 > >> register. > >> > >> The only reason to not set LOCK_CS would be to attempt an altered > >> protocol where CS pulses between each word. Though don't get your > >> hopes up if you want to do this, as the hardware doesn't appear to do > >> this in any sane manner. > > > > Can you please elaborate on this part above? The description is very vague. > > From my experience the HW deasserts CS whenever the output fifo runs empty. Lothar Waßmann
On Wed, Jul 10, 2013 at 8:29 AM, Lothar Waßmann <LW@karo-electronics.de>wrote: > > >> always be set. The DMA code will always set it, explicitly on the > > >> first segment of the first transfer, and then implicitly on all the > > >> rest by never clearing the bit from the value read from the ctrl0 > > >> register. > > >> > > >> The only reason to not set LOCK_CS would be to attempt an altered > > >> protocol where CS pulses between each word. Though don't get your > > >> hopes up if you want to do this, as the hardware doesn't appear to do > > >> this in any sane manner. > > > > > > Can you please elaborate on this part above? The description is very > vague. > > > > > >> LOCK_CS keeps CS asserted though the entire transfer. This should > From my experience the HW deasserts CS whenever the output fifo runs > empty. > > I think that might be the case. Since I have no need to add support for some non-standard SPI protocol nobody has wanted, I didn't spend the time to reverse engineer how exactly it worked. I think what would happen is that if you used multi-word transfers (and the current PIO mode in the driver ONLY uses single word transfers!) you would sometimes get a CS pulse between each word and sometimes every four words. It seemed to make a difference if one used multi-word PIO with a FIFO, which the current driver doesn't support, or multi-word DMA. I did spend enough time on it to determine that, "always set LOCK_CS all the time for normal SPI," is correct. The driver's method of setting in for all transfers but the last, except in DMA mode where it's set on the last transfer too, apparently by accident, is unnecessarily complex and inefficient. It only "works" because the current PIO code only supports single word transfers at the hardware level. ------------------------------------------------------------------------------ See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c index 22a0af0..9334167 100644 --- a/drivers/spi/spi-mxs.c +++ b/drivers/spi/spi-mxs.c @@ -91,6 +91,8 @@ static int mxs_spi_setup_transfer(struct spi_device *dev, mxs_ssp_set_clk_rate(ssp, hz); + writel(BM_SSP_CTRL0_LOCK_CS, + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) | BF_SSP_CTRL1_WORD_LENGTH (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) | @@ -159,8 +161,6 @@ static inline void mxs_spi_enable(struct mxs_spi *spi) { struct mxs_ssp *ssp = &spi->ssp; - writel(BM_SSP_CTRL0_LOCK_CS, - ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); writel(BM_SSP_CTRL0_IGNORE_CRC, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); } @@ -169,8 +169,6 @@ static inline void mxs_spi_disable(struct mxs_spi *spi) { struct mxs_ssp *ssp = &spi->ssp; - writel(BM_SSP_CTRL0_LOCK_CS, - ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR); writel(BM_SSP_CTRL0_IGNORE_CRC, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET); } @@ -244,8 +242,6 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs, ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT; ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs); - if (*first) - ctrl0 |= BM_SSP_CTRL0_LOCK_CS; if (!write) ctrl0 |= BM_SSP_CTRL0_READ;
There are two bits which control the CS line in the CTRL0 register: LOCK_CS and IGNORE_CRC. The latter would be better named DEASSERT_CS in SPI mode. LOCK_CS keeps CS asserted though the entire transfer. This should always be set. The DMA code will always set it, explicitly on the first segment of the first transfer, and then implicitly on all the rest by never clearing the bit from the value read from the ctrl0 register. The only reason to not set LOCK_CS would be to attempt an altered protocol where CS pulses between each word. Though don't get your hopes up if you want to do this, as the hardware doesn't appear to do this in any sane manner. The code can be simplified by just setting LOCK_CS once and then not needing to deal with it in the PIO and DMA transfer functions. 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 | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)