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

Feature Request: Expanded CFS I/O #13

Closed
vhdlnerd opened this issue Feb 28, 2021 · 8 comments
Closed

Feature Request: Expanded CFS I/O #13

vhdlnerd opened this issue Feb 28, 2021 · 8 comments
Labels
enhancement New feature or request

Comments

@vhdlnerd
Copy link
Contributor

Having only two 32-bit ports, one input and one output, for the CFS is a little limiting. I know I can just modify the design and add whatever I want, But, it would be nice if the core allowed more CFS I/O without core modification.

Some like:
Add this type to neorv32_package.vhd:
type cfs_io_t is array (integer range <>) of std_ulogic_vector(31 downto 0);

Add new generics to neorv32_top:

    IO_CFS_IN_VECS							 : positive := 1;
    IO_CFS_OUT_VECS							 : positive := 1;`

Change CFS I/O ports to:

    cfs_in_i    : in  cfs_io_t(0 to IO_CFS_IN_VECS-1) := (others => (others => '0')); -- custom CFS inputs conduit
    cfs_out_o   : out cfs_io_t(0 to IO_CFS_OUT_VECS-1); -- custom CFS outputs conduit

And, of course, update the port hierarchy, for the CFS to match.

This way end users could have as many CFS 32-bit I/O ports as they wished.

@stnolting
Copy link
Owner

I should have seen it coming that 32-bit of CFS in/outs might be a limiting factor...

type cfs_io_t is array (integer range <>) of std_ulogic_vector(31 downto 0);

I have to admit, this is an elegant solution!

However, personally I do not like having multi-dimensional array data types in the top entity. Also, I am not sure if high-level designs (like the Vivado IP packager) might struggle with these interface types.

How-about a trade-off? We could transform your [N][M] IO array into a single [N*M] array.

New generic for neorv32_top:
CFS_IO_SIZE : natural := 32;

And the modified CFS IO conduits:

cfs_in_i    : in  std_ulogic_vector(CFS_IO_SIZE-1 downto 0) := (others => '0');
cfs_out_o   : out std_ulogic_vector(CFS_IO_SIZE-1 downto 0);

But I am open for a discussion 😉

@stnolting stnolting added the enhancement New feature or request label Mar 1, 2021
@vhdlnerd
Copy link
Contributor Author

vhdlnerd commented Mar 1, 2021

Personally, I don't mind port types of multi-dimensional arrays (or arrays of records, record with arrays, records of records,... well, just about anything VHDL allows 😄) at any level of the hierarchy (except the true top that connects to FPGA I/O). But, that is just my coding style; I'm not saying it's better than anyone else's coding style. Plus, if you want to use a higher level IP stitching tool, it's best to avoid crazy port types.

Anyway, what you suggested is fine with me -- in the end it's just wires! I have two comments though:

  1. Can we have separate generic lengths for the CFS inputs and outputs? I would think these would rarely need to be the same length.
  2. I would use 'positive' types for the port lengths. Its an indication to the user that zero is not a valid value. This is, again, a style thing. 'Natural' type is good enough -- either way, setting the length to zero will cause an error.

Thanks!

@AWenzel83
Copy link
Collaborator

Also, I am not sure if high-level designs (like the Vivado IP packager) might struggle with these interface types.

I can confirm, that the IP packager doesn't work with multidimensional arrays

@stnolting
Copy link
Owner

@vhdlnerd

Can we have separate generic lengths for the CFS inputs and outputs? I would think these would rarely need to be the same length.

You could still leave all unused wires unconnected.
But having separate size configurations sounds fine. 👍

I would use 'positive' types for the port lengths. Its an indication to the user that zero is not a valid value. This is, again, a style thing. 'Natural' type is good enough -- either way, setting the length to zero will cause an error.

That's a nice fail-safe approach.

@AWenzel83

I can confirm, that the IP packager doesn't work with multidimensional arrays

Thanks for clearing this. What about having a generic to define the size of a top entity signal? Maybe changing the generic would have no immediate effect at all and the IP would require re-packaging to adapt signals sizes. Have you ever tried that?

@AWenzel83
Copy link
Collaborator

AWenzel83 commented Mar 3, 2021

Generic sizes are working, I have done that before.

@stnolting
Copy link
Owner

Generic sizes are working, I have done that before.

Good to know!

I will add separate size configuration generics for the CFS input and output conduits 👍

stnolting added a commit that referenced this issue Mar 4, 2021
* IO_CFS_IN_SIZE defines size of cfs_in_i signal
* IO_CFS_OUT_SIZE defines size of cfs_out_o signal
* generic type is "positive"
* default value for both generics = 32
stnolting added a commit that referenced this issue Mar 4, 2021
@stnolting
Copy link
Owner

I have added the CFS IO-size configuration generics

    IO_CFS_IN_SIZE  : positive := 32; -- size of CFS input conduit in bits
    IO_CFS_OUT_SIZE : positive := 32; -- size of CFS output conduit in bits

and modified the according CFS signals

    cfs_in_i  : in  std_ulogic_vector(IO_CFS_IN_SIZE-1  downto 0); -- custom CFS inputs conduit
    cfs_out_o : out std_ulogic_vector(IO_CFS_OUT_SIZE-1 downto 0); -- custom CFS outputs conduit

Feel free to open a new issue if something isn't working as expected.

@umarcor
Copy link
Collaborator

umarcor commented Jun 2, 2021

Also, I am not sure if high-level designs (like the Vivado IP packager) might struggle with these interface types.

FTR, GHDL allows processing any VHDL code and generating a VHDL 1993 netlist that any vendor tool can accept. It can effectively be used for adding "VHDL 2008" support to the vendors that don't support it. See https://ghdl.github.io/ghdl/using/Synthesis.html#cmdoption-ghdl-out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants