Add in a software clock control DUT #654

rslawson · 2024-10-15T15:13:53Z

This PR adds in a new SwCc DUT based on the work done in my two other PRs. As of the current PR it mostly passes CI except for linting, but that's okay since there's still work to be done.

As for what work is still yet to be done:

@lmbollen suggested that I add in UGN stability testing to this test as well. It was in some earlier versions of my work, which can be seen in the (awful) history of my rs/measure-sw-cc branch. The only other DUT this appears in is FullMeshSwCc.
Some work must be done to figure out why I need the weird hack around masking syncIn to IlaPlotSetup (see line 908 and lines 937-947 as of time of writing) in order to avoid two syncRst asserts, which then causes captureCond to become Calibrate :: CaptureCondition for a second time, which is problematic.
Commented-out legacy code needs to be removed.
Formatting needs to be fixed (thanks CI).

rslawson · 2024-10-15T15:36:48Z

I should also probably report that somewhere in the combined total of the work from the three PRs (between this one and the two it depends on) the clock update frequency variability mentioned in issue #587 seems to have disappeared. Looking at the callistoSwIla ILA data, every test reports a minimum update period of 307 cycles/2.456 μs and a maximum update period of 309 cycles/2.472 μs for a variability of 16 ns. This is consistent across all boards within all topologies. The only big unknown left is software reframing timing, but that should be easy enough to get data on with just a single change to the DUT in another branch.

rslawson · 2024-10-16T16:36:54Z

As per the discussion in the meeting earlier today, the syncIn masking hack will stay in place until the testing framework changes in the pre-process branch are merged.

The issue is, as far as I can tell, that the syncOut signal from FPGA 7 continues to propagate to the other boards until it is reset by the HITL VIO providing it with new test data. The other boards, are still receiving this on syncIn, causing them to begin the test early. This results in the plotting ILA entering the calibrating state twice per test, which is a problem for cc-plot.

The current ~~hack~~ solution in this branch looks for the syncIn signal to be unchanging for 25ms or longer, which is interpreted as FPGA 7 being reset. After this the syncIn signal is unmasked and allowed to propagate through to the test synchronisation circuitry for the duration of the test. However, through proper usage of the pre-test, monitor, and post-test functions in the HITL tests it should be possible to force all of the synchronisers into reset in the post-test phase. This would ensure there would be no sync signal on syncIn until all of the boards have been given test data in the following pre-test phase.

rslawson · 2024-10-17T14:51:11Z

Adding to work to be done:
Figure out a way to make sure that each node cannot run a diff clock against another node that has not left clock control reset.

rslawson · 2024-10-29T14:50:23Z

Also yes, I promise I'll clean up this commit history before asking for review.

rslawson · 2024-11-18T11:16:47Z

The reframe enable config option currently doesn't seem to do anything. Currently debugging that in rs/swCcTopologiesTest-rfen.

rslawson · 2024-11-20T17:16:23Z

As mentioned, reframing is currently not working and I don't believe there's a quick path to getting it to work. As such, I'll make another PR at a later time to fix that, but the testing framework for enabling it will remain in place for the time being.

lmbollen · 2024-11-26T15:08:58Z

.github/workflows/ci.yml

          export RUNREF="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}"
-          cabal run -- bittide-tools:cc-plot ${{ github.run_id }}:hwCcTopologyTest hitl-topology-plots
+          export TMP="${{ needs.bittide-instances-hardware-in-the-loop-test-matrix.outputs.report_kind }}"
+          cabal run -- bittide-tools:cc-plot ${{ github.run_id }}:$TMP hitl-topology-plots



What's this for? If you want to define an environment variable you should at least give it a descriptive name

lmbollen · 2024-11-26T15:09:28Z

.github/workflows/ci.yml

          mkdir -p plot-sources
-          mv hitl-topology-plots plot-sources/hwCcTopologyTest
+          export TMP="${{ needs.bittide-instances-hardware-in-the-loop-test-matrix.outputs.report_kind }}"
+          mv hitl-topology-plots plot-sources/$TMP



Same for here

lmbollen · 2024-11-26T15:13:03Z

.github/workflows/ci.yml

-          should_generate_report=$(jq 'map(select(.top == "hwCcTopologyTest")) | length > 0' checks.json)
+          should_generate_report=$(jq 'map(select(.top == "hwCcTopologyTest" or .top == "swCcTopologyTest")) | length > 0' checks.json)
+          if [ "${should_generate_report}" == 'true' ]; then
+            tmp=$(jq 'map(select(.top == "hwCcTopologyTest")) | length > 0' checks.json)


Descriptive variable names :)

lmbollen · 2024-11-26T15:21:05Z

bittide/src/Bittide/CircuitUtils.hs

@@ -0,0 +1,24 @@
+-- SPDX-FileCopyrightText: 2022 Google LLC


lmbollen · 2024-11-26T15:34:21Z

bittide/src/Bittide/ClockControl/CallistoSw.hs

@@ -0,0 +1,149 @@
+-- SPDX-FileCopyrightText: 2022 Google LLC


lmbollen · 2024-11-26T15:42:59Z

bittide/src/Bittide/ClockControl/CallistoSw.hs

+data SwControlConfig dom mgn fsz where
+  SwControlConfig ::
+    ( KnownNat mgn
+    , KnownNat fsz
+    , 1 <= fsz
+    , KnownDomain dom
+    ) =>
+    Signal dom Bool ->
+    SNat mgn ->
+    SNat fsz ->
+    SwControlConfig dom mgn fsz


This is not necessarily specialized for software, is it? Could you add some more haddock comments?

lmbollen · 2024-11-26T15:53:40Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+  othersNotInCCReset = go <<$>> transceivers.rxDatas
+   where
+    go (Just val) = msb val == high
+    go _ = False


Suggested change

othersNotInCCReset = go <<$>> transceivers.rxDatas

where

go (Just val) = msb val == high

go _ = False

othersNotInCCReset = maybe False (\val -> msb val == high) <$> transceivers.rxDatas

I still need to do <<$>> (since transceivers.rxDatas is Vec n (Signal rx (Maybe (BitVector 64)))), but otherwise yeah I like how you wrote that.

lmbollen · 2024-11-26T15:59:33Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+  othersNotInCCResetSync = zipWith go othersNotInCCReset transceivers.rxClocks
+   where
+    go sig rxClk = xpmCdcSingle rxClk sysClk sig


You can write this slightly shorter

Suggested change

othersNotInCCResetSync = zipWith go othersNotInCCReset transceivers.rxClocks

where

go sig rxClk = xpmCdcSingle rxClk sysClk sig

othersNotInCCResetSync = zipWith go transceivers.rxClocks othersNotInCCReset

where

go rxClk = xpmCdcSingle rxClk sysClk

