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

Review initial state of pi_ctl_cdr #17

Open
sgherbst opened this issue Apr 30, 2020 · 2 comments
Open

Review initial state of pi_ctl_cdr #17

sgherbst opened this issue Apr 30, 2020 · 2 comments
Assignees

Comments

@sgherbst
Copy link
Contributor

Based on our discussion today, we want to make sure that there is not a start-up issue here (x's or values that prevent proper starting of clk_adc)

@sgherbst sgherbst self-assigned this Apr 30, 2020
@sgherbst
Copy link
Contributor Author

sgherbst commented May 1, 2020

This is the code snippet showing how the PI code is set:

genvar j;
generate
for (j=0; j<Nout; j=j+1) begin
assign scale_value[j] = (ddbg_intf_i.en_ext_max_sel_mux ? ddbg_intf_i.ext_max_sel_mux[j] : adbg_intf_i.max_sel_mux[j])<<4;
assign unscaled_pi_ctl[j] = pi_ctl_cdr[j] + ddbg_intf_i.ext_pi_ctl_offset[j];
assign scaled_pi_ctl[j] = unscaled_pi_ctl[j]*scale_value[j];
assign int_pi_ctl_cdr[j] = scaled_pi_ctl[j] >> Npi;
end
endgenerate

Since en_ext_max_sel_mux currently defaults to 0, it relies on the analog core to have a reasonable value for max_sel_mux. The analog core, in turn, computes max_sel_mux from Qperi, which currently defaults to int_Qperi. Finally, int_Qperi is computed from combo logic on arb_out, which is all x's on startup.

At a minimum, I think we should change the default for en_ext_max_sel_mux to 1. But we might also consider solving through #62 (be able to set PI codes directly from the outside), and default to external codes on startup.

Last thing -- why does this currently work in simulation? The reason has to do with the way that Verilog simulators handle x's in conditional statements. Consider this code that computes int_Qperi:

always_comb begin
for (int i=1; i<Nunit;i++) begin // thm to bin (it is not a regular thm)
if (arb_out[i-1]&~arb_out[i]) begin
int_Qperi = unsigned'(i);
break;
end
else int_Qperi = Nunit-1;
end
end

When the boolean condition evalues to x, the simulator evaluates the the else condition, setting int_Qperi to Nunit-1. So x's are blocked at this point, resulting in a misleading sense of security...

@sgherbst
Copy link
Contributor Author

sgherbst commented May 1, 2020

I'll assign this @zamyers since it seems both solutions mentioned above involve updates to digital_core or its register defaults. But it would be good to get some feedback from @sjkim85 on this too.

@sgherbst sgherbst assigned zamyers and unassigned sgherbst May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants