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

Add SGMII support to clash-cores #2727

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

jvnknvlgl
Copy link
Collaborator

@jvnknvlgl jvnknvlgl commented May 28, 2024

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)
  • Check copyright notices are up to date in edited files

Copy link
Contributor

@lmbollen lmbollen left a comment

Choose a reason for hiding this comment

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

pls apply everywhere

clash-cores/src/Clash/Cores/Sgmii/AutoNeg.hs Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/Sgmii/AutoNeg.hs Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/Sgmii/AutoNeg.hs Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/Sgmii/AutoNeg.hs Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/Sgmii/AutoNeg.hs Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/Sgmii/Sync.hs Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/Sgmii/Sync.hs Outdated Show resolved Hide resolved
clash-cores/test/Test/Cores/Sgmii/AutoNeg.hs Outdated Show resolved Hide resolved
clash-cores/test/Test/Cores/Sgmii/EbTb.hs Outdated Show resolved Hide resolved
clash-cores/test/Test/Cores/Sgmii/EbTb.hs Outdated Show resolved Hide resolved
@jvnknvlgl jvnknvlgl force-pushed the jvnknvlgl/clash-cores/sgmii branch from c852e2a to 4a4504b Compare June 12, 2024 15:38
@jvnknvlgl jvnknvlgl self-assigned this Jun 19, 2024
@rowanG077
Copy link
Member

rowanG077 commented Jun 20, 2024

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.

@jvnknvlgl
Copy link
Collaborator Author

jvnknvlgl commented Jun 20, 2024

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.

@jvnknvlgl jvnknvlgl force-pushed the jvnknvlgl/clash-cores/sgmii branch from 67c16fd to 3bb4275 Compare June 21, 2024 09:08
@jvnknvlgl
Copy link
Collaborator Author

jvnknvlgl commented Jun 21, 2024

I have moved the 8b/10b encoder and decoder to a different branch, see #2738.

@jvnknvlgl jvnknvlgl force-pushed the jvnknvlgl/clash-cores/sgmii branch from ec5369d to 90f4649 Compare June 21, 2024 13:27
Copy link
Member

@christiaanb christiaanb left a 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.

clash-cores/clash-cores.cabal Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/EbTb.hs Outdated Show resolved Hide resolved
clash-cores/src/Clash/Cores/EbTb.hs Outdated Show resolved Hide resolved
@jvnknvlgl jvnknvlgl force-pushed the jvnknvlgl/clash-cores/sgmii branch 6 times, most recently from 9fb6324 to cae2505 Compare July 1, 2024 13:29
@jvnknvlgl jvnknvlgl force-pushed the jvnknvlgl/clash-cores/sgmii branch 4 times, most recently from fd13723 to 587600f Compare July 8, 2024 12:48
@jvnknvlgl jvnknvlgl requested a review from lmbollen July 8, 2024 15:05
@jvnknvlgl jvnknvlgl force-pushed the jvnknvlgl/clash-cores/sgmii branch 5 times, most recently from 219894c to 2223b6f Compare July 22, 2024 07:13
@jvnknvlgl jvnknvlgl force-pushed the jvnknvlgl/clash-cores/sgmii branch 3 times, most recently from 73f1eaa to d740caf Compare July 24, 2024 18:49
Copy link
Member

@christiaanb christiaanb left a 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

clash-cores/src/Clash/Cores/Sgmii/BitSlip.hs Outdated Show resolved Hide resolved
@jvnknvlgl
Copy link
Collaborator Author

Great, thank you very much!

@jvnknvlgl
Copy link
Collaborator Author

Is there anything specific that still needs to happen for this to be merged? I know that clash-cores now lives in its own repository, but I don't really know how to move a PR to a different repository.

@DigitalBrains1
Copy link
Member

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.

@jvnknvlgl
Copy link
Collaborator Author

Thanks!

The title of this PR (Add SGMII support to clash-cores) sounds very reasonable to me.

@DigitalBrains1
Copy link
Member

I'd like to suggest using the PR cover letter as detailed message (some slight reformatting):

Add SGMII support to `clash-cores`

Add support for connecting to Ethernet PHYs using SGMII to `clash-cores`.

References:
 - SGMII: https://archive.org/download/sgmii/SGMII.pdf
 - IEEE 802.3: https://standards.ieee.org/ieee/802.3/10422/

@jvnknvlgl
Copy link
Collaborator Author

Ah, that looks good to me!

@DigitalBrains1
Copy link
Member

Heh. Maybe change it a bit more. The "add X to clash-cores" is just a bit weird when clash-cores is the name of the repo. It's like a PR here titled "add X to clash-compiler". It's kinda implied.

Add SGMII support

Add support for connecting to Ethernet PHYs using SGMII

References:
 - SGMII: https://archive.org/download/sgmii/SGMII.pdf
 - IEEE 802.3: https://standards.ieee.org/ieee/802.3/10422/

That okay?

@jvnknvlgl
Copy link
Collaborator Author

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants