Pulse-width modulation module

Question

Module for generating a PWM signal. The req_value_i input gets a duration value of the signal. Furthermore, the module can be stopped by deassertion of the enable_i input.

`timescale 1ns / 1ps
    
 module pwm #
(
    parameter integer PWM_COUNTER_WIDTH = 8
)
( 
    input  logic                             clk_i,
    input  logic                             s_rst_n_i,
    input  logic                             enable_i,
        
    input  logic [PWM_COUNTER_WIDTH - 1 : 0] req_value_i,
    
    output logic                             channel_o  
    );
    
    logic [PWM_COUNTER_WIDTH - 1 : 0] counter;
        
    always_comb begin
        channel_o = (req_value_i != counter) ? 1'h1 : 1'h0;
    end
        
    always_ff @ (posedge clk_i) begin
        if (1'h0 == s_rst_n_i) begin
            counter <= {PWM_COUNTER_WIDTH{1'h0}};
        end
        else if (1'h1 == enable_i) begin
            counter <= counter + 1'h1;
        end
    end
    
endmodule

Answer 1

The layout of your code is consistent and easy to read. I don't see any functional problems.

One consideration for all digital logic is whether you can tolerate glitches on your outputs. If your PWM output is directly driving a load such as a power inverter, you likely want to avoid glitches. To avoid glitches, change your combinational output to a registered output:

always_ff @ (posedge clk_i) begin
    if (1'h0 == s_rst_n_i) begin
        channel_o <= 1'h0;
    end
    else begin
        channel_o <= (req_value_i != counter);
    end
end

I also simplified the expression by removing the ternary code ? 1'h1 : 1'h0. But, this is just a matter of coding style preference.

Another consideration is whether to use a synchronous reset (as you have) or an asynchronous reset. The advantage of asynchronous is that you don't need to have the clock toggling in order to reset your logic. For asynchronous, you would use:

always_ff @ (posedge clk_i or negedge s_rst_n_i) begin