Re-enable targeting idle eth cores on BH #14817

abhullar-tt · 2024-11-06T21:15:35Z

Ticket

Closes #14653
No ticket, removing workaround required that was impacting p100 CI

What's changed

CI FW has been downgraded so this going back to original behaviour

Checklist

Blackhole Post commit

blozano-tt · 2024-12-04T07:15:14Z

tt_metal/impl/debug/sanitize_noc_host.hpp

@@ -20,7 +20,7 @@ namespace tt {
 #define DEBUG_VALID_WORKER_ADDR(a, l) (DEBUG_VALID_L1_ADDR(a, l) || (DEBUG_VALID_REG_ADDR(a) && (l) == 4))
 #define DEBUG_VALID_DRAM_ADDR(a, l, b, e) (((a) >= b) && ((a) + (l) <= e))

-#define DEBUG_VALID_ETH_ADDR(a, l) (((a) >= HAL_MEM_ETH_BASE) && ((a) + (l) <= HAL_MEM_ETH_BASE + HAL_MEM_ETH_SIZE))
+#define DEBUG_VALID_ETH_ADDR(a, l) ((((a) >= HAL_MEM_ETH_BASE) && ((a) + (l) <= HAL_MEM_ETH_BASE + HAL_MEM_ETH_SIZE)) || (DEBUG_VALID_REG_ADDR(a) && (l) == 4))


Heads up: I think clang-format will force you to turn this into a multi line macro for readability.

Maybe consider turning this into an inline function with some code comments?

blozano-tt · 2024-12-04T07:18:31Z

tt_metal/llrt/blackhole/bh_hal.cpp

@@ -69,7 +69,8 @@ void Hal::initialize_bh() {
             (addr < NOC_OVERLAY_START_ADDR + NOC_STREAM_REG_SPACE_SIZE * NOC_NUM_STREAMS)) ||
            ((addr >= NOC0_REGS_START_ADDR) && (addr < NOC0_REGS_START_ADDR + 0x1000)) ||
            ((addr >= NOC1_REGS_START_ADDR) && (addr < NOC1_REGS_START_ADDR + 0x1000)) ||
-            (addr == RISCV_DEBUG_REG_SOFT_RESET_0));
+            (addr == RISCV_DEBUG_REG_SOFT_RESET_0) ||
+            (addr== 0xFFB14000 || addr == 0xFFB14008));  // used to program start addr for eth FW


Magic numbers? Can we come up with a descriptive name, and make it constexpr variable?

will make them consts

blozano-tt · 2024-12-04T07:20:46Z

tt_metal/llrt/llrt.cpp

-void launch_erisc_app_fw_on_core(chip_id_t chip, CoreCoord core) {
-    llrt::write_hex_vec_to_core(chip, core, tt::stl::Span(&ERISC_APP_FW_ON_CORE_FLAG, 1), eth_l1_mem::address_map::LAUNCH_ERISC_APP_FLAG);
-}
-


When you are ready to merge this, I believe you can also rm -rf:
#include "eth_l1_address_map.h" // address_map

blozano-tt · 2024-12-04T07:21:15Z

tt_metal/llrt/llrt.cpp

-    constexpr uint32_t jal_opcode = 0x6f;
-    constexpr uint32_t jal_max_offset = 0x0007ffff;
-    uint32_t opcode = jal_opcode;
-    uint32_t firmware_base = is_eth_core ? MEM_IERISC_FIRMWARE_BASE : MEM_BRISC_FIRMWARE_BASE;


pgkeller · 2024-12-04T17:49:03Z

tt_metal/llrt/blackhole/bh_hal_active_eth.cpp

@@ -74,6 +74,8 @@ HalCoreInfoType create_active_eth_mem_map() {
            .fw_base_addr = eth_l1_mem::address_map::FIRMWARE_BASE,
            .local_init_addr = eth_l1_mem::address_map::FIRMWARE_BASE,  // this will be uplifted in subsequent commits
                                                                        // enabling active erisc
+            .fw_launch_addr = 0xFFB14008,
+            .fw_launch_addr_value = (uint32_t)eth_l1_mem::address_map::FIRMWARE_BASE,


minor: wasn't clear what "fw_launch_addr_value" was in other contexts, maybe "fw_launch_jmp_insn" (yes, ties the use to the name, but I think that's ok)

hm the reason I didn't add jump into the name is because we don't always write the jump instruction at this value. like this case above we just write FW base addr directly to the PC

pgkeller · 2024-12-04T17:49:29Z

tt_metal/llrt/blackhole/bh_hal_idle_eth.cpp

@@ -70,19 +70,32 @@ HalCoreInfoType create_idle_eth_mem_map() {
    std::vector<std::vector<HalJitBuildConfig>> processor_classes(NumEthDispatchClasses);
    std::vector<HalJitBuildConfig> processor_types(1);
    for (std::uint8_t processor_class_idx = 0; processor_class_idx < NumEthDispatchClasses; processor_class_idx++) {
-        DeviceAddr fw_base, local_init;
+        DeviceAddr fw_base, local_init, fw_launch;
+        uint32_t fw_launch_value;
        switch (processor_class_idx) {
            case 0: {


not necessarily in scope of this change...but why are we using 0, 1, 2 rather than names?

no reason but these can be dm0, dm1

github-actions

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

github-actions · 2024-12-23T19:16:53Z

tt_metal/llrt/hal.cpp

+    constexpr uint32_t jal_opcode = 0x6f;
+    constexpr uint32_t jal_max_offset = 0x0007ffff;
+    uint32_t opcode = jal_opcode;
+    assert(firmware_base < jal_max_offset);


⚠️ clang-diagnostic-error ⚠️
unexpected namespace name assert: expected expression

… the HalJitBuildConfig

abhullar-tt requested review from pgkeller, aliuTT, tt-aho, tt-dma, tt-asaigal and ubcheema as code owners November 6, 2024 21:15

tt-aho approved these changes Nov 6, 2024

View reviewed changes

abhullar-tt force-pushed the abhullar/enable-bh-eth branch from 3b0ff59 to 259a471 Compare November 11, 2024 20:48

abhullar-tt force-pushed the abhullar/enable-bh-eth branch from 259a471 to d9c8f61 Compare November 20, 2024 16:39

abhullar-tt requested review from davorchap and cfjchu as code owners November 20, 2024 16:39

abhullar-tt force-pushed the abhullar/enable-bh-eth branch 2 times, most recently from 8f95992 to 5ef063a Compare November 25, 2024 03:42

abhullar-tt changed the title ~~#0: Re-enable targeting idle eth cores after downgrading BH CI FW~~ #0: Re-enable targeting idle eth cores on BH Nov 25, 2024

abhullar-tt force-pushed the abhullar/enable-bh-eth branch 2 times, most recently from 3c8dc89 to f11c6b5 Compare November 27, 2024 02:01

abhullar-tt changed the title ~~#0: Re-enable targeting idle eth cores on BH~~ Re-enable targeting idle eth cores on BH Nov 27, 2024

abhullar-tt force-pushed the abhullar/enable-bh-eth branch 4 times, most recently from 34fc134 to 778dc55 Compare December 3, 2024 04:21

abhullar-tt mentioned this pull request Dec 4, 2024

llrt.cpp uses ARCH_NAME specific include path for dev_msgs.h dev_mem_map.h eth_l1_address_map.h #14653

Open

3 tasks

blozano-tt reviewed Dec 4, 2024

View reviewed changes

blozano-tt added arch_name cleanup labels Dec 4, 2024

blozano-tt added this to the ARCH_NAME Removal milestone Dec 4, 2024

tenstorrent deleted a comment from nathan-TT Dec 4, 2024

abhullar-tt force-pushed the abhullar/enable-bh-eth branch from 778dc55 to 7c23c0a Compare December 4, 2024 17:27

pgkeller approved these changes Dec 4, 2024

View reviewed changes

#0: Re-enable targeting idle eth cores after downgrading BH CI FW

1dd808f

abhullar-tt force-pushed the abhullar/enable-bh-eth branch from 7c23c0a to 8460546 Compare December 23, 2024 19:11

github-actions bot reviewed Dec 23, 2024

View reviewed changes

abhullar-tt added 4 commits December 23, 2024 19:56

Add address and value that will launch fw on a programmable core into…

66f04f5

… the HalJitBuildConfig

Allow Host Watcher to allow writing to erisc pc registers on BH

04444c7

Update waypoint test to account for both eriscs on BH

cbd1407

Update eth kernel hash to include processor

6d70846

abhullar-tt force-pushed the abhullar/enable-bh-eth branch from 8460546 to 6d70846 Compare December 23, 2024 19:56

Add missing bank arrays in idle erisc1 fw

7bc461e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable targeting idle eth cores on BH #14817

Re-enable targeting idle eth cores on BH #14817

abhullar-tt commented Nov 6, 2024 •

edited by blozano-tt

Loading

blozano-tt Dec 4, 2024

blozano-tt Dec 4, 2024

abhullar-tt Dec 4, 2024

blozano-tt Dec 4, 2024

blozano-tt Dec 4, 2024

blozano-tt Dec 4, 2024

pgkeller Dec 4, 2024

abhullar-tt Dec 11, 2024

pgkeller Dec 4, 2024

abhullar-tt Dec 11, 2024

github-actions bot left a comment

github-actions bot Dec 23, 2024

Re-enable targeting idle eth cores on BH #14817

Are you sure you want to change the base?

Re-enable targeting idle eth cores on BH #14817

Conversation

abhullar-tt commented Nov 6, 2024 • edited by blozano-tt Loading

Ticket

What's changed

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 23, 2024

Choose a reason for hiding this comment

abhullar-tt commented Nov 6, 2024 •

edited by blozano-tt

Loading