[dt,otp] Rename misleadingly name field and its documentation#28795
[dt,otp] Rename misleadingly name field and its documentation#28795pamaury merged 2 commits intolowRISC:masterfrom
Conversation
pamaury
requested review from
a team,
Razer6,
sameo and
timothytrippel
and removed request for
a team
pamaury
force-pushed
the
dt_otp_rename_field
branch
2 times, most recently
from
2ca7d07 to
ac1955c
Compare
| name = self.OTP_PARTITION_INFO_STRUCT_DIGEST_ADDR_FIELD_NAME, | ||
| field_type = ScalarType("uint32_t"), | ||
| docstring = "The absolute OTP address at which this partition's digest starts", | ||
| docstring = "The OTP digest CSR (where the digest is buffered) offset for " + |
There was a problem hiding this comment.
Nit: the new wording here suggests that the digest occupies one CSR but in actuality an OTP digest is 8 bytes and falls across 2 CSRs. Maybe this should just be "digest_offset" as a name and the wording should be more like "The OTP address (offset from this partition) at which this partition's digest starts."
There was a problem hiding this comment.
I am not an otp_ctrl expert but I think your phrasing is incorrect because this value is set to the offset of a CSR, e.g. https://opentitan.org/book/hw/top_earlgrey/ip_autogen/otp_ctrl/doc/registers.html#hw_cfg0_digest (you can see the content here).
The idea of using the word "register" is to make it clear that this is the offset of a CSR and not the offset within the OTP partition.
Maybe I could change the wording to "The first (_DIGEST_0) OTP digest CSR" ?
There was a problem hiding this comment.
I see, I misunderstood this as the DAI digest offset rather than the buffered CSR digest (I forgot there was that duplication there and thought that the change was just absolute -> relative address completely 😅) I agree with your new wording.
| /*relative_address=*/ | ||
| otp_readable_partition_info(kOtpPartitionCreatorSwCfg) | ||
| .digest_reg_offset - | ||
| OTP_CTRL_PARAM_CREATOR_SW_CFG_OFFSET, |
There was a problem hiding this comment.
Perhaps I'm getting confused, but is this correct? It's taking the digest offset relative to the start of the partition and then subtracting the autogenerated OTP_CTRL_PARAM_CREATOR_SW_CFG_OFFSET (from the mmap), which from my memory is the offset of field from the start of OTP DAI, i.e. the absolute address. Should this not subtract the partition start address from the DT from this offset first?
There was a problem hiding this comment.
Yes the code is incorrect, as I noted this code is fixed in #28794 which is why this PR doesn't touch it.
There was a problem hiding this comment.
I am confused what you are trying to do here? are you trying to read the digest registers (i.e. CSRs that buffer the digest)? or are you trying read the digest from the memory mapped window?
There was a problem hiding this comment.
Hi Tim, the comment above explains the situation: while doing the PR I realized that the code was wrong and that it had been fixed on earlgrey_1.0.0 so I backported the fix in #28794. I was waiting for this to be merged so that I can rebase the code. The above code is now fixed which should clarify the situation.
There was a problem hiding this comment.
Ah ok. Thats what it looked like. Glad I was not the only one that made the same bug when I first implemented this :)
timothytrippel
left a comment
timothytrippel
left a comment
There was a problem hiding this comment.
Dropped a comment above
The documentation was probably copy-paste: the digest is not actually an OTP address but a CSR. Rename the field to make it clearer. Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
dt_otp_rename_field
branch
from
ac1955c to
20265d1
Compare
3 participants
This is a follow-up on #28794 which is a backport of #25975 where the misleading name lead to an error in the ROM. Since master uses the DT, the fix is a bit different but fundamentally achieves the exact same renaming.