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.

Sign up for GitHub

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

Jump to bottom

[rtl,i2c] Rename FDATA READ field to READB #19721

Merged
merged 1 commit into from
Sep 29, 2023
Merged

[rtl,i2c] Rename FDATA READ field to READB #19721

merged 1 commit into from
Sep 29, 2023

Conversation

matutem
Copy link
Contributor

@matutem matutem commented Sep 21, 2023 •
edited
Loading

The name READ shadows the uvm_reg_field read() member function in the ral generated package.

This renaming should not affect the real hardware, only the programming model.

@matutem matutem requested review from a team as code owners September 21, 2023 20:49
@matutem matutem requested review from eshapira, jon-flatley and GregAC and removed request for a team September 21, 2023 20:49
@matutem matutem force-pushed the earlgrey_i2c_fdata_read branch from cc5d895 to dc3ff90 Compare September 22, 2023 02:05
eshapira
eshapira approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@eshapira eshapira left a comment

Choose a reason for hiding this comment

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

LGTM

@matutem
[rtl,i2c] Rename FDATA READ field to READB
fe18080 
The name READ shadows the uvm_reg_field read() member function in the
ral generated package.
Increase the minor version of i2c due to this change.

Signed-off-by: Guillermo Maturana <maturana@opentitan.org>
@matutem matutem force-pushed the earlgrey_i2c_fdata_read branch from dc3ff90 to fe18080 Compare September 25, 2023 21:12
@msfschaffner
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/i2c/data/i2c.hjson
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_core.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_reg_pkg.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_reg_top.sv

marnovandermaas
marnovandermaas reviewed Sep 26, 2023
View reviewed changes
Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

I don't really understand why this is an important change to make right now. Renaming (unless there is a really good reason) should be avoided, especially in the RTL.

@andreaskurth
Copy link
Contributor

andreaskurth commented Sep 26, 2023 •
edited
Loading

I don't really understand why this is an important change to make right now. Renaming (unless there is a really good reason) should be avoided, especially in the RTL.

IIUC, ral.fdata.read shadows the read() function of the uvm_reg_field class, of which ral.fdata is an instance.

Regarding your concern of RTL changes, this change should be binary-compatible, though a minor version increase was suggested by @msfschaffner

@matutem matutem merged commit 0a8d1b2 into lowRISC:master Sep 29, 2023
@matutem matutem deleted the earlgrey_i2c_fdata_read branch September 29, 2023 15:14
@hcallahan-lowrisc hcallahan-lowrisc mentioned this pull request Mar 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marnovandermaas marnovandermaas marnovandermaas left review comments

@eshapira eshapira eshapira approved these changes

@jon-flatley jon-flatley Awaiting requested review from jon-flatley jon-flatley is a code owner automatically assigned from lowRISC/ot-c-cpp-reviewers

@GregAC GregAC Awaiting requested review from GregAC

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matutem @msfschaffner @andreaskurth @marnovandermaas @eshapira