SPI: cs_change not working properly #85

Open
corrosion opened this Issue Jan 14, 2014 · 3 comments

Comments

Projects
None yet
4 participants
@corrosion

In file drivers/spi/spi-omap2-mcspi.c funcion omap2_mcspi_work()

/* ignore the "leave it on after last xfer" hint */
if (t->cs_change) {
    omap2_mcspi_force_cs(spi, 0);
    cs_active = 0;
}

Setting cs_change is supposed to keep the chip select pin active after returning from the function.
This code does not allow to keep the transaction alive.

@ohporter

This comment has been minimized.

Show comment Hide comment
@ohporter

ohporter Jan 14, 2014

It definitely a bug...or incomplete implementation of the API. In order to add
some more background on how cs_change is supposed to behave, please
note this documentation from include/linux/spi/spi.h:

* All SPI transfers start with the relevant chipselect active.  Normally
* it stays selected until after the last transfer in a message.  Drivers
* can affect the chipselect signal using cs_change.
*
* (i) If the transfer isn't the last one in the message, this flag is
* used to make the chipselect briefly go inactive in the middle of the
* message.  Toggling chipselect in this way may be needed to terminate
* a chip command, letting a single spi_message perform all of group of
* chip transactions together.
*                                              
* (ii) When the transfer is the last one in the message, the chip may
* stay selected until the next transfer.  On multi-device SPI busses
* with nothing blocking messages going to other devices, this is just
* a performance hint; starting a message to another device deselects
* this one.  But in other cases, this can be used to ensure correctness.
* Some devices need protocol transactions to be built from a series of
* spi_message submissions, where the content of one message is determined
* by the results of previous messages and where the whole transaction
* ends when the chipselect goes inactive.

Case 1 is supported as seen in the driver code. It will deactivate the chip
select after each transfer in a message when cs_change is flagged. Case 2 is only
relevant on the last transfer in a message. In case 2, if cs_change is flagged
and the transfer is the last in a message, then the chip select shouldl be left active.

It definitely a bug...or incomplete implementation of the API. In order to add
some more background on how cs_change is supposed to behave, please
note this documentation from include/linux/spi/spi.h:

* All SPI transfers start with the relevant chipselect active.  Normally
* it stays selected until after the last transfer in a message.  Drivers
* can affect the chipselect signal using cs_change.
*
* (i) If the transfer isn't the last one in the message, this flag is
* used to make the chipselect briefly go inactive in the middle of the
* message.  Toggling chipselect in this way may be needed to terminate
* a chip command, letting a single spi_message perform all of group of
* chip transactions together.
*                                              
* (ii) When the transfer is the last one in the message, the chip may
* stay selected until the next transfer.  On multi-device SPI busses
* with nothing blocking messages going to other devices, this is just
* a performance hint; starting a message to another device deselects
* this one.  But in other cases, this can be used to ensure correctness.
* Some devices need protocol transactions to be built from a series of
* spi_message submissions, where the content of one message is determined
* by the results of previous messages and where the whole transaction
* ends when the chipselect goes inactive.

Case 1 is supported as seen in the driver code. It will deactivate the chip
select after each transfer in a message when cs_change is flagged. Case 2 is only
relevant on the last transfer in a message. In case 2, if cs_change is flagged
and the transfer is the last in a message, then the chip select shouldl be left active.

@plan44

This comment has been minimized.

Show comment Hide comment
@plan44

plan44 Jul 14, 2016

Thanks for this comment. Years later, it helped me to understand why RaspberryPi spi-bcm2735 driver did not work with my code.
It seems to me that the description of cs_change in spidev.h is misleading, as it just says:

  • @cs_change: True to deselect device before starting the next transfer.

This suggests that in order to make CS go inactive after a transfer (including the last in a message) cs_change should be set. The more thorough description in spi.h @ohporter quoted makes clear that it's exactly the opposite for the last transfer in a message.

Rpi's spi-bcm2708 driver apparently did not check the cs_change flag at all, so code using cs_change as suggested by spidev.h (user land /dev/spidev access) worked on spi-bcm2708 but not on spi-bcm2735.

A good example of someone running into this is this commit on github - where the original author set cs_change intending CS to go inactive after the transfer, see his comment. The commit just comments out touching cs_change, but with no explanation why.

Luckily, I eventually found this post!

plan44 commented Jul 14, 2016

Thanks for this comment. Years later, it helped me to understand why RaspberryPi spi-bcm2735 driver did not work with my code.
It seems to me that the description of cs_change in spidev.h is misleading, as it just says:

  • @cs_change: True to deselect device before starting the next transfer.

This suggests that in order to make CS go inactive after a transfer (including the last in a message) cs_change should be set. The more thorough description in spi.h @ohporter quoted makes clear that it's exactly the opposite for the last transfer in a message.

Rpi's spi-bcm2708 driver apparently did not check the cs_change flag at all, so code using cs_change as suggested by spidev.h (user land /dev/spidev access) worked on spi-bcm2708 but not on spi-bcm2735.

A good example of someone running into this is this commit on github - where the original author set cs_change intending CS to go inactive after the transfer, see his comment. The commit just comments out touching cs_change, but with no explanation why.

Luckily, I eventually found this post!

plan44 referenced this issue in vogelchr/sc16is17xx_linux_i2c_spi Jul 14, 2016

cs_change=1 didn't work with cs-gpio in newer bcm spi driver
With the change of the broadcom SPI driver to gpio-driven chipselects
(documented in  raspberrypi/linux#1003  )
this program stopped working. Commenting out this line seems to fix it.
@jadonk

This comment has been minimized.

Show comment Hide comment
@jadonk

jadonk Jul 17, 2016

Member

:-)

Member

jadonk commented Jul 17, 2016

:-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment