Skip to content

Add CAN driver support for Renesas RZ/G3S #83778

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

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

Conversation

nhutnguyenkc
Copy link
Collaborator

@nhutnguyenkc nhutnguyenkc commented Jan 10, 2025

Add CAN driver support for Renesas RZ/G3S.

Since RZ/G3S does not support to calculate perfectly the sample point of 87,5%, so the test update is required.

Need: zephyrproject-rtos/hal_renesas#65

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 10, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

All manifest checks OK

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

@zephyrbot zephyrbot added manifest manifest-hal_renesas DNM This PR should not be merged (Do Not Merge) labels Jan 10, 2025
@@ -527,7 +527,11 @@ ZTEST_USER(canfd, test_set_timing_data_while_started)
struct can_timing timing = { 0 };
int err;

#ifdef CONFIG_CAN_RENESAS_RZ_CANFD
Copy link
Member

Choose a reason for hiding this comment

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

No SoC/driver/board specific #ifdefs in generic API tests, please. You need to find another solution.

Copy link
Collaborator Author

@nhutnguyenkc nhutnguyenkc Jan 14, 2025

Choose a reason for hiding this comment

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

Understood. I have reverted the change. Unfortunately, RZ/G3S cannot calculate the sample point of 87.5% accurately, so the test_set_timing_data_while_started fail. Is it acceptable if we change the err check at line 531 to range check as below (in fact, 25 is enough for us)

zassert_between_inclusive(err, 0, 50, "failed to calculate data timing (err %d)", err);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@henrikbrixandersen , could you please confirm my above solution. Thanks

@zephyrbot zephyrbot removed manifest manifest-hal_renesas DNM This PR should not be merged (Do Not Merge) labels Feb 3, 2025
Copy link
Collaborator

@str4t0m str4t0m left a comment

Choose a reason for hiding this comment

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

@nhutnguyenkc I am not very familiar with the renesas ecosystem, however this driver seems to use exactly the same fsp API as the existing renesas_ra driver.
In a quick glance at the hal the only difference in the header I can see are the not-/availabilty of canfd_txmb_merge_mode, e_canfd_tx_buffer, and common fifo.
And with the exception of some initialization stuff the code seems to match that driver nearly exactly.
Therefore I would suggest to keep the different compatibles, but factor out the common stuff of the two drivers.
Ideally the clock control driver would also be merged before this driver, then they would likely match even more.

Comment on lines 70 to 94
| CANFD | on-chip | can |
+-----------+------------+-------------------------------------+
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be part of the board commit. Same for the change in the rzg3s_smarc_r9a08g045s33gbg_cm33.yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I have moved them to board commit.

Comment on lines +708 to +713
static int can_renesas_rz_get_core_clock(const struct device *dev, uint32_t *rate)
{
const struct can_renesas_rz_cfg *cfg = dev->config;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clock control driver not yet implemented, would make commonality between with the ra series even bigger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I have implemented clock control driver for can. It took a while to get clock control merged.

@nhutnguyenkc nhutnguyenkc force-pushed the rzg3s_support_can branch 2 times, most recently from 98d9143 to c93a24d Compare February 17, 2025 10:39
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

Did you consider the refactoring proposed in #83778 (review) ?

@@ -528,7 +528,7 @@ ZTEST_USER(canfd, test_set_timing_data_while_started)
int err;

err = can_calc_timing_data(can_dev, &timing, TEST_BITRATE_3, TEST_SAMPLE_POINT);
zassert_ok(err, "failed to calculate data timing (err %d)", err);
zassert_between_inclusive(err, 0, 50, "failed to calculate data timing (err %d)", err);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be split out into it's own PR with proper description instead of being somewhat "hidden" in this larger PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Understood. I have reverted this change, and will update it in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

How come CAN is not enabled on the board level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I moved it to the board commit and dropped the test commit.

Copy link
Member

Choose a reason for hiding this comment

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

How come CAN is not enabled on the board level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I moved it to the board commit and dropped the test commit.

@nhutnguyenkc
Copy link
Collaborator Author

nhutnguyenkc commented Feb 18, 2025

I am not very familiar with the renesas ecosystem, however this driver seems to use exactly the same fsp API as the existing renesas_ra driver. In a quick glance at the hal the only difference in the header I can see are the not-/availabilty of canfd_txmb_merge_mode, e_canfd_tx_buffer, and common fifo. And with the exception of some initialization stuff the code seems to match that driver nearly exactly. Therefore I would suggest to keep the different compatibles, but factor out the common stuff of the two drivers. Ideally the clock control driver would also be merged before this driver, then they would likely match even more.

Thanks for your feedback. For the coexistence of CAN driver of renesas_rz and renesas_ra, let me explain a bit. They are two different product lines (RA is MCU, RZ is MPU) having two different FSP code base (hal_renesas). You may see small differences in case of RZ/G3S, but when other RZ devices come, the difference will be bigger, and the code would look too messy if it is shared between RZ and RA. In addition to the points that you have listed out (canfd_txmb_merge_mode, e_canfd_tx_buffer, and common fifo), clock, hardware configuration, direct register accesses are the points keeping them separate. These were also considered internally before concluding implementing a different code base.

@kartben kartben added this to the v4.2.0 milestone Feb 26, 2025
@nhutnguyenkc
Copy link
Collaborator Author

I just rebased to resolve conflict

@nhutnguyenkc nhutnguyenkc force-pushed the rzg3s_support_can branch 2 times, most recently from 91dbed3 to a334f9e Compare March 17, 2025 06:56
@nhutnguyenkc
Copy link
Collaborator Author

Rebase to resolve conflicts

@nhutnguyenkc
Copy link
Collaborator Author

Just rebase to resolve conflicts

@nhutnguyenkc
Copy link
Collaborator Author

nhutnguyenkc commented Mar 28, 2025

Hi @thaoluonguw , @duynguyenxa , could you please help me review this PR to move it forward? Thanks

binhnguyen2434
binhnguyen2434 previously approved these changes Apr 1, 2025
Copy link
Collaborator

@binhnguyen2434 binhnguyen2434 left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +77 to +78
| CANFD | on-chip | can |
+-----------+------------+-------------------------------------+
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nhutnguyenkc : May I confirm that do you have plan to have a PR to update for document of current RZ boards?
If it is yes, I am okie with this temporary update. Thanks.

Copy link
Collaborator Author

@nhutnguyenkc nhutnguyenkc Apr 14, 2025

Choose a reason for hiding this comment

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

@thaoluonguw Yes, I do. Since there are some other PRs waiting for merging, so I don't want to rebase all of them. I will update it in a separate PR.

@@ -0,0 +1,11 @@
# Copyright (c) 2024 Renesas Electronics Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nhutnguyenkc : Please update for copyright year. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I updated copyright year

@@ -0,0 +1,1048 @@
/*
* Copyright (c) 2024 Renesas Electronics Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update for copyright year.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated copyright year

Comment on lines 864 to 873
LOG_DBG("CAN bus initialize failed");
return -EIO;
}

/* Put CAN controller into stopped state */
ret = cfg->fsp_api->modeTransition(data->fsp_ctrl, CAN_OPERATION_MODE_HALT,
CAN_TEST_MODE_DISABLED);
if (ret != FSP_SUCCESS) {
cfg->fsp_api->close(data->fsp_ctrl);
LOG_DBG("CAN bus initialize failed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The debug log information is same with previous failing. Can we improve for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed it. Please help me check again.

Comment on lines +44 to +49
can_clk: can-clk {
compatible = "fixed-clock";
clock-frequency = <0>;
#clock-cells = <0>;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I confirm the meaning of this adding? I think that this is external clock, and it doesn't belong to Soc layer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case of G3S, it is allowed to choose clock source from on-chip peripheral clock or external clock. Users can choose to use external clock by overriding this node's frequency at board level. Btw, I add a note External CAN clock - to be overridden by boards that provide it to this node

@@ -0,0 +1,12 @@
# Copyright (c) 2024 Renesas Electronics Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update the copyright year.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated copyright year

@@ -0,0 +1,26 @@
# Copyright (c) 2024 Renesas Electronics Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated copyright year

Hieu Nguyen added 3 commits April 14, 2025 17:14
Add CAN driver support for Renesas RZ/G3S

Signed-off-by: Hieu Nguyen <[email protected]>
Signed-off-by: Nhut Nguyen <[email protected]>
Add CAN nodes to Renesas RZ/G3S devicetree

Signed-off-by: Hieu Nguyen <[email protected]>
Signed-off-by: Nhut Nguyen <[email protected]>
Add CAN support for board RZ/G3S-SMARC

Signed-off-by: Hieu Nguyen <[email protected]>
Signed-off-by: Nhut Nguyen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants