Interfacing to Xilinx Vivado block design environment

[motylewski]

I have reached a point where axim2wbsp and wbm2axisp and wbp2classic integrate well in Xilinx block design with AXI and WB interfaces recognized as bus structure.

• I had to change localparam to param at one place because of the same reasons as discussed in Vivado block design changes #18 .
  I would be fine with replacing them completely with formulas as done there.

• As for

`default_nettype none
...
`ifndef YOSYS
`default_nettype wire
`endif

why not use this at the beginning instead?:

`ifdef FORMAL
`default_nettype none
`endif
???

• I hope that you like the naming convention for WB signals I had defined in interfaces. In this way internal in/out names may use any naming convention while they are recognized as master or slave bus with uniform signal naming.

• As for parameter AXI_LSBS I need it to support narrow transaction see
  https://community.arm.com/developer/ip-products/system/f/soc-design-forum/10107/narrow-burst-32-bit-read-on-a-64-bit-data-bus-axi-read-transaction
  That means on 64 bit data AXI addr[2:0] are significant. Reading 32 bit from 0x1000 (data[31:0] used, addr 0x1000) is different from reading 32 bit @ 0x1004 (data[63:32] used, addr=0x1004). I may connect 32 bit WB slave connecting addr[15:2] to ADR_I and {DAT_O,DAT_O} to 64 bit DAT_I WB master.
  Writing may be easily solved as well using addr[2] to multiplex masters DAT_O into slave .DAT_I( addr[2] ? DAT_O[63:32] : DAT_O[31:0]). SEL_O must be multiplexed as well.
  I agree it will probably work only for single AXI transactions, not for burst. Anyway, it is very useful for me and costs no other changes. AXI_LSBS parameter should be marked experimental.

Regards,
Tomasz

[ZipCPU]

Thank you for the work you've put into this, and your willingness/desire to share back. Please allow for the following critiques:

1. Regarding the default_nettype. If I understand correct, the problem is that someone else's code is broken and so you wish to adjust this code to compensate. Wouldn't it make more sense to adjust the other (vendor?) code instead? Have you filed bug reports with whatever vendor code has problems with a default_nettype of none? Also, according to the Verilog specification, you should also be able to adjust the synthesis order, placing the default_nettype none items at the end of the synthesis file list in order to get files to pass. Have you tried this?

2. Also, with respect to default_nettype, Yosys is a part of the problem here as well. The last time I complained, I learned that it was not standards compliant but rather applies the last default nettype given to the whole source file. If you set the default nettype to wire at the end of the file, it reverts default nettype to wire for the entire file. (Not desired. I got burned by that in the past.) It won't work to adjust things at the top of the file with ifdef FORMAL, since 1) I use Yosys for more than formal, and 2) I also want default_nettype none for other build environment (Verilator) which catch errors within my logic. Skipping default_nettype none is also a problem, because some tools (Vivado) are known to automatically define a value as an (incorrectly sized) wire the first time it is seen, and then create a second definition for it if it is created later. default_nettype none helps me avoid these synthesis mistakes, by catching them early. The "correct" answer here should be default_nettype none at the top of the file and nothing more. Designs that fail in a default_nettype none environment are broken and should be fixed. An interim solution might be to go back to the default_nettype wire statement at the end of the file, but only for non-Yosys (i.e. standards compliant) synthesis tools---and with the caution that this tends to hide bugs elsewhere in a design.

3. Regarding axim2wbsp, it look,s like you want to drive the size of the WB bus using C_AXI_DATA_WIDTH and C_AXI_ADDR_WIDTH. I'm okay with that proposal, I hadn't decided which of the two buses should drive the width of the other. However, if the C_AXI_ parameters drive the bus than the other dependent parameters, the wishbone AW, DW, and even AXI_LSBS, should be declared as localparams in order to keep the design from being built in an invalid fashion, and ideally to cause the design to fail in synthesis if it is being built in an invalid fashion.

4. I'm not sure I understand your argument regarding why AXI_LSBS should be a localparam vs a parameter and what that has to do with narrow transfers. I have read the article you posted, and it doesn't seem relevant to the issue. These axim*wbsp cores do not adjust the width of the data channel. Data width transformation is handled by a separate transform, if at all. (I have an upsizer that's still being verified, but haven't (yet) built the downsizer.)

5. Regarding the (* X_INTERFACE_INFO ... *) properties, I'm personally a stickler for 80-column lines. Let's therefore break these lines in two, one with the attributes and one with the declarations. Either way it's not going to be pretty, but if it makes the cores more usable than we can continue with them.

6. Finally, Wishbone 3 vs Wishbone 4 isn't really the right terminology. Technically, the standards are B3 and B4, as though they were both preliminary--and B4 was never really accepted by the community, despite the fact that I like to use it exclusively. Perhaps the correct terms are Wishbone classic and Wishbone pipeline. I know I've abbreviated these in the past as WBP and WBC, but an argument could be made for better names than those.

I look forward to seeing the above items addressed. Again, thank you for your work on this behalf,

Dan

[motylewski]

Hi Dan,
Thanks for thoughtful evaluation.

• default_nettype : fixing vendors code is not possible in all situations. Some source code may even be encrypted. No, I have not filled bug reports. I have just followed answers from Xilinx stating effectively " make sure that default_nettype is left as wire". Would
  ifdef NEW_LABEL default_nettype none
  `endif
  work for you? Are you able to globally define a NEW_LABEL in your projects and environments?
  Changing compile order is possible, but more work, some automatic has to be switched off.

