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

Implement Wormhole UBB support #592

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Implement Wormhole UBB support #592

wants to merge 36 commits into from

Conversation

pjanevskiTT
Copy link
Contributor

Issue

#560

Description

Implement support for UBB galaxy board. ETH firmware is changing for UBB so topology discovery from scratch is needed. Also support for general recognition of UBB board type is needed.

This is a good step in implementing Wormhole topology discovery completely from scratch. Old WH boards still use legacy create-ethernet-map path.

List of the changes

  • Topology discovery for UBB
  • Recognize UBB board type
  • Hide cluster descriptor APIs that use create-ethernet-map by default from public API.

Testing

CI + local testing. Hard to write UBB tests at the moment, we don't have this machines on CI

API Changes

Cluster descriptor API

  • tt_metal approved PR pointing to this branch - ?
  • tt_debuda approved PR pointing to this branch - ?

@pjanevskiTT pjanevskiTT requested a review from broskoTT March 7, 2025 11:38
@pjanevskiTT pjanevskiTT self-assigned this Mar 7, 2025
@@ -13,8 +13,12 @@ class WormholeTTDevice : public TTDevice {
public:
WormholeTTDevice(std::unique_ptr<PCIDevice> pci_device);

void wait_arc_core_start(const tt_xy_pair arc_core, const uint32_t timeout_ms = 1000) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in tt_device? Is this implemented for Blackhole?
Why pass position of the arc code?

}
}
} else {
ubb_eth_connections(chips, desc);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we do this here, but for blackhole we have whole implementation.
Can you create a separate function which encapsulates implementation for blackhole, and then Cluster::create_cluster_descriptor would remain as ~10 loc

Also this is a good signal that we should probably have a class TopologyDiscovery. Out of scope for this PR though. You can create an issue if you agree.

const uint64_t niu_cfg_addr = 0x1000A0000 + 0x100;
read_from_device(&niu_cfg, dram_core, niu_cfg_addr, sizeof(uint32_t));

bool noc_translation_enabled = (niu_cfg & (1 << 14)) != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

please put NOC_TRANSLATION_LOCATION from this line, and above niu_cfg_addr in some constants. wormhole::implementation probably

}

void WormholeTTDevice::wait_arc_core_start(const tt_xy_pair arc_core, const uint32_t timeout_ms) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

So you've added this without implementation? If its not used now, remove it. When we need it we will implement it.

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

Successfully merging this pull request may close these issues.

2 participants