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,263 −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
XenuIsWatching
XenuIsWatching reviewed Nov 20, 2024
View reviewed changes
drivers/i3c/i3c_stm32.c Outdated

LL_I3C_SetDataHoldTime(i3c, LL_I3C_SDA_HOLD_TIME_1_5);
LL_I3C_SetFreeTiming(i3c, free_timing);
LL_I3C_SetControllerActivityState(i3c, LL_I3C_OWN_ACTIVITY_STATE_0);
Copy link
Member

Choose a reason for hiding this comment

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

this appears to be set in multiple places, it probably should be only set once in the init

Copy link
Member

@XenuIsWatching XenuIsWatching Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

not resolved!!, this probably should be within i3c_stm32_controller_init

Copy link
Author

Choose a reason for hiding this comment

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

In this section, I calculate the free_timing value based on specific calculations and use it with LL_I3C_SetFreeTiming. The functions LL_I3C_SetDataHoldTime and LL_I3C_SetControllerActivityState are included alongside LL_I3C_SetFreeTiming as they all modify the same register.
ok ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm only referring to LL_I3C_SetControllerActivityState here but this is modifying the value it could receive from the active controller through the ENTAS# CCC. It shouldn't be modified with every call to configure

XenuIsWatching
XenuIsWatching reviewed Nov 20, 2024
View reviewed changes
drivers/i3c/i3c_stm32.c

/* After receiving all bytes for current target, move on to the next target
*/
if (target->num_xfer == target->data_len) {
Copy link
Member

Choose a reason for hiding this comment

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

A target can send an EoD when doing a read with a CCC and it's possible for the num_xfer to be less than the data_len. This can happen with variable length CCCs such as GETCAPS and GETMXDS. Does this handle this situation?

Copy link
Author

@ExaltZephyr ExaltZephyr Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No it is not handled, we are currently thinking of a way to handle this issue. The problem is that the information about the target early read termination can be obtained from the status FIFO, however, we might be receiving data from the next target before the data from the status FIFO is ready.

One suggestion we are thinking of is to disable the status FIFO for CCC requests and rely on the RXTGTENDF interrupt to detect early target termination and advance the ccc_target_payload pointer.

@benediktibk benediktibk mentioned this pull request Nov 20, 2024
Closed
@XenuIsWatching XenuIsWatching mentioned this pull request Nov 21, 2024
Open
@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
dts: Add I3C configuration for STM32.
af6a1c2 
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.
407a5d2 
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.
0b50683 
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.
97d1fb1 
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

@XenuIsWatching XenuIsWatching XenuIsWatching requested changes

@erwango erwango erwango left review comments

@dcpleung dcpleung Awaiting requested review from dcpleung dcpleung is a code owner

@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

Requested changes must be addressed 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.
7 participants
@ExaltZephyr @JarmouniA @erwango @dcpleung @XenuIsWatching @decsny @zephyrbot