lmbollen · 2024-11-26T16:00:54Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

      ( ilaConfig
-          $ "trigger_0"
-          :> "capture_0"
-          :> "probe_milliseconds"
+          $ "trigger_fdi_0"
+          :> "capture_fdi_0"
+          :> "probe_fdi_milliseconds"


Why _fdi?

I actually have no idea. I swear I copied that from another test but now I can't find where.

lmbollen · 2024-12-03T10:31:05Z

bittide/src/Bittide/ClockControl/CallistoSw.hs

+callistoSwClockControl ::
+  forall nLinks eBufBits dom margin framesize.
+  ( KnownDomain dom
+  , KnownNat nLinks
+  , KnownNat eBufBits
+  , 1 <= nLinks
+  , 1 <= eBufBits
+  , nLinks + eBufBits <= 32
+  , 1 <= framesize
+  , 1 <= DomainPeriod dom
+  ) =>
+  Clock dom ->
+  Reset dom ->
+  Enable dom ->
+  SwControlConfig dom margin framesize ->
+  Signal dom (BitVector nLinks) ->
+  Vec nLinks (Signal dom (RelDataCount eBufBits)) ->
+  Signal dom (CallistoResult nLinks)


Exported function missing documentation

martijnbastiaan

LGTM in general. I'm really looking forward to ditching all the different instances.

bittide/src/Bittide/ClockControl/Callisto/Types.hs

martijnbastiaan · 2024-11-13T13:19:01Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+
+  calibratedClockShift = capturedOnce
+   where
+    -- Possibly overkill.


Could you explain this comment? (In the comment)

So actually I think it's worth bringing this up here since I wanted to ask if there was a good way to latch a value exactly once on a given signal. I think what I wrote there might be a bit over-engineered, and I'm also not sure if there's just a utility function somewhere that I overlooked.

martijnbastiaan · 2024-11-27T16:01:52Z

bittide/src/Bittide/CircuitUtils.hs

+  Circuit (CSignal dom a) (Vec n (CSignal dom a))
+csDupe = Circuit $ \(m, _) -> (pure (), repeat m)
+
+cSigMap ::


Suggested change

cSigMap ::

cSignalMap ::

Abbreviations take brain cycles to decode :-)

martijnbastiaan · 2024-12-04T09:23:48Z

bittide/src/Bittide/ClockControl/CallistoSw.hs

+import Clash.Explicit.Prelude hiding (PeriodToCycles)
+
+-- import qualified Clash.Explicit.Prelude as E
+import Clash.Prelude (withClockResetEnable)
+
+import Language.Haskell.TH (runIO)
+import System.FilePath
+
+import Bittide.CircuitUtils
+import Bittide.ClockControl (RelDataCount)
+import Bittide.ClockControl.Callisto.Types (CallistoResult (..), ReframingState (Done))
+import Bittide.ClockControl.DebugRegister (
+  DebugRegisterCfg (DebugRegisterCfg),
+  debugRegisterWb,
+ )
+import Bittide.ClockControl.Registers (ClockControlData (..), clockControlWb)
+import Bittide.DoubleBufferedRam (ContentType (Blob), InitialContent (Reloadable))
+import Bittide.ProcessingElement (PeConfig (..), processingElement)
+import Bittide.ProcessingElement.Util (memBlobsFromElf)
+import Bittide.SharedTypes (ByteOrder (BigEndian))
+
+import Project.FilePath
+
+import Protocols
+import Protocols.Idle
+
+import Clash.Cores.Xilinx.Ila (Depth (..), IlaConfig (..), ila, ilaConfig)
+import Data.Maybe (fromMaybe, isJust)
+import VexRiscv


"Qualified deps" => we can do this with the latest fourmolu fourmolu/fourmolu#403. But, these changes haven't made it into a release yet. Something to keep in mind.

Edit: I was replying to Lucas here, but it seemed to be grouped in with my review..

martijnbastiaan · 2024-12-04T09:31:09Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+  , reframingEnabled :: Bool
+  -- ^ Whether or not to run this test with reframing enabled.


Create an issue we need to figure out how to properly test reframing and link it here.

Made #693, will link in the comment.

martijnbastiaan · 2024-12-04T09:32:02Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+   where
+    go ugn = ugnStable
+     where
+      ugn' = bitCoerce <$> ugn


Suggested change

ugn' = bitCoerce <$> ugn

ugn1 = bitCoerce <$> ugn

Use number postfixes please. Ticks don't really scale and are easy to miss.

martijnbastiaan · 2024-12-04T09:32:57Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+
+type WaitCycles dom hold prd = PeriodToCycles dom (prd - hold)
+
+data ToFincFdecState' dom hold min


It isn't clear what the ' is supposed to communicate.

It's there because I forgot you can import Foo hiding (bar). Will fix.

martijnbastiaan · 2024-12-04T09:33:16Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+  | Idle
+  deriving (Generic, NFDataX, Eq)
+
+speedChangeToFincFdec' ::


Same here. Typically ' is used for "strict" versions of a function, but that's not the case here.

