[RACL] Parse mappings and auto-connect RACL #25749
Conversation
Razer6
force-pushed
the
racl-connections
branch
3 times, most recently
from
3ab664e to
22a4113
Compare
Signed-off-by: Robert Schilling <[email protected]>
Razer6
force-pushed
the
racl-connections
branch
3 times, most recently
from
e9ce170 to
89730cc
Compare
Signed-off-by: Robert Schilling <[email protected]>
Razer6
force-pushed
the
racl-connections
branch
2 times, most recently
from
4318722 to
6b1e47c
Compare
Signed-off-by: Robert Schilling <[email protected]>
Razer6
force-pushed
the
racl-connections
branch
from
6b1e47c to
2f84455
Compare
Signed-off-by: Robert Schilling <[email protected]>
Signed-off-by: Robert Schilling <[email protected]>
| if len(mapping) != 1: | ||
| raise SystemExit('Mapping file must be a single-element dict mapping the RACL group to the ' | ||
| 'register mapping') | ||
| racl_group = list(mapping.keys())[0] |
There was a problem hiding this comment.
We might want to check that racl_group is a string too?
| racl_group = list(mapping.keys())[0] | ||
| register_mapping = list(mapping.values())[0] |
There was a problem hiding this comment.
Since we know the length, this might be easier with something like racl_group, register_mapping = mapping.items()[0] ?
| racl_group = list(mapping.keys())[0] | ||
| register_mapping = list(mapping.values())[0] |
There was a problem hiding this comment.
Do we also need to check that register_mapping is a dict?
| # Special handling of the all star assignment: | ||
| # "*": POLICY | ||
| # Assigns all registers to a given policy | ||
| if len(register_mapping) == 1 and list(register_mapping.keys())[0] == "*": |
There was a problem hiding this comment.
Possibly easier: if list(register_mapping.keys()) == ["*"] ?
| if policy_idx == -1: | ||
| raise SystemExit(f'RACL policy {policy_name} not defined in RACL config') | ||
|
|
||
| if if_name not in ip_block.reg_blocks: |
There was a problem hiding this comment.
I'd probably be inclined to assign ip_block.reg_blocks.get(if_name) to something and then check for None here, which avoids needing to do the look-up again as the next operation.
| if 'racl_mappings' in m: | ||
| for if_name, mapping_path in m['racl_mappings'].items(): |
There was a problem hiding this comment.
Maybe for if_name, mapping_path in m.get('racl_mappings', {}).items(), which will avoid a layer of indentation.
| # Auto connect RACL subscribing IPs to the assocated racl_ctrl IP | ||
| if 'racl' in topcfg: | ||
| # Find the RACL control of the defined group. This currently only works for the default | ||
| # RACL ctrl module when there is a single instance. This limitiation currently comes from |
There was a problem hiding this comment.
Nit: Spelling of "limitation"
|
|
||
| if racl_mapping and racl_mappings: | ||
| raise ValueError("Cannot specify both 'racl_mapping' and 'racl_mappings'") | ||
|
|
There was a problem hiding this comment.
I'd suggest converting racl_mapping to a value for racl_mappings here, which will make the code below a bit easier to read.
| @@ -3112,121 +3147,184 @@ module pwm_reg_top ( | |||
| addr_hit[20] = (reg_addr == PWM_BLINK_PARAM_3_OFFSET); | |||
| addr_hit[21] = (reg_addr == PWM_BLINK_PARAM_4_OFFSET); | |||
| addr_hit[22] = (reg_addr == PWM_BLINK_PARAM_5_OFFSET); | |||
|
|
|||
| if (EnableRacl) begin : gen_racl_hit | |||
| racl_addr_hit_read [ 0] = addr_hit[ 0] & (|(racl_policies_i[RaclPolicySelVec[ 0]].read_perm & racl_role_vec)); | |||
There was a problem hiding this comment.
I've not tried this, but could we move the looping into the SystemVerilog instead of the Python? I think we end up with something like this, which is a bit more obvious to read, rather shorter and doesn't need line length waivers.
if (EnableRacl) begin : gen_racl_hit
for (int unsigned slice_idx = 0; slice_idx < 23; slice_idx++) begin
racl_addr_hit_read[slice_idx] =
addr_hit[slice_idx] & (|(racl_policies_i[RaclPolicySelVec[slice_idx]].read_perm
& racl_role_vec));
racl_addr_hit_write[slice_idx] =
addr_hit[slice_idx] & (|(racl_policies_i[RaclPolicySelVec[slice_idx]].write_perm
& racl_role_vec));
end
end else begin : gen_no_racl
racl_addr_hit_read = addr_hit;
racl_addr_hit_write = addr_hit;
end| # Licensed under the Apache License, Version 2.0, see LICENSE for details. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # These lines are too long due to generated code |
There was a problem hiding this comment.
As a note: if we can simplify the generated reg-top as I'm suggesting then we won't need this waiver entry.
| .reg2hw (reg2hw), | ||
| .racl_policies_i (racl_policies_i), | ||
| .racl_error_o (racl_error_o), | ||
| .racl_error_log_o (racl_error_log_o), |
There was a problem hiding this comment.
It's not a big issue, but do we really need to go that far to the right? I think I'd probably have expected just one space after the longest name (this one).
| act: "rcv", | ||
| package: "top_racl_pkg", | ||
| desc: ''' | ||
| Policy vector distributed to the subscribing RACL IPs. |
There was a problem hiding this comment.
I wonder whether this text needs changing slightly. It looks a bit like PWM is the block that's distributing something (which is backwards). If I've got this right, PWM is a subscribing IP, so maybe something like "RACL policy vector, used by the IP to control register access permissions." ?
This PR adds support for parsing the RACL config. In the first place, a *-connection is implemented, mapping all registers to a single RACL policy. The RACL policies are distributed to subscribing IPs. A selection vector in the form of a parameter selects the right policy from the policy vector.
RACL is applied to PWM first.