bespoke-silicon-group / basejump_stl

BaseJump STL: A Standard Template Library for SystemVerilog
http://bjump.org/
Other
520 stars 99 forks source link

Request for structs as NOC links' data field #148

Open vb000 opened 5 years ago

vb000 commented 5 years ago

https://github.com/bespoke-silicon-group/basejump_stl/blob/2238da9c9228edd41d3666d40afbcf706caa5e54/bsg_noc/bsg_noc_links.vh#L27

`define declare_bsg_then_ready_link_sif_s(in_data_width,in_struct_name)\
   typedef struct packed {                                                \
      logic       v;                                                      \
      logic       then_ready_rev;                                         \
      logic [in_data_width-1:0] data;                                     \
  } in_struct_name

In macros like above, data fields in NOC links are more often than not packets and packets are almost always structs. It's hard to interpret the data section as is, while debugging. Wouldn't it be better to take a "data type" argument instead of data width?

I'm proposing we could replace in_data_width with something like data_type_s

`define declare_bsg_then_ready_link_sif_s(data_type_s,in_struct_name)\
   typedef struct packed {                                                \
      logic       v;                                                      \
      logic       then_ready_rev;                                         \
      data_type_s data;                                     \
  } in_struct_name

@proftaylor @tommydcjung @dpetrisko What do you think?

dpetrisko commented 5 years ago

Interesting idea!

I could be wrong, but I don't think this is compatible with "logic [width_p-1:0]" as a data type, because of spaces.

It probably makes sense to have two macros:

`define declare_bsg_then_ready_link_sif_s(in_data_width,in_struct_name)\
   typedef struct packed {                                                \
      logic       v;                                                      \
      logic       then_ready_rev;                                         \
      logic [in_data_width-1:0] data;                                     \
  } in_struct_name
`define declare_bsg_then_ready_typed_link_sif_s(data_type_s,in_struct_name)\
   typedef struct packed {                                                \
      logic       v;                                                      \
      logic       then_ready_rev;                                         \
     data_type_s data;
  } in_struct_name
dpetrisko commented 5 years ago

In macros like above, data fields in NOC links are more often than not packets and packets are almost always structs.

This is true for mesh links, but not wormhole links, where many payloads are raw data

vb000 commented 5 years ago

Yes, I'm also not sure about logic [data_width_p-1:0] as macro arg :p. Having two versions sounds better -- caters both use cases!

tommydcjung commented 5 years ago

if that helps debugging, then go for it.