martijnbastiaan · 2024-12-04T09:33:36Z

bittide-instances/src/Bittide/Instances/Hitl/SwCcTopologies.hs

+  syncNodePrevEnteredReset =
+    sticky sysClk testReset syncNodeEnteredReset
+
+  syncIn' = mux syncNodePrevEnteredReset syncIn (pure True :: Signal Basic125 Bool)


Suggested change

syncIn' = mux syncNodePrevEnteredReset syncIn (pure True :: Signal Basic125 Bool)

syncIn1 = mux syncNodePrevEnteredReset syncIn (pure True :: Signal Basic125 Bool)

Trying to change sync node reset detection to work better for FPGA 7. Fixes from rebasing on `rs/ilaPlot`. Forgot to sticky the CC output (again). Sending data over links to see what comes out on the other side. Adjusting ready detection/reporting. Using the MSB of link data to indicate not in CC reset. Fixes(?) to reset paths, ready reporting, and CC reset detection. Attempting to cut off testing sooner. Cutting off earlier was not the answer. Capturing calib vals differently now. UGN TESTING IS SO BACK

rslawson mentioned this pull request Oct 15, 2024

Software Clock Control Measurement and Improvements #644

Closed

rslawson force-pushed the rs/ilaPlot branch from b766d77 to c1f48f0 Compare October 17, 2024 14:40

rslawson force-pushed the rs/swCcTopologiesTest branch from 89ab9a9 to 24817b5 Compare October 17, 2024 20:53

rslawson force-pushed the rs/ilaPlot branch from c1f48f0 to 67d2191 Compare October 22, 2024 09:16

rslawson force-pushed the rs/swCcTopologiesTest branch from 10ad426 to d2a0587 Compare October 22, 2024 09:23

rslawson changed the base branch from rs/ilaPlot to staging October 22, 2024 09:30

rslawson changed the base branch from staging to rs/ilaPlot October 22, 2024 09:32

rslawson force-pushed the rs/ilaPlot branch from c240205 to 5af5933 Compare October 22, 2024 10:12

rslawson force-pushed the rs/swCcTopologiesTest branch 5 times, most recently from 79591c5 to 550add3 Compare October 23, 2024 11:37

rslawson force-pushed the rs/ilaPlot branch from 5af5933 to 76eead2 Compare October 24, 2024 21:38

rslawson force-pushed the rs/swCcTopologiesTest branch 5 times, most recently from b8eff1d to 10b23cf Compare October 28, 2024 23:57

rslawson force-pushed the rs/swCcTopologiesTest branch 4 times, most recently from 28c07d4 to 444d9aa Compare October 30, 2024 12:06

rslawson mentioned this pull request Oct 30, 2024

Topology tests - nodes should not be able to adjust to nodes that have not started #656

Closed

rslawson force-pushed the rs/swCcTopologiesTest branch 2 times, most recently from 94cc1bd to a039e5a Compare October 30, 2024 18:11

rslawson force-pushed the rs/swCcTopologiesTest branch from 3237e82 to e57b585 Compare November 4, 2024 15:32

rslawson force-pushed the rs/ilaPlot branch from bc92d27 to d62d4da Compare November 6, 2024 15:29

rslawson force-pushed the rs/swCcTopologiesTest branch from e57b585 to b559c96 Compare November 6, 2024 15:30

rslawson force-pushed the rs/ilaPlot branch from d62d4da to ff78307 Compare November 7, 2024 15:42

rslawson force-pushed the rs/swCcTopologiesTest branch from b559c96 to 49972a1 Compare November 7, 2024 15:43

rslawson force-pushed the rs/ilaPlot branch from ff78307 to fee80f6 Compare November 11, 2024 12:34

rslawson force-pushed the rs/swCcTopologiesTest branch from 49972a1 to 38be85a Compare November 11, 2024 12:35

Base automatically changed from rs/ilaPlot to staging November 15, 2024 08:25

