Skip to content

Resolve a set of critical bugs in SERCOM and unify SPI/UART/I2C baudrate calculations #381

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

Open
crabel99 wants to merge 2 commits into
base: master
Choose a base branch
from
+77 −45
Open

Resolve a set of critical bugs in SERCOM and unify SPI/UART/I2C baudrate calculations #381

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
e0a883c
Unify SERCOM clock frequency handling across UART/SPI/WIRE
crabel99 Jan 13, 2026
d2772f8
bug: resolve low speed I2C BAUD register overflow
crabel99 Jan 13, 2026
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
115 changes: 73 additions & 42 deletions cores/arduino/SERCOM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ SERCOM::SERCOM(Sercom* s)
{
sercom = s;

#if defined(__SAMD51__)
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
// A briefly-available but now deprecated feature had the SPI clock source
// set via a compile-time setting (MAX_SPI)...problem was this affected
// ALL SERCOMs, whereas some (anything read/write, e.g. SD cards) should
Expand All @@ -40,8 +40,8 @@ SERCOM::SERCOM(Sercom* s)
// per-peripheral basis. Nonetheless, we check SERCOM_SPI_FREQ_REF here
// (MAX_SPI * 2) to retain compatibility with any interim projects that
// might have relied on the compile-time setting. But please, don't.
#if SERCOM_SPI_FREQ_REF == F_CPU // F_CPU clock = GCLK0
clockSource = SERCOM_CLOCK_SOURCE_FCPU;
#if SERCOM_SPI_FREQ_REF == F_CPU // F_CPU clock = GCLK0
clockSource = SERCOM_CLOCK_SOURCE_100M;
#elif SERCOM_SPI_FREQ_REF == 48000000 // 48 MHz clock = GCLK1 (standard)
clockSource = SERCOM_CLOCK_SOURCE_48M;
#elif SERCOM_SPI_FREQ_REF == 100000000 // 100 MHz clock = GCLK2
Expand Down Expand Up @@ -80,11 +80,7 @@ void SERCOM::initUART(SercomUartMode mode, SercomUartSampleRate sampleRate, uint
// Asynchronous fractional mode (Table 24-2 in datasheet)
// BAUD = fref / (sampleRateValue * fbaud)
// (multiply by 8, to calculate fractional piece)
#if defined(__SAMD51__)
uint32_t baudTimes8 = (SERCOM_FREQ_REF * 8) / (sampleRateValue * baudrate);
#else
uint32_t baudTimes8 = (SystemCoreClock * 8) / (sampleRateValue * baudrate);
#endif
uint32_t baudTimes8 = (freqRef * 8) / (sampleRateValue * baudrate);

sercom->USART.BAUD.FRAC.FP = (baudTimes8 % 8);
sercom->USART.BAUD.FRAC.BAUD = (baudTimes8 / 8);
Expand Down Expand Up @@ -230,8 +226,8 @@ void SERCOM::initSPI(SercomSpiTXPad mosi, SercomRXPad miso, SercomSpiCharSize ch
resetSPI();
initClockNVIC();

#if defined(__SAMD51__)
sercom->SPI.CTRLA.reg = SERCOM_SPI_CTRLA_MODE(0x3) | // master mode
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
sercom->SPI.CTRLA.reg = SERCOM_SPI_CTRLA_MODE(0x3) | // master mode
SERCOM_SPI_CTRLA_DOPO(mosi) |
SERCOM_SPI_CTRLA_DIPO(miso) |
dataOrder << SERCOM_SPI_CTRLA_DORD_Pos;
Expand Down Expand Up @@ -322,13 +318,7 @@ SercomDataOrder SERCOM::getDataOrderSPI()
void SERCOM::setBaudrateSPI(uint8_t divider)
{
disableSPI(); // Register is enable-protected

#if defined(__SAMD51__)
sercom->SPI.BAUD.reg = calculateBaudrateSynchronous(freqRef / divider);
#else
sercom->SPI.BAUD.reg = calculateBaudrateSynchronous(SERCOM_SPI_FREQ_REF / divider);
#endif

enableSPI();
}

Expand Down Expand Up @@ -386,13 +376,11 @@ bool SERCOM::isDataRegisterEmptySPI()
// return sercom->SPI.INTFLAG.bit.RXC;
//}

uint8_t SERCOM::calculateBaudrateSynchronous(uint32_t baudrate) {
#if defined(__SAMD51__)
uint8_t SERCOM::calculateBaudrateSynchronous(uint32_t baudrate)
{
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
if (b > 0)
b--; // Don't -1 on baud calc if already at 0
return b;
}

Expand Down Expand Up @@ -490,12 +478,16 @@ void SERCOM::initMasterWIRE( uint32_t baudrate )
// Enable all interrupts
// sercom->I2CM.INTENSET.reg = SERCOM_I2CM_INTENSET_MB | SERCOM_I2CM_INTENSET_SB | SERCOM_I2CM_INTENSET_ERROR ;

// Synchronous arithmetic baudrate
#if defined(__SAMD51__)
sercom->I2CM.BAUD.bit.BAUD = SERCOM_FREQ_REF / ( 2 * baudrate) - 1 ;
#else
sercom->I2CM.BAUD.bit.BAUD = SystemCoreClock / ( 2 * baudrate) - 5 - (((SystemCoreClock / 1000000) * WIRE_RISE_TIME_NANOSECONDS) / (2 * 1000));
#endif
uint8_t speedBit = sercom->I2CM.CTRLA.bit.SPEED;
uint32_t topSpeeds[3] = {400000, 1000000, 3400000}; // {(sm/fm), (fm+), (hs)}
const uint32_t minBaudrate = freqRef / 512; // BAUD = 255: SAMD51 ~195kHz, SAMD21 ~94kHz
const uint32_t maxBaudrate = topSpeeds[speedBit];
baudrate = max(minBaudrate, min(baudrate, maxBaudrate));

if (speedBit == 0x2)
sercom->I2CM.BAUD.bit.HSBAUD = freqRef / (2 * baudrate) - 1;
else
sercom->I2CM.BAUD.bit.BAUD = freqRef / (2 * baudrate) - 5 - freqRef * WIRE_RISE_TIME_NANOSECONDS / (2 * 1e9f);
}

void SERCOM::prepareNackBitWIRE( void )
Expand Down Expand Up @@ -727,7 +719,7 @@ uint8_t SERCOM::readDataWIRE( void )
}
}

#if defined(__SAMD51__)
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)

