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

[cw] Update CW Python dependency #173

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Conversation

nasahlpa
Copy link
Member

@nasahlpa nasahlpa commented Oct 9, 2023

This PR updates the ChipWhisperer Python dependency to the latest available CW version.

For Husky, we need to make sure that the clkgen is locked after setting the clock.

@nasahlpa nasahlpa marked this pull request as ready for review October 9, 2023 13:45
@nasahlpa nasahlpa marked this pull request as draft October 9, 2023 13:46
@nasahlpa
Copy link
Member Author

nasahlpa commented Oct 9, 2023

Waiting until #PR466 gets merged.
Done.

This PR updates the ChipWhisperer Python dependency to the latest
available CW version.

For Husky, we need to make sure that the clkgen is locked after
setting the clock.

Signed-off-by: Pascal Nasahl <[email protected]>
@nasahlpa nasahlpa marked this pull request as ready for review October 12, 2023 05:35
Copy link
Collaborator

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @nasahlpa . LGTM with one question.

Comment on lines +155 to +158
# Make sure that clkgen_locked is true.
if husky:
scope.clock.clkgen_src = 'extclk'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? The comment seems not to be in line with the code. Also, we already set scope.clock.clkgen_src = 'extclk' above on Line 127.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this code line, the PLL was not locked making the ADC unable to lock itself. According to this, again setting clkgen_src fixes the issue, which also worked on my site.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, odd, seems to be done twice now. does the if in L125 work properly?
I'd guess it evaluates false there even on husky?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably what prevented me from separating CW310 and Husky into two separate classes ... so I'd be interested :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without Line 156 & 157, running the capture script for CW310 fails:

Traceback (most recent call last):

  File "/repo/cw/./capture.py", line 2267, in <module>
    app()

  File "/repo/cw/./capture.py", line 571, in aes_fvsr_key_batch
    capture_init(ctx, force_program_bitstream, num_traces, plot_traces)

  File "/repo/cw/./capture.py", line 183, in capture_init
    ctx.obj.ot = initialize_capture(cfg["device"], cfg["capture"])

  File "/repo/cw/./capture.py", line 100, in initialize_capture
    ot = device.OpenTitan(device_cfg["fpga_bitstream"],

  File "/repo/cw/util/device.py", line 68, in __init__
    self.scope = self.initialize_scope(scope_gain, num_samples, offset,

  File "/repo/cw/util/device.py", line 168, in initialize_scope
    raise RuntimeError(

RuntimeError: ADC failed to lock (attempts: 3).

I had a closer look into this issue and discovered that, after
scope.clock.clkgen_freq = pll_frequency
(Line 130) the PLL is not locked anymore and, subsequently, the ADC then also is not able to lock.
I don't know why again setting the clkgen_src fixes the issue but I assume that this is a bug in the CW framework. Alex from NewAE mentioned in the CW forum a similar issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, so L130 is in fact executed I understand.
tanks again - maybe will try split the two CW310 and Husky into two separate classes instead of OpenTitan in device.py later with this fix

Comment on lines +155 to +158
# Make sure that clkgen_locked is true.
if husky:
scope.clock.clkgen_src = 'extclk'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, so L130 is in fact executed I understand.
tanks again - maybe will try split the two CW310 and Husky into two separate classes instead of OpenTitan in device.py later with this fix

@nasahlpa nasahlpa merged commit 7759e1e into lowRISC:master Oct 16, 2023
3 checks passed
@nasahlpa nasahlpa deleted the update_cw branch October 16, 2023 09:54
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

Successfully merging this pull request may close these issues.

3 participants