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:

• 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]

Ah, thank you for your contribution! This looks good to me, I just have some minor points.

I know we have named functions f' in the past when we made a variant of f. But I'm not a big fan of the prime and I as well as at least some of my colleagues are moving away from it. So I'd like to suggest to call the new functions spiMasterWide and spiSlaveWide instead.

[DigitalBrains1]

@kloonbot run_ci d9b0c33

[bgamari]

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

[martijnbastiaan]

@kloonbot run_ci f161a5a

[DigitalBrains1]

Hehe, this is a bit silly, but...

So, the functions spiCommon, spiMaster, spiMasterWide and spiGen take an argument of type SPIMode. It makes sense to call this argument mode. Conversely, the functions spiSlave and spiSlaveWide take an argument of type SPISlaveConfig. It makes sense to call this argument cfg (if it needs to be bound to a name at all).

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.

Just to actually have stuff make sense, could you name that argument to spiSlave to cfg again, after that I'll merge it! :-D

[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]

[DigitalBrains1]

Don't forget to add yourself to the copyright line at the top! In fact, I forgot that as well, so I pushed a fixup! commit to my branch.

I think you already read it in the functional programming Slack channel, but we plan to update the terminology we use for SPI to the OSHWA suggested terminology: Controller, Peripheral, PICO, POCI, SCK (unchanged), CS. This will be in some future PR, not right here. Just like we are changing the terminology for our I²C stuff to the new NXP terminology.

[DigitalBrains1]

I suppose you want to keep the type-level arguments all the same across these types, but I think it's better to just declare arguments that are actually used. So SpiMasterOut doesn't need misoLanes and SpiMasterIn doesn't need mosiLanes.

[DigitalBrains1]

Could you update the Haddock for the output of this function?

[DigitalBrains1]

In the functions, you use the names mosiW and misoW, but here you use mosiLanes and misoLanes to refer to the same quantity. Could you pick one and apply that name everywhere?

[DigitalBrains1]

So the data inputs and outputs of spiMaster are of type Vec mosiW (BitVector n) and Vec misoW (BitVector n) respectively, but MOSI is defined as BitVector mosiLanes and MISO as BitVector misoLanes.

The first thing that I think when I see this is: wait, a Vec is numbered with ascending inices (!!) and a BitVector is numbered with descending indices (!)! So... how do those indices map?

And even if they were the same, I'd still wonder about the mapping to HDL indices. Does din !! 0 map to MOSI[0] or MOSI[n] in my SDC file?

However, I think BitVector _ for MOSI/MISO does have an advantage: it doesn't give the annoying

[...]: warning:
    Make sure HDL port names are correct:
Backtracked when constructing Clash.Sized.Vector.Vec
(Type appears recursive)

I'm not really sure about the type. We should clearly document the mapping of Vec indices to pins on the top entity, and ideally they'd match, so din !! 0 does map to MOSI[0]. But whether MOSI should be BitVector or Vec _ Bit I'm not so sure about.

[DigitalBrains1]

Could you add Haddock for the output tuple?