soc: added nxp rw soc i2s clock init

[JoeyFreeland-DojoFive]

I2S clock init was missing from the NXP RW soc file. I2S clock init was added to all five flexcomm interfaces. Addresses #87239.

[github-actions]

Hello @JoeyFreeland-DojoFive, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[JoeyFreeland-DojoFive]

@DerekSnell @dleach02 @mmahadevan108 I'm not sure that using the FRG is the right thing to do. At 44.1KHz sampling rate, the actual clock rate differs from the calculated clock rate by 1.2%. It may be that using the AVPLL is the right thing to do?

[DerekSnell]

Hi @JoeyFreeland-DojoFive ,
Thank you for contributing this. I do see that the MCUXpresso SDK I2S examples clock the Flexcomm from the Audio PLL. And currently in Zephyr in this SOC, the Audio PLL also clocks the DMIC. So it makes sense both the DMIC and I2S are using the same clock source.

Also, you will need to fix the compliance issues to pass CI so we can merge this. Thanks again

[JoeyFreeland-DojoFive]

@DerekSnell This is going to be problematic though... the PLL selection will really depend on what sampling rate your audio is at. For example, if you are doing 44.1KHz audio, you'll want the audio PLL to be at 11.2896 MHz since that perfectly divides into a 44.1KHz clock. However, if you are using 48Khz, you will want an audio PLL of 12.288 MHz. Since this clock init happens before the I2S init, I'm not sure how that could work. Unless the I2S init function re-configures the audio PLL. Thoughts? This is where the Zephyr board abstractions start to break down a little bit.

[DerekSnell]

@DerekSnell This is going to be problematic though... the PLL selection will really depend on what sampling rate your audio is at.

Hi @JoeyFreeland-DojoFive , you are right. There is no single clock config that can meet the needs of every use-case. Configuring the SOC clocks should be done by the application. This clock_init() is declared as weak so custom boards and apps can override it.

I am fine with either option, the Audio PLL or FRGs. To me, the goals of this SOC clock_init() are to enable as much as we can for ease of use, enable the unit tests for CI, and provide guidance for using the SOC. Therefore, I approve.

Thank you

[JoeyFreeland-DojoFive]

I'm having problems getting the AVPLL setup. The output is not matching what I'm configuring it as. I'm going to get that sorted out before pushing this across the finish line.

[JoeyFreeland-DojoFive]

@DerekSnell I have tested on my custom IRIS-W10 board, and the frdm_rw612 board now... I routed the audio PLL output to the MCLK pin to monitor on a logic analyzer. I'm finding that the actual output frequency is around 64% of whatever I specify in the code. Has the audio PLL functionality been verified on this board before?

[DerekSnell]

Has the audio PLL functionality been verified on this board before?

Hi @JoeyFreeland-DojoFive ,
Yes, the audio PLL has been verified. As I shared above, the MCUXpresso SDK I2S examples clock the Flexcomm from the Audio PLL. That example is for a different board RD-RW612-BGA. But it uses the same RW612 SOC, and the same clock config code for the audio PLL will work on the FRDM-RW612 board.

You can also use the MCUXpresso Config Tool with the Clock Config tool to configure the Audio PLL for the FRDM-RW612 board. These are snippets from the GUI in the Clock Config tool for configuring the Audio PLL:

Thanks for working on this. Best regards

[JoeyFreeland-DojoFive]

@DerekSnell I still feel like something is up here. I was able to get the I2S examples working on my FRDM-RW612 just fine, building and launching through MCUExpresso. I am even able to get the AVPLL working fine on Zephyr, using my soc.c patch. But when I enable CONFIG_SPEED_OPTIMIZATIONS=y in my zephyr build for the FRDM-RW612, the AVPLL clock becomes pretty inaccurate. Looking at a register diff, I see that the AVPLL calibration never completes. So I want to make sure I can identify the root cause here.

[JoeyFreeland-DojoFive]

@DerekSnell I tried using MCUExpresso again (without Zephyr), and I modified the example I2S project to build with -O2, the same thing that Zephyr does with CONFIG_SPEED_OPTIMIZATIONS. I wanted to test this completely outside of Zephyr to eliminate any possibility that Zephyr was causing the issue. I found the same thing happening. With -O2 enabled, the AVPLL fails to complete calibration. I was looking at fsl_clock.c, specifically at the function CLOCK_Delay, and I found that above this function it says, in the comment: /* Workaround iar/armgcc optimization issue */. Do you know the history of this? It seems like possibly optimization is messing with our ability to delay, which is preventing us from properly calibrating the AVPLL.

[JoeyFreeland-DojoFive]

@DerekSnell I have come probably as far as I want to go on this. I added __attribute__((__noinline__)) to the function CLOCK_DelayUs in fsl_clock.c, and that indeed fixed the problem. It seems that the function was being inlined due to optimizations, and something about it caused an issue. I don't really have the time or resources to track it down any further.

It's tough to determine whether or not this issue is in-scope with this PR. The Wifi shell example does enable CONFIG_SPEED_OPTIMIZATIONS for the frdm_rw612 board. And I think this is a potentially large bug in the fsl_clock.c

[DerekSnell]

Sorry @JoeyFreeland-DojoFive ,
I learned that NXP's downstream release already has I2S support, and is using the Audio PLL. That soc.c includes the clock config code for the Audio PLL and for the Flexcomms. NXP has not upstreamed this I2S feature yet. Sorry I was not aware of this before.

I built and ran the tests/drivers/i2s/i2s_speed test on the rd_rw612_bga board, and it passes. It also passes when CONFIG_SPEED_OPTIMIZATIONS=y, so I do not see the same issue you are reporting with those optimizations. We can continue to investigate that optimization issue if you can help me replicate it.

Sorry to do this after all your work, but now that I am aware of this downstream code and clock config, I am blocking your PR. Since the downstream clocks the I2S Flexcomms from the Audio PLL, I do not think we should merge this PR clocking from the FRGs.

Thanks again for your contribution and all your efforts. If you change your PR to match the downstream, I'll be happy to approve this.

[DerekSnell]

Since the downstream clocks the I2S Flexcomms from the Audio PLL, I do not think we should merge this PR clocking from the FRGs.

[decsny]

@DerekSnell we should not block upstream community contributions based on something from the downstream, if the downstream and upstream don't match, that is our fault. The i2s was enabled in the downstream for a long time and I don't know why it wasn't upstreamed. In fact I think some of the original commits were even dropped from the downstream during some history rewrite, so this has been managed poorly on the NXP side. I am okay with this PR to merge then @EmilioCBen is already going to look at it more closely and add testing support of the i2s.

[JoeyFreeland-DojoFive]

Also - skimming over tests/drivers/i2s/i2s_speed, it looks like it's just testing that transfers with various sampling rates work. It doesn't really test the actual hardware speed of the clock (correct me if I'm wrong, though). From my experience, the peripheral will still clock out data even if the source clock isn't quite correct.

I would love to get to a point where you've recreated my issue, because I do think it is important. But I don't have the bandwidth within my professional work to do that right now. Basically I took the I2S sample from the rd_rw612_bga, copied over the flash config from a frdm_rw612 example (so that it could run on frdm_rw612) and modified the main file to output the Audio PLL on the MCLK pin (GPIO5). I built with -O2 and observed that the actual MCLK output was about 65% of what it should be, based on how the AVPLL was being configured. I then rebuilt without optimizations to and verified the clock frequency was correct. Maybe I could throw that project into a repo sometime and send it over.

The best option is for sure to use the AVPLL for the clock source. The FRG can get you close, but the whole reason the AVPLL exists is to get your sampling rates exact. I think the ultimate solution would be both a modification to soc.c to set up the clocks, and then a modification to the I2S driver to re-configure the AVPLL based on the sample rate passed into i2s_configure.

[EmilioCBen]

Hey @JoeyFreeland-DojoFive
Thank you for your contributions, I will look into testing and specifically using the AVPLL for the clock source.

[DerekSnell]

Discussing with @decsny , he raises a good point above, and I want to clarify why I blocked this PR. The reason I am blocking this is not only because the code differs from NXP's downstream. Earlier in the discussion, both @JoeyFreeland-DojoFive and I agreed that clocking from the Audio PLL is ideal. That is also how the I2S is clocked in the MCUXpresso SDK examples, and in the downstream repo. And Joey already had 3 approvals for this PR clocking from the FRGs, but was still working to use the Audio PLL instead for better frequncy accuracy. Since the preferred solution is to clock from the audio PLL, I blocked the approved PR to prevent someone else from merging it. And now NXP recognizes that the I2S feature in the downstream was not upstreamed, and NXP plans to upstream this now, so Joey does not need to be burdened by this.

And @JoeyFreeland-DojoFive , thanks for clarifying about the audio PLL accuracy with optimizations. I assumed that "the AVPLL fails to complete calibration" meant the app stopped executing with this failure. So I will investigate the audio PLL frequency in the i2s_speed test, and try to replicate the issue.

Best regards

[EmilioCBen]

I have added the changes here #87773