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.

Sign up for GitHub

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

Jump to bottom

drivers: i3c: Support I3C driver for STM32. #81190

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

drivers: i3c: Support I3C driver for STM32. #81190

wants to merge 4 commits into from
+2,285 −0

Conversation

ExaltZephyr
Copy link

This PR introduces support for the I3C driver on STM32, enabling functionality APIs for I3C controllers.

XenuIsWatching
XenuIsWatching requested changes Nov 10, 2024
View reviewed changes
Copy link
Member

@XenuIsWatching XenuIsWatching left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i3c_shell.c also needs updated

zephyr/drivers/i3c/i3c_shell.c

Line 82 in c50777a

DT_FOREACH_STATUS_OKAY(cdns_i3c, I3C_CTRL_FN)

and

zephyr/drivers/i3c/i3c_shell.c

Line 96 in c50777a

DT_FOREACH_STATUS_OKAY(cdns_i3c, I3C_CTRL_LIST_ENTRY)

drivers/i3c/i3c_stm32.c Outdated Show resolved Hide resolved
drivers/i3c/i3c_stm32.c Outdated Show resolved Hide resolved
drivers/i3c/i3c_stm32.c Outdated Show resolved Hide resolved
drivers/i3c/i3c_stm32.c Outdated Show resolved Hide resolved
drivers/i3c/i3c_stm32.c Outdated Show resolved Hide resolved
@JarmouniA
Copy link
Collaborator

@ExaltZephyr You should put your legal name in Signed-off-by https://docs.zephyrproject.org/latest/contribute/guidelines.html#dco-sign-off

JarmouniA
JarmouniA suggested changes Nov 10, 2024
View reviewed changes
drivers/i3c/i3c_stm32.c Outdated Show resolved Hide resolved
dts/bindings/test/vnd,i3c.yaml Outdated Show resolved Hide resolved
dts/arm/st/h5/stm32h5.dtsi Show resolved Hide resolved
boards/st/nucleo_h563zi/nucleo_h563zi-common.dtsi Show resolved Hide resolved
@XenuIsWatching XenuIsWatching self-assigned this Nov 10, 2024
JarmouniA
JarmouniA reviewed Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@JarmouniA JarmouniA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge commit should be dropped.

JarmouniA
JarmouniA suggested changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@JarmouniA JarmouniA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title of each commit should reflect the path of where changes are introduced, see the main branch for examples.

@JarmouniA
Copy link
Collaborator

@ExaltZephyr You should put your legal name in Signed-off-by https://docs.zephyrproject.org/latest/contribute/guidelines.html#dco-sign-off

Still not fixed. Without this your PR cannot be merged.

@ExaltZephyr ExaltZephyr force-pushed the stm32-i3c-support branch 2 times, most recently from 02f4d4a to 13a048d Compare November 12, 2024 09:06
@ExaltZephyr ExaltZephyr force-pushed the stm32-i3c-support branch 2 times, most recently from 0f370bd to e6978f9 Compare November 13, 2024 09:14
@erwango
Copy link
Member

erwango commented Nov 13, 2024

About Sign-off (Signed-off-by: EXALT Technologies <[email protected]>), the names of groups are not allowed. Please provide one or several developer name. You could use :

Signed-off-by: .....
Co-authored-by: .....

@ExaltZephyr ExaltZephyr force-pushed the stm32-i3c-support branch 3 times, most recently from 36c05bf to f43a4b6 Compare November 13, 2024 16:33
@ExaltZephyr ExaltZephyr force-pushed the stm32-i3c-support branch 2 times, most recently from 97fbfed to fec23de Compare November 21, 2024 14:52
@dcpleung
Copy link
Member

Please do not resolve comments on your own, and let the original poster do it instead.

@ExaltZephyr ExaltZephyr force-pushed the stm32-i3c-support branch 2 times, most recently from 4667d8a to 97d1fb1 Compare November 25, 2024 13:19
XenuIsWatching
XenuIsWatching reviewed Nov 26, 2024
View reviewed changes
drivers/i3c/i3c_stm32.c Outdated
LL_I3C_DisableStallParityData(i3c);
LL_I3C_DisableStallTbit(i3c);
LL_I3C_DisableHighKeeperSDA(i3c);
LL_I3C_SetDataHoldTime(i3c, LL_I3C_SDA_HOLD_TIME_1_5);
Copy link
Member

@XenuIsWatching XenuIsWatching Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about this, This is related to the kernel clock input and meeting the required t_hd_pp time, and should be within i3c_stm32_config_ctrl_bus_char... but I would expect some math behind it to determine if it should be 0.5 or 1.5. Is there a reason why you have it fixed to 1.5?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the reference manual for the H5 board, the value of the minimum data hold time can be calculated as follows:

  • $t_{HDPP_{min}} = min(t_{CR}, t_{CF}) + 3ns$

We could not find how the $t_{CR}$ and $t_{CF}$ values can be calculated so we chose 1.5 data hold time as default for now to avoid any issues that might occur when choosing 0.5.

If you have any suggestion on how we can dynamically set these values according to the $t_{CR}$, $t_{CF}$ and the kernel clock, we can modify the code accordingly.

pdgendt
pdgendt reviewed Dec 2, 2024
View reviewed changes
drivers/i3c/i3c_stm32.c Outdated Show resolved Hide resolved
@ExaltZephyr ExaltZephyr force-pushed the stm32-i3c-support branch 2 times, most recently from 365c7f1 to 54e190b Compare December 2, 2024 09:46
XenuIsWatching
XenuIsWatching approved these changes Dec 2, 2024
View reviewed changes
Copy link
Member

@XenuIsWatching XenuIsWatching left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now... but it looks like there is an unrelated CI failure, you may need to rebase back on top of main

@erwango erwango mentioned this pull request Dec 3, 2024
Open
@ExaltZephyr
dts: Add I3C configuration for STM32.
2eb5d61 
This commit adds the main DTS configurations required
to enable I3C support on STM32.

Signed-off-by: Mohammad Badawi <[email protected]>
Signed-off-by: Sara Touqan <[email protected]>
@ExaltZephyr
drivers: i3c: Support I3C driver for STM32.
95608cb 
This commit introduces support for the I3C driver on STM32, enabling
functionality APIs for I3C controllers.

Signed-off-by: Mohammad Badawi <[email protected]>
Signed-off-by: Sara Touqan <[email protected]>
@ExaltZephyr
shell: Enable I3C shell support for STM32.
0fdf5fb 
This commit introduces support for I3C shell on STM32.

Signed-off-by: Mohammad Badawi <[email protected]>
Signed-off-by: Sara Touqan <[email protected]>
@ExaltZephyr
boards: Enable I3C support for STM32 nucleo_h563zi.
ca94845 
This commit enables I3C support for STM32 nucleo_h563zi boards.

Signed-off-by: Mohammad Badawi <[email protected]>
Signed-off-by: Sara Touqan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erwango erwango erwango left review comments

@pdgendt pdgendt pdgendt left review comments

@XenuIsWatching XenuIsWatching XenuIsWatching approved these changes

@dcpleung dcpleung Awaiting requested review from dcpleung

@FRASTM FRASTM Awaiting requested review from FRASTM

@gautierg-st gautierg-st Awaiting requested review from gautierg-st

@GeorgeCGV GeorgeCGV Awaiting requested review from GeorgeCGV

@marwaiehm-st marwaiehm-st Awaiting requested review from marwaiehm-st

@mathieuchopstm mathieuchopstm Awaiting requested review from mathieuchopstm

@kartben kartben Awaiting requested review from kartben

@decsny decsny Awaiting requested review from decsny

@galak galak Awaiting requested review from galak

@JarmouniA JarmouniA Awaiting requested review from JarmouniA

At least 2 approving reviews are required to merge this pull request.

Assignees

@XenuIsWatching XenuIsWatching

@dcpleung dcpleung

Labels
area: Devicetree area: I3C block: HW Test Testing on hardware required before merging platform: STM32 ST Micro STM32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ExaltZephyr @JarmouniA @erwango @dcpleung @pdgendt @XenuIsWatching @decsny @zephyrbot