Skip to content
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.

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

arch/arm/stm32/: Fix I2C to support STM32G4 devices. #15420

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

daniel-p-carvalho
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

According to the documentation of the file arch/arm/src/stm32/stm32_i2c_v2.c, the driver supports, as a clock source, only STM32_HSI_FREQUENCY = 16 MHz. In fact, the function stm32_i2c_setclock adjusts the STM32_I2C1_TIMINGR register considering fI2CCLK at 16 MHz. However, the function stm32_i2cbus_initialize returns NULL if STM32_HSI_FREQUENCY != 8000000. Thus, in my understanding, the function stm32_i2c_setclock is not adjusting the bus frequency correctly. Furthermore, no clock source verification is performed.

This patch modifies the I2C driver to allow devices in the STM32G4 family to function correctly. The driver's behavior has not been modified for devices in the STM32_STM32F30XX, STM32_STM32F33XX, and STM32_STM32F37XX families, although I believe the driver is not functioning properly.

Impact

No impact is expected

Testing

This changes was tested on STM32G474.

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small labels Jan 3, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 3, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

This PR description does not fully meet the NuttX requirements. Here's a breakdown of why and how to improve it:

Missing/Insufficient Information:

  • Summary: While the summary explains the problem, it lacks clarity on the exact code changes made. Instead of saying "modifies the I2C driver," specify which functions were changed and how the clock source selection and frequency calculation are now handled. Be explicit. For example: "This patch modifies stm32_i2c_setclock to calculate the correct timing register values based on the actual I2C clock frequency provided by stm32_i2cbus_initialize. It also removes the erroneous check in stm32_i2cbus_initialize that restricted the clock source to 8MHz."
  • Impact: Claiming "no impact" is unlikely and requires justification. At a minimum, the impact on the STM32G4 family should be explicitly stated as a positive impact (fixing I2C functionality). Even if other families are theoretically unaffected, you need to state that explicitly and explain why. For example: "Impact on hardware: Fixes I2C functionality on STM32G4 family. No intended functional changes on STM32F30XX, STM32F33XX, and STM32F37XX families, as the current (potentially incorrect) behavior is maintained for these families pending further investigation and testing." This transparency is crucial.
  • Testing: This section is severely lacking.
    • Build Host: Provide details: OS, CPU architecture, compiler version (e.g., "Linux x86_64, GCC 12.2.0").
    • Target: Be much more specific: "STM32G474RE (Nucleo-G474RE board) with the default configuration."
    • Logs: "Testing logs before change" and "Testing logs after change" are empty. Include actual logs demonstrating the I2C failure before the patch and the correct operation after the patch. Show I2C communication working (e.g., using i2ctransfer). The logs should be concise but convincing.

Recommendations for Improvement:

  1. Clarity on Code Changes: Detail the exact changes in the summary. Be specific about which functions were modified and how.

  2. Accurate Impact Assessment: Describe the positive impact on STM32G4. Explain the (lack of intended) impact on other families. Justify your reasoning.

  3. Detailed Testing Information: Provide full details of the build host and target environment. Include actual test logs showing the I2C behavior before and after the patch. Demonstrate a clear improvement in functionality.

  4. Consider Backwards Compatibility: Even if no intentional changes, address backwards compatibility directly. Explain whether existing applications using I2C on affected platforms need modification.

  5. NuttX Issue Reference: If there's no related issue, consider creating one to discuss the potential problem with the F3 families, turning this PR into a partial fix focusing on G4.

By addressing these points, you will significantly improve the quality of your PR and increase its chances of being accepted. A clear, well-documented PR makes the reviewer's job much easier and demonstrates your understanding of the codebase and the impact of your changes.

@daniel-p-carvalho
Copy link
Contributor Author

I can't find the reason CI is failling on (Build / Linux (arm-12) (pull_request)).

@raiden00pl
Copy link
Member

@daniel-p-carvalho please rebase to the latest master. CI should be fixed with ae26129

@daniel-p-carvalho daniel-p-carvalho force-pushed the stm32g4_i2c branch 2 times, most recently from e29f3c3 to cbe694d Compare January 7, 2025 12:02
@daniel-p-carvalho
Copy link
Contributor Author

@daniel-p-carvalho please rebase to the latest master. CI should be fixed with ae26129

I did the rebase twice but the CI error persist.

@raiden00pl
Copy link
Member

now I see there are other problems. Unfortunately our CI is not very useful after we had to severely limit its resources and it can't catch most error before merge PR :(

@raiden00pl
Copy link
Member

@daniel-p-carvalho please try rebase again. The current CI error should be fixed with 4e563e3

@raiden00pl
Copy link
Member

no, CI is still broken for stm32f429i-disco/systemview ...

@hujun260 hi, could you look at this this CI failure: https://github.com/apache/nuttx/actions/runs/12651153856/job/35251277261?pr=15420#step:7:526

@xiaoxiang781216
Copy link
Contributor

no, CI is still broken for stm32f429i-disco/systemview ...

@hujun260 hi, could you look at this this CI failure: https://github.com/apache/nuttx/actions/runs/12651153856/job/35251277261?pr=15420#step:7:526

fix here: #15441. but let's merge this simple patch.

@xiaoxiang781216 xiaoxiang781216 merged commit ba32b61 into apache:master Jan 7, 2025
11 of 25 checks passed
@hujun260
Copy link
Contributor

hujun260 commented Jan 7, 2025

no, CI is still broken for stm32f429i-disco/systemview ...

@hujun260 hi, could you look at this this CI failure: https://github.com/apache/nuttx/actions/runs/12651153856/job/35251277261?pr=15420#step:7:526

ok

@daniel-p-carvalho daniel-p-carvalho deleted the stm32g4_i2c branch January 7, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants