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

Driver for openFPGALoader #1402

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented May 13, 2024

A driver interfacing with the openFPGADriver universal FPGA programming tool.

Description

This driver adds support for bootstraping FPGA boards using the openFPGALoader universal FPGA programming utility (https://github.com/trabucayre/openFPGALoader).

OpenFPGALoader supports a large number of FPGA boards, boards and programming cables. It is a useful addition to the openocd support greatly expanding the amount of FPGA hardware that can be worked with using labgrid.

The code has been tested locally on a Xilinx VCU118 board.

There are no tests currently for the driver as it is not clear to me how it could be tested without real HW.

Checklist

  • Documentation for the feature
  • The arguments and description in doc/configuration.rst have been updated
  • PR has been tested

@enkiusz
Copy link
Contributor

enkiusz commented Jun 19, 2024

fyi: my username is now @enkiusz instead of @mgrela. I moved the repo but the account name in PRs doesn't migrate along with it. Sorry for the Jia Tian moment.

@enkiusz
Copy link
Contributor

enkiusz commented Jul 1, 2024

Ping. Is there any way I can assist with the review/merge process as the author of this PR?

@Emantor Emantor requested a review from jluebbe July 2, 2024 06:17
@Emantor Emantor self-requested a review July 2, 2024 06:17
Copy link
Member

@jluebbe jluebbe left a comment

Choose a reason for hiding this comment

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

Looks good otherwise.


OpenFPGALoaderDriver:
board: vcu118
image: 'bitstream.bit'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
image: 'bitstream.bit'
image: 'mybitstreamkey'

doc/configuration.rst Outdated Show resolved Hide resolved
@jluebbe
Copy link
Member

jluebbe commented Jul 17, 2024

@enkiusz Please also add the import to labgrid/driver/__init__.py.

@enkiusz
Copy link
Contributor

enkiusz commented Jul 18, 2024

I have implemented the suggested changes.

Comment on lines 1983 to 1986
- image (str): optional, key in :ref:`images <labgrid-device-config-images>` containing the path
of the bitstream, if not specified when calling `load()`
- board (str): optional, the FPGA board identifier
- frequency (int): optional, force a non-default programmer frequency in Hz
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation:

Suggested change
- image (str): optional, key in :ref:`images <labgrid-device-config-images>` containing the path
of the bitstream, if not specified when calling `load()`
- board (str): optional, the FPGA board identifier
- frequency (int): optional, force a non-default programmer frequency in Hz
- image (str): optional, key in :ref:`images <labgrid-device-config-images>` containing the path
of the bitstream, if not specified when calling `load()`
- board (str): optional, the FPGA board identifier
- frequency (int): optional, force a non-default programmer frequency in Hz

@@ -1950,6 +1951,40 @@ Arguments:
- board_config (str): optional, board config in the ``openocd/scripts/board/`` directory
- load_commands (list of str): optional, load commands to use instead of ``init``, ``bootstrap {filename}``, ``shutdown``

OpenFPGALoaderDriver
~~~~~~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

Title underline too short (see CI error).

Suggested change
~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~

filename = self.target.env.config.get_image_path(self.image)
mf = ManagedFile(filename, self.interface)
mf.sync_to_resource()
cmd += ["--bitstream", mf.get_remote_path() ]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd += ["--bitstream", mf.get_remote_path() ]
cmd += ["--bitstream", mf.get_remote_path()]

mf.sync_to_resource()
cmd += ["--bitstream", mf.get_remote_path() ]

cmd += ["--busdev-num", f"{self.interface.busnum}:{self.interface.devnum}" ]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd += ["--busdev-num", f"{self.interface.busnum}:{self.interface.devnum}" ]
cmd += ["--busdev-num", f"{self.interface.busnum}:{self.interface.devnum}"]

.. code-block:: yaml

OpenFPGALoaderDriver:
board: vcu118
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the example consistent:

Suggested change
board: vcu118
board: 'vcu118'

Otherwise YAML assumes a type, which might fail in funny ways depending on the board identifier.

super().__attrs_post_init__()

if self.target.env:
self.tool = self.target.env.config.get_tool('openFPGALoader')
Copy link
Member

Choose a reason for hiding this comment

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

Please use a lowercase toos key here and add it to the "Tools Keys" section in man/labgrid-device-config.rst (and regenerate the man pages).

Comment on lines 1961 to 1963
- https://trabucayre.github.io/openFPGALoader/compatibility/fpga.html
- https://trabucayre.github.io/openFPGALoader/compatibility/board.html
- https://trabucayre.github.io/openFPGALoader/compatibility/cable.html
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation:

Suggested change
- https://trabucayre.github.io/openFPGALoader/compatibility/fpga.html
- https://trabucayre.github.io/openFPGALoader/compatibility/board.html
- https://trabucayre.github.io/openFPGALoader/compatibility/cable.html
- https://trabucayre.github.io/openFPGALoader/compatibility/fpga.html
- https://trabucayre.github.io/openFPGALoader/compatibility/board.html
- https://trabucayre.github.io/openFPGALoader/compatibility/cable.html

Comment on lines +1967 to +1969
- `AlteraUSBBlaster`_
- NetworkAlteraUSBBlaster
Copy link
Member

Choose a reason for hiding this comment

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

You should also add this driver to AlteraUSBBlaster's "Used by" section.

@Bastian-Krause
Copy link
Member

Bastian-Krause commented Jul 18, 2024

Also, please squash/rebase your commit(s).

A driver interfacing with the openFPGADriver universal FPGA programming tool.

Signed-off-by: Maciej Grela <[email protected]>
@enkiusz
Copy link
Contributor

enkiusz commented Jul 19, 2024

I fixed the above problems, rebased and squashed everything into a single commit.

@enkiusz
Copy link
Contributor

enkiusz commented Aug 2, 2024

Ping.

1 similar comment
@enkiusz
Copy link
Contributor

enkiusz commented Aug 22, 2024

Ping.

@enkiusz
Copy link
Contributor

enkiusz commented Sep 30, 2024

ping

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.

4 participants