Skip to content

[arista] Add gearbox configs for Arista 7280cr3mk#8146

Merged
jimmyzhai merged 7 commits intosonic-net:masterfrom
byu343:bkms-config
Aug 26, 2021
Merged

[arista] Add gearbox configs for Arista 7280cr3mk#8146
jimmyzhai merged 7 commits intosonic-net:masterfrom
byu343:bkms-config

Conversation

@byu343
Copy link
Contributor

@byu343 byu343 commented Jul 9, 2021

Why I did it

This change adds the gearbox config files to Arista 7280cr3mk

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@byu343 byu343 requested a review from jleveque as a code owner July 9, 2021 22:43
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

i also think what you intended is to have 2 syncd , first with ASIC_DB and second with GB_ASIC_DB, first will have 1 switch, and 2nd will have 8 phys

Copy link
Contributor

@shyam77git shyam77git Jul 16, 2021

Choose a reason for hiding this comment

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

@kcudnik : in this context_config.json file, trying to see/understand what's the purpose of having both ASIC_DB and GB_ASIC_DB blocks. Is it like this:
If a ASIC complex on the board has both ASIC and PHY on the same chip/SOC, then specify both of them?
If a PHY chip/device is Ext to ASIC (i.e. independent chip on the board) and the goal is to manage these PHYs under gbsyncd container, then just specifying GB_ASIC_DB block is sufficient? ASIC_DB is not required?

Copy link
Contributor

Choose a reason for hiding this comment

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

from my point of view, how hardware is organized is not relevant, depends how you want to present it in database and configuration file. you CANT have multiple syncd pointing to the same databases ex. ASIC_DB, each context entry with separate GUID is a separate syncd instance. please read context config wiki page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shyam77git , for this platform, guid 0 is for ASIC, guid 1 and thereafter are for external phys now. Later, we may have a further change to cover all external phys by guid 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shyam77git, between now vs later, maybe you can just skip the "now" model, as it does not follow the design in the wiki page and will very likely be deprecated. If you are still interested in why multiple guid pointing to same database could work, the tricky part is we did only create the syncd process for guid 1. It was done in that way to bypass some other issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcudnik ,
The new property proposed is for gearbox_config.json, not for context_config.json. Let me just create the new PR for it, to let it be clear in discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcudnik The code has been updated.
For context_config.json, all the phys are defined in the same context.
For gearbox_config.json, a property of context_id is added to each phy. It is to be used by gearsyncd in sonic-swss, as gearsyncd needs to specify the context for each phy when create_switch(): https://github.com/Azure/sonic-swss/pull/1826/files#diff-be825ae02c0d05e1a802a265c12721e5747035acab211f3bc7e6a219cf500e03L421. phy_id in gearbox_config.json was used to specify the context which seems not always true. To support context_id in gearsyncd, a change to swss is created: sonic-net/sonic-swss#1826.

Copy link
Contributor

@shyam77git shyam77git Jul 21, 2021

Choose a reason for hiding this comment

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

phy_id in gearbox_config.json was used to specify the context which seems not always true.

@byu343
To elaborate more on this, share the understanding and ensure we all are on same page:
[1] Based on existing codebase in swss/saihelper, looks like each phy_id (in gearbox_config.json) corresponds to unique/separate guid (of context_config.json). whereas, with the change in context_config.json all PHYs (Switches) are under same guid (global context).
[2] At swss/sai code/workflow level, this change was done because phy_id was/is being passed as SAI_REDIS_SWITCH_ATTR_CONTEXT.
[3] phy_id is unique for each PHY. However, context_config.json puts all PHYs (switches) under one (common) guid.
[4] globalContext gets populated with phy_id for every create_switch() call which make globalContext different from every create_switch() call , whereas the expectation is to have a common (same) id for all PHYs/Switches under the guid 1 of the revised context_config.json. For this purpose, context_id field was introduced for all PHYs under the same roof (i.e. guid 1 GB_ASIC_DB of context_config.json) and populate with same value for all these PHYs.

@kcudnik - can you please share you thoughts/comments on this and related PR (1826)

saihelper.cpp initSaiPhyApi() {

...
/* Must be last Attribute */
attr.id = SAI_REDIS_SWITCH_ATTR_CONTEXT;
attr.value.u64 = phy->phy_id;
attrs.push_back(attr);

status = sai_switch_api->create_switch(&phyOid, (uint32_t)attrs.size(), attrs.data());

....
}

Sai.cpp
sai_status_t Sai::create(
{
...
if (attr_list[attr_count - 1].id == SAI_REDIS_SWITCH_ATTR_CONTEXT)
{
globalContext = attr_list[--attr_count].value.u32;
}
...
}

Thanks,
Shyam

Copy link
Contributor

Choose a reason for hiding this comment

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

whereas the expectation is to have a common (same) id for all PHYs/Switches under the guid 1 of the revised context_config.json.

what do you mean to its expected to have common id ? which id are you talking about here?

Copy link
Contributor

@shyam77git shyam77git Jul 22, 2021

Choose a reason for hiding this comment

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

Are all 8 PHYs (0 - 7) belonging to same NPU/ASIC?

Is/Are there PHY(s) which needs to be on another NPU?
In that case, would it be following SONiC multi-NPU/multi-ASIC architecture ? (i.e. have these gearbox_config, phy_config_1 , context_config json files under /usr/share/sonic/device/<hw_sku>/swss@1 syncd@1 and gbsyncd@1 for NPU=1 )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this specific platform, the setting is 8 external phy chips connected 1 ASIC.
So far for the platforms we planned to support, we haven't considered about the multi-ASIC case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is unclear how the lanes are mapped to the ports, where do we get those information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The section of lanes is not used, and the data put here is to pass the parser checking. The related info is obtained from gearbox_100G_PAM4.xml, the file only applied to credo driver.

Copy link
Contributor

Choose a reason for hiding this comment

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

@byu343 : Can you please share/update as to which azure branch you referring to to put add and validate these fields? Is it azure/master? which git commit/ reference point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base sonic-buildimage image version we are using for testing is e439676. We plan to bring the support to master and 202106, so if any issue is from on them later, we will submit some fix.

byu343 added 2 commits July 27, 2021 10:53
1) let phys share the same context;
2) add context_id to gearbox_config.json;
3) correct lib_name;
4) change to psai.profile
@jimmyzhai jimmyzhai merged commit 85a671f into sonic-net:master Aug 26, 2021
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