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

Ariane : Translation Shift fix with Piton UART Stream #75

Open
Raghavendra-Srinivas opened this issue May 30, 2020 · 3 comments
Open

Ariane : Translation Shift fix with Piton UART Stream #75

Raghavendra-Srinivas opened this issue May 30, 2020 · 3 comments

Comments

@Raghavendra-Srinivas
Copy link

Raghavendra-Srinivas commented May 30, 2020

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.

print("assign bram_addr_0 = (({{(MEM_ADDR_WIDTH-VA_ADDR_WIDTH){1'b0}}, va_byte_addr} - 64'h%s) >> `ADDR_TRANS_PHYS_WIDTH_ALIGN) + 0;" % (memBegin));

Thanks,
Raghav

@Raghavendra-Srinivas Raghavendra-Srinivas changed the title Ariane : Translation Shift fix Ariane : Translation Shift fix with Piton UART Stream May 30, 2020
@Jbalkind
Copy link
Collaborator

Jbalkind commented Jun 8, 2020

@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?

`ifdef PITON_FPGA_BRAM_TEST // 64-bit PHY
`define ADDR_TRANS_PHYS_WIDTH_ALIGN 6
`define ADDR_TRANS_SECTION_MULT 1
`elsif NEXYSVIDEO_BOARD // 16-bit PHY
`define ADDR_TRANS_PHYS_WIDTH_ALIGN 4
`define ADDR_TRANS_SECTION_MULT 4
`elsif GENESYS2_BOARD // 32-bit PHY
`define ADDR_TRANS_PHYS_WIDTH_ALIGN 5
`define ADDR_TRANS_SECTION_MULT 2
`else // 64-bit interface by default
`define ADDR_TRANS_PHYS_WIDTH_ALIGN 6
`define ADDR_TRANS_SECTION_MULT 1
`endif

At some point the shifts were done more manually:

`ifdef VC707_BOARD
// align to 64bit
assign bram_addr_0 = (({{(MEM_ADDR_WIDTH-VA_ADDR_WIDTH){1'b0}}, va_byte_addr} - 64'h80000000) >> 6) + 0;
// 1GB
assign in_section_0 = (va_byte_addr >= 64'h80000000) & (va_byte_addr < 64'hC0000000);
`elsif VCU118_BOARD
// align to 64bit
assign bram_addr_0 = (({{(MEM_ADDR_WIDTH-VA_ADDR_WIDTH){1'b0}}, va_byte_addr} - 64'h80000000) >> 6) + 0;
// 2GB
assign in_section_0 = (va_byte_addr >= 64'h80000000) & (va_byte_addr < 64'h100000000);
`elsif NEXYS4DDR_BOARD
// align to 16bit
assign bram_addr_0 = (({{(MEM_ADDR_WIDTH-VA_ADDR_WIDTH){1'b0}}, va_byte_addr} - 64'h80000000) >> 4) + 0;
// 256MB
assign in_section_0 = (va_byte_addr >= 64'h80000000) & (va_byte_addr < 64'h90000000);
`elsif GENESYS2_BOARD
// align to 32bit
assign bram_addr_0 = (({{(MEM_ADDR_WIDTH-VA_ADDR_WIDTH){1'b0}}, va_byte_addr} - 64'h80000000) >> 5) + 0;
// 1GB
assign in_section_0 = (va_byte_addr >= 64'h80000000) & (va_byte_addr < 64'hC0000000);
`elsif NEXYSVIDEO_BOARD
// align to 16bit
assign bram_addr_0 = (({{(MEM_ADDR_WIDTH-VA_ADDR_WIDTH){1'b0}}, va_byte_addr} - 64'h80000000) >> 4) + 0;
// 512MB
assign in_section_0 = (va_byte_addr >= 64'h80000000) & (va_byte_addr < 64'hA0000000);
`else
// align to 64bit
assign bram_addr_0 = (({{(MEM_ADDR_WIDTH-VA_ADDR_WIDTH){1'b0}}, va_byte_addr} - 64'h80000000) >> 6) + 0;
// 1GB
assign in_section_0 = (va_byte_addr >= 64'h80000000) & (va_byte_addr < 64'hC0000000);
`endif

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
Copy link
Author

Raghavendra-Srinivas commented Jun 9, 2020

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
Copy link
Author

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,

Frankly3 pushed a commit to bsc-loca/openpiton-fpga that referenced this issue Jul 12, 2024
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

2 participants