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

[feature] FuseSocGeneratorBuilder provide a easy way to use fusesoc #1232

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

chenbo-again
Copy link
Contributor

Context, Motivation & Description

fusesoc is a manager tool, which provide 2 main functions:

  1. It can be used as a pkg manager tool, module which from many sources(github, local etc.) can be used in a easy way
  2. edalize, user can easily ignore some eda script, and use some main function of this eda.

This PR provide a easy way to take spinalhdl component into fusesoc. It provide 2 main functions:

  1. parametrization in fusesoc
  2. generate fusesoc .core file template.

note: it depends a python script out of spinal repository

Text below describes usage:

  1. in workdir, type fusesoc library add spinal_generator https://github.com/chenbo-again/spinalhdl_fusesoc_ generator to get python script
  2. create FuseSocGeneratorBuilder
  3. use buildScript method generate .core file template
  4. create and modify .core file based on template
  5. create a entry function and execute FuseSocGeneratorBuilder#run

It is already tested locally

Impact on code generation

No

Checklist

  • [ x] Unit tests were added
  • [ x] API changes are or will be documented:

build.sbt Outdated Show resolved Hide resolved
| toplevel : $componentName
|
|""".stripMargin
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that possible to use provide section to download the generator script?
And if it is optional, shall we add an argument to switch its on-off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I can add a comment here.

Copy link
Collaborator

@andreasWallner andreasWallner left a comment

Choose a reason for hiding this comment

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

One small thing: Please run scalafmt

The bigger question from my side: do you have an example of the steps how this would be used - not sure I pieces this together correctly in my head.

build.sbt Outdated
"com.fasterxml.jackson.core" % "jackson-annotations" % "2.15.3",
"com.fasterxml.jackson.core" % "jackson-databind" % "2.15.3",
"com.fasterxml.jackson.dataformat" % "jackson-dataformat-yaml" % "2.15.3",
"com.fasterxml.jackson.module" % "jackson-module-scala_2.12" % "2.15.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we build for multiple scala versions (2.11, 2.12 & 2.13) we shouldn't hard-code the scala version, to append the correct version you can swap the first % to %%.

Suggested change
"com.fasterxml.jackson.module" % "jackson-module-scala_2.12" % "2.15.3"
"com.fasterxml.jackson.module" %% "jackson-module-scala" % "2.15.3"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for one example:
I want to write a module which may be used by others in fusesoc or I just don't want to write some eda scripts:

object GenerateFuseSocScript extends App {
  // it must be serializable
  case class Parameter(width: Int = 8, ports: Int = 2)
  val fusesoc = new FuseSocGeneratorBuilder[Parameter,MyTopLevel]("MyTopLevel", Parameter() ) {
    override def buildComponent(parameter: Parameter): MyTopLevel = {
      MyTopLevel(parameter.width, parameter.ports)
    }
  }
  println(fusesoc.buildScript)
}
object FuseSocEntryFunction extends App {
    ...
    fusesoc.run(args)
}

run GenerateFuseSocScript, then I get bunch of thing like below

CAPI=2:

name: "::MyTopLevel:0.0.0"
filesets:
  base:
    depend:
    - "chenbosoft:utils:generators:0.0.0"
generate:
  MyTopLevel_gen:
    generator: "spinalhdl"
    parameters:
      spinal_parameter:
        width: 8
        ports: 2
      output:
        files:
        - MyTopLevel.v:
            file_type: "verilogSource"
      target_directory: "./generte"
      entry_function: ""
      copy_core: false
      spinal_project_path: "."

targets:
  lint:
    default_tool : verilator
    generate: [MyTopLevel_gen]
    filesets: [base]
    tools:
      verilator:
        mode : lint-only
    toplevel : MyTopLevel

someone who want to use this module will copy this as a fusesoc .core file and modify some parameters(for example, I modify spinal_project_path to the dir which the build.sbt located, and make entry_function=xxx.FuseSocEntryFunction. For all possible parameter, see https://github.com/chenbo-again/spinalhdl_fusesoc_generator/blob/main/generators.core)

Then he use fusesoc library add spinal_generator https://github.com/chenbo-again/spinalhdl_fusesoc_generator to get the script(it parse yaml and pass spinal_parameter to FuseSocEntryFunction to build and generate verilog)

finally, let it into eda. I provide a example verilator lint. just type fusesoc soc target=lint MyTopLevel to execute it.

It's a bit complex to use fusesoc, but it can simplify many tedious tasks.

@Readon
Copy link
Collaborator

Readon commented Nov 3, 2023

fusesoc is convenient to help on generating FPGA project, such as project for Xilinx Vivado. Only source files and fusesoc core file is required to keep in version control tools.
It build the whole project by one command after setup. the generated project will contains IP repos, toplevel design, associate xdc constraint file and so on.
After the fusesoc build the generated project can be synthesised directly.

import scala.collection.mutable
import scala.reflect.ClassTag

class GeneratorOutput() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's good to have a prefix Fusesoc, it's easy to find out where it belongs when you use it.

val files = mutable.ArrayBuffer[Map1[String, Map[String, String]]]()
}
case class GeneratorParameters[P: ClassTag](
// for python only
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this comment for? Is this means there is some python part? How about refine it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to implement all things in spinal since fusesoc is a python stuff.

And refactoring this without rewrite serialize method will change *.core file structure, we don't want to leak inner implementation to core file user.(e.g. which field is implement by python or by spinal)

val name: String = s"::$componentName:$version"
val description: String = descriptions
val filesets = new Object {
val base: Map[String, List[String]] = Map("depend" -> List("chenbosoft:utils:generators:0.0.0"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? what does version 0.0.0 refer to? How about release a start version at 0.0.1?

@andreasWallner
Copy link
Collaborator

andreasWallner commented Nov 7, 2023

I think this is useful and would be good to have.

The issue that I see with the current interface:

  • because we need to overwrite buildComponent and stick to it's interface it's clunky to use if a component has more then one parameter
  • there is no way that I see to configure e.g. the default clock domain - I can't imagine this is never needed with the fusesoc flow?
  • there is no way to get at the elaboration report (return value from SpinalConfig.generate, which is a problem for many use-cases that I see where you'd e.g. want to generate a register map or something similar from the information in the netlist

what I'd propose (please provide some feedback if this can't work for your use-case):

  • instead of needing to overwrite buildComponent we pass the construction as a parameterless lambda/closure (comp: => C)
  • I see the use for a quick convenience function, but would it make sense to be able to generate the fusesoc files for a SpinalReport? then a user that wan't to have more control over the generation can do other stuff as well - or am I missing something here? is the user never running this manually and it's always just ran through fusesoc?

@chenbo-again
Copy link
Contributor Author

because we need to overwrite buildComponent and stick to it's interface it's clunky to use if a component has more then one parameter

Because we need to generate a template core file. Any fields of configure must be serializable, so I limit user to only use a specially defined configure class(in the code, it's generic type [P]).

And below is a common use case of fusesoc generator.

https://github.com/chipsalliance/VeeRwolf/blob/d48573f04247f4dafaea675d1188b5b5ac6ba9cb/veerwolf.core#L216

there is no way that I see to configure e.g. the default clock domain - I can't imagine this is never needed with the fusesoc flow?

I don't know much about ClockDomain, seems like the configure of it can be inserted to generic type [P]? Am I wrong?

And below is my test code.

object MyTopLevelVerilog extends App {
  case class Parameter(width: Int = 8, ports: Int = 2) 
  val fusesoc = new FusesocRunner[MyTopLevel, Parameter] {
    override def buildComponent(parameter: Parameter): MyTopLevel =  {
      MyTopLevel(parameter.width, parameter.ports)
    }
  }
  fusesoc.run(args)
  
  // val fusesoc = new GeneratorBuilder[MyTopLevel, Parameter]("MyTopLevel", Parameter())

  // println(fusesoc.buildScript)
}

@andreasWallner
Copy link
Collaborator

Because we need to generate a template core file. Any fields of configure must be serializable, so I limit user to only use a specially defined configure class(in the code, it's generic type [P]).

You are right, it's much simpler with only a single parameter. I'd still consider passing a closure: no need to derive and it would match how SpinalVerilog, etc. are used.

I don't know much about ClockDomain, seems like the configure of it can be inserted to generic type [P]? Am I wrong?

The default CD is passed to SpinalConfig directly to configure the settings (reset polarity/type) of the default CD of the component. The assumed click frequency is also passed to SpinalConfig (e.g. so that the UART can calculate which divider is needed for a certain fixed baudrate). Since both of those parameters are passed to SpinalConfig they don't really fit into [P] since sat that point in the code you don't know anything about P and couldn't access a member?

@andreasWallner
Copy link
Collaborator

@chenbo-again sorry for the dumb questions above, I don't know much about FuseSoC so I don't really know how it would handle different options for clock/reset polarity - or is it just fixed for all components?

@Readon
Copy link
Collaborator

Readon commented Nov 16, 2023

@chenbo-again sorry for the dumb questions above, I don't know much about FuseSoC so I don't really know how it would handle different options for clock/reset polarity - or is it just fixed for all components?

The fusesoc is just a build tool, helps to build a running project for Vivado or other tools.
It describes the files required by one IP core in a YAML file, so-called core file. Each IP core would have a description file.
It would still be necessary to connect the clock, reset and all other signals of those IP cores by top level Verilog code or Vivado's Block Diagram Tools.

However, you need not create the project manually.

Normally, I put the options for clock, reset and AXI bus signals of an IP core into a component.xml, which is in IP-XACT format.
This component.xml file is also required to be listed in the fusesoc's core file.

For short, an IP core can be defined through a core file which list Verilog files and one component.xml file.
A design contains many cores, and top level files.

So if SpinalHDL can generate the Verilog files, why not the core file?
Furthermore, the fusesoc allow calling a generator to generate Verilog file also, so here is the generator for SpinalHDL.
Also, it allows passing some parameters to it.

@andreasWallner
Copy link
Collaborator

@Readon I had understood what fusesoc is. My question was whether the current code is missing options for clock/reset settings - which it seems you'd agree that those should be added?

@Readon
Copy link
Collaborator

Readon commented Nov 19, 2023

@Readon I had understood what fusesoc is. My question was whether the current code is missing options for clock/reset settings - which it seems you'd agree that those should be added?

it's not necessary. all signals in one IP core is not appear in fusesoc core file. may be an example helps. there are still some more complex example there using generator.

@andreasWallner
Copy link
Collaborator

I'm not talking about the signals themselves: As you know in SpinalHDL we can choose clock and reset polarity, as well as whether reset is sync/async.
The question it should be able to provide those settings to the generator here, or does fusesoc anyways expect all components to have e.g. rising edge clock with active low async reset?

@Readon
Copy link
Collaborator

Readon commented Nov 27, 2023

I'm not talking about the signals themselves: As you know in SpinalHDL we can choose clock and reset polarity, as well as whether reset is sync/async. The question it should be able to provide those settings to the generator here, or does fusesoc anyways expect all components to have e.g. rising edge clock with active low async reset?

So the existing solution is to add the modifiable clock parameter to Component's argument list, so that it can be initiated through parameters set by core file.

@Readon
Copy link
Collaborator

Readon commented Nov 27, 2023

I'm not talking about the signals themselves: As you know in SpinalHDL we can choose clock and reset polarity, as well as whether reset is sync/async. The question it should be able to provide those settings to the generator here, or does fusesoc anyways expect all components to have e.g. rising edge clock with active low async reset?

So the existing solution is to add the modifiable clock parameter to Component's argument list, so that it can be initiated through parameters set by core file.

For example.

val AWrap(clockEdge: Int) extends Component {  
    var config = ClockDomain.current.config.copy(clockEdge = clockEdge)
    ClockDomain.current.copy(config = config) on {
        val inst = new A()
    }
}

@andreasWallner
Copy link
Collaborator

andreasWallner commented Nov 28, 2023

Ok, if fusesoc doesn't expect a specific config then I think there should be generic settings that can be passed to the fusesoc generator to configure the default clockdomain (clock polarity, reset polarity, reset type, assumed frequency).

@chenbo-again would it be ok for you to add that?

I'm thinking of having settings for those that are not component specific, in SpinalHDL they would then go into the clock configuration of the SpinalConfig. You mentioned above that you are not that familiar with that, so a quick example:

  val report = SpinalConfig(
    defaultConfigForClockDomains = ClockDomainConfig(
      clockEdge = RISING,
      resetKind = ASYNC,
      resetActiveLevel = LOW),
    defaultClockDomainFrequency = FixedFrequency(100 MHz),
    device = Device.XILINX
  ).generateVerilog(
    XilinxNamer(XilinxInOutWrapper(new andreasWallner.spinaltap.ApbSpinalTap()))
  )

If there is no frequency specific I'd default to UnknownFrequency

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.

None yet

3 participants