Add support for NXP S32 CAN driver #52450

congnguyenhuu · 2022-11-22T07:24:27Z

This patch introduces support for NXP S32 CANEXCEL (CANXL) peripheral. CAN protocol supporting:

CAN classic
CAN FD

Currently, remote transmission request is not supported as this feature is not available on NXP S32 CANXL HAL.

Updates in NXP HAL: zephyrproject-rtos/hal_nxp#206

henrikbrixandersen

@congnguyenhuu Thank you for your contribution. However, please split the suggested API change for the CAN RX filters into a separate PR so that it can be reviewed stand-alone (the current approach will cause all other boards to fail as they do not implement filtering of FD frames). You can put this PR as draft until a decision regarding the API change has been made and merged.

zephyrbot · 2022-12-01T10:38:34Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@`d33133d`	zephyrproject-rtos/hal_nxp#206	zephyrproject-rtos/hal_nxp#206/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

boards/arm/s32z270dc2_r52/s32z270dc2_rtu0_r52.yaml

boards/arm/s32z270dc2_r52/s32z270dc2_rtu1_r52.yaml

drivers/can/can_nxp_s32_canxl.h

henrikbrixandersen · 2023-03-29T11:12:50Z

include/zephyr/drivers/can.h

I am still not 100% sold on this change.

We need to document that the full width of the timestamp field is not used by all drivers; some timestamps will wrap around at 32 bit. You also need to look into the format specifiers used (e.g. in the CAN shell). They likely need updating to match the 64 bit size?

Another option could be to mask the upper 32 bit of the timestamp on NXP CANXL, as this seems to be the only device supporting 64 bit timestamps for now?

I still keep change timestamp 64bit. I updated document and related files.

I have tried to survey the market for other CAN controllers supporting 64 bit timestamps, but came up short. Please leave out this change for now.

Before you said that there are some CAN controllers supporting 32 bits timestamp, So I updated to support that.
What do you think that?

I am sorry, my initial comment said 32 bit, where I meant the existing 16 bits. Please leave out any changes to the timestamp counter bit size from this PR and - if deemed needed - submit a separate PR for this after the S32 CAN driver PR goes in.

dts/bindings/can/nxp,s32-canxl.yaml

drivers/can/can_nxp_s32_canxl.c

henrikbrixandersen · 2023-03-29T11:45:03Z

drivers/can/can_nxp_s32_canxl.c

How should this error be handled?

it seems my error msg is not correct with which handling here. In this handler called Canexcel_Ip_ReceiveFD to check MB empty (complete receive msg)to ready for receiving next msg. the msg should be MB %d is not ready for receiving next message

henrikbrixandersen · 2023-03-29T11:46:40Z

drivers/can/can_nxp_s32_canxl.c

If so, it needs to be enabled in listen-only mode so it does not disturb the bus with ACKs/error frames until explicitly enabled by calling can_start().

drivers/can/can_nxp_s32_canxl.c

dleach02 · 2023-04-04T13:53:10Z

@henrikbrixandersen, thank you for your feedback and review on this PR. Can you highlight which items are still considered a blocker so the author can address them?

manuargue

Just a minor request in the docs

boards/arm/s32z270dc2_r52/doc/index.rst

manuargue

Hi @henrikbrixandersen, thanks for the thoroughly review. Could you please have another look to the changes to see if there are still unresolved comments? We would like to have this feature included in v3.4 if possible. We can also consider having a first working version merged, and address further improvements with a follow up PR.

dleach02 · 2023-04-25T18:19:01Z

@henrikbrixandersen can you return to this PR to determine if your change requests have been addressed?

henrikbrixandersen · 2023-04-26T19:29:33Z

include/zephyr/drivers/can.h

I have tried to survey the market for other CAN controllers supporting 64 bit timestamps, but came up short. Please leave out this change for now.

drivers/can/can_nxp_s32_canxl.c

henrikbrixandersen · 2023-04-26T19:35:20Z

drivers/can/can_nxp_s32_canxl.c

So, does setting .retransmission = 0 mean no retransmissions? If so, it is a violation of the Zephyr can_send() API when CAN_MODE_ONE_SHOT is not set.

henrikbrixandersen · 2023-04-26T19:37:23Z

include/zephyr/drivers/can.h

Please leave these additions out.

I left out the update for support counter statistic

henrikbrixandersen · 2023-04-26T19:40:40Z

drivers/can/can_nxp_s32_canxl.c

Please point me to where this is done?

congnguyenhuu · 2023-04-27T07:35:25Z

@henrikbrixandersen Thank for revisiting this PR. I addressed all comment.

drivers/can/can_nxp_s32_canxl.c

henrikbrixandersen

Please leave out the timestamp changes, at least for now, and I think we're good to go.

This patch introduces support for NXP S32 CANEXCEL (CANXL) peripheral. CAN protocol supporting: - CAN classic - CAN FD Remote transmission request is not supported as this feature is not available on NXP S32 CANXL HAL. Signed-off-by: Cong Nguyen Huu <[email protected]>

