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

Fpgamatt/subdevid 0001 #3120

Merged
merged 2 commits into from
Apr 20, 2024
Merged

Fpgamatt/subdevid 0001 #3120

merged 2 commits into from
Apr 20, 2024

Conversation

fpgamatt
Copy link
Contributor

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):

  • Document Update Required? (Specify FIM/AFU/Scripts)

Tests added:

Tests run:

@fpgamatt fpgamatt self-assigned this Apr 15, 2024
@fpgamatt fpgamatt requested review from a team as code owners April 15, 2024 20:49
@fpgamatt fpgamatt requested a review from tk-x86 April 15, 2024 20:50
Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Is the moved functionality really common to all boards; or is this a subvariant of the N6000? Is there a more specific name for these BMC-less Agilex-based(?) devices?

@fpgamatt
Copy link
Contributor Author

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
Copy link
Contributor

The build fails due to missing link dependencies:

[ 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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

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

@fpgamatt
Copy link
Contributor Author

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
Copy link

coveralls commented Apr 15, 2024

Pull Request Test Coverage Report for Build 8711165719

Details

  • 16 of 88 (18.18%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 64.65%

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%
Totals Coverage Status
Change from base Build 8654404228: -0.02%
Covered Lines: 15825
Relevant Lines: 24478

💛 - Coveralls

Move the common code for printing phy information from board_n6000.c
to board_common.c.

Signed-off-by: Matthew Gerlach <[email protected]>
Add a board plugin for PCI Development Kits that only support
FPGA configuration via JTAG.

Signed-off-by: Matthew Gerlach <[email protected]>
@fpgamatt
Copy link
Contributor Author

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

@michael-adler
Copy link
Member

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.

@fpgamatt fpgamatt merged commit 0139f92 into master Apr 20, 2024
27 checks passed
@fpgamatt fpgamatt deleted the fpgamatt/subdevid-0001 branch April 20, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants