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

Update XIF interface #580

Merged
merged 4 commits into from
Jul 21, 2022
Merged

Conversation

michael-platzer
Copy link
Contributor

Hi,

This PR updates the XIF interface definition, the LSU, and the WB stage to the changes in openhwgroup/core-v-xif@e65cc88 and openhwgroup/core-v-xif@6c257cd.

XIF memory requests are no longer aligned by the CPU. Instead, data and byte enable signals (wdata, rdata, and be) are assumed to be already aligned to the width of the memory bus (any required alignment and splitting must be performed by the coprocessor).

The XIF memory request's transaction attribute attr[1] (indicates whether the request is naturally aligned) is included in misaligned_access and thus made available to the PMA.

Question: I am unsure how to handle the other transaction attribute (attr[0], indicating whether the transaction was modified by the coprocessor) in CV32E40X. In the current PR this attribute is ignored. Does it need to be taken in account, and, if yes, what would be the consequence if it is set for a transaction?

Update the XIF interface definition, the LSU, and the WB stage to the
changes in openhwgroup/core-v-xif@e65cc88 and
openhwgroup/core-v-xif@6c257cd.
@Silabs-ArjanB Silabs-ArjanB added the Component:RTL For issues in the RTL (e.g. for files in the rtl directory) label Jun 16, 2022
Copy link
Contributor

@Silabs-ArjanB Silabs-ArjanB left a comment

Choose a reason for hiding this comment

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

Hi Michael,

Thanks again for your contribution!

With respect to your question on how to handle attr[0]:

We had changed the PMA specification (https://docs.openhwgroup.org/projects/cv32e40x-user-manual/en/latest/pma.html) ti forbid modified/modifiable transactions to I/O regions:

Modifiable/modified transactions are not supported in I/O regions. An attempt to perform a modifiable/modified load access to an I/O region causes a precise load access fault (exception code 5). An attempt to perform a modifiable/modified store access to an I/O region causes a precise store access fault (exception code 7).

The handling is exactly the same as that for misaligned transaction to I/O, so you could use the following line as a temporary hack until we update the MPU with a dedicated input for this:

assign misaligned_access = split_q || lsu_split_0_o || misaligned_halfword || (xif_req && (xif_mem_if.mem_req.attr[0] || xif_mem_if.mem_req.attr[1])); // todo: Give MPU a separate modified_access_i input.

I undid you WB stage change and added a todo instead ('err' will eventually get handled as an imprecise NMI as opposed to a precise exception).

The main feedback is on your LSU omdifications. I think it will now be best to insert the coprocessor transactions 'after' the alignment circuit (as opposed to before as it originally was).

Move the `xif_req`-based multiplexers for merging XIF memory requests
with the core's own memory requests from the `trans.*` signals to the
`align_trans.*` signals.  Since the coprocessor handles the alignment
of its memory requests itself, the alignment logic of the main core
should be bypassed.

Signed-off-by: Michael Platzer <[email protected]>
Set the LSU's `misaligned_access` signal if either one of the two XIF
memory request attributes is set (i.e., for misaligned and for modified
XIF memory requests).  Eventually, the MPU should gain a separate input
to signal modified transactions.

Signed-off-by: Michael Platzer <[email protected]>
@michael-platzer
Copy link
Contributor Author

Hi @Silabs-ArjanB,

thanks for your feedback! Please see my answers to your comments above regarding bypassing the alignment logic.

I have also updated the assignment of the misaligned_access signal as you suggested.

@Silabs-ArjanB Silabs-ArjanB self-requested a review July 21, 2022 11:55
@Silabs-ArjanB Silabs-ArjanB merged commit 5ecb42b into openhwgroup:master Jul 21, 2022
@michael-platzer michael-platzer deleted the xif_update branch July 22, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants