-
Notifications
You must be signed in to change notification settings - Fork 802
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
[doc/top_earlgrey] Update top_earlgrey doc to reflect today #851
Conversation
Did a quick read-through, noted no obvious formatting or grammatical issues. The technical aspect matched my recollection, but I defer to the listed reviewers who are more familiar with the top level signals. |
hw/top_earlgrey/doc/index.md
Outdated
## Hardware Interfaces | ||
|
||
Below are the expected hardware interfaces of the autogenerated `top_earlgrey` netlist. | ||
These are the internal signals between that module that the pads of the platform netlist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... module that ...
-> ... module and ...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, otherwise LGTM.
It should first be **NOTED** that there is some subtlety on the notion of hierarchy within the top level. | ||
There is netlist automation to create the module `top_earlgrey` as indicated in sections of this specification that follow. | ||
**On top** of that module, hierarchically in the repo, are top level instantiation targets directed towards a particular use case. | ||
This includes `top_earlgrey_nexsysvideo` for use in FPGA, and `top_earlgrey_asic` for use (eventually) in a silicon implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also mention verilator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to just stick to one for now. "When you're explaining, you're losing".
But I agree we need some more collateral on these distinctions, just not sure
where. I think we need a) better terminology of our top-level netlists (WIP document
by Tim/Michael/Eunchan); b) better definition of the difference between the autogen'd
top and the platform top (or whatever we call it/them); c) documentation for the
different platform tops. Perhaps we restrict this one to just the autogen'd top and
introduce a separate platform document, but things get messy.
I'm very open to ideas here. I'll file an issue with this verbiage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is a related (very old) issue that we have been deferring so far due to its impact on the toplevel: #295
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is a related (very old) issue that we have been deferring so far due to its impact on the toplevel: #295
Yeah, I found it also and cut/paste there to avoid a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
hw/top_earlgrey/doc/index.md
Outdated
| `IO_DPS2` | output | Muxed functionality: JTAG `TDO` and `spi_device_miso_o` | | ||
| `IO_DPS3` | input | Muxed functionality: JTAG `TMS` and `spi_device_csb_i` | | ||
| `IO_DPS4` | input | JTAG `TRST_N` | | ||
| `IO_DPS5` | input | JTAG `TRST_N` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is actually SRSTN, not TRSTN.
It can be used like a pin reset (not really different from IO_RST_N at the moment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -210,6 +262,7 @@ See the [padctrl specification]({{< relref "hw/ip/padctrl/doc" >}}) for informat | |||
|
|||
The chip contains one UART peripheral that implement single-lane duplex UART functionality. | |||
The outputs and inputs can be configured to any chip IO via the pinmux. | |||
(Exception: in the current design the UART pins are directly connected to their own pin IO.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't make any mention of SPI's directly connected pin it seems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are called out as direct-connection in the diagram and the table, while
UART appears to be MIO in the diagram, and DIO in the table, so I felt the
need to explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This isn't perfect, but an improvement over what we have today. |
Addresses #778
At the moment, there are a lot of "workarounds" with our use of pinmux, pad
names, etc., and the documentation of
top_earlgrey
is misleading. ThisPR attempts to walk that fine line between explaining and excusing the
current state of the design. PTAL to see if it suffices.