[top] inter-module connection about opentitan HOT 10 CLOSED

Hi Eunchan,
this is a great summary! I'm going to discuss point by point below (i'm going to go in slightly different order).

Regarding Data type
I actually lean very strongly towards using packed structs here. To me the main benefit is the I/O is cleaner, and as you say we don't necessarily need to worry about bit width mismatch. I also thinks this makes porting a little easier in the long term. If we go with structs, I also feel the hjson definition can become a bit simpler. We define fewer things overall. I very much understand your point about IP dependency.. so the following is my view (not claiming it's correct, just the approach I have taken)

Regarding Dependency
My view on top level for awhile is that it can be highly customized and does not necessarily have to be automated. So this means I take more the approach of having each IP define its output and input structs. These structs do not necessarily relate to the destination, but rather is just a collection of all inter-module IOs (does that make sense?) By going this approach, we de-couple the IP dependency, however we make it manual work at the top level to stitch them up. Meaning in the top level of every design, we need to import all the related packages, and create a custom 'glue module' of sorts to make the connections between all the structs.

To me, this is a compromise between all options because it keeps the IPs isolated from each other (this might actually be a must if we assume that people can individually consume modules without the whole reference top). The connection we need to make manually is really not that different from the hjson specification of 'from<->to', so in my mind, it is not a super high burden. What do you think? So something like the following

top_level_glue
(
.inst_a_outputs,
.inst_b_outputs,
.inst_a_inputs,
.inst_b_inputs
)

module_a inst_a
(
.inst_a_outputs,
.inst_a_inputs
);

module_b inst_b
(
.inst_b_outputs,
.inst_b_inputs
);

module top_level_glue
(
...
) 
assign inst_b_inputs.xxx = inst_a_outputs.xxx

endmodule

On Clocking Information
I actually have a different view here. I was thinking that the top level should provide a hint/indication (possibly a parameter) that a signal is async to a particular IP, but it should leave the actual implementation of this crossing to the IP itself. There's a few reasons to this in my mind.

• The first is that I think ideally the top level should be free of small amounts of glue logic (other than connectivity steering). I think when we have small pockets of logic, it becomes a challenge for DFT later when they have to decide which areas to group the logic together for test, and if this is coupled with power domain crossings can take a few iterations to sort out. Especially for OT, I think being explicit about the hierarchies in our reference top would be helpful in driving a consistent PD implementation for all the consumers.

• The second is that I think each IP may have a different approach in how they want to handle the crossing. It could be a simple handshake, it could be a signal toggle, it could be a fifo and there are probably even options. It doesn't seem like a good idea to constrain that flexibility at the top level. We should let know IP know there is a crossing, and the IP should be responsible for handling it. I think the best example here is probably your xbar. Don't you also provide async information to the overall xbar_top instead of mandating the scheme outside? If for the time being we imagined that the xbar module was actually designed by someone other than you, then I think this is right approach.

Let me know what you think! We should discuss more!

from opentitan.

I'm in similar alignment with @tjaychen on this, possibly minor modifications.

1. I think having the ip.hjson define an 'internal' (or some better name) struct gives it flexibility and minimizes wiring.
   a. This could be as simple as an internal_input_list and _output_list (to mirror available_input_list and output).
   b. It does mean that we need to create or maintain this struct (or two) in each IP in another package.
2. I like the idea of the IP being agnostic to other IP, so saying where the connections should go should be in either top.hjson or like Tim says either in a glue module or just hardwired at the top level.

I'll have to think more about the clocking after conferring with the two of you. We should just sit down and hammer this out among the three of us in person next week.

from opentitan.

from opentitan.

Let me summarize the discussion having today:

1. Keep current flash_ctrl and flash macro connection as it is until we hit the other inter module connection later (SPI Passthrough or DMA or TRNG ?)

2. Define input/output for inter module connection as struct packed.

3. a struct belongs to a single clock.

4. Try to pack output signals into a struct as much as possible. But there will be always exceptions. For instance, SPI Device implementing Passthrough feature would have multiple struct and logic. One for SCK, others for MOSI/MISO and more.

5. ip.hjson will have new item with key 'intermodule_output_list', 'intermodule_input_list'. The list has multiple struct/logic entries. Each entry has name and type.

   intermodule_output_list: [{name:"fms", type:"fms_t"}, ... ]
   

6. TBD there's no strict rule where the struct definition (probably inside package file) among originator module or destination modules. But try to put it in the originator module. Exception: DMA can have common struct named as dma_credit_t and multiple IPs (uart, spi_device, etc) uses the struct to send available credit to DMA.

7. If the struct is used inter module, then create its own core file to be easily included in the other IPs.

8. Good to have Top configuration .hjson has a item connections. It defines the inter module connections as below:

   { type: "top",
     ....
     connections: [
       { A.fms: ["B.fms", "C.fms"] },
       { C.dma_credit: ["DMA0.credit0"] },
       ...,
     ]
   }
   

   Top module also can have manual assignment, if it is hard to describe as above. There will be always exceptions. :)

9. CDC logic sits in the IP not on the top. And most likely in the originators. Yet, we don't have to concern this at this moment. Punt it.

@sjgitty @tjaychen Please comment if anything to add.

from opentitan.

from opentitan.

@eunchan , has the final proposal been implemented? Suggest bump this down to P2 if it is still open.

from opentitan.

@eunchan please refresh with current thinking, has been stale for 6 months

from opentitan.

PR #1490 is merged. But still a few things need to be implemented.
A.I:

1. Define unique inter-module signal (naming rule) when it has multiple
   connections of the same struct
2. Reduce the fields of inter_signal_list in the ip.hjson
3. Add broadcast type and handling
4. Add validation check
5. Revise inter_module structure to cover the cases:
   • 1:1 -> currently supported
   • 1:N -> structurely OK, but not implemented
   • N:1 -> Not possible to cover with current structure

from opentitan.

@eunchan should we close this issue now?
I think intermodule is pretty well known now and also in use.

from opentitan.

Agree :) thanks for the ping

from opentitan.