Merge pull request #2833 from clash-lang/fix-oddr

Fix DDR primitives

Author: DigitalBrains1

changelog/2024-12-14T13_35_37+01_00_fix_ddr_prims

@@ -0,0 +1,11 @@
+FIXED: `Clash.Explicit.DDR`:
+  - `ddrIn`: VHDL: Remove data input from sensitivity list of `ddrIn_neg_latch` register as it is superfluous. This should not affect functionality.
+  - `ddrOut`: VHDL: Fix incorrect usage of `Enable` input when the domain is set to asynchronous resets. Deasserting the `Enable` exhibited wrong behavior before this fix.
+FIXED: `Clash.Xilinx.DDR`:
+  - These primitives only support clocks where the rising edge is the active edge. Using them in a domain with falling active edges now causes an error.
+  - `oddr`: Fix VHDL and SystemVerilog erroring out during HDL generation
+  - Symbols in HDL for both `iddr` and `oddr` were renamed to match their function.
+FIXED: `Clash.Intel.DDR`:
+  - These primitives only support clocks where the rising edge is the active edge. Using them in a domain with falling active edges now causes an error.
+  - Fix rendering HDL. It variously errored out or generated non-working HDL.
+  - Rendering HDL no longer causes Clash to issue a warning about an argument unused in Haskell but used in the primitive black box.

changelog/2025-01-22T16_23_00+01_00_improve_ddr

@@ -0,0 +1,4 @@
+CHANGED: `Clash.Explicit.DDR`, `Clash.Intel.DDR` and `Clash.Xilinx.DDR`:
+  - The constraints for the functions have been rewritten to use `DomainPeriod`, which makes them more readable and relaxes unnecessary constraints on the virtual DDR domain. The type-level variables for the domains have been renamed. `dom` is a real domain, `domDDR` is the virtual DDR domain.
+  - The Xilinx and Intel primitives only support domains where the rising edge is the active edge. This is now enforced at the type level by adding a constraint.
+  - The Xilinx and Intel primitives now directly support any data type that has a `BitPack` instance.

clash-lib/prims/commonverilog/Clash_Intel_DDR.primitives.yaml

@@ -5,39 +5,42 @@
     - altera_mf
     type: |-
       altddioOut#
-        :: ( HasCallStack             -- ARG[0]
-           , KnownConfi~ fast domf    -- ARG[1]
-           , KnownConfi~ slow doms    -- ARG[2]
-           , KnownNat m )             -- ARG[3]
-        => SSymbol deviceFamily       -- ARG[4]
-        -> Clock slow                 -- ARG[5]
-        -> Reset slow                 -- ARG[6]
-        -> Enable slow                -- ARG[7]
-        -> Signal slow (BitVector m)  -- ARG[8]
-        -> Signal slow (BitVector m)  -- ARG[9]
-        -> Signal fast (BitVector m)
+        :: forall deviceFamily n dom domDDR
+         . HasCallStack              -- ARG[0]
+        => KnownDomain dom           -- ARG[1]
+        => KnownDomain domDDR        -- ARG[2]
+        => DomPeriod ~ 2 * ...       -- ARG[3]
+        => DomEdge ~ Rising          -- ARG[4]
+        => KnownNat n                -- ARG[5]
+        => SSymbol deviceFamily      -- ARG[6]
+        -> Clock dom                 -- ARG[7]
+        -> Reset dom                 -- ARG[8]
+        -> Enable dom                -- ARG[9]
+        -> Signal dom (BitVector n)  -- ARG[10]
+        -> Signal dom (BitVector n)  -- ARG[11]
+        -> Signal domDDR (BitVector n)
     template: |-
       // altddioOut begin
       altddio_out
         #(
           .extend_oe_disable ("OFF"),
-          .intended_device_family (~LIT[4]),
+          .intended_device_family (~LIT[6]),
           .invert_output ("OFF"),
           .lpm_hint ("UNUSED"),
           .lpm_type ("altddio_out"),
           .oe_reg ("UNREGISTERED"),
           .power_up_high ("OFF"),
           .width (~SIZE[~TYPO])
         )
