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

Updated architectural specification #1

Merged
merged 23 commits into from
Dec 4, 2023

Conversation

marnovandermaas
Copy link
Contributor

@marnovandermaas marnovandermaas commented Nov 21, 2023

Based on Greg's comments, I have made a major update to the architectural specification.

Some sections still need some work, I'm happy to iterate on this through this PR or otherwise we can merge it and make changes in follow up PRs.

Changes to the software architecture I will leave until later PR to solve.

GregAC

This comment was marked as resolved.

The board design, FPGA configuration and software are now in separate
files and the main architecture document links to them.
Non-architecture documentation will also live in the doc folder so this
moves all the separate files into their own architecture folder. This
commit also renames architecture.md to intro.md since the folder already
caries the name.
This moves the specification of each IP block from the architecture
specification of the FPGA into separate files in the doc/ip folder.
doc/ip/dma.md Outdated
Comment on lines 50 to 53
| 16-14 | Write size |
| 13-11 | Read size |
| 10-7 | Write burst |
| 6-3 | Read burst |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that the DMA controller would address only full 32-bit transfers on a TL-UL bus, in which case it does not require either the burst or the size parameters.
On the other hand, if it's purely memory-to-memory then the 'inc' fields are meaningless, so is the intention here to support peripheral devices as source/destination and/or narrower transfers, or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I've removed the size and burst parameters. The DMA may still be used for memory to memory, so I think keeping the inc field would make sense right?

@alees24
Copy link
Contributor

alees24 commented Nov 28, 2023

I have a couple of open questions from the software perspective, please:

  • when software is downloaded and starts running, will it be able to tell much/anything about the FPGA build being used? We should at least be able to stop execution with an error message if there's a sw/build mismatch, to avoid developer confusion.
  • is software able to ascertain (via Ibex-internal CSRs say) whether 'cheri_pmode_i' is asserted, ie. it will surely need to know whether CHERI is being used? Again, if only to avoid confusion.
  • would it not be a good idea to introduce at least a single register at the start of each IP block (offset 0) indicating, say, 2 bytes of signature (ASCII text 'UA' indicating UART, for example) followed by a version/revision byte and then a flags/features field in the remaining byte. This allows detection of IP blocks in the FPGA build, as well as perhaps allowing sw to adapt to different version/revisions/features of those IP blocks.
  • should the above be extended to any other common IP block functionality such as interrupts (as done in OT)?

Hardware questions:

  • are other 'base' peripherals eg. SD host to be named and have address space allocated, or just be omitted entirely for now?
  • will there be some kind of bus watchdog to catch invalid accesses or lock ups (missing 'heart beat' signal)? I think this is an important development aid, but it could also be configured to restart the system in the event of a violation being caught and reported by CHERI... rather than just leaving the demo 'IoT system' in a dead state.

@johngt

This comment was marked as outdated.

@nbdd0121
Copy link
Contributor

I have a couple of open questions from the software perspective, please:

* when software is downloaded and starts running, will it be able to tell much/anything about the FPGA build being used? We should at least be able to stop execution with an error message if there's a sw/build mismatch, to avoid developer confusion.

* is software able to ascertain (via Ibex-internal CSRs say) whether 'cheri_pmode_i' is asserted, ie. it will surely need to know whether CHERI is being used? Again, if only to avoid confusion.

* would it not be a good idea to introduce at least a single register at the start of each IP block (offset 0) indicating, say, 2 bytes of signature (ASCII text 'UA' indicating UART, for example) followed by a version/revision byte and then a flags/features field in the remaining byte. This allows detection of IP blocks in the FPGA build, as well as perhaps allowing sw to adapt to different version/revisions/features of those IP blocks.

I am not sure if this would be the best approach, as this changes all register offsets of IP. I think encoding a hash/build number of the FPGA bitstream should be good enough.

This includes interacting with buttons, switches, LEDs and extension
headers. It also has configurable debounce for the input.
@marnovandermaas marnovandermaas force-pushed the arch-v2 branch 2 times, most recently from c9b6ca7 to c68f5fa Compare December 4, 2023 15:11
Copy link
Contributor

@GregAC GregAC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tweak to the address map but I think we're basically there. Please do get back to @alees24 and @nbdd0121 on their questions. May be fine to just address their points in a follow-up.

| 0x8110_0000 | 1 MiB | [I2C host] |
| 0x8120_4000 | 1 MiB | [SPI host] |
| 0x8400_0000 | 64 MiB | [PLIC] |
| 0xF000_0000 | 4 KiB | [Debug module] |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need to think about where the Symphony Earl Grey access port window sits in this address space, it will be 1 on the 4 1 GiB aligned regions, with this map they all have something in. Perhaps put the debug module at the top of the 1 GiB region we currently have devices in (i.e. just below 0xC000_0000)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I've changed the debug module to 0xB000_0000 so we have the top region for the access port.

- Add section on capabilities in RAM
- Add cache paragraph
- Add hyperram section that has capability tags
It is particularly important that the DMA's interaction with CHERI is
well thought through. In this text, I try to capture what the security
implications are of using a DMA that is not CHERI aware.
This is based on the XADC block that is available on Artix 7 FPGAs.
Specify that HyperRAM can be clocked higher for performance
This commit adds receive and configuration functionatlity. This is
partially inspired by the OpenTitan SPI host:
https://opentitan.org/book/hw/ip/spi_host/doc/registers.html

It also adds a general description and register descriptions for the
ones that were already specified. Finally, there can be multiple SPI
instances in Sonata.
Also say capability tags are stored in ECC bits
Sonata's interrupt controller is based on that of OpenTitan.
Defer PLIC source mapping to later point.
The pulse width modulation block is an alternative way to drive the LEDs
so that they can be dimmed.
Add CSR to clear Ibex capability exception codes
Link to OpenTitan documentation instead of copying.
Add note on multiple I2C instances.
Also, add note on multiple UART instances.
This includes:
- Some review comments from Andreas and Adrian
- Adding note that Arduino shields are 3.3V

Also removed some inaccurate comments from software architecture.

Finally, fixed a typo in the introduction and changed the interactive
and integrable requirements.
This includes sizes but also number of blocks used based on width and
depth. This commit also reduces the amount of capability tag bits for
RAM to cover 1 MiB instead of 2 MiB. This reduces the pressure on block
RAMs.
The RAM now lives at 0x4000... so that there is more space to grow.
UART, I2C and SPI are given more space because of the possiblity of
multiple instances.

This commit also includes:
- Fix spelling of lightweight
- Remove conclusion from introduction
@marnovandermaas marnovandermaas force-pushed the arch-v2 branch 2 times, most recently from 3790233 to b72c150 Compare December 4, 2023 16:21
@marnovandermaas
Copy link
Contributor Author

Minor tweak to the address map but I think we're basically there. Please do get back to @alees24 and @nbdd0121 on their questions. May be fine to just address their points in a follow-up.

Thanks Greg, I've opened up the following issues to track the questions you mentioned:
#6, #7, #8

@marnovandermaas marnovandermaas merged commit 8a3d300 into lowRISC:main Dec 4, 2023
@marnovandermaas marnovandermaas deleted the arch-v2 branch December 4, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants