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

[crabel99]

This PR supersedes #737, which can be closed.

There are a series of bugs relating to:

1. Setting of GCLK->PCHCTRL[clk_id].reg = GCLK_PCHCTRL_GEN_GCLK1_Val | (1 << GCLK_PCHCTRL_CHEN_Pos) to values that exceeded Table 54-6 (see also Table 14-9) values for clk_id =3 (FDPLL0 and FDPLL1 Reference clock frequency) by 3,750%. The code had originally missed in the datasheet where the CORE clock controls the SERCOM frequency, Section 36.5.3:

Two generic clocks are used by `SERCOM: GCLK_SERCOMx_CORE` and `GCLK_SERCOM_SLOW`. The core clock (`GCLK_SERCOMx_CORE`) can clock the I2C when working as a host. The slow clock (`GCLK_SERCOM_SLOW`) is required only for certain functions, e.g. SMBus timing. These two clocks must be configured and enabled in the Generic Clock Controller (`GCLK`) before using the I2C.

2. The calculation of baudrates particularly I2CM.BAUD.bit.BAUD and I2CM.BAUD.bit.HSBAUD that were not in accordance Section 36.6.4.2.1.1 and 36.6.4.2.1.2 for the SAM D5x/E5x and Section 28.6.2.4.1.1 and 28.6.4.2.1.2 for the SAM D21/DA1 chips.

3. It the clock of a SAM D5x/E5x was set during runtime from the default 48 MHz, there was a situation where the BAUD setting for UART and I2C was not set correctly and could have as much as 100% error between actual and desired bus baudrates.

4. There is an overflow condition in the calculation of BAUD and HSBAUD settings, recommend adding the following pseudo code to clamp I2C baudrates in initMasterWire(baudtate). With low enough baudrate it is possible to generate an overflow condition where the BAUD would be very low, e.g. 0x01 generating a baudrate that exceeds the maximum specified value.

[dhalbert]

@ladyada FYI tagging you on this PR and the correspond bug report #380.

[ladyada]

thanks for the detailed investigation & fixes! @hathach can you review this PR?

[crabel99]

@ladyada you are welcome. Found these issues while working on a refactor of the Wire library to support DMA. Once I get that working (again) and tested I'll post it as a separate PR.

[Copilot]

Pull request overview

This PR addresses critical bugs in the SERCOM peripheral driver for SAMD/SAME microcontrollers, specifically fixing clock frequency configuration issues and unifying baudrate calculations across SPI, UART, and I2C interfaces. The changes aim to correct clock source assignments that previously exceeded datasheet specifications and improve baudrate calculation accuracy across different runtime clock configurations.

Changes:

• Added SAME51/53/54 support alongside existing SAMD51 support throughout the codebase
• Unified baudrate calculations to use a consistent freqRef member variable instead of platform-specific macros
• Implemented getSercomFreqRef() to dynamically determine the correct reference frequency from hardware registers
• Modified clock source handling to prevent SERCOM from exceeding 100 MHz maximum by remapping FCPU requests to GCLK2
• Removed redundant slow clock configuration and added clamping logic for I2C baudrates

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
cores/arduino/SERCOM.h	Added getSercomFreqRef() declaration, moved freqRef outside conditional compilation, extended SAME5x support in conditionals
cores/arduino/SERCOM.cpp	Unified baudrate calculations using freqRef, implemented dynamic frequency detection, extended SAME5x support, added I2C baudrate clamping

Comments suppressed due to low confidence (1)

cores/arduino/SERCOM.cpp:49

• The clockSource member variable is not initialized if none of the preprocessor conditions match (e.g., when SERCOM_SPI_FREQ_REF is not equal to F_CPU, 48000000, or 100000000). This leaves clockSource uninitialized, which can lead to undefined behavior when it's later used in setClockSource. Consider adding a default initialization or an else clause to set a default value like SERCOM_CLOCK_SOURCE_48M.

