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

Fix renesas pll clock config #79766

Merged

Conversation

duynguyenxa
Copy link
Member

@duynguyenxa duynguyenxa commented Oct 14, 2024

The new update of clock device tree make the pll p q r clock source cannot be chosen
This fix adds 1 new dts binding for pll out p q r out line so that it can be select by others module
Change clock div value get in clock control driver to get direct value from device tree instead of via hal API.

Fix for PR: #78365

Fixes #80889

@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 14, 2024

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

Name Old Revision New Revision Diff
hal_renesas zephyrproject-rtos/hal_renesas@3dafd03 zephyrproject-rtos/hal_renesas@1032651 (main) zephyrproject-rtos/[email protected]

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 Oct 14, 2024
The new update of clock device tree make the pll p q r clock
source cannot be choose by other node
This fix add 1 new dts binding for pll out p q r out line

Signed-off-by: Duy Nguyen <[email protected]>
@duynguyenxa duynguyenxa force-pushed the fix_renesas_pll_clock_config branch from 7c37bac to 752c023 Compare October 14, 2024 05:59
@duynguyenxa duynguyenxa added the bug The issue is a bug, or the PR is fixing a bug label Oct 14, 2024
thaoluonguw
thaoluonguw previously approved these changes Oct 17, 2024
west.yml Outdated
@@ -214,7 +214,7 @@ manifest:
- hal
- name: hal_renesas
path: modules/hal/renesas
revision: 3dafd030046f8d6f8a26080e9b9c1bcc92d45999
revision: pull/40/head
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 commit ID.

@KhiemNguyenT KhiemNguyenT added this to the v4.0.0 milestone Oct 17, 2024
div:
required: true
type: int
freq:
Copy link
Member

Choose a reason for hiding this comment

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

The frequency originally input to the PLL is multiplied and divided by PLLMUL and PLLDIV, and the divided frequency is then further divided by P, Q, and R to determine the frequency.
Is the frequency set here reflecting PLLMUL and PLLDIV?
I would like to find this calculately if possible, but if it becomes too complicated I think this is fine.

Copy link
Member Author

@duynguyenxa duynguyenxa Oct 17, 2024

Choose a reason for hiding this comment

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

@soburi , The fix is not relate to the calculation of PLL P Q R frequency but to separate them as device node so that other node can choose them as clock source, the previous patch only allow the node to choose <&pll> as clock source, now we can set <&pllp>, <&pllq>, <&pllr>. Frequency calculation is as your understanding.
Your request here is to create a macro for the frequency calculation instead of set them on device tree, is my understanding correct ?

Copy link
Member

@soburi soburi Oct 17, 2024

Choose a reason for hiding this comment

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

@duynguyenxa

I understood why the change is necessary.
This change separates PLLP, Q, and R into independent nodes, so I think it becomes necessary to correctly express their relationship with the PLL.

What I'm paying attention to is whether the device tree give an appropriate expression for the hardware configuration.

From the register configuration, it appears that PLLP, Q, and R are dividers that use the PLL as their source, so I don't think they have frequency attributes here, these has only 'ratio'.
Instead, I think we need to calculate the relationship to the PLL.
This is why we need a frequency calculation macro.

I think that a "fixed-factor-clock" like definition can be used for this node.

Copy link
Member Author

@duynguyenxa duynguyenxa Oct 24, 2024

Choose a reason for hiding this comment

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

@soburi , I got it, I think for each clock node here we will need a clock driver to provide enough information for the child node, current clock design for now there is no good way to get pll frequency. So now, to keep it simple I would like to keep this implementation,
Next patch for clock driver we will update driver for each clock node and change PLL Q P R into fixed-factor-clock.

Copy link
Member

Choose a reason for hiding this comment

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

I think there is no problem with proceeding as is, but if possible, it may be preferable to incorporate this change into 4.0.0 as well, since this would not result in any changes to the external interface.

Update hal renesas commit ID to resolve PLL clock
config issue

Signed-off-by: Duy Nguyen <[email protected]>
@duynguyenxa duynguyenxa force-pushed the fix_renesas_pll_clock_config branch from 752c023 to a7fa7aa Compare October 24, 2024 06:07
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Oct 24, 2024
@thaoluonguw
Copy link
Collaborator

Hello @nordic-krch , this is bug fixing for RA8. May I confirm that it can be merged in v4.0.0 RC2?

@mmahadevan108
Copy link
Collaborator

@duynguyenxa Can you please add an issue with details and link that issue to this PR. After RC2 only bug fixes with an associated Github issue will be merged into the upcoming 4.0 release.

@duynguyenxa
Copy link
Member Author

@duynguyenxa Can you please add an issue with details and link that issue to this PR. After RC2 only bug fixes with an associated Github issue will be merged into the upcoming 4.0 release.

@mmahadevan108 , I created #80889 ticket for this PR

@mmahadevan108 mmahadevan108 merged commit 639d9ae into zephyrproject-rtos:main Nov 5, 2024
30 of 31 checks passed
@thaoluonguw thaoluonguw deleted the fix_renesas_pll_clock_config branch January 22, 2025 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Clock Control bug The issue is a bug, or the PR is fixing a bug manifest manifest-hal_renesas platform: Renesas RA Renesas Electronics Corporation, RA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renesas RA "PLL P Q R" line clock cannot be choose as source clock
7 participants