• AXI_LSBS : I was arguing to make it parameter instead of localparam so I would have a chance to pass more address bits to the slave wishbone. I have linked the discussion only to show what my use case is. The following statement: o_wb_addr = axi_waddr[C_AXI_ADDR_WIDTH-1:AXI_LSBS]; deletes the last AXI_LSBS bits. In case of narrow transfer I need them. Skipping them when connecting full width WB slaves is trivial, but not having them when connecting narrower WB slaves is fatal.
  And I have made all sizes parameter because Vivado is not able to size ports using localparam correctly in the block design. Again parameter AXI_LSBS is really needed, other parameter AW and DW are there as workaround. The other possibility (for DW and AW) would be doing it like in 39c8b7d .

• I will split (* X_INTERFACE_INFO ... *) and port declarations as you suggested. Even though I think it looks prettier now ;-)

• B3 vs B4 : I am open to any naming. I even doubt whether it makes sense to define 2 interface types instead of just one "wishbone" with many optional ports. The difference is block design GUI would not connect WB3 with WB4 if we have 2 interface definitions. Currently the only difference is that B4 has STALL port while B3 not. What do you think?
  I will let you decide which names should I use in the source code comments as well.

Regards,
Tomasz

[motylewski]

As for naming: I think I slowly start understanding that there are at least following modes:

• wishbone classic:
  assign ACK_O = CYC_I && STB_I && ! busy;

• wishbone classic synchronous:
  always @(posedge CLK_I)
  ACK_O = CYC_I && STB_I && (!ACK_O) && ! busy;

• wishbone pipelined:
  always @(posedge CLK_I) begin
  ACK_O = CYC_I && STB_I && ! busy;
  end
  assign STALL_O = busy;

where busy assertion will differ between reading and writing.

[ZipCPU]

Well, sort of ... WB pipeline is much more capable than that. STALL should be high if you aren't ready to accept a new transfer. ACK should go high when you are complete. In between the two, you might have many requests being processed. Take this SDRAM controller for example. It will allow multiple requests to be in process at the same time.

Also, WB classic is really the same as WB classic synchronous--it's just a matter of whether or not you are taking a clock to do the processing or not. Either way, WB classic often cannot maintain one beat per clock without significantly slowing the clock rate down.

Let's do this: let's apply the version names B3 and B4, and then have two protocol names--something for wb classic and something else for wb pipelined. While I'd like to call WB pipelined just WB, I'll admit that others have been confused by this in the past. Perhaps WBC and WBP? Perhaps WBClassic and WBPipeline? What do you think there?

Also, the AXI_LSBS thing is still not right. Why not, for 8-bit slaves, just let Vivado drop the bus size to 8-bits before switching to WB? This core's purpose is not bus size adjustment.

Dan

[motylewski]

Hi Dan,

I will run Monday some tests to see whether Vivado allows connecting different interfaces, or interfaces with different version IDs.

I tend towards simply using opencores.org : bus : wishbone for both B3 and B4. It might be still versioned on top of that. But I need to first play more with the tools.

Tomasz

[ZipCPU]

Awesome! Please keep me posted.

[motylewski]

OK, it looks that Vivado block design GUI connects interfaces only when the complete name including version matches.

I have decided to use the following names:
opencores.org:bus:wishbone:B3
opencores.org:bus:wishbone:B4 (most interfaces)

Therefore it is not possible to connect B3 and B4 interfaces with a bus connection, which might be a good thing. It is still possible to connect them wire-by-wire.

I have seen that Xilinx itself uses the same interface definition/name/version for both AXI and AXI-Lite, so they really have no better solution.

I have tried to pack the (* X_INTERFACE_INFO = "opencores.org:bus:wishbone:B4 M_WBP NAME *) attribute in the `define(NAME), but it did not work,

I have decided to use names like M_WBP for pipelined master and S_WPC for classic slave for example.

This is more or less my final version.

Of course the new wishbone and AXI X_INTERFACE_INFO labels might be added to all other wb2axip cores, I have only added them where I needed.

Best regards,

Tomasz

[ZipCPU]

Just writing back to see the status of this. Looks like I missed the "final version" comment. Should I test this and merge it then?

Dan