-
Notifications
You must be signed in to change notification settings - Fork 154
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
Add SGMII support to clash-cores
#2727
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls apply everywhere
c852e2a
to
4a4504b
Compare
I didn't look into the implementation just at the changed files. But is it possible to separate the 8b/10b encoding from SGMII? There are a ton of more uses for it. |
Should very much be possible, yes, and I think it would make sense to handle this as a separate PR as well to break things up a bit. Do note that this is not a very 'advanced' 8b/10b implementation as it's basically just two large lookup tables. |
67c16fd
to
3bb4275
Compare
I have moved the 8b/10b encoder and decoder to a different branch, see #2738. |
ec5369d
to
90f4649
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use 8b10b
instead of EbTb
; this makes it easier for people to find/google the 8b10b encoders and decoders.
9fb6324
to
cae2505
Compare
fd13723
to
587600f
Compare
219894c
to
2223b6f
Compare
73f1eaa
to
d740caf
Compare
d14276b
to
107535e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM module the one question/comment
Great, thank you very much! |
Is there anything specific that still needs to happen for this to be merged? I know that |
I will move it soon. With CI passing and Christiaan giving the go-ahead, I don't think there's anything else preventing this from being merged. Could you write what you'd like the commit message to be? That way I don't have to come up with one. |
Thanks! The title of this PR (Add SGMII support to |
I'd like to suggest using the PR cover letter as detailed message (some slight reformatting):
|
Ah, that looks good to me! |
Heh. Maybe change it a bit more. The "add X to
That okay? |
LGTM! |
Merged into |
Add support for connecting to Ethernet PHYs using SGMII to
clash-cores
.References: SGMII and IEEE 802.3.
Still TODO:
Write a changelog entry (see changelog/README.md)