Re: atmel_spi clock polarity
On Wednesday 20 February 2008, Haavard Skinnemoen wrote: > > > > Unfortunately it did not work. The clock state did not change by > > writing MR register. > > Ok, thanks for testing. > > In that case, I think the best fix is to let NPCS0 stay selected > permanently in MR and overwrite CSR0 with to the new slave's settings > before asserting CS. But that's a more complicated change, and I don't > know how it will affect the AT91RM9200 special cases. The rm9200 special cases which, ISTR, still don't work right ... > So I suggest we merge your patch for 2.6.25, and try to optimize it > for 2.6.26. > > David, do you want me to pass on the patch with my signoff or just ask > Andrew to add my Acked-by to the patch already in mm? It'd be good to let Andrew have your ack. It's already in MM, so if I don't need to sign off it's nice to have less to do there. :) - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Wednesday 20 February 2008, Atsushi Nemoto wrote: > > David, do you want me to pass on the patch with my signoff or just ask > > Andrew to add my Acked-by to the patch already in mm? > > The patch in mm also lacks my Signed-off line. I had thought Andrew > never take a patch without Signed-off line ;) > > Should I resend my patch with my Signed-off and Haavard's Acked-by? Good point. I think that's the way to go. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Wed, 20 Feb 2008 10:34:46 +0100, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote: > In that case, I think the best fix is to let NPCS0 stay selected > permanently in MR and overwrite CSR0 with to the new slave's settings > before asserting CS. But that's a more complicated change, and I don't > know how it will affect the AT91RM9200 special cases. > > So I suggest we merge your patch for 2.6.25, and try to optimize it > for 2.6.26. I absolutely agree. > David, do you want me to pass on the patch with my signoff or just ask > Andrew to add my Acked-by to the patch already in mm? The patch in mm also lacks my Signed-off line. I had thought Andrew never take a patch without Signed-off line ;) Should I resend my patch with my Signed-off and Haavard's Acked-by? --- Atsushi Nemoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Wed, 20 Feb 2008 14:21:09 +0900 (JST) Atsushi Nemoto <[EMAIL PROTECTED]> wrote: > On Mon, 18 Feb 2008 15:31:58 +0100, Haavard Skinnemoen <[EMAIL PROTECTED]> > wrote: > > > Anyway, I will try your patch in a few days. > > > > Ok, thanks. If it works, that would be great, but given your > > description above I'm not sure if I dare hope for it. > > Unfortunately it did not work. The clock state did not change by > writing MR register. Ok, thanks for testing. In that case, I think the best fix is to let NPCS0 stay selected permanently in MR and overwrite CSR0 with to the new slave's settings before asserting CS. But that's a more complicated change, and I don't know how it will affect the AT91RM9200 special cases. So I suggest we merge your patch for 2.6.25, and try to optimize it for 2.6.26. David, do you want me to pass on the patch with my signoff or just ask Andrew to add my Acked-by to the patch already in mm? Haavard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Mon, 18 Feb 2008 15:31:58 +0100, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote: > > Anyway, I will try your patch in a few days. > > Ok, thanks. If it works, that would be great, but given your > description above I'm not sure if I dare hope for it. Unfortunately it did not work. The clock state did not change by writing MR register. --- Atsushi Nemoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] atmel_spi clock polarity
On Mon, 18 Feb 2008 23:49:18 +0100, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote: > > > CLK __|~|___|~~~|___|~~~|___|~~~|___ > > > > ... and at T1 CPOL is changed?? That's wrong. There should > > never be a partial clock period while a chipselect is active. > > While it's inactive, sure -- no chip should care. > > ...but what I'm afraid of is that since we're using GPIO chipselects, > the controller may _think_ that no chip is selected and change the > clock polarity just before it would pull the chipselect low if we were > using automatic chipselects... Yes. That's I suppose. > If that's the case, it would be a bit strange if it actually helped to > initialize all the CSRn registers, but it would also offer us a nice > workaround... I suppose the clock state of the AT91 just follows _last_ transfer. My patch (setting all CSRn.POL for next transfer) was based on that assumption. > Atsushi, do you by any chance have debugging enabled? That would > explain the long delay from CS activation to the change of clock > polarity. Compared to printk() the DMA setup takes almost no time at > all. No, I did not enabled debugging. > If you can confirm that my patch helps, I think that's the one we want > in mainline. Unfortunately I had no time to try it today. Hopefully tomorrow... --- Atsushi Nemoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] atmel_spi clock polarity
On Mon, 18 Feb 2008 11:57:56 -0800 David Brownell <[EMAIL PROTECTED]> wrote: > On Monday 18 February 2008, Atsushi Nemoto wrote: > > IIRC the clock state follows > > CSRn.CPOL just before the real transfer. > > No ... clock state should be valid *before* chipselect goes > active. So I'm thinking the patch from Haavard is likely > the right change. Hopefully it is... > > CLK __|~|___|~~~|___|~~~|___|~~~|___ > > ... and at T1 CPOL is changed?? That's wrong. There should > never be a partial clock period while a chipselect is active. > While it's inactive, sure -- no chip should care. ...but what I'm afraid of is that since we're using GPIO chipselects, the controller may _think_ that no chip is selected and change the clock polarity just before it would pull the chipselect low if we were using automatic chipselects... If that's the case, it would be a bit strange if it actually helped to initialize all the CSRn registers, but it would also offer us a nice workaround... Atsushi, do you by any chance have debugging enabled? That would explain the long delay from CS activation to the change of clock polarity. Compared to printk() the DMA setup takes almost no time at all. If you can confirm that my patch helps, I think that's the one we want in mainline. Haavard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] atmel_spi clock polarity
On Monday 18 February 2008, Atsushi Nemoto wrote: >IIRC the clock state follows > CSRn.CPOL just before the real transfer. No ... clock state should be valid *before* chipselect goes active. So I'm thinking the patch from Haavard is likely the right change. > Like this (previous transfer > was MODE 0, new transfer is MODE 3): > >T0T1 T2 > > CS ~~~| So at T0, some chip is selected (and never deselected) ... > > CLK __|~|___|~~~|___|~~~|___|~~~|___ ... and at T1 CPOL is changed?? That's wrong. There should never be a partial clock period while a chipselect is active. While it's inactive, sure -- no chip should care. > > SO ~~|___|~~~|___|~~~|___|~~~|_ >MSB > > T0-T1 was relatively longer then T1-T2. I suppose T1 is not the > point of updating MR register, but the point of starting DMA transfer. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Mon, 18 Feb 2008 23:12:43 +0900 (JST) Atsushi Nemoto <[EMAIL PROTECTED]> wrote: > T0-T1 was relatively longer then T1-T2. I suppose T1 is not the > point of updating MR register, but the point of starting DMA transfer. Aw. I see. > Anyway, I will try your patch in a few days. Ok, thanks. If it works, that would be great, but given your description above I'm not sure if I dare hope for it. Hmm...I suppose we could just use CSR0 for all transfers and not bother updating MR. That might actually be cheaper than doing it "the right way"...and allow us to support an arbitrary number of chipselects instead of just four. But I guess the AT91RM9200 won't be too happy about that... Haavard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Mon, 18 Feb 2008 12:42:37 +0100, Haavard Skinnemoen <[EMAIL PROTECTED]> wrote: > > Here is my quick workaround for this problem. It makes all CSRn.CPOL > > match for the transfer before activating chipselect. I'm not quite > > sure my analysis is correct, and there might be better solution. > > Could you give me any comments? > > I'm not sure if I fully understand what problem you're seeing. Is the > clock state wrong when the chip select is activated? Yes. > If so, does the patch below help? Hmm... It might fix my problem. But IIRC the clock state follows CSRn.CPOL just before the real transfer. Like this (previous transfer was MODE 0, new transfer is MODE 3): T0T1 T2 CS ~~~| CLK __|~|___|~~~|___|~~~|___|~~~|___ SO ~~|___|~~~|___|~~~|___|~~~|_ MSB T0-T1 was relatively longer then T1-T2. I suppose T1 is not the point of updating MR register, but the point of starting DMA transfer. Anyway, I will try your patch in a few days. --- Atsushi Nemoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Sat, 16 Feb 2008 22:32:52 +0900 (JST) Atsushi Nemoto <[EMAIL PROTECTED]> wrote: > Here is my quick workaround for this problem. It makes all CSRn.CPOL > match for the transfer before activating chipselect. I'm not quite > sure my analysis is correct, and there might be better solution. > Could you give me any comments? I'm not sure if I fully understand what problem you're seeing. Is the clock state wrong when the chip select is activated? If so, does the patch below help? Haavard diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c index 293b7ca..4f19b82 100644 --- a/drivers/spi/atmel_spi.c +++ b/drivers/spi/atmel_spi.c @@ -90,6 +90,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) mr = spi_readl(as, MR); mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr); + spi_writel(as, MR, mr); dev_dbg(&spi->dev, "activate %u%s, mr %08x\n", gpio, active ? " (high)" : "", @@ -97,7 +98,6 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) if (!(cpu_is_at91rm9200() && spi->chip_select == 0)) gpio_set_value(gpio, active); - spi_writel(as, MR, mr); } static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
atmel_spi clock polarity
I found that atmel_spi driver does not initialize clock polarity correctly (except for at91rm9200 CS0 channel) in some case. The atmel_spi driver uses gpio-controlled chipselect. OTOH spi clock signal is controlled by CSRn.CPOL bit, but this register controls clock signal correctly only in 'real transfer' duration. At the time of cs_activate() call, CSRn.CPOL will be initialized correctly, but the controller do not know which channel is to be used next, so clock signal will stay at the inactive state of last transfer. If clock polarity of new transfer and last transfer was differ, new transfer will start with wrong clock signal state. For example, if you started SPI MODE 2 or 3 transfer after SPI MODE 0 or 1 transfer, the clock signal state at the assertion of chipselect will be low. Of course this will violates SPI transfer. Here is my quick workaround for this problem. It makes all CSRn.CPOL match for the transfer before activating chipselect. I'm not quite sure my analysis is correct, and there might be better solution. Could you give me any comments? diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c index 293b7ca..85687aa 100644 --- a/drivers/spi/atmel_spi.c +++ b/drivers/spi/atmel_spi.c @@ -87,6 +87,16 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) unsigned gpio = (unsigned) spi->controller_data; unsigned active = spi->mode & SPI_CS_HIGH; u32 mr; + int i; + u32 csr; + u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0; + + /* Make sure clock polarity is correct */ + for (i = 0; i < spi->master->num_chipselect; i++) { + csr = spi_readl(as, CSR0 + 4 * i); + if ((csr ^ cpol) & SPI_BIT(CPOL)) + spi_writel(as, CSR0 + 4 * i, csr ^ SPI_BIT(CPOL)); + } mr = spi_readl(as, MR); mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr); --- Atsushi Nemoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/