Perf counters #388
Perf counters #388
Conversation
|
Hi, One thing would be to preserve backward compatibility, i mean in particualr the mcycleAccess = CsrAccess.NONE, and others. Idea being to preserve that feature there, while also havig the possibility to have it implemented by your plugin :) |
|
I added the legacy counters back in, but made sure, they will not be generated if a CounterService is part of the pipeline. |
|
Ok thanks :) |
|
The Plugin works. |
| @@ -122,12 +122,8 @@ object VexRiscvAhbLite3{ | |||
| mscratchGen = false, | |||
| mcauseAccess = CsrAccess.READ_ONLY, | |||
| mbadaddrAccess = CsrAccess.READ_ONLY, | |||
| mcycleAccess = CsrAccess.NONE, | |||
There was a problem hiding this comment.
Would it be possible to still keep those option ?
I know that would be redundant to the CounterPlugin implementation, but that would preserve backward compatibility without any disruption. (not break users setups)
There was a problem hiding this comment.
(this is a general comment, so would also apply for the other demo)
There was a problem hiding this comment.
The options are still available in the CSRPlugin. I removed them from here and other places, since CsrAccess.NONE was and is the default value for these.
There was a problem hiding this comment.
Would be possible to revert all the removals of "mcycleAccess = CsrAccess.NONE" and similar things ?
They are there to make it clear to the user that it isn't included, and that if they want they can just turn it on there.
| @@ -150,7 +150,7 @@ class VexRiscvSmpClusterWithPeripherals(p : VexRiscvSmpClusterParameter) extends | |||
| plic.addTarget(core.cpu.externalSupervisorInterrupt) | |||
| List(clint.logic, core.cpu.logic).produce { | |||
| for (plugin <- core.cpu.config.plugins) plugin match { | |||
| case plugin: CsrPlugin if plugin.utime != null => plugin.utime := clint.logic.io.time | |||
There was a problem hiding this comment.
Same here, would need to preserve that feature in the CsrPlugin. (even if that is a duplication) to not break people code
There was a problem hiding this comment.
The feature is still in CSRPlugin, even if disabled when CounterService is present (which in this demo it is), so it should break no existing code which uses the Counters in CsrPlugin.
This reverts commit 20f638e.
|
I reverted the removal of the config options. |
|
Thanks ^^ Also note since november i'm working on VexiiRiscv : medium / long terms the idea is to migrate toward it. |
| @@ -649,7 +654,7 @@ class CsrPlugin(val config: CsrPluginConfig) extends Plugin[VexRiscv] with Excep | |||
| pipeline.update(MPP, UInt(2 bits)) | |||
|
|
|||
| if(withExternalMhartid) externalMhartId = in UInt(mhartidWidth bits) | |||
| if(utimeAccess != CsrAccess.NONE) utime = in UInt(64 bits) setName("utime") | |||
| if(pipeline.serviceExist(classOf[CounterService]) && utimeAccess != CsrAccess.NONE) utime = in UInt(64 bits) setName("utime") | |||
There was a problem hiding this comment.
Ahhh this seems broken, shouldn't it be !pipeline.serviceExist(classOf[CounterService]) ?
There was a problem hiding this comment.
Yes, this must have slipped by.
|
Fixed the condition. |
|
Thanks ^^ |
|
my pleasure |
Fixes #361
Not tested, because I did not find the time for it.
So I do not know, whether it works or not.