-        ~GENSYM[~COMPNAME_ALTDDIO_OUT][7] (~IF ~ISSYNC[2] ~THEN
-          .sclr (~ARG[6]),
+        ~GENSYM[~COMPNAME_ALTDDIO_OUT][7] (~IF ~ISSYNC[1] ~THEN
+          .sclr (~ARG[8]),
           .aclr (1'b0),~ELSE
-          .aclr (~ARG[6]),
+          .aclr (~ARG[8]),
           .sclr (1'b0),~FI
-          .datain_h (~ARG[8]),
-          .datain_l (~ARG[9]),
-          .outclock (~ARG[5]),
-          .outclocken (~IF ~ISACTIVEENABLE[7] ~THEN ~ARG[7] ~ELSE 1'b1 ~FI),
+          .datain_h (~ARG[10]),
+          .datain_l (~ARG[11]),
+          .outclock (~ARG[7]),
+          .outclocken (~IF ~ISACTIVEENABLE[9] ~THEN ~ARG[9] ~ELSE 1'b1 ~FI),
           .dataout (~RESULT),
           .aset (1'b0),
           .sset (1'b0),

clash-lib/prims/systemverilog/Clash_Explicit_DDR.primitives.yaml

@@ -2,42 +2,43 @@
     name: Clash.Explicit.DDR.ddrIn#
     kind: Declaration
     type: |-
-      ddrIn# :: forall a slow fast n pFast gated synchronous.
-                 ( HasCallStack          -- ARG[0]
-                 , NFDataX a             -- ARG[1]
-                 , KnownConfi~ fast domf -- ARG[2]
-                 , KnownConfi~ slow doms -- ARG[3]
-              => Clock slow              -- ARG[4], clk
-              -> Reset slow              -- ARG[5], rst
-              -> Enable slow             -- ARG[6], en
-              -> a                       -- ARG[7]
-              -> a                       -- ARG[8]
-              -> a                       -- ARG[9]
-              -> Signal fast a           -- ARG[10]
-              -> Signal slow (a,a)
+      ddrIn# :: forall a dom domDDR
+              . HasCallStack             -- ARG[0]
+             => NFDataX a                -- ARG[1]
+             => KnownDomain dom          -- ARG[2]
+             => KnownDomain domDDR       -- ARG[3]
+             => DomPeriod ~ 2 * ...      -- ARG[4]
+             => Clock dom                -- ARG[5]
+             -> Reset dom                -- ARG[6]
+             -> Enable dom               -- ARG[7]
+             -> a                        -- ARG[8]
+             -> a                        -- ARG[9]
+             -> a                        -- ARG[10]
+             -> Signal domDDR a          -- ARG[11]
+             -> Signal dom (a,a)
     template: |-
       // ddrIn begin
-      ~SIGD[~GENSYM[data_Pos][1]][9];
-      ~SIGD[~GENSYM[data_Neg][2]][9];
-      ~SIGD[~GENSYM[data_Neg_Latch][3]][9];
-      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[4]~IF~ISSYNC[3]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[5])~FI begin : ~GENSYM[~COMPNAME_ddrIn_pos][6]
-        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[5]~ELSE! ~ARG[5]~FI) begin
-          ~SYM[1] <= ~ARG[8];
-        end else ~IF ~ISACTIVEENABLE[6] ~THEN if (~ARG[6]) ~ELSE ~FI begin
-          ~SYM[1] <= ~ARG[10];
+      ~SIGD[~GENSYM[data_Pos][1]][11];
+      ~SIGD[~GENSYM[data_Neg][2]][11];
+      ~SIGD[~GENSYM[data_Neg_Latch][3]][11];
+      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[5]~IF~ISSYNC[2]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[6])~FI begin : ~GENSYM[~COMPNAME_ddrIn_pos][6]
+        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[6]~ELSE! ~ARG[6]~FI) begin
+          ~SYM[1] <= ~ARG[9];
+        end else ~IF ~ISACTIVEENABLE[7] ~THEN if (~ARG[7]) ~ELSE ~FI begin
+          ~SYM[1] <= ~ARG[11];
         end
       end
-      always @(~IF~ACTIVEEDGE[Rising][2]~THENnegedge~ELSEposedge~FI ~ARG[4]~IF~ISSYNC[3]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[5])~FI begin : ~GENSYM[~COMPNAME_ddrIn_neg][7]
-        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[5]~ELSE! ~ARG[5]~FI) begin
-          ~SYM[2] <= ~ARG[9];
-        end else ~IF ~ISACTIVEENABLE[6] ~THEN if (~ARG[6]) ~ELSE ~FI begin
+      always @(~IF~ACTIVEEDGE[Rising][2]~THENnegedge~ELSEposedge~FI ~ARG[5]~IF~ISSYNC[2]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[6])~FI begin : ~GENSYM[~COMPNAME_ddrIn_neg][7]
+        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[6]~ELSE! ~ARG[6]~FI) begin
           ~SYM[2] <= ~ARG[10];
+        end else ~IF ~ISACTIVEENABLE[7] ~THEN if (~ARG[7]) ~ELSE ~FI begin
+          ~SYM[2] <= ~ARG[11];
         end
       end
-      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[4]~IF~ISSYNC[3]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[5])~FI begin : ~GENSYM[~COMPNAME_ddrIn_neg_latch][8]
-        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[5]~ELSE! ~ARG[5]~FI) begin
-          ~SYM[3] <= ~ARG[7];
-        end else ~IF ~ISACTIVEENABLE[6] ~THEN if (~ARG[6]) ~ELSE ~FI begin
+      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[5]~IF~ISSYNC[2]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[6])~FI begin : ~GENSYM[~COMPNAME_ddrIn_neg_latch][8]
+        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[6]~ELSE! ~ARG[6]~FI) begin
+          ~SYM[3] <= ~ARG[8];
+        end else ~IF ~ISACTIVEENABLE[7] ~THEN if (~ARG[7]) ~ELSE ~FI begin
           ~SYM[3] <= ~SYM[2];
         end
       end
@@ -49,38 +50,40 @@
     kind: Declaration
     outputUsage: Blocking
     type: |-
-      ddrOut# :: ( HasCallStack                -- ARG[0]
-                  , NFDataX a                  -- ARG[1]
-                  , KnownConfi~ fast domf      -- ARG[2]
-                  , KnownConfi~ slow doms      -- ARG[3]
-               => Clock slow                   -- ARG[4]
-               -> Reset slow                   -- ARG[5]
-               -> Enable slow                  -- ARG[6]
-               -> a                            -- ARG[7]
-               -> Signal slow a                -- ARG[8]
-               -> Signal slow a                -- ARG[9]
-               -> Signal fast a
+      ddrOut# :: forall a dom domDDR
+               . HasCallStack                 -- ARG[0]
+              => NFDataX a                    -- ARG[1]
+              => KnownDomain dom              -- ARG[2]
+              => KnownDomain domDDR           -- ARG[3]
+              => DomPeriod ~ 2 * ...          -- ARG[4]
+              => Clock dom                    -- ARG[5]
+              -> Reset dom                    -- ARG[6]
+              -> Enable dom                   -- ARG[7]
+              -> a                            -- ARG[8]
+              -> Signal dom a                 -- ARG[9]
+              -> Signal dom a                 -- ARG[10]
+              -> Signal domDDR a
     template: |-
       // ddrOut begin
-      ~SIGD[~GENSYM[data_Pos][1]][7];
-      ~SIGD[~GENSYM[data_Neg][2]][7];
-      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[4]~IF~ISSYNC[3]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[5])~FI begin : ~GENSYM[~COMPNAME_ddrOut_pos][5]
-        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[5]~ELSE! ~ARG[5]~FI) begin
-          ~SYM[1] <= ~ARG[7];
-        end else ~IF ~ISACTIVEENABLE[6] ~THEN if (~ARG[6]) ~ELSE ~FI begin
+      ~SIGD[~GENSYM[data_Pos][1]][8];
+      ~SIGD[~GENSYM[data_Neg][2]][8];
+      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[5]~IF~ISSYNC[2]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[6])~FI begin : ~GENSYM[~COMPNAME_ddrOut_pos][5]
+        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[6]~ELSE! ~ARG[6]~FI) begin
           ~SYM[1] <= ~ARG[8];
+        end else ~IF ~ISACTIVEENABLE[7] ~THEN if (~ARG[7]) ~ELSE ~FI begin
+          ~SYM[1] <= ~ARG[9];
         end
       end
-      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[4]~IF~ISSYNC[3]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[5])~FI begin : ~GENSYM[~COMPNAME_ddrOut_neg][6]
-        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[5]~ELSE! ~ARG[5]~FI) begin
-          ~SYM[2] <= ~ARG[7];
-        end else ~IF ~ISACTIVEENABLE[6] ~THEN if (~ARG[6]) ~ELSE ~FI begin
-          ~SYM[2] <= ~ARG[9];
+      always @(~IF~ACTIVEEDGE[Rising][2]~THENposedge~ELSEnegedge~FI ~ARG[5]~IF~ISSYNC[2]~THEN)~ELSE or ~IF~ISACTIVEHIGH[2]~THENposedge~ELSEnegedge~FI ~ARG[6])~FI begin : ~GENSYM[~COMPNAME_ddrOut_neg][6]
+        if (~IF~ISACTIVEHIGH[2]~THEN~ARG[6]~ELSE! ~ARG[6]~FI) begin
+          ~SYM[2] <= ~ARG[8];
+        end else ~IF ~ISACTIVEENABLE[7] ~THEN if (~ARG[7]) ~ELSE ~FI begin
+          ~SYM[2] <= ~ARG[10];
         end
       end
 
       always @(*) begin
