Fix ADC Missed 1 Bit Error

[linuswck]

Description of Changes

I found this when I was trying to add a register to read out the ADC data for artiq_sinara_tester for production test. There is one bit that is missed and not read for each transaction. This is due to SCK in the current gateware not being idle HIGH as specified in the LTC2323-16 datasheet. See below for reference.

Note: Timing Diagram from LTC2323-16 datasheet

To fix it, I flipped the SCK period and adjust the t_conv(3->4) and t_rtt(6->5) accordingly to meet the timings,

Original Code Captured Waveform

I probed the ADC input directly at the input capacitor. That is around -557.875mV across. Given Vref of 4.096V, the LSB size is 125uV (given in the datasheet Transfer Function Section). Therefore, if it is read correctly, the readings should be 0xEE91. However, the value read is 0xF748 instead. This is due to the last bit is not clocked in by the gateware.

Notice the SCK here in the original code is idle low. Since the ADC data is clocked out at the falling edge, at the end of the transaction, there is a bit missed and is not read by the gateware (circled in red). See the captured example waveform below.

To verify the gateware did missed the last bit, I added a phaser register to access the adc readings via Artiq. The gateware did miss the last bit of the adc readings.

Note: SCK is slowed down to account for the logic analyzer bandwidth.

PR Code Captured Waveforms

Flipping the output of SCK can already fix this issue. After the change, it now reads all the 16 bit readings and close to the theoretical value(0xEE91). See the captured waveform below.

Note: SCK is slowed down to account for the logic analyzer bandwidth.

See the captured waveform below for the t_DCNVSCKL timing requirement at the original SCK frequency(83.3MHz). SCK stays quiet for at least 10ns after the falling edge of CNVN.

Note: CNVN: IC24's Output | SCK+: R5 ADC_SCK_P Net

The sampling frequency remains unchanged.

At the original SCK speed with the PR fix, I read the adc readings via Artiq with the newly added phaser register. All 16 bits are present and the value read is closed to the theoretical value. So, the gateware is sampling on the right edge.

[jordens]

I don't follow your numbers. There is information missing.
While unconventional, I don't see the problem with idle high. Apart from the drawing, there is nothing that requires it in the datasheet. Pretty sure it's fine.
You'll need to argue why your change is the right fix.
Please double check whether the timings are correct: For the actual speed, also tDCNVSCKL, and whether the gateware samples on the right edge.
Also please review @nkrackow

[linuswck]

I typed the unit wrongly. Should be mV instead of uV.
The original code has SCK idle low, which is not the same as specified in the timing diagram.

I have edited the PR message with clearer descriptions.

[jordens]

Thanks for the revised analysis.

I'm not happy with the timing on that tDCNVSCKL. We want to ensure it's at least 10 ns plus the prop delay of the CNV driver by looking at the code! Since the SCK cycle is 10 ns in the code, there needs to be at least another 4 ns delay cycle between the CNV assertion and the start of the SCK shifting. Either through the FSM logic or buffer/IO registers.

Also you don't make an attempt to argue why your change is the correct fix. You need to do that, otherwise it's accidental. Why not change the synthesizer reset value? Is that value correct?

Don't look at your scope so much. The sample rate is set in the code. It was either wrong before and/or it's wrong now. Can you check the t_read calculation and whether it meets the timing requirements now?

Regarding the sampling edge: "proof by example" doesn't work. Correct values are inconclusive. You have to analyze the code and the timings. If we touch this code, I would like the design to be correct.

[linuswck]

Correction: SCK should be 250MHz / 6 -> 83.3MHz. (Double Checked on oscilloscope)
The original comment in adc.py is misleading. I have corrected that.

The original code was designed to be idle high on simulation. I think the original author forgot that the polarity of SCK signal is swapped at the DiffrentialOutput primitives. Setting the ddr_clk_synth reset value to 0b111000 has the same effect. Flipping the polarity of SCK should be the correct change to be made.

For the measurement I took, I used two Rigol Pvp2350 Probe, which has an 50 pF rated capacitance, to probe the IC24's Output. Looking at the datasheet, the fall time is way out of spec. Therefore, without probing the IC24's Output. the fall time should be way better.

