Using datapath shift output as instruction input creates a combinational loop / unintentional latch(?)

Tip / Sign in to post questions, reply, level up, and achieve exciting badges. Know more

cross mob
RaAl_264636
Level 6
Level 6
50 sign-ins 25 sign-ins 10 solutions authored

Hello,

I want to use the LSB of the datapath value after ALU has finished as input for the next datapath instruction. So I connected the routed shift output to one of the instruction inputs. I got a warning about combinational loop / unintended latch and I assume that it's caused by this approach. Any ideas how I can get rid of this? I thought this approach wouldn't create a latch because the datapath is clocked.

Regards

0 Likes
12 Replies
Len_CONSULTRON
Level 9
Level 9
Beta tester 500 solutions authored 1000 replies posted

RaAl,

I am trying to wrap my head around the issue.  I'm not sure I fully understand it.

Can you create a simple project that illustrates this issue so the forum can test it and potentially solve it?

I've created a very simple project with what I think you said.

Are you using Verilog or the UDB Editor in your application?

It is my understanding that the ALU is clocked using the datapath clock.  

Len
"Engineering is an Art. The Art of Compromise."
0 Likes

Hello @Len_CONSULTRON 

sorry for the late reply. Yes, I will provide a simple project to show what I mean and I'll compare it against your project.

I don't use the UDB editor, I always use Verilog directly. I also think that the ALU is clocked by the datapath clock since the ALU is part of the datapath. That was the reason why I got confused regarding the warning about combinational loop / unintended latch.

Regards

0 Likes

Hello @Len_CONSULTRON 

sorry for the long delay - I came across multiple issues for this, including some confusing statements within the PSoC documentation... but now, I can provide a stripped down example to show the problem.

Attached is the PSoC project file which includes a demonstration component and a small test function, output is by UART (I used CY8CKIT-059). NOTE: this component doesn't do anything useful, it just shows the issue - please keep this in mind.

The component uses the LSB of the input value on the first clock cycle and the shift output as an input to the datapath instruction for the subsequent cycles. There is a boolean parameter 'Workaround'. If the parameter is cleared, the WARP compiler should issue a warning about a combinational loop / unintentional latch. Maybe it's needed to clean up the project between builds to make the warning appear. If the parameter is set, a registered version of the shift output is used as the instruction input. The register is updated on the falling edge of the components clock. Using this approach, the warning went away - but I don't know if this will inject other problems. At least the test function provided the same result, regardless of using the workaround or not.

Also included are the simulation files with testbench. The testbench does the same as the test function in the project. So, this part maybe might be of interest for @RodolfoGL because it shows the documentation issues I mentioned above.

First, it's not clear to me why the simulation of the unregistered version shows unknown values for the datapath shift output. Since the instruction input is cleary defined and it's a synchronous design, I'd expect known states. So I'm not sure if there's more to do for a simulation setup or if the datapath simulation files provided don't behave as the should. You can see this in the VCD file for the unregistered shift output, it seems the datapaths aren't doing what they should do.

Second, regarding the documentation, I came across the following section PSoC 5LP reference manual, document no. 001-78426 Rev. *G, page 169, datapath outputs:

RaAl_264636_0-1653833397903.png

So, this means that the outputs are registered and the value should be updated at the next rising edge of the components clock, right? A combinational output would be available immediately within the same clock cycle. The PSoC 5LP register manual shows that this option is bound to the CFG8 register of the datapath, with a 6-bit field indicating registered(0, default) or combinational(1). The CFG8 register of the datapaths also defaults to 0x00, so upon here it's clear. However, if the outputs are registered by default, why does WARP complain about a combinational loop and why is the datapath output of the signal SO_REG one clock cycle behind the SO signal? You can also see this in the simulation results:

RaAl_264636_2-1653836216365.png

 

