M-Labs/artiq-zynq
10
1
Fork 19
Code Issues 12 Pull Requests 1 Releases Wiki Activity

Clock input settings improvements #152

Merged
sb10q merged 12 commits from mwojcik/artiq-zynq:clock_input_improv into master 2021-11-29 11:18:00 +08:00
Conversation 7 Commits 12 Files Changed 4 +25 -21
1 changed files with 25 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 90b6dd3e85 - Show all commits

26
src/runtime/src/rtio_clocking.rs
View File

@ -13,6 +13,7 @@ use libboard_artiq::si5324;
#[derive(Debug, PartialEq, Copy, Clone)] #[derive(Debug, PartialEq, Copy, Clone)]
#[allow(non_camel_case_types)] #[allow(non_camel_case_types)]
pub enum RtioClock { pub enum RtioClock {
Default,
Int_125, Int_125,
Int_100, Int_100,
Int_150, Int_150,
@ -32,12 +33,15 @@ fn get_rtio_clock_cfg(cfg: &Config) -> RtioClock {
"ext0_synth0_10to125" => RtioClock::Ext0_Synth0_10to125, "ext0_synth0_10to125" => RtioClock::Ext0_Synth0_10to125,
"ext0_synth0_100to125" => RtioClock::Ext0_Synth0_100to125, "ext0_synth0_100to125" => RtioClock::Ext0_Synth0_100to125,
"ext0_synth0_125to125" => RtioClock::Ext0_Synth0_125to125, "ext0_synth0_125to125" => RtioClock::Ext0_Synth0_125to125,
_ => RtioClock::Int_125 _ => {
warn!("Unrecognised rtio_clock setting. Falling back to default.");
RtioClock::Int_125
}
} }
} }
else { else {
info!("error reading configuration. Using default internal 125MHz clock"); info!("error reading configuration. Falling back to default.");
RtioClock::Int_125 RtioClock::Default
} }
} }
@ -46,11 +50,11 @@ fn init_rtio(timer: &mut GlobalTimer, _clk: RtioClock) {
#[cfg(has_rtio_crg_clock_sel)] #[cfg(has_rtio_crg_clock_sel)]
let clock_sel = match _clk { let clock_sel = match _clk {
RtioClock::Ext0_Bypass => { RtioClock::Ext0_Bypass => {
info!("using bypassed external clock"); info!("Using bypassed external clock");
1 1
}, },
x => { x => {
info!("using clock: {:?}", x); info!("Using internal RTIO clock");
0 0
} }
}; };
@ -92,11 +96,10 @@ fn init_drtio(timer: &mut GlobalTimer)
#[cfg(has_si5324)] #[cfg(has_si5324)]
fn setup_si5324(i2c: &mut I2c, timer: &mut GlobalTimer, clk: RtioClock) { fn setup_si5324(i2c: &mut I2c, timer: &mut GlobalTimer, clk: RtioClock) {
let mut si5324_settings: Option<si5324::FrequencySettings> = None; let si5324_settings = match clk {
// 125MHz output, from crystal, 7 Hz _ => { // 125MHz output, from crystal, 7 Hz, default, also covers RtioClock::Int_125
sb10q commented 2021-10-28 15:07:09 +08:00
Outdated
Review
Copy Link

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 2021-10-28 15:08:49 +08:00
Outdated
Review
Copy Link

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 2021-10-28 15:43:52 +08:00
Outdated
Review
Copy Link

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 2021-10-28 15:57:40 +08:00
Outdated
Review
Copy Link

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.
if si5324_settings.is_none() || clk == RtioClock::Int_125 {
info!("using internal 125MHz RTIO clock"); info!("using internal 125MHz RTIO clock");
si5324_settings = Some(si5324::FrequencySettings { si5324::FrequencySettings {
n1_hs : 10, n1_hs : 10,
nc1_ls : 4, nc1_ls : 4,
n2_hs : 10, n2_hs : 10,
@ -105,10 +108,11 @@ fn setup_si5324(i2c: &mut I2c, timer: &mut GlobalTimer, clk: RtioClock) {
n32 : 4565, n32 : 4565,
bwsel : 4, bwsel : 4,
crystal_ref: true crystal_ref: true
});
} }
}
};
let si5324_ref_input = si5324::Input::Ckin2; let si5324_ref_input = si5324::Input::Ckin2;
si5324::setup(i2c, &si5324_settings.unwrap(), si5324_ref_input, timer).expect("cannot initialize Si5324"); si5324::setup(i2c, &si5324_settings, si5324_ref_input, timer).expect("cannot initialize Si5324");
} }
pub fn init(timer: &mut GlobalTimer, cfg: &Config) { pub fn init(timer: &mut GlobalTimer, cfg: &Config) {