PrincetonUniversity / openpiton

The OpenPiton Platform
http://www.openpiton.org
644 stars 216 forks source link

Ariane : Translation Shift fix with Piton UART Stream #75

Open Raghavendra-Srinivas opened 4 years ago

Raghavendra-Srinivas commented 4 years ago

Hi,

On Genesys2 with MIG with AXI interface: Discussed with Jon on problem of AXI address from NoC with UART boot option. As per his suggestion, It seems on this line, for ariane, it might to be shift by 6 bits right to achieve null translation eventually for pitonstream to load test and work correctly. Test worked properly with the fix. https://github.com/PrincetonUniversity/openpiton/blob/cddf5416fc6a7e39976fb11838254ba8c41cbd46/piton/design/chipset/rtl/storage_addr_trans_unified.v.pyv#L66

Thanks, Raghav

Jbalkind commented 4 years ago

@grigoriy-chirkov I'm looking at the definition of the ADDR_TRANS_PHYS_WIDTH_ALIGN macro and wondering if Michael chose the wrong ones here. What do you think?

https://github.com/PrincetonUniversity/openpiton/blob/3eefdef42e03d1df14242e551cda2ef8c964bcf6/piton/design/chipset/include/chipset_define.vh#L78-L90

At some point the shifts were done more manually: https://github.com/PrincetonUniversity/openpiton/blob/ec2437c11577d7fef939c80ea8ee66c29cc3c5a5/piton/design/chipset/rtl/storage_addr_trans.v#L46-L76

Are all of the boards off by some factor like Raghav is seeing?

@Raghavendra-Srinivas How much additional shift did you have to add versus what was already there? By which I mean, how much was the code above off by? Just off-by-one? Or was it 6 like you said?

Raghavendra-Srinivas commented 4 years ago

Hi @Jbalkind ,

ADDR_TRANS_PHYS_WIDTH_ALIGN is related to phy. I did not look much into it.

But My intention was to to give null translation for only ARIANE case while storing the image through UART (Pitostream flow). so "storage_addr_trans_unique" is the right file. In this file, currently it using the above define which is doing right shift by 5, then again you left shift by 3 , so eventually it giving affect of effective right shift by 2. and then later on there is another left shift by 3 in noc_bridge_write/read.sv files. so , overall it was NOT giving the intended effect of null translation.

So, what I did is right shift by ONE more additional bit like below in file "storage_addr_trans_unified.v.pyv". https://github.com/Raghavendra-Srinivas/openpitonHawk/blob/9e68f75791ebb153d0354a82b9c45716cf9190c5/piton/design/chipset/rtl/storage_addr_trans_unified.v.pyv#L67

Test cases passed on FPGA with this change.

PS : I have enabled AXI based Memory controller (MIG) for Genesys2 board at my end with Ariane. So above data path for genesys2 with ariane was tested for first time it seems as default openpiton is not supported with AXI based MIG for genesys2. So, may be that's why you did not see any such issue.

Thanks, Raghav

Raghavendra-Srinivas commented 4 years ago

The shift inside "storage_addr_trans_unified" is fine for MIG with Native interface. My issue and solution as mentioned above is applicable to MIG with AXI interface,