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

FPGA: Device and Driver #808

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

FPGA: Device and Driver #808

wants to merge 29 commits into from

Conversation

IgnoreWarnings
Copy link
Collaborator

@IgnoreWarnings IgnoreWarnings commented Aug 8, 2024

This PR adds devices and drivers with features of binding, unbinding and probing. The IpDevice class adds additional features for devices which correspond to an ip on the fpga, such as getting the ip name and baseaddress.

Used by #783.

Copy link
Member

@n-eiling n-eiling left a comment

Choose a reason for hiding this comment

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

I think this should go into common/lib/kernel. Also there we already have a class Device in pci.hpp. Maybe it makes sense to make a top class Device from which you derive PciDevice and PlatformDevice (instead of IpDevice)?

Doesn't this make some of the functions in common/kernel obsolete? Can we remove something there?
Also please add me to the CODEOWNERS file for the new files you created.

@IgnoreWarnings
Copy link
Collaborator Author

I think this should go into common/lib/kernel. Also there we already have a class Device in pci.hpp. Maybe it makes sense to make a top class Device from which you derive PciDevice and PlatformDevice (instead of IpDevice)?

Doesn't this make some of the functions in common/kernel obsolete? Can we remove something there? Also please add me to the CODEOWNERS file for the new files you created.

Thank you for the tip. It is a good Idea to move it to the Kernel.

Refactor

Naming
The existing Device in "pci.cpp", which is specific for PCI devices, should be renamed to PciDevice.

File Structure
The class should be move to its own "PciDevice" file.

Obsolete Code / Integration
PciDevice can be partially integrated with the new system. Obviously the existing PciDevice wasnt designed with inheritance in mind, which makes it tough to identify common grounds.
The whole code there is questionable: it uses hard coded c_strings as paths, c pointers, fprintf instead of logging, obscure naming... e.g.:

if (tmp[0] && strcmp(tmp, "*")) {
long int x = strtol(tmp, &e, 16);                        
if ((e && *e) || (x < 0 || x > 0x7fffffff)) {...}

I dont think it is worth to port this to the modern style in this PR. Just leave it as is.
@n-eiling Whats your opinion on the suggestions?

@IgnoreWarnings
Copy link
Collaborator Author

I will add you as author.

@IgnoreWarnings
Copy link
Collaborator Author

Also the Device class i implemented is a toplevel class. IpDevice is what is used for vfio platform devices ;)

@IgnoreWarnings
Copy link
Collaborator Author

Waiting for #814

@IgnoreWarnings
Copy link
Collaborator Author

Lets only comment the headers to avoid duplication.

IgnoreWarnings and others added 27 commits October 11, 2024 15:03
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Co-authored-by: Steffen Vogel <[email protected]>
Signed-off-by: IgnoreWarnings <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Signed-off-by: Pascal Bauer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants