[top_earlgrey] Add fourth I2C #21676
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous `kI2cPlatformPins` array mapped to pins solely based on the platform. This works for CW310 (with PMOD and HyperDebug) where I2C is mapped to one pair of pins for all I2C instances. In DV, however, each I2C instance has its own agent, which is connected to its own pair of (muxed) pins. Coincidentally, when the I2C instance ID (i.e., 0 to 2) is (incorrectly) used to index the `kI2cPlatformPins` array, that would result in the correct pair of pins for DV. As we're adding a fourth I2C instance, however, this will no longer work. This commit takes the I2C instance ID into account when mapping to pins. The result is the same as before on the condition that the platform argument to a call of `i2c_testutils_select_pinmux` in `i2c_device_tx_rx_test.c` gets fixed (which this commit also does), but now it will be possible to add a fourth I2C instance while keeping the three platforms. Signed-off-by: Andreas Kurth <[email protected]>
andreaskurth
requested review from
eshapira,
cfrantz,
pamaury and
alees24
and removed request for
a team
andreaskurth
commented
Feb 26, 2024
andreaskurth
force-pushed
the
earlgrey-4th-i2c
branch
3 times, most recently
from
ba19f44
to
72a340b
Compare
|
Force-pushed to fix problems identified by CI. Files affected are:
|
andreaskurth
force-pushed
the
earlgrey-4th-i2c
branch
2 times, most recently
from
8ac0271
to
e58dc01
Compare
vogelpi
approved these changes
Feb 26, 2024
There was a problem hiding this comment.
This looks great, thanks @andreaskurth ! It would be good to get also @a-will 's input, especially on the FPGA pin selection.
andreaskurth
force-pushed
the
earlgrey-4th-i2c
branch
from
e58dc01
to
b4c80a6
Compare
a-will
approved these changes
Feb 26, 2024
andreaskurth
force-pushed
the
earlgrey-4th-i2c
branch
2 times, most recently
from
f8a86d6
to
95a8244
Compare
|
@moidx can you evaluate internally whether this mix and match de-risking strategy works for you, or whether we should just increase buffer sizes on all three existing I2Cs for now (and forego all the top-level changes)? @andreaskurth it would be good to wait merging this until we get confirmation. |
|
Ack, changing to draft to ensure this doesn't get merged accidentally. |
The following files were edited manually:
- hw/ip/otp_ctrl/data/otp_ctrl_img_owner_sw_cfg.hjson
- hw/ip_templates/rstmgr/doc/registers.md.tpl
- hw/ip_templates/rstmgr/dv/cov/rstmgr_unr_excl.el
- hw/ip_templates/rstmgr/dv/env/rstmgr_env_pkg.sv
- hw/ip_templates/rstmgr/dv/env/rstmgr_scoreboard.sv
- hw/ip_templates/rstmgr/dv/sva/rstmgr_bind.sv
- hw/top_earlgrey/cdc/cdc_waivers.data.tcl
- hw/top_earlgrey/data/chip_conn_testplan.hjson
- hw/top_earlgrey/data/ip/chip_i2c_testplan.hjson
- hw/top_earlgrey/data/ip/chip_rstmgr_testplan.hjson
- hw/top_earlgrey/data/pins_cw310.xdc
- hw/top_earlgrey/data/pins_cw341.xdc
- hw/top_earlgrey/data/top_earlgrey.hjson
- hw/top_earlgrey/data/xbar_peri.hjson
- hw/top_earlgrey/doc/datasheet.md
- hw/top_earlgrey/doc/top_earlgrey_block_diagram.svg
- hw/top_earlgrey/dv/chip_sim_cfg.hjson
- hw/top_earlgrey/dv/cov/chip_cover_reg_top.cfg
- hw/top_earlgrey/dv/env/chip_common_pkg.sv
- hw/top_earlgrey/dv/env/chip_if.sv
- hw/top_earlgrey/dv/env/seq_lib/chip_sw_all_escalation_resets_vseq.sv
- hw/top_earlgrey/dv/env/seq_lib/chip_sw_i2c_device_tx_rx_vseq.sv
- hw/top_earlgrey/dv/tb/tb.sv
- hw/top_earlgrey/formal/conn_csvs/clkmgr_peri.csv
- hw/top_earlgrey/formal/conn_csvs/rstmgr_resets_o.csv
- hw/top_earlgrey/formal/conn_csvs/rstmgr_rst_en.csv
- hw/top_earlgrey/ip/ast/data/ast_cdc_abstract.sgdc
- rules/const.bzl
- sw/device/lib/dif/dif_alert_handler_unittest.cc
- sw/device/lib/dif/dif_rstmgr.c
- sw/device/lib/dif/dif_rv_plic_unittest.cc
- sw/device/lib/testing/i2c_testutils.c
- sw/device/lib/testing/json/pinmux.h
- sw/device/tests/alert_handler_lpg_reset_toggle.c
- sw/device/tests/i2c_target_test.c
- sw/device/tests/pmod/i2c_host_eeprom_test.c
- sw/device/tests/pmod/i2c_host_fram_test.c
- sw/device/tests/power_virus_systemtest.c
- sw/device/tests/rstmgr_alert_info_test.c
- sw/device/tests/rstmgr_sw_rst_ctrl_test.c
- sw/device/tests/sim_dv/all_escalation_resets_test.c
- sw/device/tests/sim_dv/i2c_device_tx_rx_test.c
- sw/device/tests/sim_dv/i2c_host_tx_rx_test.c
- sw/host/opentitanlib/src/otp/alert_handler.rs
Changes to all other files were autogenerated by running the following
commands:
- make -C hw
- util/cmdgen.py -u '**/*.md'
- util/regtool.py \
hw/top_earlgrey/ip_autogen/alert_handler/data/alert_handler.hjson \
-D -o alert_handlers_regs.h
- bindgen alert_handlers_regs.h -o \
sw/host/opentitanlib/src/otp/alert_handler_regs.rs
See issue lowRISC#19505 for how to compute the values in
`hw/ip/otp_ctrl/data/otp_ctrl_img_owner_sw_cfg.hjson`. To update the
expected CRC32 values in `sw/host/opentitanlib/src/otp/alert_handler.rs`
run `bazel test //sw/host/opentitanlib:opentitanlib_test` and extract
the expected values from the log file.
The following top-level tests were run for a basic integration check of
the added I2C instance, and the tests pass (for one invocation with the
configured number of reseeds):
- chip_sw_alert_handler_lpg_reset_toggle
- chip_sw_all_escalation_resets
- chip_sw_i2c_device_tx_rx
- chip_sw_i2c_host_tx_rx_idx3
- chip_sw_rstmgr_alert_info
- chip_sw_rstmgr_sw_rst
`chip_sw_all_escalation_resets` fails for ca. 3 % of the seeds but the
rate and signature of this failure matches the one found in the nightly
regressions, so this commit doesn't seem to make this worse.
`chip_sw_power_virus` still fails but the signature of this failure
matches the one found in the nightly regressions, so this commit doesn't
seem to have broken it.
The remaining top-level tests that were modified don't run in
simulation, so I didn't check them for this commit.
Signed-off-by: Andreas Kurth <[email protected]>
andreaskurth
force-pushed
the
earlgrey-4th-i2c
branch
from
95a8244
to
34909de
Compare
|
We have decided to modify the existing three I2C instances instead of adding a fourth one. I'm thus closing this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a fourth I2C instance to Earlgrey. The plan is to extend / modify this new instance to support MCTP (see #2659) in follow-up PRs. Getting the fourth I2C instance added as part of Earlgrey-PROD.M2 is important due to the impact of this change on area (and, likely to a lesser degree, timing), wiring, and flow configuration (such as constraints and waivers).
The main commit of this PR is the second one; it's description is pasted below to provide an overview of the changes in this PR. The first commit fixes a problem with
i2c_testutilsthat surfaces when adding the fourth I2C instance.The following files were edited manually:
Changes to all other files were autogenerated by running the following
commands:
hw/top_earlgrey/ip_autogen/alert_handler/data/alert_handler.hjson
-D -o alert_handlers_regs.h
sw/host/opentitanlib/src/otp/alert_handler_regs.rs
See issue #19505 for how to compute the values in
hw/ip/otp_ctrl/data/otp_ctrl_img_owner_sw_cfg.hjson.The following top-level tests were run for a basic integration check of
the added I2C instance, and the tests pass (for one invocation with the
configured number of reseeds):
chip_sw_all_escalation_resetsfails for ca. 3 % of the seeds but therate and signature of this failure matches the one found in the nightly
regressions, so this commit doesn't seem to make this worse.
chip_sw_power_virusstill fails but the signature of this failurematches the one found in the nightly regressions, so this commit doesn't
seem to have broken it.
The remaining top-level tests that were modified don't run in
simulation, so I didn't check them for this commit.