Skip to content

Conversation

@rewired-gh
Copy link
Contributor

No description provided.

@rewired-gh
Copy link
Contributor Author

rewired-gh commented May 5, 2023

In current approach, module TLB might have too many parameters:

class TLB(
  instruction: Boolean,
  lgMaxSize: Int,
  cfg: TLBConfig,
  edge: TLEdgeOut,
  pmpGranularity: Int,
  pgLevels: Int,
  minPgLevels: Int,
  pgLevelBits: Int,
  pgIdxBits: Int,
  vpnBits: Int,
  ppnBits: Int,
  vaddrBits: Int,
  vaddrBitsExtended: Int,
  paddrBits: Int,
  hypervisorExtraAddrBits: Int,
  asIdBits: Int,
  xLen: Int,
  cacheBlockBytes: Int,
  usingHypervisor: Boolean,
  usingVM: Boolean,
  usingAtomics: Boolean,
  usingAtomicsInCache: Boolean,
  usingAtomicsOnlyForIO: Boolean,
  usingDataScratchpad: Boolean) extends Module with MemoryOpConstants {

Is this a potential problem or it is acceptable?

@rewired-gh
Copy link
Contributor Author

rewired-gh commented May 10, 2023

What has not been done in this PR:

  • Decouple TLB from Tilelink
  • Migration of PMP and PTW
  • Refactor dependencies on Parameters.scala

The former two tasks may be suitable for other new PRs. And I cannot figure out a decent solution for the last task.

@rewired-gh rewired-gh marked this pull request as ready for review May 10, 2023 08:22
@rewired-gh rewired-gh marked this pull request as draft May 12, 2023 05:29
@rewired-gh rewired-gh marked this pull request as ready for review May 17, 2023 11:27
instruction: Boolean,
lgMaxSize: Int,
cfg: TLBConfig,
edge: TLEdgeOut, // TODO: Decoupled from Tilelink
Copy link
Member

Choose a reason for hiding this comment

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

Extract this

@@ -0,0 +1,136 @@
// See LICENSE.SiFive for license details.
Copy link
Member

Choose a reason for hiding this comment

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

move it to utils and deprecate it with BitSet

class TLB(
instruction: Boolean,
lgMaxSize: Int,
cfg: TLBConfig,
Copy link
Member

Choose a reason for hiding this comment

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

actually parameter below are all TLBConfig, but OK for now.

val mpu_physaddr = Cat(mpu_ppn, io.req.bits.vaddr(pgIdxBits-1, 0))
val mpu_priv = Mux[UInt](usingVM.B && (do_refill || io.req.bits.passthrough /* PTW */), PRV.S.U, Cat(io.ptw.status.debug, priv))
val pmp = Module(new PMPChecker(lgMaxSize))
val pmp = Module(new PMPChecker(lgMaxSize)) // TODO: Dependent on PMP
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand comments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it work, it requires refactor on PMP. So it won't pass compilation until PMP is migrated.

Copy link
Member

Choose a reason for hiding this comment

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

OK this PR depends on PMP refactor now.

require (!m.supportsLogical || m.supportsLogical .contains(amoSizes), s"Memory region '${m.name}' at ${m.address} only supports ${m.supportsLogical} Logical, but must support ${amoSizes}")
require (!m.supportsArithmetic || m.supportsArithmetic.contains(amoSizes), s"Memory region '${m.name}' at ${m.address} only supports ${m.supportsArithmetic} Arithmetic, but must support ${amoSizes}")
require (!(m.supportsAcquireB && m.supportsPutFull && !m.supportsAcquireT), s"Memory region '${m.name}' supports AcquireB (cached read) and PutFull (un-cached write) but not AcquireT (cached write)")
val permissions = memParameters.foreach { p =>
Copy link
Member

Choose a reason for hiding this comment

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

basically it should be decouple from TileLink, but OK for now...

@rewired-gh rewired-gh requested a review from sequencer May 22, 2023 08:20
@rewired-gh
Copy link
Contributor Author

Is it necessary to move refactor of files like Const.scala to another PR?

val mpu_physaddr = Cat(mpu_ppn, io.req.bits.vaddr(pgIdxBits-1, 0))
val mpu_priv = Mux[UInt](usingVM.B && (do_refill || io.req.bits.passthrough /* PTW */), PRV.S.U, Cat(io.ptw.status.debug, priv))
val pmp = Module(new PMPChecker(lgMaxSize))
val pmp = Module(new PMPChecker(lgMaxSize)) // TODO: Dependent on PMP
Copy link
Member

Choose a reason for hiding this comment

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

OK this PR depends on PMP refactor now.

@sequencer
Copy link
Member

Is it necessary to move refactor of files like Const.scala to another PR?

OK~

@rewired-gh rewired-gh requested a review from sequencer June 1, 2023 12:38
@rewired-gh rewired-gh marked this pull request as draft June 6, 2023 07:58
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