Off-By-1 error in glasgow.gateware.clockgen.ClockGen causes incorrect clock frequency to be generated
gateware
### 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:
```python
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_
关闭于 2025-08-14 2 条评论