static const struct {
Sercom *sercomPtr;
Expand Down Expand Up @@ -785,7 +777,47 @@ int8_t SERCOM::getSercomIndex(void) {
return -1;
}

#if defined(__SAMD51__)
uint32_t SERCOM::getSercomFreqRef(void)
{
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
int8_t idx = getSercomIndex();
uint8_t gen = 1; // default to GCLK1 (48 MHz) if we can't resolve

if (idx >= 0)
{
uint8_t pch = sercomData[idx].id_core;
gen = GCLK->PCHCTRL[pch].bit.GEN;
}

switch (gen)
{
case 0:
freqRef = 100000000UL;
break;
case 1:
freqRef = 48000000UL;
break;
case 2:
freqRef = 100000000UL;
break;
case 3:
freqRef = 32768UL;
break;
case 4:
freqRef = 12000000UL;
break;
default:
freqRef = 48000000UL;
break;
}
#else
freqRef = SystemCoreClock;
#endif

return freqRef;
}

#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
// This is currently for overriding an SPI SERCOM's clock source only --
// NOT for UART or WIRE SERCOMs, where it will have unintended consequences.
// It does not check.
Expand All @@ -804,16 +836,19 @@ void SERCOM::setClockSource(int8_t idx, SercomClockSource src, bool core) {
if(core) clockSource = src; // Save SercomClockSource value

// From cores/arduino/startup.c:
// GCLK0 = F_CPU
// GCLK0 = F_CPU (this is 120 MHz and exceeds SERCOM maximum)
// GCLK1 = 48 MHz
// GCLK2 = 100 MHz
// GCLK3 = XOSC32K
// GCLK4 = 12 MHz
if(src == SERCOM_CLOCK_SOURCE_FCPU) {
GCLK->PCHCTRL[clk_id].reg =
GCLK_PCHCTRL_GEN_GCLK0_Val | (1 << GCLK_PCHCTRL_CHEN_Pos);
if(core) freqRef = F_CPU; // Save clock frequency value
} else if(src == SERCOM_CLOCK_SOURCE_48M) {
GCLK_PCHCTRL_GEN_GCLK2_Val | (1 << GCLK_PCHCTRL_CHEN_Pos); // Guard Sercom from exceeding 100 MHz maximum
if (core)
freqRef = 100000000; // Save clock frequency value
}
else if (src == SERCOM_CLOCK_SOURCE_48M)
{
GCLK->PCHCTRL[clk_id].reg =
GCLK_PCHCTRL_GEN_GCLK1_Val | (1 << GCLK_PCHCTRL_CHEN_Pos);
if(core) freqRef = 48000000;
Expand All @@ -840,21 +875,15 @@ void SERCOM::initClockNVIC( void )
int8_t idx = getSercomIndex();
if(idx < 0) return; // We got a problem here

#if defined(__SAMD51__)
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)

for(uint8_t i=0; i<4; i++) {
NVIC_ClearPendingIRQ(sercomData[idx].irq[i]);
NVIC_SetPriority(sercomData[idx].irq[i], SERCOM_NVIC_PRIORITY);
NVIC_EnableIRQ(sercomData[idx].irq[i]);
}

// SPI DMA speed is dictated by the "slow clock" (I think...maybe) so
// BOTH are set to the same clock source (clk_slow isn't sourced from
// XOSC32K as in prior versions of SAMD core).
// This might have power implications for sleep code.

setClockSource(idx, clockSource, true); // true = core clock
setClockSource(idx, clockSource, false); // false = slow clock
setClockSource(idx, clockSource, true); // true = core clock

#else // end if SAMD51 (prob SAMD21)

Expand All @@ -875,4 +904,6 @@ void SERCOM::initClockNVIC( void )
while(GCLK->STATUS.reg & GCLK_STATUS_SYNCBUSY); // Wait for synchronization

#endif // end !SAMD51

getSercomFreqRef();
}
7 changes: 4 additions & 3 deletions cores/arduino/SERCOM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ class SERCOM
int availableWIRE( void ) ;
uint8_t readDataWIRE( void ) ;
int8_t getSercomIndex(void);
uint32_t getSercomFreqRef(void);
#if defined(__SAMD51__)
// SERCOM clock source override is only available on
// SAMD51 (not 21) ... but these functions are declared
Expand All @@ -254,10 +255,10 @@ class SERCOM
#endif

private:
Sercom* sercom;
#if defined(__SAMD51__)
Sercom *sercom;
uint32_t freqRef = 48000000; // Frequency corresponding to clockSource
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
SercomClockSource clockSource;
uint32_t freqRef; // Frequency corresponding to clockSource
#endif
uint8_t calculateBaudrateSynchronous(uint32_t baudrate);
uint32_t division(uint32_t dividend, uint32_t divisor) ;
Expand Down