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 support for QSFP56 and QSFP56_DD_TYPE3 form factor identities #564

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

LeonGWang
Copy link
Contributor

QSFP56-DD transceiver form-factor encompasses more variants
than just type1 (8 optical signals) and type2 (4 optical signals)
defined in OpenConfig.

We should either use an umbrella QSFP56-DD identity to encompass all
variants, or add an additional identity to represent QSFP-DD 400G-ZR
(1 optical signal).

@OpenConfigBot
Copy link

OpenConfigBot commented Jan 25, 2022

Compatibility Report for commit da629ae:
pyangbind@b1762a3

@dplore
Copy link
Member

dplore commented Jan 26, 2022

@ejbrever can you provide any comments?

@ejbrever
Copy link
Contributor

I think there was some desire to specify the types, so keeping with that I'd lean towards adding the Type3 as done here.

Adding an umbrella QSFP56 type might be problematic though because it allows for vendor implementations to choose either a specific type or the generic one, so we will likely have inconsistencies between implementations.

@LeonGWang LeonGWang changed the title Add support for QSFP56-DD form factor identities Add support for QSFP56 and QSFP56_DD_TYPE3 form factor identities Jan 29, 2022
@LeonGWang
Copy link
Contributor Author

I updated my commit to only keep the backward compatible QSFP56_DD_TYPE3 identity.
I also added a QSFP56 identity that is missing in the OpenConfig model.

@ejbrever
Copy link
Contributor

This looks good to me.

@thofmeister @yawei-yin would either you be able to review this as well? Thanks.

@jqjqlee
Copy link

jqjqlee commented Feb 2, 2022

We might need an identity QSFP56_DD to cover all the QSFP56-DD types. The count of optical lanes can be identified by ETHERNET_PMD_TYPE. For instance, TYPE2 corresponds to ETH_400GBASE_FR4, ETH_400GBASE_LR4, ETH_400GBASE_DR4, while TYPE1 corresponds to ETH_400GBASE_SR8, ETH_400GBASE_LR8. Therefore, we need to delete QSFP56_DD_TYPE1 and QSFP56_DD_TYPE2.

@ejbrever @yawei-yin @thofmeister Actually, this issue has been addressed by PR#1016. We could revisit it in this public PR, and I will close PR#1016. What do you think?

@jqjqlee
Copy link

jqjqlee commented Feb 2, 2022

BTW, could we add more peer identities in a single PR, such as QSFP112, and QSFP112_DD which will be soon commercially available.
For each generation of form factor, we are facing similar issue to what this PR addressed. That is, each form factor would have multiple variants/types in terms of optical interface standard. In this regards, it is suggested to decouple the form factor identities from optical interface specifications.

@dplore
Copy link
Member

dplore commented Oct 11, 2022

@ejbrever can you also follow up with @thofmeister offline for us?

@jqjqlee
Copy link

jqjqlee commented Oct 11, 2022

I think there was some desire to specify the types, so keeping with that I'd lean towards adding the Type3 as done here.

Adding an umbrella QSFP56 type might be problematic though because it allows for vendor implementations to choose either a specific type or the generic one, so we will likely have inconsistencies between implementations.

@ejbrever Eric, if we still maintain multiple identities/types for QSFP56-DD form factor, how could we cope with other form factors? Other form factors have multiple variants/types in implementation as well. This would result in inconsistency among different form factors. In order to reach a consensus, could we keep a generic identity QSFP56_DD in parallel to QSFP56_DD_TYPE1, QSFP56_DD_TYPE2 and the newly-proposed QSFP56_DD_TYPE3? So the vendors can select what they would like to implement.

@OpenConfigBot
Copy link

OpenConfigBot commented Jul 24, 2023

Major YANG version changes in commit da629ae:

@LeonGWang
Copy link
Contributor Author

Hi all, this PR is stalled. Could we let this PR merge as a band-aid solution and discuss refactoring and/or backward-incompatible changes to transceiver identities in a separate issue/PR?

Many thanks for considering my request.

@LeonGWang
Copy link
Contributor Author

LeonGWang commented Jul 24, 2023

@dplore Do you have any opinions or suggestions for this PR? This PR is similar to PR #585

Thanks in advance!

@dplore
Copy link
Member

dplore commented Jul 25, 2023

QSFP56_DD in parallel to QSFP56_DD_TYPE1, QSFP56_DD_TYPE2 and the newly-proposed QSFP56_DD_TYPE3

@ejbrever can you take another look here?

In my opinion we should have a different transceiver form factor type whenever the physical packaging and/or electrical interface changes. Further, it seems useful to only define transceiver form factor types that are a discreet, installable part. A transceiver form factor that aggregates multiple incompatible electrical interfaces (even if backwards compatible) seems like it's not a useful type to define.

@LeonGWang LeonGWang force-pushed the QSFP-DD branch 3 times, most recently from 1c6e44b to 9d47598 Compare July 26, 2023 01:17
@LeonGWang
Copy link
Contributor Author

Gentle bump

@dplore
Copy link
Member

dplore commented Aug 1, 2023

Hi Leon, after some internal discussion we came to the following recommendation:

Introduce a new identity of "QSFP56_DD". This should be inclusive of type 1,2,3. Please add a reference statement to the MSA for QSFP56_DD.

Change the existing identities of QSFP56_DD_TYPE1 and QSFP56_DD_TYPE2 to be deprecated.
(Use the yang status: deprecated statement)

The rationale is that the TYPE 1,2,3 variations are not configuration or state that is reported by the device. Furthermore, it is our understanding that the physical form factor / cage for Type 1,2,3 devices is interchangable. This is based on our understanding on the specifications for QSFP56_DD. Please feel free to provide references if our conclusion is not well informed.

Also, this may be obvious, but for community documentation benefit, I wanted to mention to differentiate link speeds delivered, the PMD is used.

@jqjqlee
Copy link

jqjqlee commented Aug 1, 2023

Hi Leon, after some internal discussion we came to the following recommendation:

Introduce a new identity of "QSFP56_DD". This should be inclusive of type 1,2,3. Please add a reference statement to the MSA for QSFP56_DD.

Change the existing identities of QSFP56_DD_TYPE1 and QSFP56_DD_TYPE2 to be deprecated. (Use the yang status: deprecated statement)

The rationale is that the TYPE 1,2,3 variations are not configuration or state that is reported by the device. Furthermore, it is our understanding that the physical form factor / cage for Type 1,2,3 devices is interchangable. This is based on our understanding on the specifications for QSFP56_DD. Please feel free to provide references if our conclusion is not well informed.

Also, this may be obvious, but for community documentation benefit, I wanted to mention to differentiate link speeds delivered, the PMD is used.

Finally, the consensus is reached. :-)

QSFP56_DD transceiver form-factor encompasses more variants
than just type1 (8 optical signals) and type2 (4 optical signals)
defined in OpenConfig. Therefore, adding a generic QSFP56_DD
form-factor identity that encompasses all possible variants and
deprecated the existing QSFP56_DD_TYPE1 and QSFP56_DD_TYPE2
form-factor identities.

QSFP56 form-factor identity is also added.
@LeonGWang
Copy link
Contributor Author

Thanks for the feedback @dplore and @jqjqlee!

The QSFP56_DD form factor identity has been added instead of a TYPE3 variant. TYPE1/2 identities are also marked as deprecated as per consensus.

I still left the QSFP56 identity as-is, continuing to use the same description language as the existing QSFP28.
Let me know if changes are required for that identity.

Copy link
Contributor

@ejbrever ejbrever 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 making these changes!

Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@dplore dplore merged commit 33f5349 into openconfig:master Aug 3, 2023
4 checks passed
@LeonGWang LeonGWang deleted the QSFP-DD branch August 3, 2023 22:41
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.

5 participants