Skip to content

SPIFBlockDevice doesn't play nice on shared SPI bus #11732. #12681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 1, 2020
Merged

SPIFBlockDevice doesn't play nice on shared SPI bus #11732. #12681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
116a8a7
[IOTSTOR-990] SPIFBlockDevice doesn't play nice on shared SPI bus. Fi…
Mar 24, 2020
699a689
[IOTSTOR-990] SPIFBlockDevice doesn't play nice on shared SPI bus. Ba…
Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 7 additions & 14 deletions components/storage/blockdevice/COMPONENT_DATAFLASH/DataFlashBlockDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,7 @@ DataFlashBlockDevice::DataFlashBlockDevice(PinName mosi,
PinName cs,
int freq,
PinName nwp)
: _spi(mosi, miso, sclk),
_cs(cs, 1),
: _spi(mosi, miso, sclk, cs, use_gpio_ssel),
_nwp(nwp),
_device_size(0),
_page_size(0),
Expand Down Expand Up @@ -331,8 +330,7 @@ int DataFlashBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)

uint8_t *external_buffer = static_cast<uint8_t *>(buffer);

/* activate device */
_cs = 0;
_spi.select();

/* send read opcode */
_spi.write(DATAFLASH_OP_READ_LOW_FREQUENCY);
Expand All @@ -352,8 +350,7 @@ int DataFlashBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)
external_buffer[index] = _spi.write(DATAFLASH_OP_NOP);
}

/* deactivate device */
_cs = 1;
_spi.deselect();

result = BD_ERROR_OK;
}
Expand Down Expand Up @@ -546,8 +543,7 @@ uint16_t DataFlashBlockDevice::_get_register(uint8_t opcode)
_mutex.lock();
DEBUG_PRINTF("_get_register: %" PRIX8 "\r\n", opcode);

/* activate device */
_cs = 0;
_spi.select();

/* write opcode */
_spi.write(opcode);
Expand All @@ -556,8 +552,7 @@ uint16_t DataFlashBlockDevice::_get_register(uint8_t opcode)
int status = (_spi.write(DATAFLASH_OP_NOP));
status = (status << 8) | (_spi.write(DATAFLASH_OP_NOP));

/* deactivate device */
_cs = 1;
_spi.deselect();

_mutex.unlock();
return status;
Expand All @@ -579,8 +574,7 @@ void DataFlashBlockDevice::_write_command(uint32_t command, const uint8_t *buffe
{
DEBUG_PRINTF("_write_command: %" PRIX32 " %p %" PRIX32 "\r\n", command, buffer, size);

/* activate device */
_cs = 0;
_spi.select();

/* send command (opcode with data or 4 byte command) */
_spi.write((command >> 24) & 0xFF);
Expand All @@ -595,8 +589,7 @@ void DataFlashBlockDevice::_write_command(uint32_t command, const uint8_t *buffe
}
}

/* deactivate device */
_cs = 1;
_spi.deselect();
}

