Issues routing clock signal with himbaechel for gowin

[loco-choco]

I am having some problems with nextpnr-himbaechel (both latest commit and 0.7) warning that it can't use dedicated routing for a clk signal, and then asserting at this line:

nextpnr/common/route/router1.cc

Line 345 in e4115e8

valid_wires_for_net.insert(wire);

Here is the end of the output:

Info: No Fmax available; no interior timing paths found in design.
Info: Checksum: 0x0e143406
Info: Routing globals...
Warning: Failed to route net 'rpll_display_clk' from X2Y9/F4 to X5Y6/A7 using dedicated routing.
Info:     routed net 'rpll_display.clock_in' using global resources

Info: Routing..
Info: Setting up routing queue.
ERROR: Assert `net_info->wires.count(wire)' failed in /build/nextpnr/common/route/router1.cc:345.
ERROR: Routing design failed.
1 warning, 2 errors

And the call for nextpnr:

nextpnr-himbaechel --json top_tangnano_9k.json \                                            
                   --write pnrtop_tangnano_9k.json \
                   --device GW1NR-LV9QN88PC6/I5 \
                   --vopt family=GW1N-9C \
                   --vopt cst=../global/top_tf_reader_tangnano_9k.cst

Weirdly enough, if I use nextpnr-gowin (0.7) it can route without an issue.

This project uses a rPLL, but removing it doesn't stop the error from happening (the warning changes to complain about another clock signal), and I have another project that uses a rPLL which routes fine.

Any ideas why it could be failing?

[yrabbit]

Can I have the example files with the error? Especially since you have both working and non-working designs.

[loco-choco]

After doing some not so fancy binary search on the code, I found the block that was causing the issue. Trying to synthesize it alone, and then routing it, results in the same error:

module byte_serial_spi #(
    parameter NUMBER_OF_BITS = 8,
    localparam WIDTH_CURRENT_BIT = $clog2(NUMBER_OF_BITS),
    parameter CLOCK_POLARITY = 1'b0, // CPOL
    parameter CLOCK_PHASE    = 1'b0  // CPHA
) (
    // Clock
    input clk_in,
    input enable,
    input reset,
    // Part of SPI interface
    output serial_clk,
    output main_out_sub_in,
    input main_in_sub_out,
    // Block interface
    output [NUMBER_OF_BITS - 1:0] sub_data_in,
    input [NUMBER_OF_BITS - 1:0] main_data_out,
    output data_sent_received  // signals that all the data has been sent, and that we received a new byte
);

  reg [WIDTH_CURRENT_BIT - 1:0] current_bit;
  reg [ NUMBER_OF_BITS - 1 : 0] received_data;
  reg                           send_rcv_data;
  reg                           data_sent_rcv;

  // Data Shift ------------------
  // NUMBER_OF_BITS bits + 1 cycle with the last one skipped to signal that the data was sent/received
  assign serial_clk = (clk_in & send_rcv_data) ^ CLOCK_POLARITY;
  assign data_sent_received = data_sent_rcv;

  always @(negedge (clk_in ^ CLOCK_PHASE)) begin
    if (reset) send_rcv_data <= 0;
    else if (send_rcv_data) begin
      if (current_bit > 0) current_bit <= current_bit - 1;
      else begin
        data_sent_rcv <= 1;
        send_rcv_data <= 0;
      end
    end else if (enable) begin
      send_rcv_data <= 1;
      data_sent_rcv <= 0;
      current_bit   <= NUMBER_OF_BITS - 1;
    end
  end

  // Data going to sub
  assign main_out_sub_in = main_data_out[current_bit];
  // Data Sampling --------------
  // Data coming from sub
  assign sub_data_in = received_data;
  always @(posedge ((serial_clk ^ CLOCK_POLARITY) ^ CLOCK_PHASE)) begin
    received_data[current_bit] <= main_in_sub_out;
  end

endmodule

The line that is the issue is this one:

 assign serial_clk = (clk_in & send_rcv_data) ^ CLOCK_POLARITY;

If I comment out that & send_rcv_data the router doesn't error out, and has the following output (in the complete project):

Info: No Fmax available; no interior timing paths found in design.
Info: Checksum: 0xcf61d923
Info: Routing globals...
Info:     routed net 'rpll_display_clk' using global resources
Info:     routed net 'rpll_display.clock_in' using global resources

Info: Routing..
Info: Setting up routing queue.
Info: Routing 1091 arcs.
Info:            |   (re-)routed arcs  |   delta    | remaining|       time spent     |
Info:    IterCnt |  w/ripup   wo/ripup |  w/r  wo/r |      arcs| batch(sec) total(sec)|
Info:       1000 |       13        857 |   13   857 |       110|       1.49       1.49|
Info:       1114 |       17        949 |    4    92 |         0|       0.18       1.68|
Info: Routing complete.
Info: Router1 time 1.68s
Info: Checksum: 0x4fff13ba

Info: No Fmax available; no interior timing paths found in design.

Info: Program finished normally.

Looks like the router doesn't like me creating a way to toggle the clock. But sadly, I do need a way to do that.

Also, sorry for this cursed verilog block, I thought it could be something with it and even made are more sensible version, but after finding the reason was the toggling clock, I went back to the one in here just so we can figure out what is really going wrong.

[yrabbit]

This snippet with nextpnr version e4115e8 works with warnings but no errors.

There are two possible reasons for this: I have a different random number and I made up a bad .CST file.

Add “--seed 10” when calling nextpnr-himbaechel if it stops crashing then I will continue experimenting with other random numbers, otherwise I need a link to your full project repository or full project archive.

[loco-choco]

Setting the seed to 10 didn't make a difference. Here is that snipped with the script and .cst I was using:
test.zip
Inside global/ is the cst, rt/ the verilog files, and scripts/ the bash script with the calls for yosys and nextpnr.
I will try to make an archive with the whole project, but I just gotta make sure there isn't anything in there I can't share.
And I just noticed I forgot to tell what version of yosys I am using:

yosys -V     
Yosys 0.49 (git sha1 427b5a251bfb511528a177bfa92567b4b3ada3dd, g++ 14.2.1 -fPIC -O3)

[loco-choco]

After looking around at the gowin primitives, I have a theory behind why it is failing.
From what I read, the proper way to multiplex or toggle a clock signal is with the DCS and DQCE. So I think that Yosys isn't able to identify (probably from a lack of context) that the (clk_in & send_rcv_data) should be elaborated like this:

  wire sclk_en;
  wire clk_switched;
  DQCE clk_switch (
      .CE(sclk_en),
      .CLKIN(clk_in),
      .CLKOUT(clk_switched)
  );
  assign serial_clk = clk_in ^ CLOCK_POLARITY;

Full module for context:

module byte_serial_spi #(
    parameter NUMBER_OF_BITS = 8,
    localparam WIDTH_CURRENT_BIT = $clog2(NUMBER_OF_BITS),
    parameter CLOCK_POLARITY = 1'b0, // CPOL
    parameter CLOCK_PHASE    = 1'b0  // CPHA
) (
    // Clock
    input clk_in,
    input enable,
    input reset,
    // Part of SPI interface
    output serial_clk,
    output main_out_sub_in,
    input main_in_sub_out,
    // Block interface
    output [NUMBER_OF_BITS - 1:0] sub_data_in,
    input [NUMBER_OF_BITS - 1:0] main_data_out,
    output finished
);
  localparam SPI_STATE_IDLE = 2'd0, SPI_STATE_RUNNING = 2'd1, SPI_STATE_FINISH = 2'd2;
  reg  [                    1:0] current_state;
  reg  [                    1:0] next_state;

  reg  [WIDTH_CURRENT_BIT - 1:0] current_bit;
  reg  [ NUMBER_OF_BITS - 1 : 0] received_data;

  // Data Shift ------------------
  // NUMBER_OF_BITS bits + 1 cycle with the last one skipped to signal that the data was sent/received
  wire                           sclk_en;
  wire                           clk_switched;
  DQCE clk_switch (
      .CE(sclk_en),
      .CLKIN(clk_in),
      .CLKOUT(clk_switched)
  );
  assign serial_clk = clk_switched ^ CLOCK_POLARITY;
  assign sclk_en = current_state == SPI_STATE_RUNNING;
  assign finished = current_state == SPI_STATE_FINISH;

  always @(negedge (clk_in ^ CLOCK_PHASE)) begin
    next_state = current_state;
    if (reset) begin
      next_state = SPI_STATE_IDLE;
    end else begin
      case (current_state)
        SPI_STATE_IDLE: begin
          if (enable) begin
            next_state  = SPI_STATE_RUNNING;
            current_bit = NUMBER_OF_BITS - 1;
          end
        end
        SPI_STATE_RUNNING: begin
          if (current_bit > 0) current_bit = current_bit - 1;
          else next_state = SPI_STATE_FINISH;
        end
        SPI_STATE_FINISH: begin
          next_state = SPI_STATE_IDLE;
        end
        default: begin
        end
      endcase
    end
    current_state = next_state;
  end

  // Data going to sub
  assign main_out_sub_in = main_data_out[current_bit];
  // Data Sampling --------------
  // Data coming from sub
  assign sub_data_in = received_data;
  always @(posedge ((serial_clk ^ CLOCK_POLARITY) ^ CLOCK_PHASE)) begin
    received_data[current_bit] <= main_in_sub_out;
  end

Luckly with this explicit instantiation Yosys elaborates it correctly, but sadly nextpnr fails not knowing what to do with the DQCE:

Info: Using uarch 'gowin' for device 'GW1NR-LV9QN88PC6/I5'

Info: Reading constraints...
Info: Create constant nets...
Info: Modify LUTs...
Info: Pack IOBs...
Info: Pack diff IOBs...
Info: Pack IO logic...
Info: Pack DESER16 logic...
Info: Pack GSR..
Info: Pack INV..
Info: Pack wide LUTs...
Info: Packed MUX2_LUT8:1, MUX2_LU7:0, MUX2_LUT6:4, MUX2_LUT5:26
Info: Pack ALUs...
Info: Pack PLL..
Info: Pack RAMs...
Info: Pack BSRAMs...
Info: Pack buffered nets..
Info: Checksum: 0xeed2c4f1

Info: Device utilisation:
Info: 	                VCC:     1/    1   100%
Info: 	                IOB:    12/  274     4%
Info: 	               LUT4:   225/ 8640     2%
Info: 	             OSER16:     0/   80     0%
Info: 	             IDES16:     0/   80     0%
Info: 	            IOLOGIC:     0/  276     0%
Info: 	          MUX2_LUT5:    42/ 4320     0%
Info: 	          MUX2_LUT6:     8/ 2160     0%
Info: 	          MUX2_LUT7:     2/ 1080     0%
Info: 	          MUX2_LUT8:     1/ 1080     0%
Info: 	                ALU:    30/ 6480     0%
Info: 	                GND:     1/    1   100%
Info: 	                DFF:   114/ 6480     1%
Info: 	          RAM16SDP4:     0/  270     0%
Info: 	              BSRAM:     0/   26     0%
Info: 	                GSR:     1/    1   100%
Info: 	                OSC:     0/    1     0%
Info: 	               rPLL:     1/    2    50%
Info: 	               BUFG:     0/   22     0%

Info: Placed 0 cells based on constraints.
ERROR: Unable to place cell 'tf_card.spi_driver.clk_switch', no BELs remaining to implement cell type 'DQCE'

I am using the latest release of Apicula, and I also noticed that those two blocks I mentioned were added to the wiki after v0.15, so maybe the issue now is an outdated cell map, or something.

Also, just so we can test with the same project, here is an archive of the project like you asked.
project.zip

[yrabbit]

Thank you for the archive. Unfortunately for me, the situation is not repeated :(

Project test log with modified script - log-test.txt

Project build log with modified script - log.txt

It's a shame I don't have anything to work with yet, so we'll see how to solve your problem.

First of all Yosys - as you can see from the log I don't use the latest version and there is a good reason for that - it has memory corruption and it leads to all sorts of effects, both unnoticeable and segmentation fault YosysHQ/yosys#4883.

Nextpnr - the master branch knows about DCE and DQCE.

Further I don't think DCS and DQCE are the right mechanisms here: the latter disconnects the whole chip segment from the clock at once and its purpose is rather to save power (when powered by batteries for example). The first one in theory can be used to turn off the clock although it is intended for dynamic source selection and there are not so many of them.... well, in principle it can (https://github.com/YosysHQ/apicula/blob/master/examples/himbaechel/dcs.v)

And lastly (I'm not an expert here) - I think the right way is to redesign the code so that the CE input (clock enable) of flip-flops is used.

And it can be done I suppose (again, I'm not an expert on this) with something like:

always @(posedge clk) begin
     if (some_condition) begin
         ...
     end
end

The method may differ (in the direction of correctness), but the essence of the approach - to move the clock manipulation into the DFF itself - remains true.

[loco-choco]

I see, I could do that for the internal logic, but sadly this won't be enough as I need this toggling behavior on the signal exiting the fpga.
I will make these changes and see if somehow the issue stops.

[loco-choco]

Also, I found out that routing with nextpnt-gowin works, but when testing on the board something doesn't seem to be right.
Here is the log just in case:
log.txt

[yrabbit]

With nextpnr-himbaechel there is a chance that someone will come across this bug in a different context and I can replicate it for myself and find the cause and fix it. So there's hope, though not anytime soon.

With nextpnr-gowin there is almost no chance - I made the last commit making functional changes in Jun 20, 2023 (#1181),
and I still believe that this router variant should have been removed in May 2024 because it has bugs that are very hard to find - large designs like Attosoc or RISC-V systems don't work.

Nextpnr-himbaechel has no problems with these things and supports more primitives, but for reasons beyond my control nextpnr-gowin is still available and gives me a lot of feelings of various kinds:)

[whitequark]

Note that I don't believe there is still any reason to keep nextpnr-gowin around.