Enable CAN instances on s32z270dc2_r52 boards Signed-off-by: Cong Nguyen Huu <[email protected]>

Add CAN document to the list of supported features Signed-off-by: Cong Nguyen Huu <[email protected]>

congnguyenhuu · 2023-04-28T13:43:21Z

@henrikbrixandersen I left out the timestamp change. Do you have any blocker for PR?

henrikbrixandersen

Thank you, looks good - and thank you for your patience with the review process!

dleach02 · 2023-04-28T18:55:26Z

@congnguyenhuu please remove the west.yml from your PR. The HAL change was merged a while ago and is part of the current Zephyr west.yml.

congnguyenhuu · 2023-04-29T03:09:51Z

@dleach02 I removed it as soon as the HAL change was merged.

congnguyenhuu requested review from alexanderwachter, henrikbrixandersen, manuargue, nashif, pfalcon, rlubos and tbursztyka as code owners November 22, 2022 07:24

zephyrbot added area: Networking area: Devicetree Binding PR modifies or adds a Device Tree binding area: CAN platform: NXP NXP area: Sockets Networking sockets labels Nov 22, 2022

zephyrbot requested review from DerekSnell, danieldegrasse, dleach02, karstenkoenig, martinjaeger, mengxianglinx, mmahadevan108 and str4t0m November 22, 2022 07:25

zephyrbot assigned henrikbrixandersen Nov 22, 2022

henrikbrixandersen requested changes Nov 22, 2022

View reviewed changes

congnguyenhuu marked this pull request as draft November 23, 2022 01:44

congnguyenhuu mentioned this pull request Nov 23, 2022

driver: can: add new filter to match CAN-FD frames #52483

Merged

zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Dec 1, 2022

congnguyenhuu marked this pull request as ready for review January 4, 2023 13:54

zephyrbot added the area: native port Host native arch port (native_sim) label Jan 4, 2023

zephyrbot requested review from EmilioCBen, galak, karstenkoenig, martinjaeger, mmahadevan108, str4t0m and yvanderv March 29, 2023 01:55

henrikbrixandersen requested changes Mar 29, 2023

View reviewed changes

manuargue requested changes Apr 4, 2023

View reviewed changes

boards/arm/s32z270dc2_r52/doc/index.rst Outdated Show resolved Hide resolved

boards/arm/s32z270dc2_r52/doc/index.rst Outdated Show resolved Hide resolved

manuargue previously approved these changes Apr 19, 2023

View reviewed changes

congnguyenhuu requested a review from henrikbrixandersen April 19, 2023 02:26

henrikbrixandersen requested changes Apr 26, 2023

View reviewed changes

congnguyenhuu dismissed manuargue’s stale review via b8b24df April 27, 2023 07:27

congnguyenhuu commented Apr 27, 2023

View reviewed changes

drivers/can/can_nxp_s32_canxl.c Outdated Show resolved Hide resolved

henrikbrixandersen requested changes Apr 28, 2023

View reviewed changes

congnguyenhuu added 3 commits April 28, 2023 20:36

boards: arm: s32z270dc2_r52: enable CAN support

7bab9f6

Enable CAN instances on s32z270dc2_r52 boards Signed-off-by: Cong Nguyen Huu <[email protected]>

boards: s32z270dc2_r52: add CAN document to list of supported features

2e2f637

Add CAN document to the list of supported features Signed-off-by: Cong Nguyen Huu <[email protected]>

henrikbrixandersen approved these changes Apr 28, 2023

View reviewed changes

henrikbrixandersen requested a review from manuargue April 28, 2023 16:16

manuargue approved these changes Apr 29, 2023

View reviewed changes

carlescufi merged commit 55fa936 into zephyrproject-rtos:main Apr 29, 2023

manuargue deleted the s32ze-support-can branch April 30, 2023 15:30

henrikbrixandersen mentioned this pull request Nov 13, 2023

RFC: API change: drivers: can: remove CAN_FILTER_FDF flag #65109

Closed

Add support for NXP S32 CAN driver #52450

Add support for NXP S32 CAN driver #52450

Uh oh!

Conversation

congnguyenhuu commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Uh oh!

zephyrbot commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dleach02 commented Apr 4, 2023

Uh oh!

manuargue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

manuargue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleach02 commented Apr 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

congnguyenhuu commented Apr 27, 2023

Uh oh!

Uh oh!

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Uh oh!

congnguyenhuu commented Apr 28, 2023

Uh oh!

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Uh oh!

dleach02 commented Apr 28, 2023

Uh oh!

congnguyenhuu commented Apr 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

congnguyenhuu commented Nov 22, 2022 •

edited

Loading

zephyrbot commented Dec 1, 2022 •

edited

Loading

manuargue left a comment •

edited

Loading