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

hwset on multibit field #99

Open
darrylring opened this issue Apr 16, 2024 · 5 comments
Open

hwset on multibit field #99

darrylring opened this issue Apr 16, 2024 · 5 comments
Labels
feature request New feature or request

Comments

@darrylring
Copy link

The hwset property does not work as I expect on multi-bit fields. Instead of a hwset input port matching the width of the field, it is always a single bit, and sets the field to 1 regardless of the width.

I feel like the spec is a bit unclear about this, so I'm not sure if this is a bug or as designed and matches the spec.

In the following minimal example, the 32-bit field b only gets a single hwset bit in the generated code:

addrmap test {
    reg {
        field {
            hw   = r;
            sw   = rw;
            hwset;
        } b[31:0] = 0x0;
    } a;
};

Command: peakrdl regblock test.rdl -o .

test_pkg.sv:

package test_pkg;
...
    typedef struct {
        logic hwset;
    } test__a__b__in_t;
...
    typedef struct {
        logic [31:0] value;
    } test__a__b__out_t;
...
endpackage

test.sv:

module test (
...
);
...
    always_comb begin
        automatic logic [31:0] next_c = field_storage.a.b.value;
        automatic logic load_next_c = '0;
        if(decoded_reg_strb.a && decoded_req_is_wr) begin // SW write
            next_c = (field_storage.a.b.value & ~decoded_wr_biten[31:0]) | (decoded_wr_data[31:0] & decoded_wr_biten[31:0]);
            load_next_c = '1;
        end else if(hwif_in.a.b.hwset) begin // HW Set
            next_c = '1;
            load_next_c = '1;
        end
        field_combo.a.b.next = next_c;
        field_combo.a.b.load_next = load_next_c;
    end
...
endmodule
@amykyta3
Copy link
Member

In SystemVerilog, the '1 literal (with a leading ') is a shortcut to assign all bits of the assignee to ones.
For example:

logic [31:0] example;

// The following two lines are equivalent
assign example = '1;
assign example = 32'hFFFFFFFF;

// Not to be confused with:
assign example = 1;
// which is identical to:
assign example = 32'h00000001;

@darrylring
Copy link
Author

Sorry, yes, that's correct; however, my expectation was that the hwset port would be 32 bits wide, and each bit would be set independently, not all at once.

That being said, the spec says "set the field (hwclr and hwset) by asserting a single pin" and so perhaps my expectation is wrong. Allowing each bit to be set independently seems more useful than setting all bits, but perhaps my use case is unusual.

Is there a better way to accomplish this? Do I need to manually add 32 1-bit fields with the hwset property?

@amykyta3
Copy link
Member

Ah I see. I misunderstood your question.

That is correct - for the hwset property, it infers a single-bit input to set all bits.

Aside from splitting to individual 1-bit fields, if you want to set individual bits in a field you probably have to handle this outside of the regblock. I can't think of a good way to do it entirely in RDL. One way would be using the hardware write-enable mechanism:

field {
    sw = rw;
    hw = rw;
    we;
} b[31:0] = 0;

Then in RTL use the write-enable to apply the updated per-bit set behavior:

assign hwif_in.a.b.we = |my_bit_set_vector;
assign hwif_in.a.b.next = my_bit_set_vector | hwif_out.a.b.value;

@darrylring
Copy link
Author

Thank you! And thanks for PeakRDL!

Would you be open to a PR which added an option for this? I'm not sure if I'm going to take that on right now, still evaluating, but thinking about it. It's useful enough to us.

@amykyta3 amykyta3 added the feature request New feature or request label Apr 18, 2024
@amykyta3
Copy link
Member

Sure! I can see how this could be useful.

This would definitely have to be a separate property. Changing the behavior of hwset and hwclr would be inappropriate since the SystemRDL spec pretty clearly dictates this is a single-bit strobe to set/clear the entire field (I actually use hwclr like this on multi-bit fields intentionally in some applications)

I would recommend implementing this as separate user-defined properties like bitwise_hwset and bitwise_hwclr, much like some of the other extended properties I have implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Development

No branches or pull requests

2 participants