Bugfix/wb pipelined

[jdavidberger]

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]

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)

[Dolu1990]

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]

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.

[andreasWallner]

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?

[andreasWallner]

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

[andreasWallner]

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

[jdavidberger]

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).

[andreasWallner]

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]

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

[jdavidberger]

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?

[andreasWallner]

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]

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]

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]

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

[jdavidberger]

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]

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?

[jdavidberger]

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]

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]

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]

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]

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]

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.