adafruit / ArduinoCore-samd

114 stars 116 forks source link

Actual SPI speed can be higher than maxSpeed requested in SPISettings #334

Open greiman opened 1 year ago

greiman commented 1 year ago

The SAMD SPI library has several problems with setting SPI speed. I found these problems while using a logic analyzer to investigate a problems reported by a user in my SdFat library.

  1. The actual speed is greater than or equal to the maxSpeed requested in SPISettings(maxSpeed, dataOrder, spiMode). For example if maxSpeed is 13 MHZ, the actual result is 24 MHz. The actual speed is only correct if maxSpeed exactly divides 24 MHz.

  2. If the requested speed is less than about 94 kHz, divisor overflow happens for the 8-bit SERCOM BAUD register.

  3. SAMD51 seems to work at 24 MHz but SAMD21 should be limited to 12 MHz max see: https://microchipsupport.force.com/s/article/SPI-max-clock-frequency-in-SAMD-SAMR-devices

The problems are in uint8_t SERCOM::calculateBaudrateSynchronous(uint32_t baudrate) at about line 389 of this file:

https://github.com/adafruit/ArduinoCore-samd/blob/master/cores/arduino/SERCOM.cpp

I tried the following fix but only did minimal tests on a Feather M0 SAMD21 and a Feather M4 Express SAMD51.

#define USE_SPI_BAUD_FIX
#ifdef USE_SPI_BAUD_FIX
// This will return a divisor such that the actual SPI rate is <= baudrate. 
uint8_t SERCOM::calculateBaudrateSynchronous(uint32_t baudrate) {
  if (baudrate == 0) {
    // return max 8-bit divisor for minimum speed.
    return 255;
  }
#if defined(__SAMD51__)
  // This will calculate the equivalent of (uint32_t)ceil((double)freqRef/(2*baudrate)) - 1;
  uint32_t b = (freqRef - 1) / (2 * baudrate);
#else
  // Limit SAMD21 to 12 MHz max.  SAMD51 seems to work at 24MHz.
  // https://microchipsupport.force.com/s/article/SPI-max-clock-frequency-in-SAMD-SAMR-devices
  if (baudrate > 12000000UL) {
    baudrate = 12000000UL;
  }
  uint32_t b = (SERCOM_SPI_FREQ_REF - 1) / (2 * baudrate);
#endif
  // Handle divisor overflow.
  return b > 255 ? 255 : b;
}
#else  // USE_SPI_BAUD_FIX
// Current version of the function.
uint8_t SERCOM::calculateBaudrateSynchronous(uint32_t baudrate) {
#if defined(__SAMD51__)
  uint16_t b = freqRef / (2 * baudrate);
#else
  uint16_t b = SERCOM_SPI_FREQ_REF / (2 * baudrate);
#endif
  if(b > 0) b--; // Don't -1 on baud calc if already at 0
  return b;
}
#endif  // USE_SPI_BAUD_FIX
caternuson commented 1 year ago

Just ran into this also testing a simple SD sketch on an Itsy M0. Using the Adafruit fork of the SDFat library: https://github.com/adafruit/SdFat but could also recreate same issue testing with latest upstream repo release: https://github.com/greiman/SdFat

Basic test sketch:

#include <SPI.h>
#include "SdFat.h"

const uint8_t SD_CHIP_SELECT_PIN = 7;

SdFat sd;

void setup() {
  Serial.begin(115200);
  while (!Serial);
  Serial.println("SDFat Hardware SPI test.");
  Serial.println();

  if (!sd.begin(SD_CHIP_SELECT_PIN) ) {
    Serial.println("Could not init SD card.");
    while (1);
  }

  Serial.println("SD card init success.");
}

void loop() {
}

results in this output:

SDFat Hardware SPI test.

Could not init SD card.

Forcing a lower clock gets it working:

  if (!sd.begin(SD_CHIP_SELECT_PIN, SD_SCK_MHZ(12)) ) {

results in this output:

SDFat Hardware SPI test.

SD card init success.

Arduino SAMD BSP 1.8.13 Adafruit SAMD BSP 1.7.10