2010-11-29 09:38:31 Bug in SPI slave chip select control
Andrew Rook (UNITED KINGDOM)
Message: 96289
drivers/spi/spi_bfin5xx.c in the Subversion trunk and in the 2010R1 branch has a bug (or unexpected feature) in its handling of SPI slave chip select signals, as explained below.
When bfin_spi_setup() runs for a SPI slave device, it ends by calling bfin_spi_cs_enable(), which will set the appropriate FSLx bit (0 to 7) of the SPI_FLG register if (chip->chip_select_num < MAX_CTRL_CS), i.e. if the device uses a SPI slave chip select pin and not a software-controlled GPIO pin.
Once set, the SPI_FLG FSLx bit stays set unless bfin_spi_cs_disable() clears it, which only happens if bfin_spi_cleanup() runs, or bfin_spi_suspend() runs.
Having the SPI_FLG FSLx bit set gives a problem if the SPI controller does a SPI access to another device with the SPI_CTL register bit CPHA set to 0, for SPI mode 0 or 2, as then the SPI controller pulses low all the SPISELx pins which
have SPI_FLG FSLx bits set.
As an example, assume 2 SPI slave devices,
First device uses chip->chip_select_num == MAX_CTRL_CS + GPIO_PF1 (BF533 SPISEL1/PF1 pin but using GPIO control) in SPI mode 0,
Second device uses chip->chip_select_num == 2 (SPISEL2 pin) in SPI mode 3.
Then accessing the first device in SPI mode 0 will give SPISEL1 going low due to software control of it and also SPISEL2 going low due to SPI hardware control as SPI_FLG will be 0xFF04 and SPI_CTL has CPHA == 0. Hence the second SPI device is also accessed, with the wrong write data and the wrong SPI mode for it, giving a driver conflict on the MISO wire and potentially wrong operation by the second device.
This problem was found on a custom board using the BF533, but it should also occur on any other type of Blackfin processor where SPI_CTL CPHA == 0 gives hardware control of the SPI slave select pins, with the SPI_FLG FLGx bits (8 to 15) having no effect when SPI_CTL CPHA == 0.
bfin_spi_setup() gives a warning message for any SPI slave device using SPI mode 0 or 2 (without SPI_CPHA) if (chip->chip_select_num < MAX_CTRL_CS), but that warning would not be generated in the example above.
A work-around is to use GPIO for all SPI slave chip selects when any SPI device uses SPI mode 0 or 2, as then none of the SPI_FLG FSLx bits will ever be set.
The 2009R1.1 release does not have this problem, so a real solution would be to remove functions bfin_spi_cs_enable() and bfin_spi_cs_disable(), with the appropriate SPI_FLG FSLx bit being set by bfin_spi_cs_active() and reset by
bfin_spi_cs_deactive(), like in drivers/spi/spi_bfin5xx.c in the 2009R1 branch (although struct chip_data .flag has a different meaning there).
There may also be a bug in bfin_spi_pump_transfers() unrelated to the above problem.
In the following code (line 621 onwards in the 2010R1 branch), there is nothing to do for this transfer when (transfer->len == 0), so bfin_spi_pump_transfers() possibly should return (as added below) rather than continuing to process the transfer.
if (transfer->len == 0) {
/* Move to next transfer of this msg */
message->state = bfin_spi_next_transfer(drv_data);
/* Schedule next transfer tasklet */
tasklet_schedule(&drv_data->pump_transfers);
return; /* This line possibly should be added. */
}
Andrew Rook
QuoteReplyEditDelete
2010-11-30 01:33:21 Re: Bug in SPI slave chip select control
Sonic Zhang (CHINA)
Message: 96305
>Having the SPI_FLG FSLx bit set gives a problem if the SPI controller does a SPI access to another device with the SPI_CTL register bit CPHA set to 0, for SPI mode 0 or 2, as then the SPI controller pulses low all the SPISELx pins which
have SPI_FLG FSLx bits set.
The Linux SPI bus framework assume the CS is active during a complete message transfer. So, using hardware CS (change CS per byte in a message) in SPI mode 0 or 2 on blackfin breaks the assumption and is invalid. The only valid approach to configure SPI slave device in mode 0 or 2 is to enable GPIO based software CS. If your SPI slave device asks for CS change per byte, don't use the Linux SPI framework driver. So, this is not a bug. I
> bfin_spi_setup() gives a warning message for any SPI slave device using SPI mode 0 or 2 (without SPI_CPHA) if (chip->chip_select_num < MAX_CTRL_CS), but that warning would not be generated in the example above.
Why do you think this warning wonld not be generated? It should, because chip->chip_select_num is less than MAX_CTRL_CS and spi->mode & SPI_CPHA is 0 in your above example.
I prefer to turn this warning into failure.
> if (transfer->len == 0) {
> /* Move to next transfer of this msg */
> message->state = bfin_spi_next_transfer(drv_data);
> /* Schedule next transfer tasklet */
> tasklet_schedule(&drv_data->pump_transfers);
> return; /* This line possibly should be added. */
> }
This looks like a real problem. Thanks.
Sonic
QuoteReplyEditDelete
2010-11-30 03:40:22 Re: Bug in SPI slave chip select control
Andrew Rook (UNITED KINGDOM)
Message: 96330
Regarding my example and the warning about using SPI mode 0 or 2,
First device uses chip->chip_select_num == MAX_CTRL_CS + GPIO_PF1 (BF533 SPISEL1/PF1 pin but using GPIO control) in SPI mode 0, so it does not get the warning as chip_select_num is >= MAX_CTRL_CS.
Second device uses chip->chip_select_num == 2 (SPISEL2 pin) in SPI mode 3, so it does not get the warning as it wants SPI mode 3.
The real problem is that the second device gets its FSL2 bit permanently set in the SPI_FLG register, which means that when the SPI controller is used to access the first device in SPI mode 0, the FLS2 bit means that the SPISEL2 slave chip select signal to the second device goes active, due to the SPI_CTL register having CPHA == 0. The driver software correctly sets the GPIO chip select to the first device active. Hence both devices are accessed at the same time.
The 2009R1.1 release does not have this problem as it only sets each SPI_FLAG FSLx bit active during a SPI access of the device.
Andrew Rook
QuoteReplyEditDelete
2010-11-30 05:12:03 Re: Bug in SPI slave chip select control
Sonic Zhang (CHINA)
Message: 96336
In your case, we recommend to use GPIO as CS for all SPI devices. Let me check if only enabling CS during SPI transfer breaks SPI flash devices.