m-labs / migen

A Python toolbox for building complex digital hardware
https://m-labs.hk/migen
Other
1.23k stars 210 forks source link

Invalid synthesis output for TSTriple width > 1 (Vivado) #266

Closed kaolpr closed 2 years ago

kaolpr commented 2 years ago

I've came across a strange behaviour of Vivado and proposed PR is a workaround for that.

As an example - let's have two separate inouts that we want to enable with one signal using TSTriple:

class TopBad(Module):

    def __init__(self):
        self.oe = Signal()
        self.cs_a_n = Signal()
        self.cs_b_n = Signal()

        # # #

        cs = TSTriple(2)
        port = Cat(self.cs_a_n, self.cs_b_n)
        self.specials += cs.get_tristate(port)
        self.comb += cs.oe.eq(self.oe)

Above translates to the following Verilog:

/* Machine-generated using Migen */
module top(
    input oe,
    inout cs_a_n,
    inout cs_b_n
);

reg [1:0] o = 2'd0;
wire oe1;
wire [1:0] i;

// synthesis translate_off
reg dummy_s;
initial dummy_s <= 1'd0;
// synthesis translate_on

assign oe1 = oe;

assign {cs_b_n, cs_a_n} = oe1 ? o : 2'bz;
assign i = {cs_b_n, cs_a_n};

endmodule

What gives the following schematics after synthesis in Vivado (2021.1): image

Not exactly what was intended (actually this behavior was indirectly reported in #192).

It can be avoided by declaring two signals of interest as a vector:

class TopGood(Module):

    def __init__(self):
        self.oe = Signal()
        self.cs_n = Signal(2)

        # # #

        cs = TSTriple(2)        
        self.specials += cs.get_tristate(self.cs_n)
        self.comb += cs.oe.eq(self.oe)

Verilog for above:

/* Machine-generated using Migen */
module top(
    input oe,
    inout [1:0] cs_n
);

reg [1:0] o = 2'd0;
wire oe1;
wire [1:0] i;

// synthesis translate_off
reg dummy_s;
initial dummy_s <= 1'd0;
// synthesis translate_on

assign oe1 = oe;

assign cs_n = oe1 ? o : 2'bz;
assign i = cs_n;

endmodule

And synthesis result:

image

Of course we can say that one can just use vectors. However, this can be pretty misleading. Even taking into account that the rule is to instantiate tristates only in top entity, as we clearly are in the top entity. I also did not notice any warning from synthesis engine.

I came across that issue while trying to instantiate single MiSoC SPI master with generic interface (SPIInterface) for the setup with nCS lines defined as two different 1-bit ports (what is pretty legible and would prefer to keep it that way).

It also took me a while to figure out, as Verilog looks basically fine. Only after looking into synthesized design I've noticed lack of output buffers for some of my ports.

It can be changed in SPIInterface class (so that every CS lines gets its own TSTriple) and that's the workaround I intend to use for now, but maybe it could be modified at Migen level? I'm thinking of modification that could output something like that:

/* Machine-generated using Migen */
module top(
    input oe,
    inout cs_a_n,
    inout cs_b_n
);

reg [1:0] o = 2'd0;
wire [1:0] port;
wire oe1;
wire [1:0] i;

// synthesis translate_off
reg dummy_s;
initial dummy_s <= 1'd0;
// synthesis translate_on

assign oe1 = oe;
assign {cs_b_n, cs_a_n} = port;

assign port[0] = oe1 ? o[0] : 1'bz;
assign port[1] = oe1 ? o[1] : 1'bz;
assign i = port;

endmodule

With intermediate signal port Vivado cooperates smoothly:

image

Unfortunately I'm not that familiar with the internals of Migen FHD to Verilog translation to submit PR :(

kaolpr commented 2 years ago

Well, just after submitting this issue I've noticed that it's improper use of Cat :man_facepalming:. When used properly it actually results in output I've suggested as a solution. So no work needed here, closing issue.

However, the behavior of Vivado is still misleading (not that it's the first time).