Bugfix/wb pipelined #1375

jdavidberger · 2024-03-26T15:08:51Z

Closes #1310

Context, Motivation & Description

Fix wishbone pipelined mode

Impact on code generation

None

Checklist

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

Fix stall usage in wishboneslavefactory

This fixes the bug. Mainly setting stall to false is the wrong thing; setting stall to the inverse of ack fixs the logic.

Clean up wishbone flags; mark questionable ones for deprecation

Probably needs some discussion. The main thing is that some of the current status checks are not right / meaningful for pipelined mode. I opted to deprecate and make new fields for backwards compatibility reasons. The non pipelined logic makes sense for these but the pipeline path is wrong for some of them:

isAck is maybe the most harmful. ACK and STALL are independent and it is very possible that there is no timestep where 'ACK && !STALL' is true.
isTransfer is a little inconsistent. It's pipelined semantics are effectively if a request exists and is accepted. The non pipelined semantics check if a request exists.
isStall is maybe mapped wrong for standard. I'm not sure what use it would have in it's current state. 'isCycle && STALL' seems wrong though since AFAICT STALL is only meaningful when CYC && STB.

jdavidberger · 2024-03-26T21:56:38Z

I cleaned up WishboneBusInterface too with additional tests for it.

Which of the interfaces is meant to be used more? I made the slave factory variant support proper pipelining, whereas the other one still has classic latencies.

Unrelated thing I found: Seems like if you set a field with "#=" in the simulation with the IVerilog backend, it'll never warn you not to do that and then all future retrievals of that value from the simulation of that signal will be what you set it to. The verilator backend doesn't do this. This caused a very confusing bug for me since the simulator disagreed with the fst file. It probably should warn you if you set an OUT signal in the simulation, or make those signals ineligble for the caching layer (readBypass)

…ding asserts that one ack is generated per one request

…erent granularities

Dolu1990 · 2024-03-29T11:44:26Z

Ahhh wishbone is such hell XD

I will notify people in gitter for them to know that if they use wishbone, they need to take a look in this issue.

andreasWallner · 2024-04-01T22:43:08Z

Looking at it in more detail, I'm not sure we should keep the totally broken version of isAck etc. around. No working module could have come from the current implementation of the pipelined mode. So the risk of breaking something "working" is really small and many users may not even see the deprecation notices depending on which editor they use.

I know that all behavior can be depended upon - but this is so far broken that we shouldn't keep it in.

One general question: do we want <>, << and >> to automatically convert between standard / pipelined bus and insert the needed HW, or should this be something that a user does manually? (also for some of the other possible differences, like presence of ERR, CTI, ...) Looking through the code I'm not sure we know what the correct way to connect different busses (especially if they use non-standard byte addressing) is?

andreasWallner

A mixture of stuff from some small random number stuff in the simulation to a few questions where I'm not sure we're doing the right thing when connecting to bus instances with differing configurations.

I hope I got everything in this round - there was quite a bunch of jumping around.

core/src/main/scala/spinal/core/IODirection.scala

andreasWallner · 2024-04-01T21:05:15Z

lib/src/main/scala/spinal/lib/bus/wishbone/Wishbone.scala

 import spinal.lib._

+import scala.language.postfixOps


IntelliJ generates a warning that those are needed, but they are not - we should at some point decide whether to use them or not (and e.g. just set the compiler option if we have issues in future versions)
@Dolu1990 what do you think?

lib/src/main/scala/spinal/lib/bus/wishbone/Wishbone.scala

tester/src/test/scala/spinal/tester/scalatest/SpinalSimWishboneAdapterTester.scala

lib/src/main/scala/spinal/lib/bus/wishbone/WishboneSlaveFactory.scala

tester/src/test/scala/spinal/tester/scalatest/SpinalSimWishboneBusInterfaceTester.scala

tester/src/test/scala/spinal/tester/scalatest/SpinalSimWishboneSlaveFactoryTester.scala

andreasWallner · 2024-04-02T00:05:58Z

tester/src/test/scala/spinal/tester/scalatest/SpinalSimWishboneSlaveFactoryTester.scala


      val dri = new WishboneDriver(dut.io.bus, dut.clockDomain)
      dri.drive(scala.collection.immutable.Seq(WishboneTransaction(10*wordInc), WishboneTransaction(0)), we = false)
      dri.drive(scala.collection.immutable.Seq(WishboneTransaction(10*wordInc, 200), WishboneTransaction(0, 2)), we = true)
      dri.drive(scala.collection.immutable.Seq(WishboneTransaction(10*wordInc), WishboneTransaction(0)), we = false)
