Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First active edge at least one clock period from start #2007

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

christiaanb
Copy link
Member

@christiaanb christiaanb commented Nov 19, 2021

Fixes #2001

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@christiaanb
Copy link
Member Author

For reviewers: with the change to the DDR test the Clash simulation and the HDL simulation now actually correspond. Before this change, it used to be that in the Clash simulation, outputverifier would trigger an assertion; where this assertion wasn't triggered in HDL simulation. Now neither the Clash simulation nor the HDL simulation trigger an assertion.

@alex-mckenna
Copy link
Contributor

@christiaanb I unchecked the copyright notice TODO, I can see non-updated notices in at least clash-lib

@christiaanb
Copy link
Member Author

All the files that have a copyright notice were originally published in 2021, and the copyright header mentions 2021; so it's all good. I rechecked the copyright notice TODO

Copy link
Member

@leonschoorl leonschoorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was there a difference in the resetGen prims behaviour between synchronous and asynchrous domains?
And why is that not longer needed?


makeSystemVerilog :: IORef ClashOpts -> [FilePath] -> InputT GHCi ()
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend :: Int -> HdlSyn -> Bool -> PreserveCase -> Maybe (Maybe Int) -> AggressiveXOptBB -> RenderEnums -> SystemVerilogState)
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend :: Int -> HdlSyn -> Bool -> PreserveCase -> Maybe (Maybe Int) -> AggressiveXOptBB -> RenderEnums -> DomainMap -> SystemVerilogState)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend :: Int -> HdlSyn -> Bool -> PreserveCase -> Maybe (Maybe Int) -> AggressiveXOptBB -> RenderEnums -> DomainMap -> SystemVerilogState)
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend @SystemVerilogState)

Clash.hs Outdated

genVerilog
:: String
-> IO ()
genVerilog = doHDL (initBackend WORD_SIZE_IN_BITS HDLSYN True PreserveCase Nothing (AggressiveXOptBB False) (RenderEnums True) :: VerilogState)
genVerilog = doHDL (initBackend WORD_SIZE_IN_BITS HDLSYN True PreserveCase Nothing (AggressiveXOptBB False) (RenderEnums True) :: DomainMap -> VerilogState)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
genVerilog = doHDL (initBackend WORD_SIZE_IN_BITS HDLSYN True PreserveCase Nothing (AggressiveXOptBB False) (RenderEnums True) :: DomainMap -> VerilogState)
genVerilog = doHDL (initBackend @VerilogState WORD_SIZE_IN_BITS HDLSYN True PreserveCase Nothing (AggressiveXOptBB False) (RenderEnums True))


makeSystemVerilog :: Ghc () -> IORef ClashOpts -> [(String, Maybe Phase)] -> Ghc ()
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend :: Int -> HdlSyn -> Bool -> PreserveCase -> Maybe (Maybe Int) -> AggressiveXOptBB -> RenderEnums -> SystemVerilogState)
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend :: Int -> HdlSyn -> Bool -> PreserveCase -> Maybe (Maybe Int) -> AggressiveXOptBB -> RenderEnums -> DomainMap -> SystemVerilogState)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend :: Int -> HdlSyn -> Bool -> PreserveCase -> Maybe (Maybe Int) -> AggressiveXOptBB -> RenderEnums -> DomainMap -> SystemVerilogState)
makeSystemVerilog = makeHDL' (Clash.Backend.initBackend @SystemVerilogState)

@christiaanb
Copy link
Member Author

christiaanb commented Jan 28, 2022

The difference has always bogus, we used to have:

, { "BlackBox" :
    { "name"      : "Clash.Signal.Internal.asyncResetGen"
    , "kind" : "Declaration"
    , "type" :
"asyncResetGen :: Reset domain 'Asynchronous"
    , "template" :
"-- pragma translate_off
~RESULT <= '1',
           '0' after 2000 ps;
-- pragma translate_on"
    }
  }
, { "BlackBox" :
    { "name"      : "Clash.Signal.Internal.syncResetGen"
    , "kind" : "Declaration"
    , "type" :
"syncResetGen :: ( domain ~ 'Dom n clkPeriod
                 , KnownNat clkPeriod )
              => Reset domain 'Synchronous"
    , "template" :
"-- pragma translate_off
~RESULT <= '1',
           '0' after (2999 ps + ~LIT[1]0 ps);
-- pragma translate_on"
    }
  }

then in #428 we changed to:

, { "BlackBox" :
    { "name"      : "Clash.Signal.Internal.asyncResetGen"
    , "workInfo"  : "Always"
    , "kind" : "Declaration"
    , "type" :
"asyncResetGen :: Reset domain 'Asynchronous"
    , "template" :
"-- asyncResetGen begin
-- pragma translate_off
~RESULT <= '1',
           '0' after 3001 ps;
-- pragma translate_on
-- asyncResetGen end"
    }
  }
, { "BlackBox" :
    { "name"      : "Clash.Signal.Internal.syncResetGen"
    , "workInfo"  : "Always"
    , "kind" : "Declaration"
    , "type" :
"syncResetGen :: ( domain ~ 'Dom n clkPeriod
                 , KnownNat clkPeriod )
              => Reset domain 'Synchronous"
    , "template" :
"-- syncResetGen begin
-- pragma translate_off
~RESULT <= '1',
           '0' after (2999 ps + ~LIT[1] ps);
-- pragma translate_on
-- syncResetGen end"
    }
  }

so that the de-assertion always happened after 3000ps (which is the time all the clocks start (before this PR)).

Then in #527 those definitions got merged:

, { "BlackBox" :
    { "name"      : "Clash.Signal.Internal.resetGen"
    , "workInfo"  : "Always"
    , "kind" : "Declaration"
    , "type" :
"resetGen :: KnownDomain dom      => Reset dom"
    , "template" :
"-- resetGen begin
-- pragma translate_off
~RESULT <= ~IF ~ISACTIVEHIGH[0] ~THEN '1' ~ELSE '0' ~FI,
           ~IF ~ISACTIVEHIGH[0] ~THEN '0' ~ELSE '1' ~FI after ~IF~ISSYNC[0]~THEN(2999 ps + ~PERIOD[0] ps)~ELSE3001 ps~FI;
-- pragma translate_on
-- resetGen end"
    }
  }

By which time we should've already dropped the difference. Because both 3001ps and 2999ps + PERIOD[0]ps deassert before the second active edge; i.e. we cannot observe their difference.

@DigitalBrains1 DigitalBrains1 added this to the 1.6 milestone Feb 2, 2022
Copy link
Member

@leonschoorl leonschoorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First active edge of clock in testbench not half of clock period
5 participants