Move block RAM reset value intoClearOnReset

[gergoerdi]

As seen on TV the Clash Stack.

[rowanG077]

To keep the same behavior as is done elsewhere in clash keep the initial value the same as the reset value. ResetStrategy r () should be the reset argument. The actual reset value is then the given bare a which is also the initial value.

[DigitalBrains1]

This function is a bit different. The initial value is undefined, but you can specify a function which will be used to fill the RAM with data when you assert reset long enough (this is an actual circuit running, filling one data position every cycle, so you need to have a running clock and keep reset asserted while the clock ticks for as many cycles as the RAM is large).

This behaviour isn't changed by this PR; it just moves the argument that holds the function to populate the RAM if you assert reset.

[DigitalBrains1]

I don't think we have a block RAM that has both an initial value and a reset value like you propose... we have block RAMs with initial values, but those are lost if you write to them, you can't restore the initial value on reset. If you needed both an initial value and a reset value in an FPGA, you're pretty much going to need two block RAMs: one as a ROM to restore the initial value from, and one that is your actual writable block RAM.

It's quite different from a set or clear input to a register where the bitstream just selects one of those to use as initial/reset.

[rowanG077]

I don't think the initial value is undefined. This is the type of blockRam1

-- memory positions
blockRam1
   :: forall n dom a r addr
   . ( KnownDomain dom
     , HasCallStack
     , NFDataX a
     , Enum addr
     , NFDataX addr
     , 1 <= n )
  => Clock dom
  -- ^ 'Clock' to synchronize to
  -> Reset dom
  -- ^ 'Reset' line. This needs to be asserted for at least /n/ cycles in order
  -- for the BRAM to be reset to its initial state.
  -> Enable dom
  -- ^ 'Enable' line
  -> ResetStrategy r a
  -- ^ Whether to clear BRAM on asserted reset ('ClearOnReset') or
  -- not ('NoClearOnReset'). The reset needs to be asserted for at least /n/
  -- cycles to clear the BRAM.
  -> SNat n
  -- ^ Number of elements in BRAM
  -> a
  -- ^ Initial content of the BRAM (replicated /n/ times)
  -> Signal dom addr
  -- ^ Read address @r@
  -> Signal dom (Maybe (addr, a))
  -- ^ (write address @w@, value to write)
  -> Signal dom a
  -- ^ Value of the BRAM at address @r@ from the previous clock cycle

So it has an initial value a as bare argument and a resetValue a inside ResetStrategy. If you look at the implementation in this PR currently:

    ClearOnReset a' ->
      -- Use reset infrastructure
      blockRam1# clk en n a rd1 we1 wa1 w1
      where
        rd1 = mux rstBool 0 (fromEnum <$> rd0)
        we1 = mux rstBool (pure True) we0
        wa1 = mux rstBool (fromInteger . toInteger <$> waCounter) (fromEnum <$> wa0)
        w1  = mux rstBool (pure a') w0

It uses the original a as the initial repeated contents of the blockram by passing it to blockRam1#. And then writes the value in ResetStrategy a' into the blockram on reset via w1. So now you indeed have a different initial and reset value.

Am I overlooking something here?

[DigitalBrains1]

Well I was talking about blockRamU. I now see you commented on blockRam1; I had misinterpreted the collapsed diff and not noticed that blockRam1 changed at all.

I agree that it doesn't make sense for blockRam1.

[DigitalBrains1]

In fact, I don't see why blockRam1 needs a ResetStrategy. If you don't want a reset to do anything, just don't assert the reset?

Initially I thought "well maybe you want the pipeline registers cleared but not the contents", but this code doesn't reset the pipeline registers ever?

[edit]
Ah that also goes for blockRamU, it also doesn't clear the pipeline registers on reset.
[/edit]

[DigitalBrains1]

It's just that for the version with implicit resets, it's easier to pass NoClearOnReset than get rid of the implicit reset...

[rowanG077]

I agree. Now that I'm reading the code I see more wrong with it... I think the reset logic is broken if the reset is async:

  rstInv = invertReset rst0

  waCounter :: Signal dom (Index n)
  waCounter = register clk rstInv en 0 (satSucc SatBound <$> waCounter)

Even if rst0 is synchronously de-asserted. rstInv is not. Which means there is a chance waCounter is poisoned.

[rowanG077]

Anyway that's all out of scope of this PR imo. For now I would say just use ResetStrategy r ().

[DigitalBrains1]

@kloonbot run_ci 1481dc1