STM32H7: Compute I2C timing according current I2C clock source #14030

facchinm · 2020-12-11T13:20:40Z

Summary of changes

I2C clock dividers were calculated based on the assumption that the feed clock is always 54MHz.
Borrow the proper calculation from STM32CubeH7

Impact of changes

Migration actions required

Documentation

None

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@jeromecoutant @pennam

ciarmcom · 2020-12-11T13:30:30Z

@facchinm, thank you for your changes.
@jeromecoutant @ARMmbed/team-st-mcd @ARMmbed/mbed-os-maintainers please review.

jeromecoutant · 2020-12-11T13:50:20Z

Is code coming from
https://github.com/stm32duino/Arduino_Core_STM32/blob/master/libraries/Wire/src/utility/twi.c#L378-L510
?

@ABOSTM @LMESTM

facchinm · 2020-12-11T13:58:41Z

@jeromecoutant nope, it's from https://github.com/STMicroelectronics/STM32CubeH7/blob/beced99ac090fece04d1e0eb6648b8075e156c6c/Drivers/BSP/STM32H747I-DISCO/stm32h747i_discovery_bus.c (as reported in the description).
BSD 3-clause should be compatible with mbed allowed licenses AFAIU.

jeromecoutant

I2C tests are failed with this patch with NUCLEO_H743ZI2

facchinm · 2021-01-04T17:07:34Z

@jeromecoutant you mean in your internal hardware CI?

jeromecoutant · 2021-01-04T17:11:35Z

you mean in your internal hardware CI?

Yes, with this test:
https://github.com/ARMmbed/ci-test-shield/tree/master/TESTS/API/I2C

facchinm · 2021-01-04T17:13:19Z

A particular test case or just all of them?

jeromecoutant · 2021-01-04T17:20:45Z

A particular test case or just all of them?

Running case 1: 'I2C - Instantiation of I2C Object'... => OK
Running case 2: 'I2C - LM75B Temperature Read'... => OK
Running case 3: 'I2C - EEProm WR Single Byte'... => OK
Running case 4: 'I2C - EEProm 2nd WR Single Byte'... => OK
Running case 5: 'I2C - EEProm WR 2 Bytes'... => CRASH

[1609780483.61][CONN][RXD] ++ MbedOS Error Info ++
[1609780483.66][CONN][RXD] Error Status: 0x80FF013D Code: 317 Module: 255
[1609780483.69][CONN][RXD] Error Message: Fault exception
[1609780483.72][CONN][RXD] Location: 0x8005D30
[1609780483.74][CONN][RXD] Error Value: 0x240033E4
[1609780483.87][CONN][RXD] Current Thread: main <handler> Id: 0x24005154 Entry: 0x800EAC7 StackSize: 0x1000 StackMem: 0x24002078 SP: 0x2407FF78
[1609780484.01][CONN][RXD] For more info, visit: https://mbed.com/s/error?error=0x80FF013D&osver=60600&core=0x411FC271&comp=1&ver=6150002&tgt=NUCLEO_H743ZI2
[1609780484.02][CONN][RXD] -- MbedOS Error Info --

Note I added quick printf to display new values:
[1609780125.55][CONN][RXD] hz=100000 tim=0x70d04648
[1609780125.63][CONN][RXD] hz=400000 tim=0x60f00c16

mergify · 2021-01-05T16:08:22Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

pennam · 2021-01-05T16:59:48Z

Hi @jeromecoutant i've updated the patch adding

mbed-os/targets/TARGET_STM/TARGET_STM32H7/i2c_device.c

Line 354 in 8915a8a

I2c_valid_timing_nbr = 0;

that was missing. I2c_valid_timing_nbr must be cleared each time a new calculation is performed to make sure only valid values of I2c_valid_timig array are considered. Now it shouldn't raise any exception. Could you please re-run tests? If everything runs fine i will then rebase.

jeromecoutant · 2021-01-06T13:16:20Z

Hi @pennam

ci-test-shield I2C test is now OK!

But unfortunately, FPGA I2C test was OK, and is now FAILED...
https://github.com/ARMmbed/mbed-os/blob/master/hal/tests/TESTS/mbed_hal_fpga_ci_test_shield/i2c/main.cpp

FPGA_I2C_TEST.zip

pennam · 2021-01-07T13:56:21Z

@jeromecoutant thanks for the reports, at least is not crashing anymore. Next week, most probably on wednesday, i will have an fpga shield to make further investigations on this issue.

pennam · 2021-01-18T10:53:35Z

@jeromecoutant i've received the fpga shield and made several tests, from my side I2C tests are always ok. I'm using STM32H747I-DISCO board. Could you please check if test are always failing on your board or it was just a glitch?

i2c_tests.log

ciarmcom · 2021-01-20T12:51:08Z

This pull request has automatically been marked as stale because it has had no recent activity. @facchinm, please carry out any necessary work to get the changes merged. Thank you for your contributions.

jeromecoutant · 2021-01-20T14:03:51Z

@jeromecoutant i've received the fpga shield and made several tests, from my side I2C tests are always ok. I'm using STM32H747I-DISCO board. Could you please check if test are always failing on your board or it was just a glitch?

I confirm that with H747 DISCO is OK...
And there is still failure with H743 NUCLEO...
I am checking

jeromecoutant · 2021-01-20T15:01:48Z

targets/TARGET_STM/TARGET_STM32H7/i2c_device.c

+  I2c_valid_timing_nbr = 0;
+
+  /* we will use D2PCLK1 to calculate I2C timings */
+  MBED_ASSERT(RCC_I2C1CLKSOURCE_D2PCLK1 ==__HAL_RCC_GET_I2C1_SOURCE());


This is strange as H747-DISCO is suing I2C4 for D14/D15 pins.

NUCLEO H743 is using I2C1...

When i wrote this line i was making wrong assumptions and it is completely useless. On DISCO I2C1_SOURCE is actually D2PCLK1 so we never hit the assert, moreover as you wrote I2C4 is used for the tests... On DISCO tests are ok because PCLK1_Frequency, PCLK2_Frequency, PCLK3_Frequency and PCLK4_Frequency have all the same value.

On the other side I2C1 instance is used for running tests on the NUCLEO and we doesn't hit the assert too, this means we are using the correct clock source to compute timings... so i thinks there is something else i'm missing to make things work on the NUCLEO.

jeromecoutant

Changes seems not compliant with all I2C instances

jeromecoutant · 2021-01-20T15:15:03Z

targets/TARGET_STM/TARGET_STM32H7/i2c_device.c

+  LL_RCC_ClocksTypeDef rcc_clocks;
+  LL_RCC_GetSystemClocksFreq(&rcc_clocks);
+
+  tim = I2C_ComputeTiming(rcc_clocks.PCLK1_Frequency, hz);


I2C4 seems connected to PCLK2

from my understanding I2C4 on DISCO uses RCC_I2C4CLKSOURCE_D3PCLK1 as clock source that is the same of rcc_clocks.PCLK4_Frequency. I don't know why here https://github.com/STMicroelectronics/STM32CubeH7/blob/beced99ac090fece04d1e0eb6648b8075e156c6c/Drivers/BSP/STM32H747I-DISCO/stm32h747i_discovery_bus.c#L224 PCLK2 is used. From the reference manual I2C4 can be used with PCLK4, PLL3, HSI and CSI.

pennam · 2021-01-20T15:30:43Z

I need to understand how to get the correct clock source of each I2C instance. I start working on it now.

pennam · 2021-01-21T16:01:49Z

I've found a way to use the proper clock source for any I2C instance, hopefully tomorrow i will get a NUCLEO board to repeat test on my side.

…ired I2C clock

mbed-ci · 2021-02-03T13:55:14Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	❌
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	❌
jenkins-ci/mbed-os-ci_build-greentea-ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-ARM	❌

jeromecoutant · 2021-02-03T14:19:18Z

targets/TARGET_STM/i2c_api.c

+#ifdef TARGET_STM32H7
+    handle->Init.Timing = get_i2c_timing(obj_s->i2c, hz);
+#else
+    handle->Init.Timing = get_i2c_timing(hz);
+#endif
+


This should be kept within #ifdef I2C_IP_VERSION_V2

I've fixed it and added i2c_device.c to CMakeLists.txt

Pull request has been modified.

0xc0170 · 2021-02-04T20:03:56Z

CI started

mbed-ci · 2021-02-04T20:41:53Z

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	❌
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️

jeromecoutant

About last correction in stm32h7xx_hal_rcc_ex.h
correction is approved by ST driver team and will be present in the next delivery.

Thx a lot

pennam · 2021-02-10T17:00:14Z

@0xc0170 last CI issue should be fixed by dc2d860 . Is it possible to run CI again?

jeromecoutant · 2021-02-15T10:09:26Z

@0xc0170 ping :-)

0xc0170 · 2021-02-15T10:32:58Z

CI started

mbed-ci · 2021-02-15T11:07:48Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Dec 11, 2020

ciarmcom requested review from jeromecoutant and a team December 11, 2020 13:30

ciarmcom added the needs: review label Dec 11, 2020

facchinm mentioned this pull request Dec 22, 2020

I2C frequency and waveform on Portenta H7 arduino/ArduinoCore-mbed#89

Closed

jeromecoutant suggested changes Jan 4, 2021

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jan 5, 2021

pennam force-pushed the i2c_stm32_wrong_clock branch from 6a0ed52 to 86ee300 Compare January 5, 2021 16:07

ciarmcom added the stale Stale Pull Request label Jan 20, 2021

jeromecoutant reviewed Jan 20, 2021

View reviewed changes

jeromecoutant suggested changes Jan 20, 2021

View reviewed changes

STM32: Compute I2C timing according current I2C clock source and requ…

28a3475

…ired I2C clock

mergify bot removed the needs: work label Feb 3, 2021

pennam added 2 commits February 3, 2021 15:14

manage I2C_IP_VERSION_V1 boards builds

6db7342

Added i2c_device.c to CMakeLists.txt

d059fee

jeromecoutant reviewed Feb 3, 2021

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Feb 4, 2021

Fixed __HAL_RCC_GET_I2C4_SOURCE() if RCC_VER_2_0 is used

dc2d860

jeromecoutant approved these changes Feb 5, 2021

View reviewed changes

0xc0170 approved these changes Feb 15, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Feb 15, 2021

mergify bot added ready for merge and removed needs: CI labels Feb 15, 2021

0xc0170 merged commit 1dd4112 into ARMmbed:master Feb 15, 2021

mergify bot removed the ready for merge label Feb 15, 2021

mbedmain added release-version: 6.8.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Feb 16, 2021

pennam mentioned this pull request Mar 11, 2021

STM32: Compute I2C timing according current I2C clock source and required I2C clock arduino/mbed-os#6

Closed

pennam mentioned this pull request Mar 29, 2021

I2C clock fix backport arduino/mbed-os#16

Closed

pennam mentioned this pull request Jul 13, 2021

STM32 Fix i2c_compute_timing() API #14915

Merged

STM32H7: Compute I2C timing according current I2C clock source #14030

STM32H7: Compute I2C timing according current I2C clock source #14030

Uh oh!

Conversation

facchinm commented Dec 11, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Dec 11, 2020

Uh oh!

jeromecoutant commented Dec 11, 2020

Uh oh!

facchinm commented Dec 11, 2020

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

facchinm commented Jan 4, 2021

Uh oh!

jeromecoutant commented Jan 4, 2021

Uh oh!

facchinm commented Jan 4, 2021

Uh oh!

jeromecoutant commented Jan 4, 2021

Uh oh!

mergify bot commented Jan 5, 2021

Uh oh!

pennam commented Jan 5, 2021

Uh oh!

jeromecoutant commented Jan 6, 2021

Uh oh!

pennam commented Jan 7, 2021

Uh oh!

pennam commented Jan 18, 2021

Uh oh!

ciarmcom commented Jan 20, 2021

Uh oh!

jeromecoutant commented Jan 20, 2021

Uh oh!

jeromecoutant Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

pennam Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

jeromecoutant Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

pennam Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

pennam commented Jan 20, 2021

Uh oh!

pennam commented Jan 21, 2021

Uh oh!

mbed-ci commented Feb 3, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

jeromecoutant Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

pennam Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 4, 2021

Uh oh!

mbed-ci commented Feb 4, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

jeromecoutant left a comment