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

About wordlinedriver area calculation in fpga.py #36

Open
yc2367 opened this issue Oct 17, 2022 · 6 comments
Open

About wordlinedriver area calculation in fpga.py #36

yc2367 opened this issue Oct 17, 2022 · 6 comments

Comments

@yc2367
Copy link
Contributor

yc2367 commented Oct 17, 2022

In coffe/fpga.py, line 3593 when calculating and updating the area for wordline drivers.

At line 4529, the calculation only considers the areafac which equals to 2**self.row_decoder_bits. However, should this area be multiplied by an additional factor of 2 to account for the wordline drivers for 2 BRAM ports (i.e., 2 wordlines per BRAM row)? When instantiating the BRAM wordlinedriver for FPGA at line 5766, the area for wordlinedriver still doesn't account for this additional factor of 2 (unlike the RAM_decoder_area which is multiplied by 2 at line 5721).

@vaughnbetz
Copy link
Owner

I'm not sure. @sadegh68 @aman26kbm : any opinion?

@aman26kbm
Copy link
Collaborator

I believe you're right. We should have a factor of two (for two ports). I haven't thoroughly looked at the code to confirm that the factor isn't present anywhere else though.

While you're at it, if it is easy, maybe check if this is missing for other subcircuits too (sense amps, write drivers, etc).

@sadegh68
Copy link
Collaborator

I think you're correct; it seems to be missing a factor of two.

@sadegh68 sadegh68 reopened this Oct 18, 2022
@sadegh68
Copy link
Collaborator

My apologies for closing the issue by accident. I have reopened it. Please simply multiply the word line driver area by 2.

@yc2367
Copy link
Contributor Author

yc2367 commented Oct 18, 2022

Thanks a lot for the clarification! I will read other BRAM parts and see if a factor of 2 is missing for other subcircuits. Will you update the code or should I create a pull request for this?

@aman26kbm
Copy link
Collaborator

It'll be great if you can create a PR. Appreciate your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants