SPI: Generalize to multi-lane MOSI/MISO

[bgamari]

It is fairly common for single SPI bus to consist of a set of parallel MISO/MOSI lanes (c.f. QSPI FLASH). For instance:

• many multi-channel ADCs allow each converter to clock out over its own MISO lane to reduce the clockrate needed to achieve the designed conversion rate.

• similarly, QSPI FLASH relies upon four bidirectional outputs to increase data rate.

Here we extend Clash.Cores.SPI to facilitate this use-case by introducing spiMaster' and spiSlave', which allow arbitrary MISO/MOSI lane widths.

Still TODO:

• [x] Write a changelog entry (see changelog/README.md) Not a public package, so no changelog needed yet
• [ ] Check copyright notices are up to date in edited files

[DigitalBrains1]

@kloonbot run_ci d9b0c33

[bgamari]

Thanks for the review, @DigitalBrains1. I have addressed your feedback.

[martijnbastiaan]

@kloonbot run_ci f161a5a3f6bc84806949c28130cd66761840bfd2

[lmbollen]

^ Open for discussion of course, but given the fact that clash-cores is not officially released yet, I'd suggest not making separate functions for single lane and multi lane SPI, but changing the API. What do you think @DigitalBrains1 @bgamari ?

[DigitalBrains1]

I don't have a strong opinion on this.

[DigitalBrains1]

Although if we're going to change API's, I also want to change the type of the S̅S̅ (Slave Select) signal. Currently it's a Bool, but S̅S̅ is active-low, so it's asserted when it's False. *shudder*

(added overline to emphasise the active-low character)

[bgamari]

Your first version of this PR had that with one exception: spiMaster's argument name got changed from mode to cfg, and I asked whether you could rename it back to mode.

You did that in your second version, but you also changed the name for the argument to spiSlave to mode, but that should just have stayed the cfg you initially made it.

Sigh, yes. Sorry about that, @DigitalBrains1. I will fix this now.

Open for discussion of course, but given the fact that clash-cores is not officially released yet, I'd suggest not making separate functions for single lane and multi lane SPI, but changing the API. What do you think

I also don't have a strong opinion here. I see little harm having the simpler wrappers available for the common, single-lane case.

Although if we're going to change API's, I also want to change the type of the S̅S̅ (Slave Select) signal. Currently it's a Bool, but S̅S̅ is active-low, so it's asserted when it's False. shudder

(added overline to emphasise the active-low character)

Indeed, I think changing the type here would be a very good idea.

[bgamari]

Also, I do wonder whether packaging the SPI signals into a record might improve usability by eliminating repetition in the types.

[DigitalBrains1]

Yes, let's make it record types. Thanks for collaborating on this! While you're in there, could you also rename this

  , spiSlaveConfigLatch :: Bool
  -- ^ Whether to latch the SPI pins
  --
  -- Recommended:
  --
  -- * Set to /True/ when core clock is /more/ than twice as fast as the SCK
  --   Clock: 2*SCK < Core
  --
  -- * Set to /False/ when core clock is twice as fast, or as fast, as the SCK

to reflect the fact that this option inserts a flip-flop, not a latch... we don't even have latches in Clash. Within the Clash ecosystem, we use the terminology in a way such that a latch and a flip-flop are two distinct things (I'm aware in general there is ambiguity). Perhaps rename the record spiSlaveConfigBuffer or something that seems good to you.

That is the only use of the word latch that affects the API. Other occurences should probably also be corrected but don't affect the API, so we can do that at a later time.

With record types, I don't think there's much call for a convenience function for single-lane applications. I'd imagine there'll be a type SPIMasterOut wordSize numLanes. I don't think it adds much to add a type synonym for the numLanes == 1 case.

[DigitalBrains1]

(By the way, I'm concerned that the sampling of SCK is prone to metastability. Even with the spiSlaveConfigLatch, there is but a single flip-flop, not a dual flip-flop synchroniser by half. But this is of a completely different order than what we are discussing here.)

(Also, Set to /False/ when core clock is twice as fast, or as fast, as the SCK, whut? SCK is sampled as data, surely it is absolutely impossible to sample when SCK isn't at least twice as slow as the core clock)

[DigitalBrains1]

cntOld in spiCommon is completely unused? The signal is computed and maintained in a register but nothing ever refers to it.

[bgamari]

Yes, let's make it record types. Thanks for collaborating on this!

My pleasure!

The next question is precisely how to structure these types. The obvious encoding would be:

data Spi_M2S f = Spi_M2S { spiSclk, spiCs, spiMosi :: Bit }

data Spi_S2M f = Spi_S2M { spiMiso :: Bit }

However, in my own projects I generally use higher-rank types to represent such products:

data Spi_M2S f = Spi_M2S { spiSclk, spiCs, spiMosi :: f Bit }

data Spi_S2M f = Spi_S2M { spiMiso :: f Bit }

This gives a pleasing symmetry between bundled and unbundled representations (although admittedly the bundled representation is a bit less convenient to use). However, it may not be worthwhile in this case as the user generally shouldn't need to work with this type in unbundled form.

[DigitalBrains1]

I agree that in this particular instance, dealing with the unbundled form is not really something you'd do.

[DigitalBrains1]

I was thinking along these lines

data SpiSlaveIn (ds :: BiSignalDefault) (dom :: Domain) (numLanes :: Nat) =
  SpiSlaveIn {
    spiSlaveMosi :: "MOSI" ::: Signal dom (BitVector numLanes),
    spiSlaveMisoIn :: "MISO" ::: BiSignalIn ds dom numLanes,
    spiSlaveSck :: "SCK" ::: Signal dom Bit,
    spiSlaveSs :: "SS" ::: Signal dom Bit
  }

data SpiSlaveOut (ds :: BiSignalDefault) (dom :: Domain) (numLanes :: Nat) =
  SpiSlaveOut {
    spiSlaveMisoOut :: "MISO" ::: BiSignalOut ds dom numLanes
  }

and similar for SpiMaster{In,Out}. For the order of the tyvars, I took the order that BiSignal uses, in the hope people might remember it :-).

[edit] It occurs to me that you support asymmetric lane counts and I just flattened that with this example. So the example should be adjusted to have separate tyvars for MOSI and MISO lane count. [/edit]

[DigitalBrains1]

Don't forget to add a copyright line at the top of the file.

[DigitalBrains1]

Hmmmm, I think we don't support BiSignalIns as part of a record. It suddenly struck me that we might run afoul of issue #2168, but looking at that issue further I notice this comment which states we don't support it (yet); it seems the issue is primarily that the design there should have triggered an error.

Also, I'm pretty sure spiSlave has metastability issues in sampling SCK when spiSlaveConfigLatch == False, and that I can come up with a metastability scenario where it misses an SCK edge and shifts a data bit too few. With spiSlaveConfigLatch == True, the situation is very much improved, but still, one flip-flop is still far from a dual flip-flop synchroniser.

However, the question is what to do about this. Are you just going to use the SPI master? I'm leaning towards just documenting the deficiencies of the slave for now. In an ideal world we'd be able to work on everything, in the real world we have to prioritise.

[bgamari]

Are you just going to use the SPI master?

Yes, my interests lie only with the master.

[DigitalBrains1]

As far as I'm concerned, if you want to, we can convert just the spiMaster to record syntax. I can do a documentation PR that notes for spiSlave that the record syntax is currently blocked on not being able to put a BiSignalIn in a record and also noting its synchronisation issues.

[martijnbastiaan]

Seems to me that if we can't support records for all functions, we should either:

1. Not use BiSignal, but split it up into three (in, out, out_enable) and ask users to instantiate a tristate buffer themselves. (And perhaps provide a convenient function for it.)
2. Accept the PR as is, i.e., no records.

What are the authors's / reviewers's preferences?

[DigitalBrains1]

First of all, I'm fine with accepting it as-is, as I've indicated before.

I'm also not against your proposal point 1, but I don't think it needs to be part of this PR. @bgamari is only interested in the master, so we can do the work on the slave ourselves after this PR is merged. Note that only the slave needs BiSignal! So @bgamari could convert the master to record syntax¹ and we could do any work we consider needed on the slave. Of course if he wants, @bgamari is more than welcome to do the work :-), but it is not a bar to clear for this PR to be merged, in my opinion.

Finally, diverging from your proposal point 1, I'm also fine with this follow-up work not being done in the short term. I think the SPI master is going to see much more use than the slave. I also think the slave needs to be improved to be generally useful.

The mode without a register on the SCK line is just prone to metastability, in my opinion it has no right to exist. I'm pretty sure I can construct a really basic metastability scenario where it already misses SCK clock edges. The mode that registers the input with a single register is pretty resilient; the special flip-flops on input pins make it difficult to drive them into metastability. But still, I'd feel even better with a proper DFF synchroniser. I don't know how important a DFF synchroniser is. We'd have to think about and document maximum clock rate ratios for this to work; the slave will drive its own data out later when its SCK input is delayed by the synchroniser. This means the setup time for the MISO input in the master is reduced, and the maximum clock rate is reduced accordingly. But on the flip side, we can reduce the response time by dropping the output register in the slave on MISO; since this is a synchronous protocol, I don't think there is any downside to MISO glitching for a bit other than power draw and EMI emissions. Those might be relevant, so it should probably be optional, for the people who want that extra oomph of interface speed. [edit] We could also consider a slave where the SCK is routed directly to the flip-flop clock inputs if people want the highest clock rates possible. It would make configuring the SPI mode more difficult (specifically, which clock edge is the sample clock edge).[/edit]

The whole BiSignal is only useful with multiple slaves on a single bus; but often SPI is used with a single slave. So we could also create a special case for a slave that doesn't even use a bidirectional pin, which can be used more simply.

¹ Please use Clash.NamedTypes to support naming the ports based on that information

[bgamari]

@DigitalBrains1, I have rebased this and (I believe) addressed your comments. Thanks again for your help and reviews.

[DigitalBrains1]

Thanks for your further work!

I will post more review later, but I started with a couple of proposed changes which I have put in another branch.

First of all, we have the issue that you cannot make a BiSignalIn part of a record! It would be nice if it were possible, but Clash chokes on it currently. Your current code for the SPI slave will generate faulty HDL!

That's why I suggest you leave the SPI slave completely alone. We'll deal with it later. Therefore, I have created an alternative branch where only the master (and spiCommon) are altered. The change to spiSlave is limited to making it work with the changed spiCommon.

Secondly, I'd like to make the change of Bool to Bit for S̅S̅ and SCK more pervasive. It makes no sense that they are Bool in spiCommon and spiGen either, and those functions contain weird-looking code because of it. Weird-looking as in, "slave select" is a boolean signal which is matched in a condition, where True means slave select is deasserted instead of asserted. It just runs in reverse of intuition.

So I added a commit in my alternative branch which changes S̅S̅ and SCK to be Bit throughout.

My suggested branch is at peter/multilane-spi. It is supposed to be identical to your branch except for the two points above.

[edit] I've been playing with the BiSignal. If it's part of a record in the top entity, the generated HDL is clearly faulty. If you unpack the record into its constituent parts, the code looks correct at a glance. So the bug rears its ugly head when the record is actually on the top entity. [/edit]