i2c: unable to configure SAMD51 i2c clock frequency for standard (100 KHz) speeds

[maxmclau]

Describe the bug
When trying to set i2c clock frequency for SAMDxx cores i2c_sam0_set_apply_bitrate() uses GCLK0 frequency for baud rate computation. On SAMD51 cores, typically clocked at 120MHz, this results in a baud range error. I believe this is incorrect for SAMD51 cores as they use a separate 48MHz peripheral clock for SERCOMs and should use this value for computing baud.

This is solved in the Adafruit ArduinoCore-samd implementation by checking for SAMD51 cores and calculating their rate off of the 48MHz peripheral clock.

To Reproduce

1. open SAMD51 based board (eg. adafruit_itsybitsy_m4_express.dts)
2. add atmel,sam0-i2c compatible SERCOM

...
&sercom2 {
	status = "okay";
	compatible = "atmel,sam0-i2c";
	clock-frequency = <I2C_BITRATE_STANDARD>;

	#address-cells = <1>;
	#size-cells = <0>;
}
...

3. compile and run

Expected behavior
i2c_sam0_set_apply_bitrate() should set i2c->BAUD.reg to a register value representing I2C_BITRATE_STANDARD (100KHz). Instead it returns -ERANGE as the computed baud is above 255.

Impact
There are still many i2c peripherals not compatible with <I2C_BITRATE_FAST> (400KHz).

Modifying the implementation to reflect something similar to the Adafruit implementation shouldn't be very difficult.

[nandojve]

Hi @maxmclau ,

Feel free to send a patch. Maybe we can have this fix for v3.0.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[joelguittet]

Hello
I confirm this bug. It is visible when the main CPU clock is too high (divider to get a 100kbits/s SCL clock is not computed and ERANGE error is returned).
Not sure how to exactly fix the issue today, but it is there (v3.3.0-rc3).
Joel

[nandojve]

Hi @joelguittet ,

