search

vaughnbetz / COFFE

38 stars 25 forks source link

About wordlinedriver area calculation in fpga.py #36

Open yc2367 opened 2 years ago

yc2367 commented 2 years ago

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 commented 2 years ago

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

aman26kbm commented 2 years ago

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 commented 2 years ago

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

sadegh68 commented 2 years ago

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

yc2367 commented 2 years ago

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 commented 2 years ago

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