At 20ns, the input values are provided, the trigger condition occurs one clock cycle later. When the trigger is recognized, the input value is loaded and shifted, therefore the shift output is cleared (now we have 0x2AA). This is okay. But as soon as the input values are provided, the shift output is set - why? Any instruction for the component does a right shift, so shouldn't the shift output be cleared? For the SO_REG signal, I assume this is the registered shift output as indicated by the manual. If not, please clarify.

Regards

0 Likes
RaAl_264636
Level 6
Level 6
50 sign-ins 25 sign-ins 10 solutions authored

Hello @Len_CONSULTRON 

sorry for the long delay - I came across multiple issues for this, including some confusing statements within the PSoC documentation... but now, I can provide a stripped down example to show the problem.

Attached is the PSoC project file which includes a demonstration component and a small test function, output is by UART (I used CY8CKIT-059). NOTE: this component doesn't do anything useful, it just shows the issue - please keep this in mind.

The component uses the LSB of the input value on the first clock cycle and the shift output as an input to the datapath instruction for the subsequent cycles. There is a boolean parameter 'Workaround'. If the parameter is cleared, the WARP compiler should issue a warning about a combinational loop / unintentional latch. Maybe it's needed to clean up the project between builds to make the warning appear. If the parameter is set, a registered version of the shift output is used as the instruction input. The register is updated on the falling edge of the components clock. Using this approach, the warning went away - but I don't know if this will inject other problems. At least the test function provided the same result, regardless of using the workaround or not.

Also included are the simulation files with testbench. The testbench does the same as the test function in the project. So, this part maybe might be of interest for @RodolfoGL because it shows the documentation issues I mentioned above.

First, it's not clear to me why the simulation of the unregistered version shows unknown values for the datapath shift output. Since the instruction input is cleary defined and it's a synchronous design, I'd expect known states. So I'm not sure if there's more to do for a simulation setup or if the datapath simulation files provided don't behave as the should. You can see this in the VCD file for the unregistered shift output, it seems the datapaths aren't doing what they should do.

Second, regarding the documentation, I came across the following section PSoC 5LP reference manual, document no. 001-78426 Rev. *G, page 169, datapath outputs:

RaAl_264636_0-1653833397903.png

So, this means that the outputs are registered and the value should be updated at the next rising edge of the components clock, right? A combinational output would be available immediately within the same clock cycle. The PSoC 5LP register manual shows that this option is bound to the CFG8 register of the datapath, with a 6-bit field indicating registered(0, default) or combinational(1). The CFG8 register of the datapaths also defaults to 0x00, so upon here it's clear. However, if the outputs are registered by default, why does WARP complain about a combinational loop and why is the datapath output of the signal SO_REG one clock cycle behind the SO signal? You can also see this in the simulation results:

RaAl_264636_2-1653836216365.png

 

At 20ns, the input values are provided, the trigger condition occurs one clock cycle later. When the trigger is recognized, the input value is loaded and shifted, therefore the shift output is cleared (now we have 0x2AA). This is okay. But as soon as the input values are provided, the shift output is set - why? Any instruction for the component does a right shift, so shouldn't the shift output be cleared? For the SO_REG signal, I assume this is the registered shift output as indicated by the manual. If not, please clarify.

Also, I wonder how the combinational option is activated: the datapath configuration fixes CFG8 register to be 0x00. Now, how to enable it? Writing to the register, configuring the CFG8 value within the verilog file or both?

Regards

0 Likes

Ralf,

This might take me a while to analyze since I'm fairly new to Verilog.

I noticed something.

