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

[hw,dma,rtl] Add STATUS.chunk_done bit #24108

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Jul 24, 2024

This PR adds a new STATUS.chunk_done bit to the status register to determine the cause when the DMA stopped an operation. In multi-chunk mem2mem transfers, where each chunk is transferred with a separate SW-go, chunk_done goes high when finishing every chunk, except the last chunk, where done goes high. The done interrupt fires for every chunk for now.

@Razer6 Razer6 requested review from a team as code owners July 24, 2024 15:24
@Razer6 Razer6 requested review from eshapira, timothytrippel, andreaskurth and alees24 and removed request for a team July 24, 2024 15:24
Copy link
Contributor

@alees24 alees24 left a comment

Choose a reason for hiding this comment

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

I think this does essentially what's intended, but there are issues with the interrupt-handling at present. The OpenTitan project was changed a few months ago so that Status type interrupt are specified explicitly in the IP block description and all blocks were updated accordingly; see my comments below, please.

hw/ip/dma/rtl/dma.sv Outdated Show resolved Hide resolved
Comment on lines 1263 to 1264
// transfer.
hw2reg.intr_state.dma_done.de = reg2hw.status.done.q | chunk_done | test_done_interrupt;
Copy link
Contributor

@alees24 alees24 Sep 6, 2024

Choose a reason for hiding this comment

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

With the present logic the dma_done and the dma_error interrupts should be declared as Status type interrupts in the dma.hjson file since the only way to deassert these interrupts and clear their bits in the intr_state register is to clear the bits in the status register, because dma_done|error.de is driven by reg2hw.status.done|error.q:

  interrupt_list: [
    { name: "dma_done"
      desc: "DMA operation has been completed."
      type: "status"
    }
    { name: "dma_error"
      desc: "DMA error has occurred. DMA_STATUS.error_code register shows the details."
      type: "status"
    }

This is a requirement of comportable IP blocks, for the CIP base tests to pass.

The present behavior of the 'intr_test' register will also be subtly incorrect, in that Status type interrupts should remain assert whilst the 'intr_test' bit is asserted. It would be simpler/easier to use the prim_intr_hw submodule to implement the interrupt lines.

Alternatively: It may have been intended that the intr_state bits be set in the event of the status.dma_done|error bits rising, ie. Event-type interrupts, although in this case the bits in the status register are somewhat redundant since the intr_state register shall capture that information too.

@Razer6 Razer6 force-pushed the dma-chunk-done-status branch 4 times, most recently from 9f91a87 to 309720a Compare September 20, 2024 10:02
@Razer6 Razer6 requested a review from alees24 September 23, 2024 07:57
@Razer6
Copy link
Member Author

Razer6 commented Sep 23, 2024

@alees24 Thanks for the review. Could you take another look? CI is currently failing because the integrated_dev branch is not able to correctly create the auto-generated unittests for status interrupts, which is fixed on master. The plan is to rebase that PR on top of master and merge it there once the DMA is in there.

Copy link
Contributor

@alees24 alees24 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes to the interrupt handling, and using the prim_intr_hw module. That looks fine.

If you resolve the issue with the default hw2reg assignment and are happy to sign off the compatibility breakage by inserting a bit into the status register then I will approve this.

hw2reg = '0;

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not safe to drop this assignment at present because the following signals will be left undriven/undefined:

hw2reg.control.initial_transfer.d
hw2reg.control.initial_transfer.de
hw2reg.sha2_digest[i].d

In particular the control.initial_transfer signals must be driven at all times; simulations fail presently because initial_transfer.q becomes undefined as soon as a transfer starts.

It is perhaps worth leaving the default assignment in place (I may have missed other signals) and annotating that it is required for driving the above signals, as well as avoiding latches/other undriven signals.

An alternative would be to ensure that the above signals are always driven explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I had to explicitly drive these signals since the the prim_intr's added a second driver to the hw2reg. Thus, the default statement would cause a conflicting driver.

hw/ip/dma/data/dma.hjson Outdated Show resolved Hide resolved
@Razer6 Razer6 changed the base branch from integrated_dev to master October 17, 2024 11:25
@Razer6 Razer6 requested review from vogelpi, cfrantz and a team as code owners October 17, 2024 11:25
@Razer6 Razer6 force-pushed the dma-chunk-done-status branch 2 times, most recently from 6185e1d to cccc653 Compare October 17, 2024 12:43
@Razer6 Razer6 force-pushed the dma-chunk-done-status branch 4 times, most recently from 45ecec8 to 2411b1d Compare October 31, 2024 12:44
error = get_field_val(ral.status.error, item.a_data);
// Clearing the status bits also clears the status interrupt
predict_interrupts(CSRtoIntrLatency, done << DMA_DONE & `gmv(ral.intr_enable), 0);
predict_interrupts(CSRtoIntrLatency, error << DMA_ERROR & `gmv(ral.intr_enable), 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

With the new status interrupts, I had to re-predict the interrupts when writing the status register, ie., clerating the status.

@Razer6 Razer6 requested review from alees24 and removed request for mundaym, msfschaffner, cfrantz and a team October 31, 2024 12:48
@Razer6
Copy link
Member Author

Razer6 commented Oct 31, 2024

@alees24 Thanks for the review. Could you take another look to get this merged?

I updated the DV environment to deal with the status interrupts. All tests except the automated dma_intr_test (comfortable test) fail. Could you take a look into this one? Otherwise, maybe. @rswarbrick. Note, tests need to include #24962 to pass.

@Razer6 Razer6 force-pushed the dma-chunk-done-status branch 2 times, most recently from 707e55b to 9a3cc9d Compare November 7, 2024 08:35
@Razer6 Razer6 force-pushed the dma-chunk-done-status branch 2 times, most recently from 4f22661 to d87c34f Compare November 7, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants