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

CML/GTX supports for EVR-VME300 #162

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Conversation

hongran
Copy link
Contributor

@hongran hongran commented Jun 13, 2024

The original PR #157 (#157) is moved here.

Quoted from PR #157:
Because the EVR-VME230 uses 20bit CML word rather than 40bit, I need to handle this exception for the VME64 form factor.
In the setPattern function for waveform mode, the bits and word arrangement does not work for the EVR-VME300. Since I don't have a EVR-VME230RF card at hand, it is hard for me to decide if it is an generic difference between the firmware versions. Therefore, I kept the old code only for the firmware version of the 230-series hardware.

Will people at ESS test if this patch works for the mtca EVR? (This change only affects the waveform mode, not the 4x pattern or the frequency mode.)

@gabrielfedel
Copy link
Contributor

gabrielfedel commented Jun 18, 2024

@hongran we won't be able to test this at ESS soon, as we don't have a EVR-MTCARF available. What I could do is to check if that change doesn't break anything that is still working, and it doesn't

@zioven mentioned that would test, but if you can't no problem.

My plan is to merge this at the end of the week to not block the work you have been doing for EVR-VME. Later when I get one EVR-MTCARF I will test this change properly.

@gabrielfedel gabrielfedel self-assigned this Jun 18, 2024
@hongran
Copy link
Contributor Author

hongran commented Jun 18, 2024

@hongran we won't be able to test this at ESS soon, as we don't have a EVR-MTCARF available. What I could do is to check if that change doesn't break anything that is still working, and it doesn't

@zioven mentioned that would test, but if you can't no problem.

My plan is to merge this at the end of the week to not block the work you have been doing for EVR-VME. Later when I get one EVR-MTCARF I will test this change properly.

Thanks for handling this. Making sure that your system is not broken is a good start. I will collaborate with you when you get the new hardware for the test.

@zioven
Copy link
Contributor

zioven commented Jun 20, 2024

@gabrielfedel Please don't merge this in yet.

The order of the bits seems to be swapped between the master and this pull request.

Below are the pictures from our tests.
We have used mTCA-EVR-300RF.
Outputs were triggered by Prescaler 7.

  1. Channel 1 of the oscilloscope was connected to the FP UNIV3
  2. Channel 2 of the oscilloscope was connected to the FP UNIV0

image

The pictures labeled master were tested on commit 115a53d
The pictures labeled patch were tested with applied 162.patch


Test 1

image
image


Test 2

image
image

I didn't have the time to test on the backplane with FCLKA or FCLKB but if possible I would like to see that this get's addressed before we do any additional tests.

@gabrielfedel
Copy link
Contributor

gabrielfedel commented Jun 20, 2024

Thanks for checking and testing this @zioven . I will wait until that issue is fixed and you give as a green-light to merge this.

@hongran
Copy link
Contributor Author

hongran commented Jun 20, 2024

Here are my test results. It seems that it is exactly the opposite to what Zioven saw.
Thest test was also done with a prescaler. My frequency is 29.34MHz.
With the patch, this is what I saw.
For bit 0-7
ConfigBits0-7
bit0-7

For bit 33-39
ConfigBits33-39
bit33-39

If I reverse the cpu word order is reversed to the existing one
For bit 0-7
ConfigBits0-7
bit0-7-oldwordorder

For bit 33-39
ConfigBits33-39
bit33-39-oldwordorder

Because the word order in the master branch does not match that for EVR-VME-300, if I use the waveform mode, the waveform may look like this.
waveform-oldwordorder

@hongran
Copy link
Contributor Author

hongran commented Jun 20, 2024

I guess this difference is either caused by a firmware version difference, or mtca and VME firmwares are different in this.
If mtca and VME different, in the code I will choose the bit order depending on the form factor.

@zioven What is the firmware version do you use?

@zioven
Copy link
Contributor

zioven commented Jun 21, 2024

@hongran the MTCA EVRs that we used for the testing have firmware 207.20 (checked by reading PV encding with *FwVer-I)

Based on what you described I guess the reason is the CPU word order, and I remembered that there is a special case of Endianness swap for the 300 series of cards (described here)

Could this affect be dependent on CPU/OS you are running on the VME? Or hardware difference between MTCA and VEM (if it uses PLX chip or not)?

The other comment that I have is that we haven't used the Waveform mode of the GTX outputs, at this time I can't comment on whether this mode works on MTCA platform.

Can you check the CPU word order with an asymmetric pattern to see how the CPU word swap affects the pattern generation?
I propose the pattern to be set to high between bits 25 and 36.

@hongran
Copy link
Contributor Author

hongran commented Jun 21, 2024

@hongran the MTCA EVRs that we used for the testing have firmware 207.20 (checked by reading PV encding with *FwVer-I)

Based on what you described I guess the reason is the CPU word order, and I remembered that there is a special case of Endianness swap for the 300 series of cards (described here)

Could this affect be dependent on CPU/OS you are running on the VME? Or hardware difference between MTCA and VEM (if it uses PLX chip or not)?

The other comment that I have is that we haven't used the Waveform mode of the GTX outputs, at this time I can't comment on whether this mode works on MTCA platform.

Can you check the CPU word order with an asymmetric pattern to see how the CPU word swap affects the pattern generation? I propose the pattern to be set to high between bits 25 and 36.

The firmware I use is 207.16. Since your version is even later and this issue appeared in earlier versions like 207.0C, I believe this difference is caused by the difference between mTCA and VME firmware builds.

I think the Endianness affects the bit order within a byte or word, but the problem here is about the order of the two whole 32-bit cpu words. Since we need 2 CPU words to store the 40-bits needed by 1 CML word, the issue is which word goes first to the CML first and the bit order for the partly filled word.

I tested this in mvme3100 and mvme2500 and they behave the same, but both of them are power PC. I haven't tried other architectures.

As for the waveform mode, it is affected in the same way as that for the 4x pattern. The function of writing to the CPU words is the setPattern function, which is used by both the 4x pattern mode and the waveform mode. The only difference is that the 4x pattern mode only uses the first 2 words in shadowPattern. So we can test the word order using either 4x pattern or the waveform.

@hongran
Copy link
Contributor Author

hongran commented Jun 21, 2024

I just compared the correct and incorrect word orders for my VME system. I used the CML output because it is faster can able to resolve single bit.
For the correct word order:
AsymmetricSetting
asymmetric1
The output agrees with the setting.

For the incorrect word order
AsymmetricSetting2
asymmetric2
Note that I made a longer consecutive bits from 28 to 33, and from the output the longer chunk comes later, so for the second word, it comes before the first word and the order of bits is reversed, too.

@hongran
Copy link
Contributor Author

hongran commented Jun 21, 2024

@zioven @gabrielfedel I just updated this commit so that it only affects the VME-300 card. Cards with other form factors or with the 230 series firmware are not affected.

@zioven, please verify that your mtca card works as expected.

Thanks!


formFactor form = owner.getFormFactor();

if (owner.version() >= MRFVersion(2, 7) && form == formFactor_VME64){
Copy link
Contributor

Choose a reason for hiding this comment

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

@hongran can the last condition be converted to be Endianness based check (EPICS_BYTE_ORDER) , and not platform dependent.

this check could also be implemented inside the for loop, where it would make it clear that only the indexes are calculated differently, but other parts are common between the different CPUs

Comment on lines 380 to 384
cpubit = 19 - cmlbit;
} else {
first = cmlbit==0 || cmlbit==32;
cpuword = 2*cmlword + (cmlbit<32 ? 1 : 0);
cpubit = cmlbit<32 ? cmlbit : (cmlbit-32);
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the changes in the line 384, I assume that the line 380 should be adapted as well.

@mdavidsaver can you please comment, or provide info, who might be using this at NSLS and who can test.

Copy link
Contributor Author

@hongran hongran Jun 24, 2024

Choose a reason for hiding this comment

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

HI @zioven , I am not sure if I can use the Endianness based check. I haven't tried any VME CPUs with little endian. I can certainly change the condition to an endianness check, if it is preferred.

I just moved the condition check into the for loop as you recommended. I put the condition outside the for loop so that it is not executed for each iteration. I am trying to minimize the time of writing. During the write time no signals will be generated or the timing is wrong. Since the Endianness check are macros, it won't affect the performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the changes in the line 384, I assume that the line 380 should be adapted as well.

@mdavidsaver can you please comment, or provide info, who might be using this at NSLS and who can test.

Line 380 affects only the hardware with 20-bit CML words, like EVR-VME-230RF. I don't have one for testing. I would keep it untouched so that existing systems are not affected. If people using the EVR-VME-230RF also found this issue, we can correct it then.

@hongran hongran force-pushed the cml-dev branch 2 times, most recently from c81c507 to 8c3aca0 Compare June 24, 2024 20:48
@hongran
Copy link
Contributor Author

hongran commented Jun 26, 2024

@zioven @gabrielfedel Is the PR ready for merging to master after my last update?

@gabrielfedel
Copy link
Contributor

gabrielfedel commented Jun 27, 2024

It looks ok for me but I would like that @zioven gives a green light from his side, as he has a setup for test the changes on MTCA.

@zioven
Copy link
Contributor

zioven commented Jun 27, 2024

I am working remotely this week, and can test early next week.

@hongran
Copy link
Contributor Author

hongran commented Jun 27, 2024

I am working remotely this week, and can test early next week.

Yes, Take your time.

@hongran
Copy link
Contributor Author

hongran commented Jul 17, 2024

Any updates on this test recently?

@hongran
Copy link
Contributor Author

hongran commented Aug 12, 2024

@zioven Hello, have you got a chance to test my most recent update yet? I still have quite a few updates to push. If this PR is merged, it will be easier for me to handle the rest of PRs.
Thanks!

@hongran
Copy link
Contributor Author

hongran commented Sep 9, 2024

I am working remotely this week, and can test early next week.

Hi @zioven , could we pick up this effort again and finish it up? I was busy with some RTEMS6 development and now I hope to come back and finish all my merges. This PR is a critical point for my later commits, so I hope to have the final test of my code regarding this issue and then move on to the PR.

Thanks!

Ran Hong

@jerzyjamroz
Copy link
Contributor

@zioven, @gabrielfedel, I checked the code, and it looks like this code does not affect anything that is little-endian, as it always has been. This means this commit is neutral for us, and tested by @hongran so useful for him.
Please advise if I missed something because I would merge that if no objections (I will wait 7 days for the answer).

@jerzyjamroz jerzyjamroz requested a review from zioven September 19, 2024 11:27
@jerzyjamroz jerzyjamroz mentioned this pull request Sep 20, 2024
@jerzyjamroz jerzyjamroz merged commit 6bb3b9a into epics-modules:master Sep 25, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants