| Message ID | 20131001201425.13660.72740.stgit@Graphine (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 PIO code will explicitly set it for the first transfer, leave it > set for intermediate transfers, and then clear it for the final > transfer. It should not clear it. > > 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. It appears to be related to the hardware > FIFO fill level. > > The code can be simplified by just setting LOCK_CS once and then not > needing to deal with it at all 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> [...] This is not V1 of patches, but changelog is missing. Otherwise, I'm out of the office, so I'll review and test on monday earliest. Best regards, Marek Vasut ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk
Dear Mark Brown, > On Tue, Oct 01, 2013 at 01:14:25PM -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. > > Applied all, thanks. Did the patches undergo any kind of testing? I was busy so I couldn't even review them yet, sorry. Best regards, Marek Vasut ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
On Thu, Oct 17, 2013 at 9:33 PM, Marek Vasut <marex@denx.de> wrote: > Dear Mark Brown, > >> On Tue, Oct 01, 2013 at 01:14:25PM -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. >> >> Applied all, thanks. > > Did the patches undergo any kind of testing? I was busy so I couldn't even > review them yet, sorry. I've tested them extensively on my hardware and not found any bugs. While the driver in it's current state does have bugs that are fixed by the patches. I'd be interested in a benchmark with SPI flash. In my application there was a significant speed up, but it's somewhat different than SPI flash. ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
Dear Trent Piepho, > On Thu, Oct 17, 2013 at 9:33 PM, Marek Vasut <marex@denx.de> wrote: > > Dear Mark Brown, > > > >> On Tue, Oct 01, 2013 at 01:14:25PM -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. > >> > >> Applied all, thanks. > > > > Did the patches undergo any kind of testing? I was busy so I couldn't > > even review them yet, sorry. > > I've tested them extensively on my hardware and not found any bugs. What hardware is that? Can you please describe it? > While the driver in it's current state does have bugs that are fixed > by the patches. I'd be interested in a benchmark with SPI flash. In > my application there was a significant speed up, but it's somewhat > different than SPI flash. SPI flashes are the most significant users of this IP block on the MX23/MX28, that's why I'm unhappy patches that might break them were pulled in without any Tested-by/Reviewed-by/Acked-by . Best regards, Marek Vasut ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
On Thu, Oct 17, 2013 at 11:23 PM, Marek Vasut <marex@denx.de> wrote: > Dear Trent Piepho, > >> On Thu, Oct 17, 2013 at 9:33 PM, Marek Vasut <marex@denx.de> wrote: >> > Dear Mark Brown, >> > >> >> On Tue, Oct 01, 2013 at 01:14:25PM -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. >> >> >> >> Applied all, thanks. >> > >> > Did the patches undergo any kind of testing? I was busy so I couldn't >> > even review them yet, sorry. >> >> I've tested them extensively on my hardware and not found any bugs. > > What hardware is that? Can you please describe it? It's proprietary hardware and the design isn't mine to release. Does it really matter? Is there anything about the Linux SPI driver that you would like to know in particular? >> While the driver in it's current state does have bugs that are fixed >> by the patches. I'd be interested in a benchmark with SPI flash. In >> my application there was a significant speed up, but it's somewhat >> different than SPI flash. > > SPI flashes are the most significant users of this IP block on the MX23/MX28, > that's why I'm unhappy patches that might break them were pulled in without any > Tested-by/Reviewed-by/Acked-by . I first posted the patches in March. Plenty of time for an adequately interested party to test them. If there is a problem that I missed found then you know where to find me. ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
Hi Mark, > On Fri, Oct 18, 2013 at 08:23:21AM +0200, Marek Vasut wrote: > > SPI flashes are the most significant users of this IP block on the > > MX23/MX28, that's why I'm unhappy patches that might break them were > > pulled in without any Tested-by/Reviewed-by/Acked-by . > > If the patches aren't getting reviewed by peole working with the > hardware on the lists one of the things getting them into -next should > hopefully do is expose them to a wider audience and help get that > testing. We've got until the merge window and then all the -rcs to > identify and deal with any problems. I don't quite understand this, is it OK to merge untested patches? I have them in my review queue, but I was simply busy so I didn't get to them for a while. Fabio is unfortunatelly also N/A now, so things are slow. Best regards, Marek Vasut ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
Dear Trent Piepho, > On Thu, Oct 17, 2013 at 11:23 PM, Marek Vasut <marex@denx.de> wrote: > > Dear Trent Piepho, > > > >> On Thu, Oct 17, 2013 at 9:33 PM, Marek Vasut <marex@denx.de> wrote: > >> > Dear Mark Brown, > >> > > >> >> On Tue, Oct 01, 2013 at 01:14:25PM -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. > >> >> > >> >> Applied all, thanks. > >> > > >> > Did the patches undergo any kind of testing? I was busy so I couldn't > >> > even review them yet, sorry. > >> > >> I've tested them extensively on my hardware and not found any bugs. > > > > What hardware is that? Can you please describe it? > > It's proprietary hardware and the design isn't mine to release. Does > it really matter? Is there anything about the Linux SPI driver that > you would like to know in particular? I'd like to know if this was tested with anything but hardware we know nothing about. > >> While the driver in it's current state does have bugs that are fixed > >> by the patches. I'd be interested in a benchmark with SPI flash. In > >> my application there was a significant speed up, but it's somewhat > >> different than SPI flash. > > > > SPI flashes are the most significant users of this IP block on the > > MX23/MX28, that's why I'm unhappy patches that might break them were > > pulled in without any Tested-by/Reviewed-by/Acked-by . > > I first posted the patches in March. Plenty of time for an adequately > interested party to test them. If there is a problem that I missed > found then you know where to find me. I see them being posted at the begining of October though. Best regards, Marek Vasut ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
Dear Mark Brown, > On Fri, Oct 18, 2013 at 04:38:52PM +0200, Marek Vasut wrote: > > I don't quite understand this, is it OK to merge untested patches? I have > > them in my review queue, but I was simply busy so I didn't get to them > > for a while. Fabio is unfortunatelly also N/A now, so things are slow. > > So, I'd prefer not to merge untested stuff but I'm balancing other > considerations here, including getting exposure in -next prior to the > merge window (which is getting close now and the next two weeks are full > of conferences) for a relatively long series. Trent is someone who > generally does good stuff, the code all makes sense and the MXS SPI > driver seems robust so there's nothing setting off alarm bells for me > that this is risky. > > We've still got time to fix or drop the code if there's an issue found > prior to the merge widow opening and even if it makes it into Linus' > tree we can drop anything that causes trouble during the -rcs. CCing Lothar and Hector, so we have more eyes on this. I got around to test it on M25P80 briefly and so far so good, thanks. Please do test this if possible. Best regards, Marek Vasut ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
Dear Marek, On 10/19/2013 02:43 AM, Marek Vasut wrote: > Dear Mark Brown, > >> On Fri, Oct 18, 2013 at 04:38:52PM +0200, Marek Vasut wrote: >>> I don't quite understand this, is it OK to merge untested patches? I have >>> them in my review queue, but I was simply busy so I didn't get to them >>> for a while. Fabio is unfortunatelly also N/A now, so things are slow. >> >> So, I'd prefer not to merge untested stuff but I'm balancing other >> considerations here, including getting exposure in -next prior to the >> merge window (which is getting close now and the next two weeks are full >> of conferences) for a relatively long series. Trent is someone who >> generally does good stuff, the code all makes sense and the MXS SPI >> driver seems robust so there's nothing setting off alarm bells for me >> that this is risky. >> >> We've still got time to fix or drop the code if there's an issue found >> prior to the merge widow opening and even if it makes it into Linus' >> tree we can drop anything that causes trouble during the -rcs. > > CCing Lothar and Hector, so we have more eyes on this. I got around to test it > on M25P80 briefly and so far so good, thanks. Please do test this if possible. I don't have anything connected to MXS SPI bus. Since it is half-duplex its usage is pretty limited. Anyway, what is this supposed to fix that was working wrong? What's the behavior of the CS signal before and after the patch? Best regards, -- Hector Palacios ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c index de7b114..e6172ae 100644 --- a/drivers/spi/spi-mxs.c +++ b/drivers/spi/spi-mxs.c @@ -79,6 +79,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) | @@ -147,8 +149,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); } @@ -157,8 +157,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); } @@ -232,8 +230,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 PIO code will explicitly set it for the first transfer, leave it set for intermediate transfers, and then clear it for the final transfer. It should not clear it. 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. It appears to be related to the hardware FIFO fill level. The code can be simplified by just setting LOCK_CS once and then not needing to deal with it at all 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(-) ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk