Clock input settings improvements #152

Merged
sb10q merged 12 commits from mwojcik/artiq-zynq:clock_input_improv into master 1 year ago
mwojcik commented 1 year ago
Owner

As mentioned in https://github.com/m-labs/artiq/issues/1735 - this is the Zynq version.

The ARTIQ part was also created but I couldn't test it yet, so I did the Zynq part (logically it's pretty much the same). All builds compile, I tested this with zc706 of course, on standalone nist_qc2 (which has rtio_crg_clk_sel but no si5324) and master (si5324 but no rtio_crg_clk_sel) and of course since there's no external clock connected PLL won't lock. *

As currently there are only two configurations available for Zynq (125MHz internal or bypass, kind of - bypass/"external" was available for crg_clk_sel but not for si5324, so I added that akin to mainline ARTIQ), there wasn't much to test.

I'm also not sure on the terminology. I only inferred that before, "internal" (or "i" in ARTIQ) referred to anything that didn't bypass the PLL or CRG, and "external" (or "e") was bypassing whatever there was. Now when you think about it, there's external bypass, internal (crg?), external reference synthesized, and external from crystal (si5324) and I'm not sure if it makes much of a difference.

The RtioClock enum in rtio_clocking.rs also reflects my findings from ARTIQ as I started working on that first. I could upload the ARTIQ branch as a WIP pull request, but I'd rather have all questions answered and fixed here and move all the changes at once.

* there's also the issue that if PLL won't lock, program will not continues, nothing else will run, and thus changing the setting with artiq_coremgt is not possible, the only option being recompiling the firmware to a version that ignores the setting changing the setting manually on the SD card (or connecting external bypass clock). I mean, it was already a present issue, but with limited scope (only two options) and to change them you had to take out the SD card anyway. Could ignore it, or try to mitigate it (e.g. by trying to lock the PLL for x seconds, and returning to default int_125 if that doesn't work, with a stern warning).

As mentioned in https://github.com/m-labs/artiq/issues/1735 - this is the Zynq version. The ARTIQ part was also created but I couldn't test it yet, so I did the Zynq part (logically it's pretty much the same). All builds compile, I tested this with zc706 of course, on standalone nist_qc2 (which has rtio_crg_clk_sel but no si5324) and master (si5324 but no rtio_crg_clk_sel) and of course since there's no external clock connected PLL won't lock. * As currently there are only two configurations available for Zynq (125MHz internal or bypass, kind of - bypass/"external" was available for crg_clk_sel but not for si5324, so I added that akin to mainline ARTIQ), there wasn't much to test. I'm also not sure on the terminology. I only inferred that before, "internal" (or "i" in ARTIQ) referred to anything that didn't bypass the PLL or CRG, and "external" (or "e") was bypassing whatever there was. Now when you think about it, there's external bypass, internal (crg?), external reference synthesized, and external from crystal (si5324) and I'm not sure if it makes much of a difference. The ``RtioClock`` enum in rtio_clocking.rs also reflects my findings from ARTIQ as I started working on that first. I could upload the ARTIQ branch as a WIP pull request, but I'd rather have all questions answered and fixed here and move all the changes at once. \* there's also the issue that if PLL won't lock, program will not continues, nothing else will run, and thus changing the setting with ``artiq_coremgt`` is not possible, the only option being ~~recompiling the firmware to a version that ignores the setting~~ changing the setting manually on the SD card (or connecting external bypass clock). I mean, it was already a present issue, but with limited scope (only two options) and to change them you had to take out the SD card anyway. Could ignore it, or try to mitigate it (e.g. by trying to lock the PLL for x seconds, and returning to default int_125 if that doesn't work, with a stern warning).
mwojcik added 4 commits 1 year ago
mwojcik added 1 commit 1 year ago
sb10q reviewed 1 year ago
#[cfg(has_si5324)]
fn setup_si5324(i2c: &mut I2c, timer: &mut GlobalTimer, clk: RtioClock) {
let si5324_settings = match clk {
_ => { // 125MHz output, from crystal, 7 Hz, default, also covers RtioClock::Int_125
sb10q commented 1 year ago
Owner

This isn't right - if the clock setting (e.g. int_100, ext0_synth0_125to125, ...) is unrecognized, it should error out, not silently select one Si5324 configuration.

It is theoretically possible to support exactly the same clock settings on KC705, ZC706, Kasli and Kasli-SoC. For KC705 and ZC706 you'd need to forward the external clock input to the Si5324 and clock RTIO from the Si5324. But I don't think NIST (@dhslichter) want this change and we should only support the existing options (which would be int_125 and ext0_bypass_125 - and ext0_bypass_100 as per their new request) on KC705 and ZC706.

This isn't right - if the clock setting (e.g. int_100, ext0_synth0_125to125, ...) is unrecognized, it should error out, not silently select one Si5324 configuration. It is theoretically possible to support exactly the same clock settings on KC705, ZC706, Kasli and Kasli-SoC. For KC705 and ZC706 you'd need to forward the external clock input to the Si5324 and clock RTIO from the Si5324. But I don't think NIST (@dhslichter) want this change and we should only support the existing options (which would be ``int_125`` and ``ext0_bypass_125`` - and ``ext0_bypass_100`` as per their new request) on KC705 and ZC706.
sb10q commented 1 year ago
Owner

Also the only reason why all the mainline ARTIQ Si5324 settings aren't in artiq-zynq is they haven't been ported yet.

Also the only reason why all the mainline ARTIQ Si5324 settings aren't in artiq-zynq is they haven't been ported yet.
mwojcik commented 1 year ago
Poster
Owner

if the clock setting (e.g. int_100, ext0_synth0_125to125, ...) is unrecognized, it should error out, not silently select one Si5324 configuration.

True that. Unlike in mainline, there are now options that can pass by unnoticed, and this should be taken care of. I'll break it down and add a warning.

Also the only reason why all the mainline ARTIQ Si5324 settings aren't in artiq-zynq is they haven't been ported yet.

I assume then since they're not necessary yet, porting them makes no sense at this point? Even though it would be the matter of just moving these settings as is?

ext0_bypass_125 - and ext0_bypass_100 as per their new request

I know you mentioned exactly ext0_bypass_125 in artiq #1735, but from the perspective of this code I can't find a functional difference between bypass_125 and _100. Would you like to have them added as separate, recognised options (that would map to just bypass internally) so it's more explicit?

> if the clock setting (e.g. int_100, ext0_synth0_125to125, ...) is unrecognized, it should error out, not silently select one Si5324 configuration. True that. Unlike in mainline, there are now options that can pass by unnoticed, and this should be taken care of. I'll break it down and add a warning. > Also the only reason why all the mainline ARTIQ Si5324 settings aren't in artiq-zynq is they haven't been ported yet. I assume then since they're not necessary yet, porting them makes no sense at this point? Even though it would be the matter of just moving these settings as is? > ext0_bypass_125 - and ext0_bypass_100 as per their new request I know you mentioned exactly ``ext0_bypass_125`` in artiq #1735, but from the perspective of this code I can't find a functional difference between bypass_125 and \_100. Would you like to have them added as separate, recognised options (that would map to just bypass internally) so it's more explicit?
sb10q commented 1 year ago
Owner

Unlike in mainline, there are now options that can pass by unnoticed

The situation here has absolutely no difference with mainline.
Mainline has KC705 (external/internal) and Kasli (via Si5324).
Here we have ZC706 (external/internal) and Kasli-SoC (via Si5324).

I assume then since they're not necessary yet, porting them makes no sense at this point?

It makes sense for Kasli-SoC.

from the perspective of this code I can't find a functional difference between bypass_125 and _100

Right, just match any of them (explicitly) and select the bypass.

> Unlike in mainline, there are now options that can pass by unnoticed The situation here has absolutely no difference with mainline. Mainline has KC705 (external/internal) and Kasli (via Si5324). Here we have ZC706 (external/internal) and Kasli-SoC (via Si5324). > I assume then since they're not necessary yet, porting them makes no sense at this point? It makes sense for Kasli-SoC. > from the perspective of this code I can't find a functional difference between bypass_125 and _100 Right, just match any of them (explicitly) and select the bypass.
mwojcik added 2 commits 1 year ago
mwojcik added 1 commit 1 year ago
mwojcik added 1 commit 1 year ago

For KC705 and ZC706 you'd need to forward the external clock input to the Si5324 and clock RTIO from the Si5324. But I don't think NIST (@dhslichter) want this change and we should only support the existing options (which would be int_125 and ext0_bypass_125 - and ext0_bypass_100 as per their new request) on KC705 and ZC706.

For our KC705 and ZC706, we use an external clock at the SMA pins, so it does not pass through the Si5324. It would need to be looped back through the FPGA to the Si5324 and then sent back in to the FPGA from there, I suppose? However, all of this seems like an inferior solution to just using the clock directly from the SMA, and skipping the Si5324 entirely, if KC705 and ZC706 are the DRTIO root.

Am I understanding your question correctly on this @sb10q?

> For KC705 and ZC706 you'd need to forward the external clock input to the Si5324 and clock RTIO from the Si5324. But I don't think NIST (@dhslichter) want this change and we should only support the existing options (which would be int_125 and ext0_bypass_125 - and ext0_bypass_100 as per their new request) on KC705 and ZC706. For our KC705 and ZC706, we use an external clock at the SMA pins, so it does not pass through the Si5324. It would need to be looped back through the FPGA to the Si5324 and then sent back in to the FPGA from there, I suppose? However, all of this seems like an inferior solution to just using the clock directly from the SMA, and skipping the Si5324 entirely, if KC705 and ZC706 are the DRTIO root. Am I understanding your question correctly on this @sb10q?

I would add that for debugging purposes, it's useful for us to have int_100 available as well, so that we can do bench tests and verify performance at 100 MHz DRTIO clock without having to hook up to an external signal source to supply that clock.

I would add that for debugging purposes, it's useful for us to have `int_100` available as well, so that we can do bench tests and verify performance at 100 MHz DRTIO clock without having to hook up to an external signal source to supply that clock.
mwojcik added 1 commit 1 year ago
mwojcik added 1 commit 1 year ago
Poster
Owner

I would add that for debugging purposes, it's useful for us to have int_100 available as well, so that we can do bench tests and verify performance at 100 MHz DRTIO clock without having to hook up to an external signal source to supply that clock.

This should be covered now: a82e7a332b/src/runtime/src/rtio_clocking.rs (L158)

> I would add that for debugging purposes, it's useful for us to have `int_100` available as well, so that we can do bench tests and verify performance at 100 MHz DRTIO clock without having to hook up to an external signal source to supply that clock. This should be covered now: https://git.m-labs.hk/M-Labs/artiq-zynq/src/commit/a82e7a332bd1091b46f618f5ab438e2354e1d324/src/runtime/src/rtio_clocking.rs#L158
Owner

They don't have Kasli-SoC, they have ZC706. The root clock doesn't go through the Si5324 there and int_100 would be more complicated - needs DRP of a FPGA PLL. I don't think it's worth the trouble.

They don't have Kasli-SoC, they have ZC706. The root clock doesn't go through the Si5324 there and ``int_100`` would be more complicated - needs DRP of a FPGA PLL. I don't think it's worth the trouble.
Poster
Owner

Ah, I thought it goes through Si5324 on DRTIO variants - Standalone of course wouldn't work. But yeah, getting DRP to work would be more work than it's worth.

Ah, I thought it goes through Si5324 on DRTIO variants - Standalone of course wouldn't work. But yeah, getting DRP to work would be more work than it's worth.
Owner

Ah, I thought it goes through Si5324 on DRTIO variants

Only for the satellites.

At some point we probably want a similar clock config entry for the satellites, though I think only the RTIO frequency needs to be set. In all systems currently, the clock source is always the recovered transceiver clock. If that's doable at all - there are FPGA PLLs that need to be kept within operating specs and the simplest thing in this case would be to define the clock frequency at gateware compilation time (as is done now).

> Ah, I thought it goes through Si5324 on DRTIO variants Only for the satellites. At some point we probably want a similar clock config entry for the satellites, though I think only the RTIO frequency needs to be set. In all systems currently, the clock source is always the recovered transceiver clock. If that's doable at all - there are FPGA PLLs that need to be kept within operating specs and the simplest thing in this case would be to define the clock frequency at gateware compilation time (as is done now).

If there is no int_100 that's not a huge deal for us -- we can always find ways to supply an external 100 MHz clock. It's a convenience feature only, and if it's a lot of work it's not worth it.

If there is no `int_100` that's not a huge deal for us -- we can always find ways to supply an external 100 MHz clock. It's a convenience feature only, and if it's a lot of work it's not worth it.
mwojcik added 1 commit 1 year ago
sb10q merged commit 8be5048cd3 into master 1 year ago
The pull request has been merged as 8be5048cd3.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.