rslawson force-pushed the rs/swCcTopologiesTest branch 2 times, most recently from f1740f4 to ed7574e Compare November 18, 2024 09:56

rslawson force-pushed the rs/swCcTopologiesTest branch 2 times, most recently from 3361c2f to c51feff Compare November 21, 2024 11:08

rslawson mentioned this pull request Nov 27, 2024

Implement fast/efficient execution deadlines in software #587

Open

lmbollen requested changes Dec 3, 2024

View reviewed changes

martijnbastiaan requested changes Dec 4, 2024

View reviewed changes

rslawson force-pushed the rs/swCcTopologiesTest branch from c51feff to a9ba56c Compare December 4, 2024 10:14

rslawson added 2 commits December 4, 2024 11:31

Adding new SwCc function.

ff01d56

Adding config type for SwCc.

80ae8ba

rslawson force-pushed the rs/swCcTopologiesTest branch 2 times, most recently from 8747cd4 to 74540bb Compare December 4, 2024 14:58

rslawson added 4 commits December 4, 2024 16:16

Made CI CC-type agnostic.

d001620

Small fix, formatting, and comment cleanup.

6439398

Remove obsoleted tests.

169c564

rslawson force-pushed the rs/swCcTopologiesTest branch from 74540bb to 169c564 Compare December 4, 2024 15:16

martijnbastiaan mentioned this pull request Dec 4, 2024

Make all GTH tx interfaces operate in the same domain #688

Merged

rslawson requested review from lmbollen and martijnbastiaan December 6, 2024 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in a software clock control DUT #654

Add in a software clock control DUT #654

rslawson commented Oct 15, 2024

rslawson commented Oct 15, 2024

rslawson commented Oct 16, 2024

rslawson commented Oct 17, 2024

rslawson commented Oct 29, 2024

rslawson commented Nov 18, 2024

rslawson commented Nov 20, 2024

lmbollen Nov 26, 2024

lmbollen Nov 26, 2024

lmbollen Nov 26, 2024

lmbollen Nov 26, 2024

lmbollen Nov 26, 2024

lmbollen Nov 26, 2024

lmbollen Nov 26, 2024

rslawson Dec 4, 2024 •

edited

Loading

lmbollen Nov 26, 2024

lmbollen Nov 26, 2024

rslawson Dec 4, 2024

lmbollen Dec 3, 2024

martijnbastiaan left a comment

martijnbastiaan Nov 13, 2024

rslawson Dec 4, 2024

martijnbastiaan Nov 27, 2024

martijnbastiaan Dec 4, 2024 •

edited

Loading

martijnbastiaan Dec 4, 2024

rslawson Dec 4, 2024

martijnbastiaan Dec 4, 2024

martijnbastiaan Dec 4, 2024

rslawson Dec 4, 2024

martijnbastiaan Dec 4, 2024

martijnbastiaan Dec 4, 2024

		, reframingEnabled :: Bool
		-- ^ Whether or not to run this test with reframing enabled.


		type WaitCycles dom hold prd = PeriodToCycles dom (prd - hold)

		data ToFincFdecState' dom hold min

	syncIn' = mux syncNodePrevEnteredReset syncIn (pure True :: Signal Basic125 Bool)
	syncIn1 = mux syncNodePrevEnteredReset syncIn (pure True :: Signal Basic125 Bool)

Add in a software clock control DUT #654

Are you sure you want to change the base?

Add in a software clock control DUT #654

Conversation

rslawson commented Oct 15, 2024

rslawson commented Oct 15, 2024

rslawson commented Oct 16, 2024

rslawson commented Oct 17, 2024

rslawson commented Oct 29, 2024

rslawson commented Nov 18, 2024

rslawson commented Nov 20, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rslawson Dec 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

martijnbastiaan left a comment

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

martijnbastiaan Dec 4, 2024 • edited Loading

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

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

rslawson Dec 4, 2024 •

edited

Loading

martijnbastiaan Dec 4, 2024 •

edited

Loading