On line 218 of your Verilog file:  (This is the same for line 318 as well.)

 

    8'hFF, 8'h00,  /*CFG9:                        */

 

 Now the comment lists "CFG9:".  CFG9 is the AMASK value.  However it only applied to the second byte listed (8'h00).  The first byte should be CFG8 register setting which appears to tell ALL the outputs to be combinatorial.

I changed the 8'FF for CFG8 to 8'h00 on lines 218 and 219.  No change.  I still get the warning.

In either case of   parameter Workaround = 0; or  parameter Workaround = 1; I still get the "combinational loop" warning.

Len
"Engineering is an Art. The Art of Compromise."
0 Likes

Ralf,

Are you wanting a registered or combinatorial output?

Len
"Engineering is an Art. The Art of Compromise."
0 Likes

Ralf,

Please forgive my ignorance about Verilog syntax (still learning):  It appears the offending statement seems to be

	assign DP_L_INSTR =	State == STATE_LOAD ? {STATE_LOAD, A_L[0]} : (
						State == STATE_WORK ? {STATE_WORK, SO_LSB} : (
						{STATE_LAST, SO_LSB}));

Can 'assign's be used with conditionals at run0time.  I've consulted the Infineon Warp/Verilog  Reference doc.  I think the assign statement creates a design-time logic assignment.  Besides it looks like you're trying to modify the instruction state from the SO bit output.

Isn't state logic changing suppose to happen inside the

// *** main state machine ******************************************************

portion of the .v file?

Normally I would try to model my state logic using the UDB editor and then go into the Verilog viewing tab to see how Infineon implemented it.

Len
"Engineering is an Art. The Art of Compromise."
0 Likes
RaAl_264636
Level 6
Level 6
50 sign-ins 25 sign-ins 10 solutions authored

Hello @Len_CONSULTRON 

Now the comment lists "CFG9:". CFG9 is the AMASK value. However it only applied to the second byte listed (8'h00). The first byte should be CFG8 register setting which appears to tell ALL the outputs to be combinatorial.
I think it's descending order, so it would be CFG9-CFG8 which suits to the AMASK default value of 0xFF.

In either case of parameter Workaround = 0; or parameter Workaround = 1; I still get the "combinational loop" warning.
Strange... did you clean up the project before rebuilding it with Workaround = 1?

Are you wanting a registered or combinatorial output?
To be honest: I don't know 😄 I know that I want to use the shifted output from current instruction as input for the next instruction.

Please forgive my ignorance about Verilog syntax (still learning): ...
I'm still learning, too 😄

It appears the offending statement seems to be...
This is when the shift output is directly connected to the instruction input. If the Workaround parameter is set, it should use the registered shift output. That's what the 'generate' block is used for:

generate
if(Workaround == 0)
begin
	assign DP_L_INSTR =	State == STATE_LOAD ? {STATE_LOAD, A_L[0]} : (
						State == STATE_WORK ? {STATE_WORK, SO_LSB} : (
						{STATE_LAST, SO_LSB}));
	assign DP_H_INSTR =	State == STATE_LOAD ? {STATE_LOAD, A_L[0]} : (
						State == STATE_WORK ? {STATE_WORK, SO_LSB} : (
						{STATE_LAST, SO_LSB}));
end
else
begin
	reg REG_SO_LSB;			//registered shift output (workaround)

	always @(negedge Clk)	//register the shift output at negative clock edge
	begin
		REG_SO_LSB <= SO_LSB;
	end

	assign DP_L_INSTR =	State == STATE_LOAD ? {STATE_LOAD, A_L[0]} : (
						State == STATE_WORK ? {STATE_WORK, REG_SO_LSB} : (
						{STATE_LAST, REG_SO_LSB}));
	assign DP_H_INSTR =	State == STATE_LOAD ? {STATE_LOAD, A_L[0]} : (
						State == STATE_WORK ? {STATE_WORK, REG_SO_LSB} : (
						{STATE_LAST, REG_SO_LSB}));
end
endgenerate


Can 'assign's be used with conditionals at run0time.
This is (nearly) the same as if I'd use a n-bit register for the instruction signals and assign the values within the state machine. The most significant difference is that the signal are combinational in my design (I want to save resources as much as possible).

Regards

0 Likes

Ralf,

I've attached another version of ALU_shifter project which is closer to your design intent (no PIs or POs and uses UDB editor).  It doesn't generate a combinatorial logic error.

Do you think you can modify the state logic for this project to be closer to your intended design while still using the UDB_Editor.  It might help me visualize your design better.


Now the comment lists "CFG9:". CFG9 is the AMASK value. However it only applied to the second byte listed (8'h00). The first byte should be CFG8 register setting which appears to tell ALL the outputs to be combinatorial.
I think it's descending order, so it would be CFG9-CFG8 which suits to the AMASK default value of 0xFF.

I believe you correct.  However the ARM architecture is little-endian hence the confusion.

In either case of parameter Workaround = 0; or parameter Workaround = 1; I still get the "combinational loop" warning.
Strange... did you clean up the project before rebuilding it with Workaround = 1?


I performed the Application Build phase without and with a clean prior.  Same warning in both cases.

Are you wanting a registered or combinatorial output?
To be honest: I don't know 😄 I know that I want to use the shifted output from current instruction as input for the next instruction.

I've done something similar to that in my first version of DCmp_thresh2 component.

Here's the UDB editor definition in the DataPath:

Len_CONSULTRON_0-1653923695227.png

I used two_inp input to trigger the INST_ADDR[0] bit and an internal register variable calc_sign for INST_ADDR[1] bit.

This yields a Verilog output of:

assign DCMP_dp_select[0] = (two_inp);
assign DCMP_dp_select[1] = (calc_sign);
assign DCMP_dp_select[2] = (1'b0);

 

Len
"Engineering is an Art. The Art of Compromise."
0 Likes
RaAl_264636
Level 6
Level 6
50 sign-ins 25 sign-ins 10 solutions authored

Hello @Len_CONSULTRON 

I've attached another version of ALU_shifter project which is closer to your design intent (no PIs or POs and uses UDB editor). It doesn't generate a combinatorial logic error.


I checked the UDB project. It doesn't create the warning because the shift output is not directly feed back to the instruction input. The instruction input is fed by the value of the state machine register.

Do you think you can modify the state logic for this project to be closer to your intended design while still using the UDB_Editor. It might help me visualize your design better.


I tried it, please check the attachment. I've never worked with the UDB editor, so it might be that this doesn't reflect my implementation exactly (aside of using PI/PO) but I think it's very close. And it throws the warnings.

I've done something similar to that in my first version of DCmp_thresh2 component.
Here's the UDB editor definition in the DataPath: [...]
I used two_inp input to trigger the INST_ADDR[0] bit and an internal register variable calc_sign for INST_ADDR[1] bit. This yields a Verilog output of: [...]

Okay, but this design doesn't use a datapath output directly as instruction input, right? At least I can't identify this feedback from the UDB editor screenshot.

Regards

0 Likes

Ralf,

I confirmed your changes to the UDB editor version yields the same warnings.

I modified your version and got rid of the adds (A0 + D0) in Inst Addresses b001, b011, and b101 of DP_L.

With the adds in any of these instructions, the warning is 'thrown'.   Without the adds, no warnings.

I left the adds in DP_H.  no issue there.

Now if we can configure out the combinatorial loop.

My understanding of a combinatorial loop as an unintended latch is from this schematic illustration:

Len_CONSULTRON_0-1653940720038.png

If this is what WARP is warning then I can see why you what to prevent this.   Once SO_LSB_out is true, it will be latched and ALWAYS BE TRUE.  Only a power down will clear it.

Len
"Engineering is an Art. The Art of Compromise."
0 Likes

Hello @Len_CONSULTRON 

Thank you for your confirmation. I've to check why my provided example always throws the error on your system. In my case, it went away when the parameter is set. And the solution was to introduce a register which is updated on the negative edge of the clock signal. But I don't know if this is a good solution. Maybe someone of the support team can clarify.

Regarding my modifications of your UDB editor example, I think I forgot to link the carry output of DP_L to carry input of DP_H. I'll why it doesn't throw the error if you remove the ADD operation. Maybe I forgot the shift operation there or something like that.

Regards

0 Likes