From 12a1126a44fcc6686bf2d72eb99c41c2808d9f47 Mon Sep 17 00:00:00 2001 From: Antoine van Gelder Date: Thu, 6 Jun 2024 14:46:20 +0200 Subject: [PATCH 1/3] gateware.cdc: fix an order-of-operations bug with synchronize where output signals would not be skipped if they were of Pin type --- luna/gateware/utils/cdc.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/luna/gateware/utils/cdc.py b/luna/gateware/utils/cdc.py index cc11dbac..3fc48960 100644 --- a/luna/gateware/utils/cdc.py +++ b/luna/gateware/utils/cdc.py @@ -53,6 +53,11 @@ def create_synchronizer(signal, output): # Otherwise, we'll need to make sure we only synchronize # elements with non-output directions. for name, layout, direction in signal.layout: + # Skip any output elements, as they're already + # in our clock domain, and we don't want to drive them. + if (direction == DIR_FANOUT) or (hasattr(signal[name], 'o') and ~hasattr(signal[name], 'i')): + m.d.comb += signal[name].eq(output[name]) + continue # If this is a record itself, we'll need to recurse. if isinstance(signal[name], (Record, Pin)): @@ -60,12 +65,6 @@ def create_synchronizer(signal, output): o_domain=o_domain, stages=stages) continue - # Skip any output elements, as they're already - # in our clock domain, and we don't want to drive them. - if (direction == DIR_FANOUT) or (hasattr(signal[name], 'o') and ~hasattr(signal[name], 'i')): - m.d.comb += signal[name].eq(output[name]) - continue - m.submodules += create_synchronizer(signal[name], output[name]) return output From b041f3b229ce9709c78b46445055aa3df6412bcf Mon Sep 17 00:00:00 2001 From: Antoine van Gelder Date: Fri, 7 Jun 2024 16:57:15 +0200 Subject: [PATCH 2/3] gateware.cdc: handle output elements for Record and Pin separately --- luna/gateware/utils/cdc.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/luna/gateware/utils/cdc.py b/luna/gateware/utils/cdc.py index 3fc48960..afdabd2f 100644 --- a/luna/gateware/utils/cdc.py +++ b/luna/gateware/utils/cdc.py @@ -55,9 +55,12 @@ def create_synchronizer(signal, output): for name, layout, direction in signal.layout: # Skip any output elements, as they're already # in our clock domain, and we don't want to drive them. - if (direction == DIR_FANOUT) or (hasattr(signal[name], 'o') and ~hasattr(signal[name], 'i')): + if (direction == DIR_FANOUT): m.d.comb += signal[name].eq(output[name]) continue + elif hasattr(signal[name], 'o') and ~hasattr(signal[name], 'i'): + m.d.comb += signal[name].o.eq(output[name]) + continue # If this is a record itself, we'll need to recurse. if isinstance(signal[name], (Record, Pin)): From 2d6df09e988d8093b21e57b78c1a0cc6954d4b3e Mon Sep 17 00:00:00 2001 From: Antoine van Gelder Date: Fri, 7 Jun 2024 16:58:01 +0200 Subject: [PATCH 3/3] gateware.cdc: add a test case that exercises cdc.synchronize() on Pin elements --- tests/test_cdc.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_cdc.py b/tests/test_cdc.py index 2852ac25..628b792c 100644 --- a/tests/test_cdc.py +++ b/tests/test_cdc.py @@ -8,6 +8,7 @@ from unittest import TestCase from amaranth import Module, Record, Signal +from amaranth.lib.io import Pin from amaranth.hdl.rec import DIR_FANIN, DIR_FANOUT from luna.gateware.utils.cdc import synchronize, stretch_strobe_signal @@ -39,6 +40,17 @@ def test_nested_record(self): ]) synchronize(m, record) + def test_pins(self): + m = Module() + pins = { + "sig_in": Pin(1, "i"), + "sig_out": Pin(1, "o"), + } + record = Record([ + (f_name, f.layout) for (f_name, f) in pins.items() + ], fields=pins) + synchronize(m, record) + class StrobeStretcherTest(LunaGatewareTestCase): """ Test case for our strobe stretcher function. """