-        if (~ARG[4]) begin
+        if (~ARG[5]) begin
           ~RESULT = ~IF~ACTIVEEDGE[Rising][2]~THEN~SYM[1]~ELSE~SYM[2]~FI;
         end else begin
           ~RESULT = ~IF~ACTIVEEDGE[Rising][2]~THEN~SYM[2]~ELSE~SYM[1]~FI;

clash-lib/prims/systemverilog/Clash_Intel_DDR.primitives.yaml

@@ -1,42 +1,45 @@
 - BlackBox:
-    name: Clash.Intel.DDR.altddioIn
+    name: Clash.Intel.DDR.altddioIn#
     kind: Declaration
     libraries:
     - altera_mf
     type: |-
-      altddioIn
-        :: ( HasCallStack               -- ARG[0]
-           , KnownConfi~ fast domf      -- ARG[1]
-           , KnownConfi~ slow doms      -- ARG[2]
-           , KnownNat m )               -- ARG[3]
-        => SSymbol deviceFamily         -- ARG[4]
-        -> Clock slow                   -- ARG[5]
-        -> Reset slow                   -- ARG[6]
-        -> Enable slow                  -- ARG[7]
-        -> Signal fast (BitVector m)    -- ARG[8]
-        -> Signal slow (BitVector m,BitVector m)
+      altddioIn#
+        :: forall deviceFamily n dom domDDR
+         . HasCallStack                 -- ARG[0]
+        => KnownDomain dom              -- ARG[1]
+        => KnownDomain domDDR           -- ARG[2]
+        => DomPeriod ~ 2 * ...          -- ARG[3]
+        => DomEdge ~ Rising             -- ARG[4]
+        => KnownNat n                   -- ARG[5]
+        => SSymbol deviceFamily         -- ARG[6]
+        -> Clock dom                    -- ARG[7]
+        -> Reset dom                    -- ARG[8]
+        -> Enable dom                   -- ARG[9]
+        -> Signal domDDR (BitVector n)  -- ARG[10]
+        -> Signal dom (BitVector n, BitVector n)
     template: |-
       // altddioIn begin
-      ~SIGD[~GENSYM[dataout_l][1]][8];
-      ~SIGD[~GENSYM[dataout_h][2]][8];
+      ~SIGD[~GENSYM[dataout_l][1]][10];
+      ~SIGD[~GENSYM[dataout_h][2]][10];
 
       altddio_in
         #(
-          .intended_device_family (~LIT[4]),
+          .intended_device_family (~LIT[6]),
           .invert_input_clocks ("OFF"),
           .lpm_hint ("UNUSED"),
           .lpm_type ("altddio_in"),
           .power_up_high ("OFF"),
-          .width (~SIZE[~TYP[8]])
+          .width (~SIZE[~TYP[10]])
         )
-        ~GENSYM[~COMPNAME_ALTDDIO_IN][7] (~IF ~ISSYNC[2] ~THEN
-          .sclr (~ARG[6]),
+        ~GENSYM[~COMPNAME_ALTDDIO_IN][7] (~IF ~ISSYNC[1] ~THEN
+          .sclr (~ARG[8]),
           .aclr (1'b0),~ELSE
-          .aclr (~ARG[6]),
+          .aclr (~ARG[8]),
           .sclr (1'b0),~FI
-          .datain (~ARG[8]),
-          .inclock (~ARG[5]),
-          .inclocken (~IF ~ISACTIVEENABLE[7] ~THEN~ARG[7]~ELSE1'b1,~FI),
+          .datain (~ARG[10]),
+          .inclock (~ARG[7]),
+          .inclocken (~IF ~ISACTIVEENABLE[9] ~THEN~ARG[9]~ELSE1'b1~FI),
           .dataout_h (~SYM[2]),
           .dataout_l (~SYM[1]),
           .aset (1'b0),