/**
Expand Down
1 change: 0 additions & 1 deletion components/storage/blockdevice/COMPONENT_DATAFLASH/DataFlashBlockDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ class DataFlashBlockDevice : public mbed::BlockDevice {
private:
// Master side hardware
mbed::SPI _spi;
mbed::DigitalOut _cs;
mbed::DigitalOut _nwp;

// Device configuration
Expand Down
41 changes: 19 additions & 22 deletions components/storage/blockdevice/COMPONENT_SD/SDBlockDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,15 +252,14 @@ const uint32_t SDBlockDevice::_block_size = BLOCK_SIZE_HC;

#if MBED_CONF_SD_CRC_ENABLED
SDBlockDevice::SDBlockDevice(PinName mosi, PinName miso, PinName sclk, PinName cs, uint64_t hz, bool crc_on)
: _sectors(0), _spi(mosi, miso, sclk), _cs(cs), _is_initialized(0),
: _sectors(0), _spi(mosi, miso, sclk, cs, use_gpio_ssel), _is_initialized(0),
_init_ref_count(0), _crc_on(crc_on)
#else
SDBlockDevice::SDBlockDevice(PinName mosi, PinName miso, PinName sclk, PinName cs, uint64_t hz, bool crc_on)
: _sectors(0), _spi(mosi, miso, sclk), _cs(cs), _is_initialized(0),
: _sectors(0), _spi(mosi, miso, sclk, cs, use_gpio_ssel), _is_initialized(0),
_init_ref_count(0)
#endif
{
_cs = 1;
_card_type = SDCARD_NONE;

// Set default to 100kHz for initialisation and 1MHz for data transfer
Expand All @@ -274,15 +273,14 @@ SDBlockDevice::SDBlockDevice(PinName mosi, PinName miso, PinName sclk, PinName c

#if MBED_CONF_SD_CRC_ENABLED
SDBlockDevice::SDBlockDevice(const spi_pinmap_t &spi_pinmap, PinName cs, uint64_t hz, bool crc_on)
: _sectors(0), _spi(spi_pinmap), _cs(cs), _is_initialized(0),
: _sectors(0), _spi(spi_pinmap, cs), _is_initialized(0),
_init_ref_count(0), _crc_on(crc_on)
#else
SDBlockDevice::SDBlockDevice(const spi_pinmap_t &spi_pinmap, PinName cs, uint64_t hz, bool crc_on)
: _sectors(0), _spi(spi_pinmap), _cs(cs), _is_initialized(0),
: _sectors(0), _spi(spi_pinmap, cs), _is_initialized(0),
_init_ref_count(0)
#endif
{
_cs = 1;
_card_type = SDCARD_NONE;

// Set default to 100kHz for initialisation and 1MHz for data transfer
Expand Down Expand Up @@ -538,7 +536,7 @@ int SDBlockDevice::program(const void *b, bd_addr_t addr, bd_size_t size)
_spi.write(SPI_STOP_TRAN);
}

_deselect();
_postclock_then_deselect();
unlock();
return status;
}
Expand Down Expand Up @@ -585,7 +583,7 @@ int SDBlockDevice::read(void *b, bd_addr_t addr, bd_size_t size)
buffer += _block_size;
--blockCnt;
}
_deselect();
_postclock_then_deselect();

// Send CMD12(0x00000000) to stop the transmission for multi-block transfer
if (size > _block_size) {
Expand Down Expand Up @@ -753,7 +751,7 @@ int SDBlockDevice::_cmd(SDBlockDevice::cmdSupported cmd, uint32_t arg, bool isAc

// Select card and wait for card to be ready before sending next command
// Note: next command will fail if card is not ready
_select();
_preclock_then_select();

// No need to wait for card to be ready when sending the stop command
if (CMD12_STOP_TRANSMISSION != cmd) {
Expand Down Expand Up @@ -789,17 +787,17 @@ int SDBlockDevice::_cmd(SDBlockDevice::cmdSupported cmd, uint32_t arg, bool isAc

// Process the response R1 : Exit on CRC/Illegal command error/No response
if (R1_NO_RESPONSE == response) {
_deselect();
_postclock_then_deselect();
debug_if(SD_DBG, "No response CMD:%d response: 0x%" PRIx32 "\n", cmd, response);
return SD_BLOCK_DEVICE_ERROR_NO_DEVICE; // No device
}
if (response & R1_COM_CRC_ERROR) {
_deselect();
_postclock_then_deselect();
debug_if(SD_DBG, "CRC error CMD:%d response 0x%" PRIx32 "\n", cmd, response);
return SD_BLOCK_DEVICE_ERROR_CRC; // CRC error
}
if (response & R1_ILLEGAL_COMMAND) {
_deselect();
_postclock_then_deselect();
debug_if(SD_DBG, "Illegal command CMD:%d response 0x%" PRIx32 "\n", cmd, response);
if (CMD8_SEND_IF_COND == cmd) { // Illegal command is for Ver1 or not SD Card
_card_type = CARD_UNKNOWN;
Expand Down Expand Up @@ -857,7 +855,7 @@ int SDBlockDevice::_cmd(SDBlockDevice::cmdSupported cmd, uint32_t arg, bool isAc
return BD_ERROR_OK;
}
// Deselect card
_deselect();
_postclock_then_deselect();
return status;
}

Expand Down Expand Up @@ -908,7 +906,7 @@ int SDBlockDevice::_read_bytes(uint8_t *buffer, uint32_t length)
// read until start byte (0xFE)
if (false == _wait_token(SPI_START_BLOCK)) {
debug_if(SD_DBG, "Read timeout\n");
_deselect();
_postclock_then_deselect();
return SD_BLOCK_DEVICE_ERROR_NO_RESPONSE;
}

Expand All @@ -930,13 +928,13 @@ int SDBlockDevice::_read_bytes(uint8_t *buffer, uint32_t length)
if (crc_result != crc) {
debug_if(SD_DBG, "_read_bytes: Invalid CRC received 0x%" PRIx16 " result of computation 0x%" PRIx32 "\n",
crc, crc_result);
_deselect();
_postclock_then_deselect();
return SD_BLOCK_DEVICE_ERROR_CRC;
}
}
#endif

_deselect();
_postclock_then_deselect();
return 0;
}

Expand Down Expand Up @@ -1135,23 +1133,22 @@ void SDBlockDevice::_spi_init()
_spi.format(8, 0);
_spi.set_default_write_value(SPI_FILL_CHAR);
// Initial 74 cycles required for few cards, before selecting SPI mode
_cs = 1;
_spi_wait(10);
_spi.unlock();
}

void SDBlockDevice::_select()
void SDBlockDevice::_preclock_then_select()
{
_spi.lock();
_spi.write(SPI_FILL_CHAR);
_cs = 0;
_spi.select();
_spi.unlock();
}

void SDBlockDevice::_deselect()
void SDBlockDevice::_postclock_then_deselect()
{
_cs = 1;
_spi.write(SPI_FILL_CHAR);
_spi.unlock();
_spi.deselect();
}

#endif /* DEVICE_SPI */
7 changes: 2 additions & 5 deletions components/storage/blockdevice/COMPONENT_SD/SDBlockDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,11 +272,8 @@ class SDBlockDevice : public mbed::BlockDevice {
int _read_bytes(uint8_t *buffer, uint32_t length);
uint8_t _write(const uint8_t *buffer, uint8_t token, uint32_t length);
int _freq(void);

/* Chip Select and SPI mode select */
mbed::DigitalOut _cs;
void _select();
void _deselect();
void _preclock_then_select();
void _postclock_then_deselect();

virtual void lock()
{
Expand Down
23 changes: 9 additions & 14 deletions components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ SingletonPtr<PlatformMutex> SPIFBlockDevice::_mutex;
//***********************
SPIFBlockDevice::SPIFBlockDevice(PinName mosi, PinName miso, PinName sclk, PinName csel, int freq)
:
_spi(mosi, miso, sclk), _cs(csel), _prog_instruction(0), _erase_instruction(0),
_spi(mosi, miso, sclk, csel, use_gpio_ssel), _prog_instruction(0), _erase_instruction(0),
_page_size_bytes(0), _init_ref_count(0), _is_initialized(false)
{
_address_size = SPIF_ADDR_SIZE_3_BYTES;
Expand All @@ -110,7 +110,7 @@ SPIFBlockDevice::SPIFBlockDevice(PinName mosi, PinName miso, PinName sclk, PinNa
tr_error("SPI Set Frequency Failed");
}

_cs = 1;
_spi.deselect();
}

int SPIFBlockDevice::init()
Expand Down Expand Up @@ -470,8 +470,7 @@ spif_bd_error SPIFBlockDevice::_spi_send_read_command(int read_inst, uint8_t *bu
uint32_t dummy_bytes = _dummy_and_mode_cycles / 8;
int dummy_byte = 0;

// csel must go low for the entire command (Inst, Address and Data)
_cs = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be replaced with _spi.select()?
This also applies to the lines below...
Maybe there is a good reason, but I don't understand it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is write function:
int SPI::write(int value)
{
select();
int ret = spi_master_write(&_peripheral->spi, value);
deselect();
return ret;
}
so there is done select and deselect.
additionaly lock and unlock is made in select and unselect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True about select and deselect, but looking at the comment csel must go low for the entire command (Inst, Address and Data) flipping the chip select off and on in every write function actually changes the behaviour of the whole function here. Chip select isn't low for the entire command now. Was that intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i correct understand SPI is only interface between block device, so when we want write/ready to block device something we select cs and do operations on spi (one block device operation can take few spi actions). Function _

int DataFlashBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)

_ make few that operation and i cannot see reason to all time hold down cs (in my opinion cs should be down only when write spi operation).
@SeppoTakalo please review my understanding.

_spi.select();

// Write 1 byte Instruction
_spi.write(read_inst);
Expand All @@ -491,8 +490,8 @@ spif_bd_error SPIFBlockDevice::_spi_send_read_command(int read_inst, uint8_t *bu
buffer[i] = _spi.write(0);
}

// csel back to high
_cs = 1;
_spi.deselect();

return SPIF_BD_ERROR_OK;
}

Expand All @@ -519,8 +518,7 @@ spif_bd_error SPIFBlockDevice::_spi_send_program_command(int prog_inst, const vo
int dummy_byte = 0;
uint8_t *data = (uint8_t *)buffer;

// csel must go low for the entire command (Inst, Address and Data)
_cs = 0;
_spi.select();

// Write 1 byte Instruction
_spi.write(prog_inst);
Expand All @@ -540,8 +538,7 @@ spif_bd_error SPIFBlockDevice::_spi_send_program_command(int prog_inst, const vo
_spi.write(data[i]);
}

// csel back to high
_cs = 1;
_spi.deselect();

return SPIF_BD_ERROR_OK;
}
Expand All @@ -561,8 +558,7 @@ spif_bd_error SPIFBlockDevice::_spi_send_general_command(int instruction, bd_add
uint32_t dummy_bytes = _dummy_and_mode_cycles / 8;
uint8_t dummy_byte = 0x00;

// csel must go low for the entire command (Inst, Address and Data)
_cs = 0;
_spi.select();

// Write 1 byte Instruction
_spi.write(instruction);
Expand All @@ -583,8 +579,7 @@ spif_bd_error SPIFBlockDevice::_spi_send_general_command(int instruction, bd_add
// Read/Write Data
_spi.write(tx_buffer, (int)tx_length, rx_buffer, (int)rx_length);

// csel back to high
_cs = 1;
_spi.deselect();

return SPIF_BD_ERROR_OK;
}
Expand Down
2 changes: 0 additions & 2 deletions components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,6 @@ class SPIFBlockDevice : public mbed::BlockDevice {
private:
// Master side hardware
mbed::SPI _spi;
// Enable CS control (low/high) for SPI driver operations
mbed::DigitalOut _cs;

// Mutex is used to protect Flash device for some SPI Driver commands that must be done sequentially with no other commands in between
// e.g. (1)Set Write Enable, (2)Program, (3)Wait Memory Ready
Expand Down