atmel_spi clock polarity - Kernel

This is a discussion on atmel_spi clock polarity - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 12 of 12

Thread: atmel_spi clock polarity

  1. 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: atmel_spi clock polarity

    On Sat, 16 Feb 2008 22:32:52 +0900 (JST)
    Atsushi Nemoto 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: atmel_spi clock polarity

    On Mon, 18 Feb 2008 12:42:37 +0100, Haavard Skinnemoen 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):

    T0 T1 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: atmel_spi clock polarity

    On Mon, 18 Feb 2008 23:12:43 +0900 (JST)
    Atsushi Nemoto 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. 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):
    >
    > T0 T1 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [spi-devel-general] atmel_spi clock polarity

    On Mon, 18 Feb 2008 11:57:56 -0800
    David Brownell 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [spi-devel-general] atmel_spi clock polarity

    On Mon, 18 Feb 2008 23:49:18 +0100, Haavard Skinnemoen 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: atmel_spi clock polarity

    On Mon, 18 Feb 2008 15:31:58 +0100, Haavard Skinnemoen 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: atmel_spi clock polarity

    On Wed, 20 Feb 2008 14:21:09 +0900 (JST)
    Atsushi Nemoto wrote:

    > On Mon, 18 Feb 2008 15:31:58 +0100, Haavard Skinnemoen 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: atmel_spi clock polarity

    On Wed, 20 Feb 2008 10:34:46 +0100, Haavard Skinnemoen 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread