-
Notifications
You must be signed in to change notification settings - Fork 242
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
apicula: add support for magic sip pins #1370
Conversation
himbaechel/uarch/gowin/cst.cc
Outdated
bool gowin_apply_constraints(Context *ctx, std::istream &in) | ||
{ | ||
// implicit constraints from SiP pins | ||
const Extra_package_data_POD *extra = reinterpret_cast<const Extra_package_data_POD *>(ctx->package_info->extra_data.get()); | ||
if(extra != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't actually work, because relptrs are relative so get()
will add it's own location to the zero offset first. you'd need to add something like an is_null()
or similar function to RelPtr that checks if the offset is zero (because a relptr is never going to be pointing to itself in any useful context, so zero is useful as a null offset value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concerning part is that I wasn't sure how to check if a RelPtr is null, and found other cases that seemed to do this, but maybe they were subtly different.
You mean add a method to the RelPtr class that checks if the offset is 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those places are probably just wrong...
Yes, that's what I'd recommend doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what I found before but it may have been a slightly different pattern that checks if a pointer to a RelPtr is nullptr.
auto db_ptr = reinterpret_cast<const RelPtr<DatabasePOD> *>(get_chipdb(chipdb));
if (db_ptr == nullptr)
log_error("Failed to load chipdb '%s'\n", chipdb.c_str());
db = db_ptr->get();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't checking if the result of getting a RelPtr is nullptr, rather if a regular pointer to a RelPtr is nullptr (as it would be if mapping the database failed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Does this still need to be a draft or are we okay merging this? |
I still need to test it actually works |
Okay so this works on all the devices I've been able to test so I think this is ready |
While we have documented the sdram pins of some devices we lack the ability to automatically place them like the vendor tools do. For better compatibility and user friendliness this PR adds the ability to constrain these magic pin locations.
It seems to load the constraints but I have yet to test if sdram actually functions, which isn't super trivial.