But, I agree with you that I should take a closer look at the timing. Previously, I didn't considered the IOB Delay. Turns out in the boundary cases, CNV_N relative to the SCK in worse case scenario can be arrived at the IC slower by ~4 ns.

If I also take the IOB delay into account, t_DCNVSCKL will be violated if I do not adjust the t_conv & t_rtt value.
I change the following values should meet the timings in all scenarios.
t_conv: 3 -> 4 & t_rtt: 6 -> 5.

Timings

t_read counter value is responsible for generating the required number of SCK cycles to clock out the register data. Since the PR only flips the SCK polarity, there is no timing changes related to t_read during the transaction. adc.done is issued just a sys clk cycle before the deassertion of CNVN. Given the long t_rtt time, the SDO should have been sampled correctly already before it is processed by other gateware logic.

Looking at the datasheet, swapping the polarity of SCK should only affect t_DCNVSCKL, tDCNVSDOZ timings.

Timing Requirements

SN65LVDS2DBVR propagation delay @ 30 degree = CNVN_Delay = 2.65 ns
t_DCNVSCKL = 1st SCK Falling Edge from CNVN assertion >= 9.5 ns
t_DCNVSDOZ = Last SCK Falling Edge to CNVN deassertion >= 19.1 ns

t_DCNVSDOZ should have no problem after flipping the SCK polarity. So, let's consider the worse case scenario for t_DCNVSCKL, let's name the delay to be ODDR_Delay = 3.836+0.113 ~ 4 ns

Expected Timings Before Change

t_DCNVSCKL = t_conv * sys_period + SCK_period/2 + ODDR_Delay - CNVN_Delay = 3 * 4 + 6 - ODDR_Delay - CNVN_Delay = 11.35 ns
t_DCNVSDOZ = t_rtt * sys_period + SCK_period/2 + ODDR_Delay + CNVN_Delay= 6 * 4 + 6 + ODDR_Delay + CNVN_Delay = 36.65 ns

Expected Timings After Change

t_DCNVSCKL = t_conv * sys_period + sys_period + ODDR_Delay - CNVN_Delay= 4 * 4 + 4 - ODDR_Delay - CNVN_Delay = 13.35 ns.
t_DCNVSDOZ = t_rtt * sys_period + SCK_Period + ODDR_Delay + CNVN_Delay= 5 * 4 + 12 + ODDR_Delay + CNVN_Delay = 38.65 ns

Simulations

I changed t_conv =4 & t_rtt = 5 for the upcoming simulations. Other parameters remain unchanged. For the sck to be visible on the timing simulation, I connect an ODDR like block with a fixed delay of 1 ns. For the actual timing calculations, we should take the report_datasheet values.

DO NOTE that as we cannot simulate any output primitives, the polarity SCK shown in the simulations should have been swapped when it got deployed on the hardware.

Original Code

t_DCNVSCKL = 64 ns - 42 ns - 4 ns - 2.65 ns = 15.35 ns
t_DCNVSDOZ = 274 ns - 244 ns + 4 ns + 2.65 ns = 36.65 ns

PR Code

t_DCNVSCKL = 62 ns - 42 ns - 4 ns - 2.65 ns = 13.35 ns
t_DCNVSDOZ = 274 ns - 242 ns + 4 ns + 2.65 ns = 38.65 ns

Set the ddr_clk_synth Reset Value to 0b111000

It shows the same CNVN and SCK waveform as the above PR Code.

[jordens]

Correction: SCK should be 250MHz / 6 -> 83.3MHz. (Double Checked on oscilloscope)

Certainly not.

Setting the ddr_clk_synth reset value to 0b111000 has the same effect.

Do that then! Less logic, fewer inversions, simpler.

Changing t_rtt needs a rationale as well. t_rtt is not just for tDSCKLCNVH but also for synchronizing the ret->sys crossing (not for sdo sampling, that's done by the source synchronous design). Changing clock polarity then has an affect on both factors.