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]: Add mem_multibanked_pwrgate for correct power management #246

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

Conversation

Lore0599
Copy link

Power Gated MutliBanked Memory

This PR introduces the mem_multibank_pwrgate.svmodule.

Goal

When designing power-aware systems, memory macro cells often include control signals dedicated to powering down/up the bank and entering retention mode. Additionally, when each bank in the memory is made of different cuts, the power control signals must be properly managed to maintain the memory’s interleaved addressing scheme..

Addition:

  • mem_multibank_pwrgate.sv: In a memory system with multiple banks and applied power strategies, each bank must be correctly managed to avoid disruptions in memory interleaving. For memories with multiple cuts and interleaved banks, it is important to ensure that power signals do not disrupt the internal bank interleaving. This module is responsible for correctly addressing each internal bank and connecting the appropriate power control signals.
  • mem_multibank_pwrgate_tb.sv: A testbench to verify the correct addressing functionality of the implemented module.

@Lore0599 Lore0599 force-pushed the lleone/pwr-mbank-mem branch from b4fc6c8 to 44a66c7 Compare October 28, 2024 10:32
Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

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

Hi @Lore0599, thank you for the contribution! I have one general question: would there be a point in generalising the module further? Specifically, I don't see the direct relevance of the deepsleep and powergate signals in the module implementation. They are "just" packed into the impl_i ports of the tc_srams. Would it make sense to expose the impl_i ports directly (instead of deepsleep_i and powergate_i) to make this module independent of the technology, perhaps even of the use case (power management), and pack them outside the module?

Other than that, I have added a couple of nitpicks inline. Furthermore, I would like to ask you the following:

Thanks a lot!

src/mem_multibank_pwrgate.sv Outdated Show resolved Hide resolved
src/mem_multibank_pwrgate.sv Outdated Show resolved Hide resolved
src/mem_multibank_pwrgate.sv Outdated Show resolved Hide resolved
src/mem_multibank_pwrgate.sv Outdated Show resolved Hide resolved
src/mem_multibank_pwrgate.sv Outdated Show resolved Hide resolved
src/mem_multibank_pwrgate.sv Outdated Show resolved Hide resolved
@Lore0599
Copy link
Author

Hi @niwis, thank you for your comments. I'll implement all of them ASAP.
Just one note regarding the impl_i port: this module is intended to be used with a Power Management Unit (PMU). The deepsleep_i and powergated_i signals will be driven by the PMU. The connection can be made through the SystemVerilog system description, but since the module’s goal is to enable power-aware simulation, the ports are likely to be floating initially and then properly connected using the UPF system description.
I don't have much experience with UPF, but over the past month, I discovered that complex types like structs can pose challenges, as UPF seems unable to recognize them effectively. This is the main reason I chose to keep these signals flattened at the interface.
That said, I can implement all your suggestions and try using the impl_i at the interface to see if UPF can handle it :).

@niwis
Copy link
Collaborator

niwis commented Oct 31, 2024

I see. Otherwise, perhaps you could add another wrapping layer that packs the signals locally? I'm just thinking that feeding through the implementation ports might increase the reusability of this module.

@Lore0599
Copy link
Author

Lore0599 commented Oct 31, 2024

@niwis Regarding the CI linting failure, the source of the error is the missing type specification for some parameters at the interface

parameter SimInit = "none", // Simulation initialization

These parameters are of string type, and similarly, in the tc_sram module (https://github.com/pulp-platform/tech_cells_generic/blob/7968dd6e6180df2c644636bc6d2908a49f2190cf/src/rtl/tc_sram.sv#L62) their type is also unspecified.
Would you prefer that I explicitly specify the type as string, or should we consider suppressing the linting error as done here? On this forum it appears that not specifying the string type can serve as a workaround for compatibility with tools like Vivado.
Let me know :)

src/mem_multibank_pwrgate.sv Show resolved Hide resolved
src/mem_multibank_pwrgate.sv Outdated Show resolved Hide resolved
@niwis
Copy link
Collaborator

niwis commented Nov 1, 2024

Would you prefer that I explicitly specify the type as string, or should we consider suppressing the linting error as done here? On this forum it appears that not specifying the string type can serve as a workaround for compatibility with tools like Vivado.

Good point. You can add a waiver for this line here:

# That is a known issue with string parameter in Synopsys DC
waive --rule=explicit-parameter-storage-type --line=19 --location="src/stream_arbiter.sv"
waive --rule=explicit-parameter-storage-type --line=19 --location="src/stream_arbiter_flushable.sv"

@Lore0599 Lore0599 force-pushed the lleone/pwr-mbank-mem branch from 7a53b12 to 766fa7c Compare November 1, 2024 07:45
  - [Lint]: Waive linting check on string datat type declaration
  - [HW]: Re-introduce IF to check design parameters
@Lore0599 Lore0599 force-pushed the lleone/pwr-mbank-mem branch from b24caf3 to aee15a3 Compare November 1, 2024 07:52
@niwis
Copy link
Collaborator

niwis commented Dec 4, 2024

Hi @Lore0599, sorry for the long silence. Have you had any success with exposing the impl_i ports?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants