OFS / opae-sdk

Open Programmable Acceleration Engine
https://ofs.github.io
BSD 3-Clause "New" or "Revised" License
251 stars 84 forks source link

Fpgamatt/subdevid 0001 #3120

Closed fpgamatt closed 4 months ago

fpgamatt commented 4 months ago

PR Title should start with one of the following tags: [Fix]/[Feature]/[Style]/[Update]

[Fix]- Bug Fix

[Feature]- for new feature

[Style]- Grammar or branding fix

[Update]-For an update to an existing feature

------------- Keep everything below this line -------------------------

Description

Describe the issue, update, change or fix and why

Collateral (docs, reports, design examples, case IDs):

Tests added:

Tests run:

fpgamatt commented 4 months ago

The functionality moved to board_common exists on any board that has qsfp(s) and hssi subsystems.

We could call these boards something like pci_jtag_devkit.

pcolberg commented 4 months ago

The build fails due to missing link dependencies: https://github.com/OFS/opae-sdk/blob/afb66cdc8e53b3d39fcce1afd40a0f6751280852/libraries/libboard/board_n6000/CMakeLists.txt#L35

[ 86%] Linking CXX executable ../../bin/test_board_d5005_c
/usr/bin/ld: ../../lib/libboard-common-static.a(board_common.c.o): in function `print_common_phy_info':
/home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1083: undefined reference to `opae_uio_open'
/usr/bin/ld: /home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1089: undefined reference to `opae_uio_region_get'
/usr/bin/ld: /home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1092: undefined reference to `opae_uio_close'
/usr/bin/ld: /home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1101: undefined reference to `opae_uio_close'
collect2: error: ld returned 1 exit status
make[2]: *** [tests/board/CMakeFiles/test_board_d5005_c.dir/build.make:124: bin/test_board_d5005_c] Error 1
make[1]: *** [CMakeFiles/Makefile2:6683: tests/board/CMakeFiles/test_board_d5005_c.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 86%] Building CXX object tests/board/CMakeFiles/test_board_c6100_c.dir/__/framework/mock/opae_mock.cpp.o
[ 87%] Building CXX object tests/board/CMakeFiles/test_board_n6000_c.dir/__/framework/mock/opae_mock.cpp.o
[ 87%] Building CXX object tests/board/CMakeFiles/test_board_n5010_c.dir/__/framework/mock/opae_mock.cpp.o
[ 87%] Linking CXX executable ../../bin/test_board_n5010_c
/usr/bin/ld: ../../lib/libboard-common-static.a(board_common.c.o): in function `print_common_phy_info':
/home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1083: undefined reference to `opae_uio_open'
/usr/bin/ld: /home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1089: undefined reference to `opae_uio_region_get'
/usr/bin/ld: /home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1092: undefined reference to `opae_uio_close'
/usr/bin/ld: /home/runner/work/opae-sdk/opae-sdk/libraries/libboard/board_common/board_common.c:1101: undefined reference to `opae_uio_close'
collect2: error: ld returned 1 exit status
make[2]: *** [tests/board/CMakeFiles/test_board_n5010_c.dir/build.make:124: bin/test_board_n5010_c] Error 1
make[1]: *** [CMakeFiles/Makefile2:6713: tests/board/CMakeFiles/test_board_n5010_c.dir/all] Error 2
michael-adler commented 4 months ago

Can someone please explain why this very invasive change is needed and why something simpler won't work? I see complexity being added both here and to the HW side.

fpgamatt commented 4 months ago

I would not classify this change as invasive. We are just adding a new board to the board plug-in framework of opae. This new board is a generic PCI devkit without a board BMC and is jtag only. Without this change, boards were incorrectly identifying themselves, and then opae would in turn mishandle the misidentified board.

pcolberg commented 4 months ago

I would not classify this change as invasive. We are just adding a new board to the board plug-in framework of opae. This new board is a generic PCI devkit without a board BMC and is jtag only. Without this change, boards were incorrectly identifying themselves, and then opae would in turn mishandle the misidentified board.

Could you extend the commit message with the case?

Closes: https://hsdes.intel.com/appstore/article/#/16023271460
michael-adler commented 4 months ago

So is the rule: set subdevice ID to 1 if there is no BMC and to 0x1771 if there is a BMC?

fpgamatt commented 4 months ago

There are currently quite a few subdevice ids being used. The intent is help identify the board with just lspci, and to address concerns from the kernel community of having just a generic DFL Device id with out any subdevice info. The subdevice of 0x1771 is actually a n6001, and 0x1770 is a n6000. 0x17d4 is used for C6100.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 8711165719

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/libboard/board_jtag_pci_dk/board_jtag_pci_dk.c 0 10 0.0%
libraries/libboard/board_common/board_common.c 15 77 19.48%
<!-- Total: 16 88 18.18% -->
Totals Coverage Status
Change from base Build 8654404228: -0.02%
Covered Lines: 15825
Relevant Lines: 24478

💛 - Coveralls
fpgamatt commented 4 months ago

can I get an approve so this can be merged into master. The RTL has been merged.

michael-adler commented 4 months ago

can I get an approve so this can be merged into master. The RTL has been merged.

I can't help there. Not a code owner.