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

CHANGE: Organize abstract config into sections and change comment format to docstrings #1722

Merged
merged 8 commits into from
Feb 11, 2024

Conversation

T-K-233
Copy link
Member

@T-K-233 T-K-233 commented Jan 2, 2024

Related PRs / Issues:

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

@T-K-233 T-K-233 requested a review from jerryz123 January 2, 2024 01:16
@T-K-233 T-K-233 self-assigned this Jan 2, 2024
@T-K-233
Copy link
Member Author

T-K-233 commented Jan 2, 2024

@jerryz123 I'm not sure if I put the all fragments into correct sections.

@T-K-233 T-K-233 requested a review from jerryz123 January 2, 2024 02:00
@jerryz123
Copy link
Contributor

It might be easier to converge if we agree on a outline of the organization, and then modify the Abstract config.

A proposal:

  • HarnessBinders
  • IOBinders
  • MMIO Devices
    • I/O devices (GPIO/UART/PWM/I2C/SPI)
    • Debug/bringup (JTAG/SerialTL/CustomBoot)
    • Interrupts (PLIC/CLINT)
  • Tile settings
  • Memory system settings
    • On-chip memory (Scratchpad, BootROM)
    • Off-chip memory (AXI4)
    • LLC
    • Bus settings
  • Clocking/reset

@T-K-233
Copy link
Member Author

T-K-233 commented Jan 2, 2024

I think this outline looks good in general. However, i want to put everything that is on the memory map to the same section, basically putting the "On-chip memory", "Off-chip memory", and "MMIO Devices" together.

@T-K-233
Copy link
Member Author

T-K-233 commented Jan 2, 2024

and clock/reset section will also include power (if any), right?

@jerryz123
Copy link
Contributor

jerryz123 commented Jan 2, 2024

I think this outline looks good in general. However, i want to put everything that is on the memory map to the same section, basically putting the "On-chip memory", "Off-chip memory", and "MMIO Devices" together.

I feel like this is hard to accomplish, since a bunch of random things will alter the memory map

and clock/reset section will also include power (if any), right?

Yes, but we don't have anything for that right now

@T-K-233
Copy link
Member Author

T-K-233 commented Jan 2, 2024

Could we do this instead?:

  • HarnessBinders
  • IOBinders
  • External Memory and IO Devices
    • Off-chip memory (AXI4 / SerialTL / SPI Flash)
    • MMIO devices (GPIO / UART / PWM / I2C / SPI)
    • DMA devices
  • Debug Settings
    • JTAG
    • CustomBoot
  • Interrupts (CLINT / PLIC)
  • Tile settings
    • Core settings (FPU / MultiplyUnit)
  • Memory system settings
    • On-chip memory (Scratchpad, BootROM)
    • Other cache settings
    • LLC
    • Bus settings
  • PRC Settings
    • clocking
    • reset
    • power
  • Base settings
    • chipname
    • BaseConfig

This way, the config fragment corresponds the blocks in the SoC block diagram.

Sections still subject to change. I think we can try and see if it works, and if not, we can always make changes later.

@jerryz123
Copy link
Contributor

That organization looks good to me.

@jerryz123
Copy link
Contributor

@T-K-233 I made some minor changes.
I also removed WithL2TLBs... this option actually has no effect when applied to the AbstractConfig, as it needs to override tile-settings that are already specified

Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

Overall LGTM

Comment on lines +96 to +99
// ================================================
// Set up Tiles
// ================================================
// tile-local settings goes here
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this section if it's empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

As a placeholder. The idea is that ChipLikeRocket config will also use this template, and configs like WithFPU will go here.

@jerryz123
Copy link
Contributor

@T-K-233 can you rebase then address Abe's comments

@T-K-233
Copy link
Member Author

T-K-233 commented Feb 11, 2024

@T-K-233 can you rebase then address Abe's comments

Fixed

@jerryz123 jerryz123 merged commit 5fcac96 into main Feb 11, 2024
53 checks passed
@T-K-233 T-K-233 deleted the improve-abstract branch February 11, 2024 22:36
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.

3 participants