Off-By-1 error in glasgow.gateware.clockgen.ClockGen causes incorrect clock frequency to be generated #1007
Description
Version
Glasgow 0.1.dev2629+g9205dde.d20250811 (CPython 3.13.5 on macOS-15.6-x86_64-i386-64bit-Mach-O)
Description
Not sure if ClockGen is still part of the Glasgow API, as it doesn't seem to be widely used. Happy for this bug to be closed WONTFIX if it's being deprecated. Also happy to write a PR to fix this if ClockGen is going to be hanging around.
The "common" case of cyc >= 2 contains the following code:
if self.cyc >= 2:
# General case.
# Implementation: counter.
counter = Signal(range(self.cyc))
clk_r = Signal()
m.d.sync += counter.eq(counter - 1)
with m.If(counter == 0):
m.d.sync += counter.eq(self.cyc - 1)
with m.If(counter == self.cyc // 2):
m.d.sync += self.clk.eq(1)
with m.If(counter == 0):
m.d.sync += self.clk.eq(0)
m.d.sync += clk_r.eq(self.clk)
m.d.comb += [
self.stb_r.eq(~clk_r & self.clk),
self.stb_f.eq(clk_r & ~self.clk),
]The off-by-one issue is because counter = Signal(range(self.cyc)) produces a range of 0 to (self.cyc - 1), i.e. for the case self.cyc == 2 the counter can only hold values 0 or 1. This ends up being a half-divider because when counter == 0 it is reloaded with self.cyc - 1 == 1, and the next tick the counter is decremented and is now 0 so the counter is reloaded. This can be fixed by increasing the size of counter by 1 i.e. counter = Signal(range(self.cyc) + 1) and reloading with self.cyc i.e. m.d.sync += counter.eq(self.cyc). This issue is visible in simulation, and can also be observed with an oscilloscope that supports frequency measurement when clk is hooked up to a GPIO pin. The error is more significant at lower divider values (higher frequency) with the worst case being a divider value of 2 producing 24MHz instead of the expected 16MHz from Glasgow's source 48MHz clock.
However if the fix is applied, then the calculation for the half-way point of the tick at with m.If(counter == self.cyc // 2) is now off-by-one, which can be fixed by adding 1 to self.cyc in the statement i.e. with m.If(counter == (self.cyc + 1) // 2). I have verified this fix in simulation and by measuring with an oscilloscope.
Log file
No response