#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
  // not exceed the standard 24 MHz setting.  Newer code, if it needs faster
  // write-only SPI (e.g. to screen), should override the SERCOM clock on a
  // 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_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
  clockSource = SERCOM_CLOCK_SOURCE_100M;
 #endif

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In the getSercomFreqRef function, case 0 sets freqRef to 100000000UL, but according to the comment at line 839, GCLK0 is F_CPU which is 120 MHz, not 100 MHz. This inconsistency could lead to incorrect baudrate calculations. If GCLK0 is truly F_CPU, this should be freqRef = F_CPU or 120000000UL. However, if the intention is to cap it at 100 MHz as a safety measure (as suggested by line 846's comment), this logic should be clarified with a comment explaining the intentional discrepancy.

Suggested change

	freqRef = 100000000UL;
	// GCLK0 is the main CPU clock; use F_CPU to avoid hardcoding a specific value
	freqRef = F_CPU;

[crabel99]

freqRef is the reference frequency for a SERCOM channel. It has a maximum value of 100MHz if a user sets freqRef = F_CPU at run time this will throw off the baud rate calculations due to the maximum setting for the SERCOM clock reference to 100MHz, GLCK2, if GCLK0 is selected. This code needs to remain as is to match the restrictions on clock selection.

[Copilot]

The SPEED bits are being read at line 481 before they are set. The CTRLA register is set at line 471 but doesn't set the SPEED field. This means speedBit will be 0 (the reset default) and not the intended speed mode, which could lead to incorrect I2C speed configuration and baudrate calculation. The SPEED bits should be set in CTRLA before reading them, or the intended speed mode should be passed as a parameter to initMasterWIRE.

[crabel99]

Copilot is correct about the placement of setting speedBit. It needs to be done before it is used. This comment brings up two additional issues:

1. Dynamic setting of SPEED

The initMasterWIRE(baudrate) API sets the BAUD register based on how the registers are configured and baudrate. The setting of the two SPEED bits is a different programatic change in the API than what this bug fix is. I wanted to keep the code constrained.

If you wanted to change the API's function and dynamically assign the SPEED based on baudrate, replace int8_t speedBit = sercom->I2CM.CTRLA.bit.SPEED; with:

 // Determine speed mode based on requested baudrate
  uint32_t topSpeeds[3] = {400000, 1000000, 3400000}; // {(sm/fm), (fm+), (hs)}
  uint8_t speedBit = 0;
  bool clockStretchMode = false;
  sercom->I2CM.CTRLA.bit.SCLSM = 0; // See: 28.6.2.4.6

  if (baudrate > topSpeeds[0] && baudrate <= topSpeeds[1])
    {
    speedBit = 1; // Fast mode+
    clockStretchMode = false; // See: 28.6.2.4.6
    }
  else if (baudrate > topSpeeds[1])
    {
    speedBit = 2; // High speed
    clockStretchMode = true; // See: 28.6.2.4.6
    }
  // else speedBit = 0 and clockStretchMode = false for Standard/Fast mode (up to 400kHz)
  
  sercom->I2CM.CTRLA.bit.SPEED = speedBit;
  sercom->I2CM.CTRLA.bit.SCLSM = clockStretchMode; // See: 28.6.2.4.6

But, you need to be careful as (Hs-mode) requires very specific register configurations that need to be set.

• Requires an API to configure I2CS.CTRLA.bit.SPEED I2CS.CTRLA.bit.SCLSM for High-speed mode Client operations 28.6.2.5 and 28.6.2.5.4.
• I have not reviewed how the transfers for both read and write in Host/Client are done, nor tested these in High-speed or even Fast-mode +. These changes correctly activate full dynamic control of all transfer rates, but whether they work or not is another issue.
• I2CM.ADDR.bit.HS needs to be set to enable High-speed transfers in Host mode 28.6.2.4.6 and 28.10.9. This would need to be done when the address is written to ADDR. See:

bool SERCOM::startTransmissionWIRE(uint8_t address, SercomWireReadWriteFlag flag)
{
  ...
  sercom->I2CM.ADDR.reg |= address | ((sercom->I2CM.CTRLA.bit.SPEED == 0x2) << 14);
  ...
}

2. Error in the PR variable specification

The snippet Copilot provided should be freqRef not SystemCoreClock:

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

Edits

• Added comment about the status of the implementation of dynamic SPEED control.

[hathach]

I think we should just keep the API and use baudrate to also set the SPEED bit accordingly to the topspeed e.g < 400khz, speed=0, <1Mhz speed =1, otherwise speed =2. and also configure other register if neccessary. Would mind updating the PR to do so if possible. Don't worry about doing actual speed test for this function, since there maybe other factor as well.

[hathach]

thanks for the detailed investigation & fixes! @hathach can you review this PR?

reviewing it just now, will merge this soon enough :)