PWM Peripheral - #1803
Conversation
| // | ||
| // WALLY-pwm | ||
| // | ||
| // Author: Naiche Whyte-Aguayo naichewa@gmail.com> |
There was a problem hiding this comment.
Tests should have some introductory text explaining what they are testing. The reference_output of pwm-01.S suggests it isn't testing much. Is it worth keeping this one?
Most of the test is commented out. Is it worth keeping the commented out parts? If so, explain why they are there.
All the features of the module should be fully tested in a self-checking fashion.
Same comments on pwms.
| // Purpose: PWM module | ||
| // https://docs.google.com/document/d/1erHBVchBtwmgZ0bCNjb88spYfN7CpRbhmSNFH6cO8CY/edit?tab=t.0 | ||
| // | ||
| // Documentation: RISC-V System on Chip Design |
There was a problem hiding this comment.
Not documented in the book.
| // Modified: | ||
| // | ||
| // Purpose: PWM module | ||
| // https://docs.google.com/document/d/1erHBVchBtwmgZ0bCNjb88spYfN7CpRbhmSNFH6cO8CY/edit?tab=t.0 |
There was a problem hiding this comment.
This URL doesn't describe the PWM.
| // https://docs.google.com/document/d/1erHBVchBtwmgZ0bCNjb88spYfN7CpRbhmSNFH6cO8CY/edit?tab=t.0 | ||
| // | ||
| // Documentation: RISC-V System on Chip Design | ||
| // Based on PWM design from SiFive 5U540-C000 manual version 1.0 |
There was a problem hiding this comment.
Is this the FU540 chip, not 5U540? Include a link i possible, such as https://pdos.csail.mit.edu/6.828/2025/readings/FU540-C000-v1.0.pdf
or one to the SiFive site if available.
| assign PWMCountPrescaled = ((Memwrite & (Entry == PWM_COUNT)) ? Din[30:0] : PWMCountIncrement) >> PWMScale; | ||
|
|
||
|
|
||
| // Combinatorial signal logic |
| end else if (Memwrite & (Entry == PWM_COUNT)) begin | ||
| PWMCount <= Din[30:0]; | ||
| PWMScaled <= PWMCountPrescaled[15:0]; | ||
| end else if (Memwrite & (Entry == PWM_S))PWMScaled <= Din[15:0]; |
| PWMScaled <= PWMCountPrescaled[15:0]; | ||
| end | ||
|
|
||
| flop #(1) pwmholdreg(PCLK, |
There was a problem hiding this comment.
weird line break. Several places
| PWMHoldIn, PWMHoldOut); | ||
|
|
||
| //pwm comparators | ||
| assign PWMDeglitchMux[0] = PWMScaled[15] & PWMCompareCenter[0]; |
There was a problem hiding this comment.
This needs a better name. It is not a mux, and is also an input to an xor.
| //pwm comparators | ||
| assign PWMDeglitchMux[0] = PWMScaled[15] & PWMCompareCenter[0]; | ||
| assign PWMCompareXNOR[15:0] = PWMDeglitchMux[0] ? ~PWMScaled : PWMScaled; | ||
| assign PWMCompareBoolean[0] = PWMCompareXNOR[15:0] >= PWMCompare0; |
There was a problem hiding this comment.
Rename. Boolean isn't a helpful name for a binary signal.
| always_ff @(posedge PCLK) | ||
| if (PWMCountReset | ~PRESETn) begin | ||
| PWMCount <= 31'b0; | ||
| PWMScaled <= 16'b0; |
There was a problem hiding this comment.
I'm not clear whether pwmscaled needs to be a separate 16-bit register, or if it can just be the result of combinational logic shifting pwmcount by up to 15. Although it is drawn as a register in Fig 6, it looks like it could be combinational because it doesn't hold any new state.
There was a problem hiding this comment.
Spec has it as an acessible read/write-able register, although I agree other than following that it could be combinational. I also don't see why you would want to write to it and not PWM Count
Parameterized, streamlined + extra tests, Scaled combinational, formatting and variable names
2 participants
Adds a PWM peripheral with 4 16 bit comparators and 2 tests. WALLY-pwm-01.S contains "dumb" functions that wait the estimated duration of X pwm cycles, allowing for tests by inspection. WALLY-pwms-01.S counts the number of high/low cycles and compares to reference