Zephyr will be release in two days 17th Feb. In my case, I don`t have a board to test it and time to look at.
Would you able to send a patch until Friday morning? Worst case it is possible to backport fix.

[joelguittet]

Hello @nandojve

I will be happy to do it, but actually thinking to the right solution because it's a bit harder that a few lines playing with the baud rate generator probably...

A bit of details on this:

• the SERCOM engine is clocked from GCLK_SERCOMx_CORE or from an external clock, according to figure 33-2 in the datasheet: https://ww1.microchip.com/downloads/aemDocuments/documents/MCU32/ProductDocuments/DataSheets/SAM-D5x-E5x-Family-Data-Sheet-DS60001507.pdf
• the issue we are discussing is related to the frequency of GCLK_SERCOMx_CORE which is too much important on the board I have and on the adafruit itsybitsy m4 express: we have 120MHz (which seems out of range according to the same datasheet above - maximum is indicated to be 100MHz)
• the computation of the baud rate register value is provided at page 917 of the datasheet, and it is correctly implemented in i2c_sam0.c driver

Solution to use standard i2c speed would be:

• to decrease the CPU clock so that we are inside the wanted range - probably not what most of the users want to do
• using the external clock mentioned above to clock the baud rate generator. This is controlled by CTRLA.MODE[0]. BUT: mode is 0x5 to be an I2C master and it is not possible to change it.

If somebody can confirm/oppose some explanation to this details I have given I will be happy to discuss of that.
It seems decreasing the CPU clock is the only solution to enter in the wanted range of the baud rate generator.

Joel

PS: problem disappear when using 400kHz and more frequency for the clock because division to do is 4 time smaller.

[nandojve]

Hi @joelguittet ,

I think the problem is that the clock source for GCLK_SERCOMx_CORE is GCLK_PCHCTRL_GEN_GCLK0.

zephyr/drivers/i2c/i2c_sam0.c

Lines 686 to 699 in c9225e4

	#ifdef MCLK
	/* Enable the GCLK */
	GCLK->PCHCTRL[cfg->gclk_core_id].reg = GCLK_PCHCTRL_GEN_GCLK0 |
	GCLK_PCHCTRL_CHEN;
	/* Enable SERCOM clock in MCLK */
	*cfg->mclk |= cfg->mclk_mask;
	#else
	/* Enable the GCLK */
	GCLK->CLKCTRL.reg = cfg->gclk_clkctrl_id | GCLK_CLKCTRL_GEN_GCLK0 |
	GCLK_CLKCTRL_CLKEN;
	/* Enable SERCOM clock in PM */
	PM->APBCMASK.reg |= cfg->pm_apbcmask;
	#endif

The GCLK0 is defined as a high speed clock at

zephyr/soc/arm/atmel_sam0/common/soc_samd5x.c

Line 134 in c4c8583

gclk_connect(0, GCLK_SOURCE_DPLL0, dfll_div);

and because of that slow peripherals can't configure a proper clock.

On my view, the solution is define another generic clock source to be used by slow peripherals, like I2C. With that, you can switch the source in the driver. Problem is, this MUST be made for all SAM0 series, otherwise you will broke the driver. The selected GLKx to solve the issue must be shared between all series and should not be used yet, otherwise a refactor must be made in preparation to this change. I'll even suggest that GLKCx should have a period to allow slower speeds like 50 kbit, maybe 10 kbit, but allow high speeds like 3.4Mbit too.

So, I think change is not about reduce GCLK0 speed but apply a correct source of clock into peripheral.

[joelguittet]

Hello @nandojve

Not bad, I'm not used to work with Atmel devices to be honest, I just focused on the clock generators and realized we have 12 clocks generators available on SAMD51. GCLK0 is just the first one.

I have just realized we have GLKC2 configured at 48MHz on SAMD51:

zephyr/soc/arm/atmel_sam0/samd51/soc.h

Line 54 in 4d4e252

#define SOC_ATMEL_SAM0_GCLK2_FREQ_HZ 48000000

So I have replaced GCLK0 by GCLK2 in the i2c_sam0 driver : working, I can reach the 100kbits/s SCL clock! 400kbits/s and 1Mbits/s are also working. Not able to use 3.4Mbits/s with GCLK2 but I don't succeed even with GLKC0 (I don't know why at the moment).

So a solution can be to replace GLKC0 by GCLK2 in i2c_sam0.c

Problem: GCLK2 not defined for all SAM0 devices.

Some have GLKC1 or GLKC3 defined but not always the same clock frequency, for example:

zephyr/soc/arm/atmel_sam0/samd21/soc.h

Line 72 in 4d4e252

#define SOC_ATMEL_SAM0_GCLK3_FREQ_HZ SOC_ATMEL_SAM0_OSC8M_FREQ_HZ

The devices with GLKC2 defined are always defining this clock at 48MHz. Is it a convention in Zephyr ? In this case probably we can write something like that in the sam0 i2c driver:

#ifdef SOC_ATMEL_SAM0_GCLK2_FREQ_HZ
//use it
#else
//use GLKC0
#endif

I'm new to Zephyr and I already develop a small project, but not able to known if this is a good approach. You will probably indicate me the best is to define the clock in the dts? Do you have an example for that ? another simple Atmel driver doing this ?

Joel

PS: as you mentioned the modification will impact all sam0 devices so it's important to do it properly. I don't want to submit a PR that will break plenty of users :-)

[nandojve]

Hi @joelguittet ,

So I have replaced GCLK0 by GCLK2 in the i2c_sam0 driver : working, I can reach the 100kbits/s SCL clock! 400kbits/s and 1Mbits/s are also working. Not able to use 3.4Mbits/s with GCLK2 but I don't succeed even with GLKC0 (I don't know why at the moment).

Thank you for the info. With this info here at least people have a workaround till a proper solution is in place.

So a solution can be to replace GLKC0 by GCLK2 in i2c_sam0.c
Problem: GCLK2 not defined for all SAM0 devices.
Some have GLKC1 or GLKC3 defined but not always the same clock frequency, for example:

As I already mentioned, it will be necessary define a new GCLKx for the all series to have the correct solution.

The devices with GLKC2 defined are always defining this clock at 48MHz. Is it a convention in Zephyr ? In this case probably we can write something like that in the sam0 i2c driver:
I'm new to Zephyr and I already develop a small project, but not able to known if this is a good approach. You will probably indicate me the best is to define the clock in the dts? Do you have an example for that ? another simple Atmel driver doing this ?

Currently there is no convention on Zephyr and project should not impose restrictions. I see that complexity is growing and maybe at least a recommendation should be in place. This has became a problem because there is no clock drivers for Atmel SAM/SAM0 series.

On my perspective below is a good compromise:

• GCLK0 is usually connected to the high speed bus / main clock to feed CPU, which is probably a high speed clock.
• GCLK1 could be derived from GCLK0 using a divider. This could generate a base clock to be used for slower peripherals.
• GCLK2 is usually a slow clock, mostly of time 32768 Hz for RTC and LPM mode.

I think above can be be achieved but it will require mode investigation and it will be necessary changes in some series.

[TomzBench]

hello @nandojve ,

i just ran into this and i don't follow the solution here. i have a atsame54p20a and i am trying to use the slow clock which seems hard coded to fail with my chip so im not sure what to change

EDIT: so i have GCLK2 as 48M so i will give a try and just replace GCK0 with GCLK2 and see how it goes.

Why is this issue closed? it should be open i think

[joelguittet]

Was closed because it was Stale, but I agree it should remain opened. Any way to block the bot for this ?

[TomzBench]

a discord user mentioned #36649 as perhaps more relevant issue, which is open. so maybe that is sufficient

[nandojve]

Any way to block the bot for this ?

I don`t know, usually it should close PRs not issues.

Anyway, anyone is free to reopen when it is relevant. For a more short term, the solution should be something like #42432 (comment) .

Are there someone with free time to implement? Remember that Atmel is community driven.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.