stm32f0/i2c.c: calculate correct bus frequency

[nefelim4ag]

Right now i2c frequency is hardcoded, I think for stm32f0.
STM32f0 pclk is 48Mhz.
My STM32H7 has 400Mhz -> 100Mhz pclk.
it runs faster at around 200kHz
https://klipper.discourse.group/t/stm32h7-wrong-i2c-timings-200khz/18676

>>> 44 / 9
4.88
>>> 1 / 0.0000048
208333.33
>>> 208333.33 / 100 * 48
99999.99

I added some actual timing calculations, but I can't get them precise.
100kHz, 1 / 0.0000085= 117647

400kHz, 1 / 0.0000025 = 400000

1000kHz, 1 / 0.0000013 = 769230

This is as close, as I can get to target values.

[bevanweiss]

I think for the same rate selection, it should result in identical SCL/SDA timings as currently exists (i.e. at 100kHz i2c clk, the SCL and SDA should be the same... I do understand this seeks to 'fix' devices which don't current output 100kHz where expected.. but changing ALL SDA/SCL timings seems unnecessary to achieve this outcome).

It would also need a check for if get_pclock_frequency(i2c) works for all of the relevant devices that use this function, and across the various i2c options on them:

• STM32F0
• STM32F7
• STM32G0
• STM32G4
• STM32H7
• STM32L4

[nefelim4ag]

I think for the same rate selection, it should result in identical SCL/SDA timings as currently exists (i.e. at 100kHz i2c clk, the SCL and SDA should be the same... I do understand this seeks to 'fix' devices which don't current output 100kHz where expected.. but changing ALL SDA/SCL timings seems unnecessary to achieve this outcome).

We can't reuse values, they are too low, for high-frequency peripheral clocks.
Like we have 4 bit presc and pclk one of 48Mhz, 64Mhz (stm32g0), 100Mhz (stm32h7/f7), 400Mhz (stm32g4/stm32L4).

Old values adds up like (4+1) + 2 + (15+1) + (19+1) = 43
48_000_000 / 100_000 / 0xB (11) ~= 43

There is no room to change presc to match anything above 64Mhz.
64_000_000 / 100_000 / 15 = 42.66

So, as a consequence of the hacky formula here, it just changed.
Looks like I messed up scldel/sdadel, because 255 / 10 > 15, I will fix that

I do not see a way to match existing timings precisely, but indeed - I can try "be close" to them.

I can do #ifdef f0 and leave old behaviour for this specific chip, and new for all others.

It would also need a check for if get_pclock_frequency(i2c) works for all of the relevant devices that use this function, and across the various i2c options on them:

• STM32F0
• STM32F7
• STM32G0
• STM32G4
• STM32H7
• STM32L4

I literally did it grep -A3 -R get_pclock_frequency\(uint

src/stm32/stm32g4.c:get_pclock_frequency(uint32_t periph_base)
src/stm32/stm32g4.c-{
src/stm32/stm32g4.c-    return FREQ_PERIPH;
src/stm32/stm32g4.c-}

And checked all the constants here.

• STM32F0 - 48000000
• STM32F7 - 400000000 / 4
• STM32G0 - 64000000
• STM32G4 - 400000000 / 1
• STM32H7 - 400000000 / 4
• STM32L4 - 400000000 / 1

---

Thanks

[bevanweiss]

My comment in the discourse was:

But I suspect a number of MCUs don’t have accurate logic to identify the peripheral clock speed… so that would be tricky.

I can assure you that the STM32L4 with a max CPU clock of 80MHz is NOT running the peripheral clock at 400MHz.
Nor is the STM32G4 with its max CPU clock of 170MHz.

• STM32F0 - 48000000
• STM32F7 - 400000000 / 4
• STM32G0 - 64000000
• STM32G4 - 400000000 / 1
• STM32H7 - 400000000 / 4
• STM32L4 - 400000000 / 1

I would expect the majority of the Peripheral Clocks configured for the I2C are between 8MHz and 48MHz (some of the F7/H7 high speed devices might hit 64MHz). If using a calculated prescaler, it will be important to confirm that the peripheral clocks are correct (since you would be basing the prescaler from that... whilst currently it's fixed).

From the STM32G4 doco there is a set of tables like this, the STM32H7 doco has almost an identical set of tables

I think it would be advised to keep general timing in line with the STMicro doco recommendations. So for 100kHz the low and high times should match what they are currently configured for.. and scaled in an equivalent ratio to be very close to the STMicro guidance at other bus speeds. It almost seems that a switch case could be done for the various low/high/delay times, and then the prescaler could be calculated from the residual, prescaler = periph_clock / (desired_rate * (SCLL+1) + (SCLH+1)) - 1;
and you'd want to round the division up so that you're not having a higher frequency than requested (to err on the safe side).
If the periph_clock is really too high that the prescaler can't accommodate the rate, then you could send a warning message back to klippy so that at least it's captured in the logs (and have the prescaler clamped at the max for that IC)

[nefelim4ag]

is NOT running the peripheral clock at 400MHz.

I sprinkle ashes on my head, I forgot that make reload some headers, and they will change between MCUs.
There are wrong assumptions from my side.

I cannot find that table, It is not included in the datasheet.
Does this document have another name?

I think I have two ways right now, one is just to specifically fix stm32h7 (plan B) or try to recalculate all timings according to the table above (plan A).

BTW, Thank you for your help.

---

I did like that and it just doesn't provide correct timings.
Something is wrong, looks like the timer is faster than I expected, or mathematics doesn't add up.

So, scaling original values, give bad result:

        uint32_t presc;
        uint32_t sdadel, scldel, scll, sclh;
        if (rate >= 1000000) {
            presc = (0x5 + 1) * 2 - 1;
            sdadel = 0x0;
            scldel = 0x1;
            scll = 0x3;
            sclh = 0x1;
        } else if (rate >= 400000) { // 2.2us instead of 2.5us
            presc = (0x5 + 1) * 2 - 1;
            sdadel = 0x3;
            scldel = 0x3;
            scll = 0x9;
            sclh = 0x3;
        } else {
            presc = 0xB;
            sdadel = 0x2 * 2;
            scldel = (0x4 + 1) * 2 - 1;
            scll = (0x13 + 1) * 2 - 1;
            sclh = (0xF + 1) * 2 - 1;
        }

This one should just work, but it also gives us wrong timings.

        uint32_t presc = 0;
        uint32_t scaled_pclk = pclk;
        // Minimal pclk 48Mhz stm32f0
        while (DIV_ROUND_UP(scaled_pclk, rate) > 120) {
            presc += 1;
            scaled_pclk = DIV_ROUND_CLOSEST(pclk, presc + 1);
        }
        uint32_t total_cycles = scaled_pclk / rate;
        uint32_t sdadel, scldel, scll, sclh;
        if (rate >= 1000000) { // 1.4us
            // total cycles is ~875ns
            sdadel = 0;
            scldel = DIV_ROUND_CLOSEST(total_cycles * 250, 875) - 1;
            scll = DIV_ROUND_CLOSEST(total_cycles * 500, 875) - 1;
            sclh = DIV_ROUND_CLOSEST(total_cycles * 250, 875) - 1;
        } else if (rate >= 400000) { // 2.3us instead of 2.5us
            // total cycles is ~2.5us
            sdadel = DIV_ROUND_CLOSEST(total_cycles * 375, 2500);
            scldel = DIV_ROUND_CLOSEST(total_cycles * 500, 2500) - 1;
            scll = DIV_ROUND_CLOSEST(total_cycles * 1250, 2500) - 1;
            sclh = DIV_ROUND_CLOSEST(total_cycles * 500, 2500) - 1;
        } else {
            // total cycles is ~10us
            sdadel = DIV_ROUND_CLOSEST(total_cycles * 500, 10000);
            scldel = DIV_ROUND_CLOSEST(total_cycles * 1250, 10000) - 1;
            scll = DIV_ROUND_CLOSEST(total_cycles * 5000, 10000) - 1;
            sclh = DIV_ROUND_CLOSEST(total_cycles * 4000, 10000) - 1;
        }

[nefelim4ag]

Plan B:
STM32H7 100kHz

STM32H7 400kHz

STM32H7 1MHz

If we want, we can add +1 to SCLL, to be on safe side here.

[bevanweiss]

I cannot find that table, It is not included in the datasheet. Does this document have another name?

The datasheets are very much summary data for the functions available, not so much how to get that functionality working.
If selecting a part you'd consult the 'datasheet'. If writing code / designing for a part, you'd generally use the reference manual, or the programming manual.
https://www.st.com/resource/en/reference_manual/rm0440-stm32g4-series-advanced-armbased-32bit-mcus-stmicroelectronics.pdf (pg 1903)
https://www.st.com/resource/en/reference_manual/rm0468-stm32h723733-stm32h725735-and-stm32h730-value-line-advanced-armbased-32bit-mcus-stmicroelectronics.pdf (pg 2021)

[bevanweiss]

The code itself looks good, a small step in the direction of 'better' from what was there before :)
nit would be that an enum could have been used instead of the magic numbers for the array indexes. i.e. to call out that [0] = 100kHz, [1] = 400kHz, [2] = 1000kHz

[KevinOConnor]

What about something like this:

--- a/src/stm32/stm32f0_i2c.c
+++ b/src/stm32/stm32f0_i2c.c
@@ -148,11 +148,26 @@ i2c_setup(uint32_t bus, uint32_t rate, uint8_t addr)
         gpio_peripheral(ii->sda_pin, ii->function | GPIO_OPEN_DRAIN, 1);
 
         // Set 100Khz frequency and enable
-        i2c->TIMINGR = ((0xB << I2C_TIMINGR_PRESC_Pos)
-                        | (0x13 << I2C_TIMINGR_SCLL_Pos)
-                        | (0xF << I2C_TIMINGR_SCLH_Pos)
-                        | (0x2 << I2C_TIMINGR_SDADEL_Pos)
-                        | (0x4 << I2C_TIMINGR_SCLDEL_Pos));
+        uint32_t nom_i2c_clock = 4000000;  // 4mhz internal clock = 250ns ticks
+        uint32_t scll = 20; // 20 * 250ns = 5us
+        uint32_t sclh = 16; // 16 * 250ns = 4us
+        uint32_t sdadel = 2; // 2 * 250ns = 500ns
+        uint32_t scldel = 5; // 5 * 250ns = 1250ns
+
+        uint32_t presc = get_pclock_frequency((uint32_t)i2c) / nom_i2c_clock;
+        if (presc > 0x10) {
+            uint32_t adj = 2;
+            presc /= adj;
+            scll *= adj;
+            sclh *= adj;
+            sdadel *= adj;
+            scldel *= adj;
+        }
+        i2c->TIMINGR = (((presc - 1) << I2C_TIMINGR_PRESC_Pos)
+                        | ((scll - 1) << I2C_TIMINGR_SCLL_Pos)
+                        | ((sclh - 1) << I2C_TIMINGR_SCLH_Pos)
+                        | (sdadel << I2C_TIMINGR_SDADEL_Pos)
+                        | ((scldel - 1) << I2C_TIMINGR_SCLDEL_Pos));
         i2c->CR1 = I2C_CR1_PE;
     }
 

Along with:

--- a/src/stm32/stm32g4.c
+++ b/src/stm32/stm32g4.c
@@ -12,7 +12,7 @@
 #include "internal.h" // enable_pclock
 #include "sched.h" // sched_main
 
-#define FREQ_PERIPH_DIV 1
+#define FREQ_PERIPH_DIV 2
 #define FREQ_PERIPH (CONFIG_CLOCK_FREQ / FREQ_PERIPH_DIV)
 
 // Map a peripheral address to its enable bits
@@ -142,7 +142,7 @@ clock_setup(void)
     RCC->PLLCFGR |= RCC_PLLCFGR_PLLREN;
 
     // Switch system clock to PLL
-    RCC->CFGR = RCC_CFGR_HPRE_DIV1 | RCC_CFGR_PPRE1_DIV1 | RCC_CFGR_PPRE2_DIV1
+    RCC->CFGR = RCC_CFGR_HPRE_DIV1 | RCC_CFGR_PPRE1_DIV2 | RCC_CFGR_PPRE2_DIV2
                 | RCC_CFGR_SW_PLL;
     while ((RCC->CFGR & RCC_CFGR_SWS_Msk) != RCC_CFGR_SWS_PLL)
         ;

-Kevin

[KevinOConnor]

Or, actually, how about this (along with the stm32g4.c change above):

--- a/src/stm32/stm32f0_i2c.c
+++ b/src/stm32/stm32f0_i2c.c
@@ -148,11 +148,19 @@ i2c_setup(uint32_t bus, uint32_t rate, uint8_t addr)
         gpio_peripheral(ii->sda_pin, ii->function | GPIO_OPEN_DRAIN, 1);
 
         // Set 100Khz frequency and enable
-        i2c->TIMINGR = ((0xB << I2C_TIMINGR_PRESC_Pos)
-                        | (0x13 << I2C_TIMINGR_SCLL_Pos)
-                        | (0xF << I2C_TIMINGR_SCLH_Pos)
-                        | (0x2 << I2C_TIMINGR_SDADEL_Pos)
-                        | (0x4 << I2C_TIMINGR_SCLDEL_Pos));
+        uint32_t nom_i2c_clock = 8000000;  // 8mhz internal clock = 125ns ticks
+        uint32_t scll = 40; // 40 * 125ns = 5us
+        uint32_t sclh = 32; // 32 * 125ns = 4us
+        uint32_t sdadel = 4; // 4 * 125ns = 500ns
+        uint32_t scldel = 10; // 10 * 125ns = 1250ns
+
+        uint32_t pclk = get_pclock_frequency((uint32_t)i2c);
+        uint32_t presc = DIV_ROUND_UP(pclk, nom_i2c_clock);
+        i2c->TIMINGR = (((presc - 1) << I2C_TIMINGR_PRESC_Pos)
+                        | ((scll - 1) << I2C_TIMINGR_SCLL_Pos)
+                        | ((sclh - 1) << I2C_TIMINGR_SCLH_Pos)
+                        | (sdadel << I2C_TIMINGR_SDADEL_Pos)
+                        | ((scldel - 1) << I2C_TIMINGR_SCLDEL_Pos));
         i2c->CR1 = I2C_CR1_PE;
     }
 

It'll run i2c slightly slower on stm32h7, but I doubt anyone will notice.

-Kevin

[nefelim4ag]

With the above changes (on stm32h7), it is close to 100k
(stm32g4.c + stm32f0_i2c.c)

[KevinOConnor]

Okay, thanks. I committed the two changes I mentioned above (commit 8b7cc43 and 064eee6).

Feel free to add 400Khz support on top if you wish.

Cheers,
-Kevin

[nefelim4ag]

Current 100Khz on stm32h7

Added 400Khz on stm32h7

>>> 1 // 0.0000026
384615.0

sclh slightly increased to be on the safe side.
with sclh equal to 5, timings are 2.4 us > 400KHz.

Thanks.

[KevinOConnor]

Thanks. In general it looks fine.

For what it is worth, I'd use the numbers from the stm document (ie, 4 for sclh). The actual transfer rate is dependent on the rise and fall time of the wires, which is directly tied to the strength of the pull-up resister on the wires - which is board dependent. So, if stm has a recommendation (of sorts) for what works on many boards - I'd just go with that. That said, this isn't a big concern for me either way, so if you'd prefer to stick with your current settings I'm okay with that.

Along with this change should come an update to the Config_Reference.md document to indicate that some stm32 chips support 400K mode (see the blurb on i2c speeds near the very end of that document).

-Kevin

[nefelim4ag]

The actual transfer rate is dependent on the rise and fall time of the wires

I somewhat expected that controllers blindly do everything by timing.

I am only worried about cases where sensors should use 400kHz, expect 400kHz like MPU9250, and I don't know how those will behave - I don't have one.
I think all other sensors either support higher i2c clocks (sht3*, bme*) or the application does not require a high transfer rate (LDC1612).

So, let's give it a try with recommended values.

Config_reference got updated.

Thanks.

[KevinOConnor]

Thanks. I committed this change.

For future reference, the convention for the first line of the commit message is: module: short description where "module" is the name of a directory or file (without extension) in the repo ( https://www.klipper3d.org/CONTRIBUTING.html#format-of-commit-messages ). So, it wouldn't make sense to put a '/' in the module name. When it's not immediately clear what file/directory to use I choose the closest file that is generally related - the goal of the "module" is just to give context to the commit message.

-Kevin