+      dri.drive(scala.collection.immutable.Seq(WishboneTransaction(21*wordInc, 0)), we = false)


simply having read/write methods would make this a bunch simpler - but I guess not worth it since it works as is...

tester/src/test/scala/spinal/tester/scalatest/SpinalSimWishboneSlaveFactoryTester.scala

andreasWallner · 2024-04-02T14:05:26Z

And what I forgot to mention in my original reply: thanks for looking into this!

Co-authored-by: Andreas Wallner <A.Wallner@innovative-solutions.at>

This reverts commit facaa9d.

jdavidberger · 2024-04-02T20:55:04Z

Looking at it in more detail, I'm not sure we should keep the totally broken version of isAck etc. around. No working module could have come from the current implementation of the pipelined mode. So the risk of breaking something "working" is really small and many users may not even see the deprecation notices depending on which editor they use.

We should maybe just map it to isRequestAck. That will keep the behavior for non pipelined usages and I also suspect that there are far fewer people using pipelined mode.

One general question: do we want <>, << and >> to automatically convert between standard / pipelined bus and insert the needed HW, or should this be something that a user does manually? (also for some of the other possible differences, like presence of ERR, CTI, ...) Looking through the code I'm not sure we know what the correct way to connect different busses (especially if they use non-standard byte addressing) is?

My take on this is that <> / << / >> should do reasonable conversions whenever possible to stay out of users way. It'd be sort of annoying if it detected differences in the configuration and errored out a message saying to map it out one way or another manually.

So things I think it should automatically pave over:

Address granularity - See current iteration
Pipeline vs standard (STALL) - In current code, as given by the standard
BTE / CTI (defaults to 0 for classic)
LOCK
SEL (If the master doesn't have it enabled, it's all 1's. If the slave doesn't have it enabled, drop it. Possibly we require an opt in for this though)
TAGS (Just dropping seems reasonable. I'm not super well versed on what these are for or how they are used in practice though).

I think ERR/RTY does require extra attention though. The standard helpfully notes:

If the SLAVE supports the [ERR_O] or [RTY_O] signals, but the MASTER does not
support these signals, deadlock may occur.

with no real remedy or solution. I think it'd be reasonable to either emit a warning when RTY/ERR are dropped, or disallow it by default, and add a tag that lets you drop it (ie, the same kind of pattern as resized).

lib/src/main/scala/spinal/lib/sim/bus/wishbone/WishboneSequencer.scala

tester/src/test/scala/spinal/tester/scalatest/SpinalSimWishboneArbiterTester.scala

andreasWallner · 2024-04-03T23:22:49Z

As a sidenote: found the reason again why I proposed to drop the & CYC from the condition in the slave if not pipelined: it's what all the examples in chapter 8.6 and 8.7 show and explain...

andreasWallner · 2024-04-04T00:08:02Z

lib/src/main/scala/spinal/lib/sim/bus/wishbone/WishboneMonitor.scala

@@ -7,7 +7,8 @@ import spinal.lib.bus.wishbone._
 import scala.collection.mutable._

 object WishboneMonitor{
-  def apply(bus : Wishbone, clockdomain: ClockDomain)(callback: (Wishbone) => Unit) = new WishboneMonitor(bus,clockdomain).addCallback(callback)
+  def apply(bus : Wishbone, clockdomain: ClockDomain = null)(callback: (Wishbone) => Unit) =
+    new WishboneMonitor(bus, Option(clockdomain).getOrElse(bus.CYC.clockDomain)).addCallback(callback)


I understand why you put this in, but it's actually not "safe".
Reading the clockDomain of a signal that might not be a register is not really useful, since the property will be set to the clockDomain of the component where the signal was declared, not to the actual clockDomain as you'd want it. For this you'd have to search the driving FF.

Simple (and quite contrived) example - but it shows the issue:

object Blub extends App { SpinalVerilog { new Component { val x = in port Bool() val xx = RegNext(x) val myArea = new ClockingArea(ClockDomain.external("myClockName")) { val y = CombInit(xx) } val z = out port Bool() z.setAsReg() z := myArea.y println(xx.clockDomain) println(myArea.y.clockDomain) println(z.clockDomain) } } }

Prints:

clk myClockName_clk clk

which is not what you'd expect.
IMO we should leave the parameter mandatory

That is surprising but makes sense in retrospect. I can change those back, but I think this form is still useful for the tests that don't involve any new clocks / clock areas so this logic will hold?

This behavior is surprising to many and can easily trip someone up - we've had a few question along those lines. That's why I'd keep the parameter mandatory, if it's automatic it should work. And if you have vastly different clocks it might be easy to see in simulation, if it's just a clock enable it's much harder (I actually found out about this because I wanted to make a similar change to AHB and had to debug for a while a week later)

jdavidberger · 2024-04-04T00:14:14Z

As a sidenote: found the reason again why I proposed to drop the & CYC from the condition in the slave if not pipelined: it's what all the examples in chapter 8.6 and 8.7 show and explain...

It is super weird they specify:

RULE 3.40
As a minimum, the MASTER interface MUST include the following signals: [ACK_I], [CLK_I], [CYC_O], [RST_I], and [STB_O]. As a minimum, the SLAVE interface MUST include the following signals: [ACK_O], [CLK_I], [CYC_I], [STB_I], and [RST_I]. All other signals are optional.

and then go on to use a slave device without a CYC_I as the example?? The only thing that makes sense to me is there should be some verbiage that STB there is mapped to STB && CYC? Since it's not that much more expensive to keep the check as the && of the two I'd say we keep it like that for now.

I've been wanting to write up something like this for the unit tests and my own usages which could check for instances of STB && !CYC. It'd be a weird thing for a driver to do for sure.

andreasWallner · 2024-04-04T00:53:16Z

It is super weird

Agreed. For now I'd still keep it in... It shouldn't be that expensive and some interconnects might depend on it.

I've been wanting to write up something like this for the unit tests

This is going to be fun if you want to support all the different modes...

One doubt that just came up to me (not directly a request for this PR) looking at the WishboneArbiter:
Do I see it correctly that the arbiter will stall every master except one for a cycle when the Bus is idle? (but for that save on buffers for address etc.)

andreasWallner · 2024-04-04T00:55:56Z

lib/src/main/scala/spinal/lib/sim/bus/wishbone/WishboneDriver.scala

  /** Drive the wishbone bus as master with a transaction.
    * @param transaction The transaction to send.
    */
-  def sendAsMaster(transaction : WishboneTransaction, we: Boolean): Unit = {
+  private def sendAsMaster(transaction : WishboneTransaction, we: Boolean): Unit = {


We shouldn't make functions private if we don't want to break users of the standard mode functionality

I can reverse this; but I think the intent of the class is that everything uses send and it delegates out to the other functions. I'm not sure if anyone should be calling sendAsMaster directly? I guess the bus can be directionless in the sim but I'm not sure I see a use case for that.

But yeah, I guess since it can break existing code I'll reverse it. Maybe tagged as deprecated?

andreasWallner · 2024-04-04T01:04:07Z

lib/src/main/scala/spinal/lib/bus/wishbone/WishboneSlaveFactory.scala

  if(bus.config.isPipelined) bus.STALL := False

  val askWrite = bus.isWrite.allowPruning()
  val askRead = bus.isRead.allowPruning()
  val doWrite = bus.doWrite.allowPruning()
  val doRead = bus.doRead.allowPruning()

+  val readData = bus.DAT_MISO.clone
+  readData.assignDontCare()
  if(!reg_fedback){


sorry, I didn't notice last time - is it actually spec-compliant to have a slave where the bus has bus.config.isPipelined == true but we configure the WishboneSlaveFactory with reg_feedback == false?

The spec, as far as I can tell, doesn't say much about an asynchronous ACK specific to pipelined mode.

It says this:

RULE 3.59 In pipelined mode the MASTER must accept [ACK_I] signals at any time after a transaction is initiated.

and then has this:

Even though not initially designed for pipelined mode many simple wishbone slaves directly support this mode. For this to be true the following characteristics must be true 1. [ACK_O] should be registered

so maybe async isn't in spec for pipelined?

For signalling, it "CYC && STB && !STALL && ACK" is ambiguous but maybe too much to mandate that all master controllers can handle it.

Whether it's in spec or not, it seems prudent to disallow it here. Should it throw an assertion or just make it registered?

andreasWallner · 2024-04-04T02:07:14Z

So things I think it should automatically pave over:

* Address granularity - See current iteration

* Pipeline vs standard (STALL) - In current code, as given by the standard

* BTE / CTI (defaults to 0 for classic)

* LOCK

* SEL (If the master doesn't have it enabled, it's all 1's. If the slave doesn't have it enabled, drop it. Possibly we require an opt in for this though)

* TAGS (Just dropping seems reasonable. I'm not super well versed on what these are for or how they are used in practice though).

STALL - totally agree
CTI - as well, can only impact performance
LOCK - same, if a slave doesn't have it nothing lost (it might be a bit different on the way to an interconnect, but you'd have to do weird stuff to drop lock before it reaches that interconnect)
TAGS - drop them, haven't found much use of them online... - set them to 0 if the slave has them but the master hasn't?
ERR - somehow faking an OK seems wrong but I can't think of many huge things that can go wrong... If we already have a way to do the opt-in maybe still include it?

dropping SEL: has the potential to break a system (CPU does byte write, slave only support word write) - IMO this should need an opt-in

address granularity: really not sure on this one - it's hard to predict the impact since it's out-of-spec anyways. It wouldn't have an impact on stuff that comes from one of the generators in SpinalHDL, but dropping parts of the address (lower bits) w/o opt-in seems like a good way to hide a bug. So I see potential issues if the driver is using byte address (as it could e.g. do an unaligned byte write), no issues if the driver is using word.

Looking at other bus implementations in lib we have a bit of a mix:

Axi4Lite requires an exact match of the config
Axi4 does a bunch of conversion, e.g. dropping aw.lock but doesn't automatically drop w.strb (equivalent of SEL)
AHBLite/APB also require a mostly matching config (except increasing the address width)

If we require some opt-in then I'd just make a conversion function like we have on some other bits implementations that returns the new bus - the way resize works goes seems overkill for this

andreasWallner · 2024-04-14T22:14:28Z

I had a discussion with @Dolu1990: we were wondering if it would make more sense to move this to a WishboneB4 package, keeping the current one as is (probably with some asserts that users will get an error if they use something that does not work at all).

This would give us the option to get rid of all the duplicate/deprecated names... Would you be willing to do tackle that?

jdavidberger · 2024-04-15T21:40:53Z

ERR - somehow faking an OK seems wrong but I can't think of many huge things that can go wrong... If we already have a way to do the opt-in maybe still include it?

After thinking about this some, this might not work. The standard says after an ERR asserts, CYC should be dropped even if there are outstanding requests. The best I can think to do for this is to institute a timeout mechanism to unfreeze the bus but this isn't great. Maybe bridging in this direction requires manual intervention.

I think the connectTo function has all the needed overrides, its just a matter of default behaviors. Probably with an additional function adaptTo(config : WishboneConfig) and things for the manual overrides like "dropError" / "dropSEL".

I had a discussion with @Dolu1990: we were wondering if it would make more sense to move this to a WishboneB4 package, keeping the current one as is (probably with some asserts that users will get an error if they use something that does not work at all).

If I understand this right -- basically create a new WishboneBusB4/config/etc set of classes in a new package -- I don't know that I like this idea. This would be really confusing to people who just want to pick the right one, and I can't think of a way it wouldn't fragment the existing codebase wrt arbiters/decoders/bus conversions. I think aggressively deprecating foot-guns and being diligent about not breaking existing usages (or existing non-pipelined usages) ends up being better long term.

andreasWallner · 2024-04-28T23:32:11Z

The drawback is that deprecation is often either ignored, or right out not seen due to whatever editor is being used. This doesn't mix really well with keeping around functions that are incorrect for both use-cases, but named more logical.
That is why we ended up at the point to make a new version of this so that the old stuff can be fixed...

jdavidberger · 2024-04-29T17:54:24Z

In any case, you either have to modify / remove those flags or just mark them as deprecated. If the idea is that we preserve the functionality of those fields for non pipelined mode, but are free to modify the behavior for the pipelined flag returns that makes sense to me. This breaks backwards compatibility but in a narrow already broken subset.

For creating a WishboneB4 interface, whats the desired strategy to handle all the library tooling for arbiter / decoder / bus conversion / busIf infrastructure? Copy paste seems wrong, retooling to a higher class interface seems like its adding complexity. I have reservations about it still, but don't mind making it happen so long as I understand what the intended end state is.

jdavidberger added 4 commits March 26, 2024 08:37

Fix stall usage in wishboneslavefactory

05ef666

Clean up wishbone flags; mark questionable ones for deprecation

ad53f8d

Test reg feedback; make sure memories work in wb slave

30dee02

Make WB BusInterface work with pipelined mode; add tests for it

258f15f

jdavidberger added 3 commits March 27, 2024 00:03

Fix the arbiter unit tests; fix pipeline related bugs uncovered by ad…

36b48a2

…ding asserts that one ack is generated per one request

Enabling commits and typo fixs

facaa9d

Make autoconnect do the right thing with wishbone; test bridging diff…

435f635

…erent granularities

andreasWallner requested changes Apr 2, 2024

View reviewed changes

jdavidberger and others added 2 commits April 2, 2024 12:11

Apply suggestions from code review

1299063

Co-authored-by: Andreas Wallner <A.Wallner@innovative-solutions.at>

Revert "Enabling commits and typo fixs"

ae42daa

This reverts commit facaa9d.

jdavidberger added 3 commits April 3, 2024 12:42

Fix unit tests per last round of modifications

9b99164

Check latency times

223ba22

Make bounds a little more clear

f50d024

jdavidberger force-pushed the bugfix/wb_pipelined branch from a88d773 to f50d024 Compare April 3, 2024 19:11

andreasWallner reviewed Apr 3, 2024

View reviewed changes

lib/src/main/scala/spinal/lib/sim/bus/wishbone/WishboneSequencer.scala Outdated Show resolved Hide resolved

andreasWallner reviewed Apr 3, 2024

View reviewed changes

tester/src/test/scala/spinal/tester/scalatest/SpinalSimWishboneArbiterTester.scala Outdated Show resolved Hide resolved

Clean up fstWave usage

ad9b594

andreasWallner reviewed Apr 4, 2024

View reviewed changes

Rework WishboneMonitor

95f754f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/wb pipelined #1375

Bugfix/wb pipelined #1375

jdavidberger commented Mar 26, 2024

jdavidberger commented Mar 26, 2024

Dolu1990 commented Mar 29, 2024

andreasWallner commented Apr 1, 2024 •

edited

andreasWallner left a comment

andreasWallner Apr 1, 2024

andreasWallner Apr 2, 2024

andreasWallner commented Apr 2, 2024

jdavidberger commented Apr 2, 2024

andreasWallner commented Apr 3, 2024

andreasWallner Apr 4, 2024 •

edited

jdavidberger Apr 4, 2024

andreasWallner Apr 4, 2024

jdavidberger commented Apr 4, 2024

andreasWallner commented Apr 4, 2024 •

edited

andreasWallner Apr 4, 2024

jdavidberger Apr 4, 2024

andreasWallner Apr 4, 2024

jdavidberger Apr 5, 2024

andreasWallner commented Apr 4, 2024 •

edited

andreasWallner commented Apr 14, 2024

jdavidberger commented Apr 15, 2024

andreasWallner commented Apr 28, 2024

jdavidberger commented Apr 29, 2024

Bugfix/wb pipelined #1375

Are you sure you want to change the base?

Bugfix/wb pipelined #1375

Conversation

jdavidberger commented Mar 26, 2024

Context, Motivation & Description

Impact on code generation

Checklist

jdavidberger commented Mar 26, 2024

Dolu1990 commented Mar 29, 2024

andreasWallner commented Apr 1, 2024 • edited

andreasWallner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasWallner commented Apr 2, 2024

jdavidberger commented Apr 2, 2024

andreasWallner commented Apr 3, 2024

andreasWallner Apr 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdavidberger commented Apr 4, 2024

andreasWallner commented Apr 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasWallner commented Apr 4, 2024 • edited

andreasWallner commented Apr 14, 2024

jdavidberger commented Apr 15, 2024

andreasWallner commented Apr 28, 2024

jdavidberger commented Apr 29, 2024

andreasWallner commented Apr 1, 2024 •

edited

andreasWallner Apr 4, 2024 •

edited

andreasWallner commented Apr 4, 2024 •

edited

andreasWallner commented Apr 4, 2024 •

edited