ucb-bar / chipyard

An Agile RISC-V SoC Design Framework with in-order cores, out-of-order cores, accelerators, and more
https://chipyard.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
1.55k stars 618 forks source link

DividerOnlyClockGenerator improve naming #787

Open timsnyder-siv opened 3 years ago

timsnyder-siv commented 3 years ago

https://github.com/ucb-bar/chipyard/blob/255452f20af2eac4bbcf4888d6252b2260888b85/generators/chipyard/src/main/scala/clocking/DividerOnlyClockGenerator.scala#L89

@davidbiancolin, the ${name} in the code above has spaces in it. Should it be sanitized to swap them out for underscores?

Also, we receive output that looks like:

FireSim RationalClockBridge Frequency Summary
  Input Reference Frequency: 16000.0 MHz
  Output clock implicit_clock, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_mbus_address_adjuster_0, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_mbus_address_adjuster_1, requested: 1000.0 MHz, actual: 1000.0 MHz (division of 16)
  Output clock subsystem_sbus_0, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_sbus_core_0, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_sbus_2, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_sbus_3, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_sbus_4, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_sbus_5, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)
  Output clock subsystem_sbus_6, requested: 3200.0 MHz, actual: 3200.0 MHz (division of 5)

how do I translate subsystem_sbus_X into the actual thing that is being driven? For example, I how do I know which of the indices is driving the mbus vs the pbus? Can this naming be improved somehow to be more descriptive of what the clock is eventually going to? Will the naming automagially improve if the naming of the clockSinkNodes improve because that's how the names are generated in: https://github.com/ucb-bar/chipyard/blob/255452f20af2eac4bbcf4888d6252b2260888b85/generators/chipyard/src/main/scala/clocking/ClockGroupNamePrefixer.scala#L30-L39

I know you don't use it but it would be useful to me if the graphml output also included the negotiated frequency information in it's meta-data or perhaps on the clock edges (similar to how the data edges show their width).

davidbiancolin commented 3 years ago

Right, naming... So in our Subsystem we've made it so all the buses get useful names by doing this:

https://github.com/ucb-bar/chipyard/blob/master/generators/chipyard/src/main/scala/Subsystem.scala#L60-L75

The comment there should better explain what's happening. Basically if you want useful names you first need to provide one in the clock sink, and then also ensure it's not lost in the graph.

Go ahead and sanitize the name. Perhaps a more sane thing to do is to use the name of the instance for which this summary is being generated, though that's not strictly necessary with the CB because there can only be one. That would already sanitized.

timsnyder-siv commented 3 years ago

Thanks for the pointer to getting useful names. I had a feeling it was something missing in how I've integrated things.

davidbiancolin commented 3 years ago

What negotiated freq information are you referring to? Do diplomatic parameters get rendered in the graph?

timsnyder-siv commented 3 years ago

The widths of datapaths are rendered in the graph. I see edges for clocks that don't currently have a label. Since they are always width 1, perhaps labeling them with the frequency would be interesting & useful?