OpenTitan Earlgrey pinmux implementations #3707
Conversation
2ce7207 to
429fde6
Compare
429fde6 to
93465e1
Compare
| // Built for Earlgrey-M2.5.1-RC1-389-g21ce4e9761 | ||
| // https://github.com/lowRISC/opentitan/tree/21ce4e9761abdf5c919b46e5ae64a5a8e24992f7 | ||
| // Built for Earlgrey-M2.5.2-RC0-904-g9a5280ee33 | ||
| // https://github.com/lowRISC/opentitan/tree/9a5280ee3331bc20f2659eee37a56f270454bdcf |
There was a problem hiding this comment.
We should also update the Makefile sha. It looks like there's a mismatch already too.
There was a problem hiding this comment.
I think it's ok if those SHAs don't match. Since the HW isn't changing, we shouldn't need to update these definitions for correctness, but changes like this renaming might come up time-to-time.
There was a problem hiding this comment.
@cfrantz Can you check if it will be Ok to update SHA ?
There was a problem hiding this comment.
This commit is based on upstream/master OpenTitan version of file top_earlgrey.rs.
If it will be possible i would like to stay on top of Earlgrey ES branch because this branch will be used for HW samples. Changes in top_earlgrey.rs are pure syntax sugar.
There was a problem hiding this comment.
As long as this file (top_earlgrey.rs) came from the earlgrey_es branch, then we don't need to update the SHA referenced in the Makefile. There are no hardware changes to anything on the earlgrey_es branch (apart from the already-known FPGA clocking changes).
The SHA in the Makefile (7df72d61...) refers to the correct hardware version and the bitstream one should download from https://storage.googleapis.com/opentitan-bitstreams/
There was a problem hiding this comment.
Generating registers is separate from the HW bitstream, and I don't see any issue with the commits being different.
How is it separate? The whole idea is that the registers are generated from the RTL so they will update together. By keeping them in sync we know the registers match the hardware and it's all automatic
If I'm understanding correctly, what is changing in new commits isn't the actual register meanings, but instead the tool. Updating the tool for the auto code generation shouldn't require new hardware testing.
There was a problem hiding this comment.
Generating registers is separate from the HW bitstream, and I don't see any issue with the commits being different.
How is it separate? The whole idea is that the registers are generated from the RTL so they will update together. By keeping them in sync we know the registers match the hardware and it's all automatic
These changes to the register definition are software changes, not hardware changes. The tool that processes the register description files (which are stable) has proved to generate better definitions. We want to use the better definitions here in Tock, so we want to generate the definitions using the tool from latest master.
Since HW changes are nearly forbidden[^1] on
master, we're going to generate these constants from master.This doesn't really explain why they can't be in sync. If a hardware change happens on master the register definition will change, but the bitstream we are using won't? That seems prone to breakage.
If hardware changes were to happen on master, sure, but (except for ECOs) hardware changes are banned on master too.
Like you say, there isn't going to be a lot of churn on master, so why not just update it?
We could pull the bitstream from master, but our goal here is to run Tock on the real hardware, and earlgrey_es is the closest bitstream we have to that.
That actually makes sense to me, if the HW is frozen then the tools shouldn't need to be fixed to the same commit. Much in the same way that libtock-c and the kernel evolve separately.
But the hardware isn't frozen. There has already been one change since tapeout for FPGAs and @cfrantz just said that some changes can go in.
ECOs are very unlikely to result in register changes.
libtock-c and the kernel are both written to be API compatible. A change in the hardware would generally be an "ABI breakage", so it doesn't seem exactly the same
We want to keep the SHA in the makefile the same because that SHA refers to the version of the codebase that would construct the exact bitstream, verilator model, test_rom and mask_rom that are being used for the chip tape-out. If you're running tock on the FPGA or in a simulator, that is the version you want.
Isn't that not the case? We have already bumped the SHA we use in the Makefile since tapeout to get faster clock frequencies. Why can't we do the same to update register layouts.
We are already bumping the Makefile from tape out: https://github.com/tock/tock/pull/3703/files. So we are no longer targeting the bitstream from hardware production. It seems strange that it wasn't an issue for #3703 but it's an issue now. What am I missing here?
The SHA is #3703 is just a newer commit from the earlgrey_es branch. You're asking us to either copy a bunch of software changes into earlgrey_es or use a hardware bitstream from master, and both of those ideas have issues with no upside other than "the SHAs match".
There was a problem hiding this comment.
Generating registers is separate from the HW bitstream, and I don't see any issue with the commits being different.
How is it separate? The whole idea is that the registers are generated from the RTL so they will update together. By keeping them in sync we know the registers match the hardware and it's all automatic
If I'm understanding correctly, what is changing in new commits isn't the actual register meanings, but instead the tool. Updating the tool for the auto code generation shouldn't require new hardware testing.
Yes, exactly.
There was a problem hiding this comment.
I understand that the master OT branch has software changes for the register layout generation.
Like Johnathon just said, there might be hardware changes on master to fix issues. When/If that happens and we bump the register layout again the hardware used to generate the registers and the actual bitstream will be out of sync. I understand you expect it to be unlikey that there will be register changes, but we don't know for sure that won't happen.
Hardware wise earlgrey_es and master should be practically the same as you just said. There are no new features being developed on master, so we don't expect a lot of churn.
So what is wrong with just updating the bitstream SHA to point to master? You are worried it is out of sync? But then the registers generated would be out of sync.
The SHA is #3703 is just a newer commit from the
earlgrey_esbranch. You're asking us to either copy a bunch of software changes intoearlgrey_esor use a hardware bitstream frommaster, and both of those ideas have issues with no upside other than "the SHAs match".
I never said anything about copying. I just think that the register definitions we are using should match the hardware we are targeting. Which means they should be generated from the same RTL source. That way we know they are in sync.
We wouldn't generate Rustdocs for a release from a different source for example, I don't see why this is any different.
There was a problem hiding this comment.
The PR with SW changes to earlgrey_eswas merged:
lowRISC/opentitan#19988
I updated code with all earlgrey_es SHA:
- makefile
- register
- soc constans
- README
let me know if i miss something
93465e1 to
213acc6
Compare
213acc6 to
0d3320e
Compare
| // Built for Earlgrey-M2.5.1-RC1-389-g21ce4e9761 | ||
| // https://github.com/lowRISC/opentitan/tree/21ce4e9761abdf5c919b46e5ae64a5a8e24992f7 | ||
| // Built for Earlgrey-M2.5.2-RC0-904-g9a5280ee33 | ||
| // https://github.com/lowRISC/opentitan/tree/9a5280ee3331bc20f2659eee37a56f270454bdcf |
There was a problem hiding this comment.
Why not just update them both at the same time? If there are no HW changes it's a very simple change in Tock and that way everything is clear and in sync.
It seems strange to be using different hardware revisions for the registers and the bitstreams
| // SPDX-License-Identifier: Apache-2.0 OR MIT | ||
| // Copyright Tock Contributors 2022. | ||
|
|
||
| //! Mux'ing between physical pads and GPIO or other peripherals. |
There was a problem hiding this comment.
Can we add tests for these? How do we verify that they are working
There was a problem hiding this comment.
Multiplexing is done in hardware, so you can observe only side effect of this in SW.
For example you set redirections between GPIO and PAD and then check if your signal reach GPIO software handler. More problematic functionality for example Hi-Z can only be test in hardware.
I am not sure if verilator/qemu will be capable of doing this, the best way is to check it on FPGA or ASIC.
There was a problem hiding this comment.
I understand that a pinmux affects the GPIO hardware. We can still add tests that change those and can then verify them externally. It's what we do with LEDs on some platforms for example.
Otherwise we have a driver that isn't called, so it's very difficult to test and catch regressions
There was a problem hiding this comment.
How did you end up testing this?
There was a problem hiding this comment.
Short answer: I used CW310 as DUT and Hyperdebug & opentitantool for GPIO stimulations.
But as you wrote we need some driver to call this code, unfortunately this drivers aren't on upstream TockOS.
TLDR:
I share your concerns about code quality but we currently don't have any decent test framework in TockOS and OpenTitan. (that can test hardware feature like this). We are working on some integration testing for TockOS in OpenTitan but it probably take some time.
This PR #3710 with GPIO will help with testing pinmux on upstream TockOS.
We can continue this topic in PR described above.
There was a problem hiding this comment.
Tock on OpenTitan has a few integrated unit tests which we use to test the drivers. I think the majority of drivers have at least some tests now. We have used that for a few years to catch regressions when updating the hardware. That is a good place to start for adding tests and shouldn't take too much time.
It doesn't have something to automatically check the GPIO output though. I agree that is a limitation, but at least setting the registers and ensuring they match what we expect is something. And a lot better then nothing :)
0d3320e to
9b4331f
Compare
* This update does't make any changes to Earlgrey SOC layout. * This updated add traits with allow for easy coping and comparing value of generated enumerations. * Updated definitions don't include `TOP_EARLGREY` as name prefix. Following commit fix existing code that was using old naming scheme. * Re-licencing auto-generated data to TockOS compatble licence. Content of this file was generated by using following commands: ``` cd $OPENTITAN_REPO_DIR make -C hw topgen_rust_pk tar -C $TOCK_REPO_DIR/chips/earlgrey/src \ -xvf build/autogen/chip_top_earlgrey.tar \ chip/top_earlgrey.rs --stript-components 1 ``` Signed-off-by: Michał Mazurek <mazurekm@google.com>
* This commit update register definition based on OpenTitan ` earlgrey_es` branch. * Update recomended bitstream version for Opentitan board. New recomended version: Earlgrey-M2.5.1-RC1-493-gedf5e35f5d Based on SHA: edf5e35f5d50a5377641c90a315109a351de7635 Signed-off-by: Michał Mazurek <mazurekm@google.com>
9b4331f to
c550349
Compare
This commit introduce new `pinmux` driver for Earlgrey. Documentation for mentioned IP can be in link below. `https://opentitan.org/book/hw/ip/pinmux/index.html` Supported function: * Pad attribute setting * MIO I/O pad/peripheral selections Co-authored-by: Jes Klinke <jbk@google.com> Signed-off-by: Michał Mazurek <mazurekm@google.com> Signed-off-by: Michał Mazurek <maz@semihalf.com>
c550349 to
d5ae79c
Compare
|
I think we have to make a decision, and I think it is important that we not require the register definitions to be generated from the same commit as the hardware bitstream. The hardware for this version of OT is frozen and the particular SHA should stay the same. Users shouldn't have to change their OT checkout for the same hardware. However, the registers might change due to things like spelling mistakes, bugs, or adding new peripherals. They may also use SVDs in the future, but those type of changes don't affect the hardware. |
|
Re: testing: as we've discussed on the OT call several times, the OT team has a plan for testing tock as part of their larger testing framework, and that is where the effort should go. We can mark as a todo for more unit-test style tests, but in this case there just isn't much benefit since we have no way to observe the output. |
Discussed on OT call that this makes sense going forward.
|
This PR was part of ongoing task described in OpenTitan issue: lowRISC#28 |
BCA2
Pull Request Overview
Testing Strategy
Manual test with Hyperdebug + CW310 FPGA board and custom application.
TODO
Help Wanted
Documentation Updated
Formatting
make prepush.