newbie's VHDL code, comment please

[kurukuru]

Hi,

I write VHDL code by separating main state and output control from each others. Main process contains only change of state and in output process I monitor main state and change output value accordingly. For me it is very easy to design and read but I don't know whether it is a poor design or not. Would you give me some comments about my coding style and if it is not a good way, would you give some advices? Thank you very much in advance.

below is example of my code

--{-----------------------------------------------------------------------------}
--{ Process
--{ Describe : Main CF control process
--{-----------------------------------------------------------------------------}

process (iCLK, iRST)
begin
	if (iRST = '1') then
		wCF_STATE <= CF_IDLE;
		wDLL_START <= '0';
	elsif (iCLK'event) and (iCLK = '1') then
		case wCF_STATE is
			when CF_IDLE =>
				if (wATREG_RD = '1') or (wATREG_WR = '1') or (wIO_RD = '1') or (wIO_WR = '1') or (wIOCON_RD = '1') or (wIOCON_WR = '1') then
					wCF_STATE <= ADR_SETUP;
				else
					wCF_STATE <= CF_IDLE;
				end if;
			when ADR_SETUP =>
				if (pDLL_COMP =  '1') then
					wDLL_START <= '0';
					wCF_STATE <= CMD_SETUP;
				else
					wDLL_START <= '1';
					wCF_STATE <= ADR_SETUP;
				end if;
			when CMD_SETUP =>
				if (pDLL_COMP =  '1') then
					wDLL_START <= '0';
					wCF_STATE <= CHK_WAIT;
				else
					wDLL_START <= '1';
					wCF_STATE <= CMD_SETUP;
				end if;
			when CHK_WAIT =>
				if (iWAIT_L = '1') then
					wCF_STATE <= RD_DATA;
				else
					wCF_STATE <= CHK_WAIT;
				end if;
			when RD_DATA =>
				wCF_STATE <= ADR_HOLD;
			when ADR_HOLD =>
				if (pDLL_COMP =  '1') then
					wDLL_START <= '0';
					wCF_STATE <= SEND_ACK;
				else
					wDLL_START <= '1';
					wCF_STATE <= ADR_HOLD;
				end if;
			when SEND_ACK =>
				wCF_STATE <= NEGATE_ALL;
			when NEGATE_ALL =>
				wCF_STATE <= CF_IDLE;
		end case;
	end if;
end process;

--{-----------------------------------------------------------------------------}
--{ Process
--{ Describe : Attribute memory access delay time control process
--{-----------------------------------------------------------------------------}

process (iCLK, iRST)
begin
	if (iRST = '1') then
		wDLL_VAL1 <= 60;
	elsif (iCLK'event) and (iCLK = '1') then
		if (wATREG_RD = '1') or (wATREG_WR = '1') then
			case wCF_STATE is
				when CF_IDLE =>
					wDLL_VAL1 <= 60;
				when ADR_SETUP =>
					if (wATREG_RD = '1') then
						wDLL_VAL1 <= kADR_ACC;
					else	-- (ATREG_WR = '1')
						wDLL_VAL1 <= kADR_SET;
					end if;
				when CMD_SETUP =>
					if (wATREG_RD = '1') then
						wDLL_VAL1 <= kOE_ACC;
					else	-- (ATREG_WR = '1')
						wDLL_VAL1 <= kWR_ACC;
					end if;
				when CHK_WAIT =>
				
				when RD_DATA =>
				
				when ADR_HOLD =>
					if (wATREG_RD = '1') then
						wDLL_VAL1 <= kRD_DIS;
					else	-- (ATREG_WR = '1')
						wDLL_VAL1 <= kDAT_HD;
					end if;
				when SEND_ACK =>
				
				when NEGATE_ALL =>
				
			end case;
		else
			wDLL_VAL1 <= 0;
		end if;
	end if;
end process;

--{-----------------------------------------------------------------------------}
--{ Process
--{ Describe : wREG_L control process
--{-----------------------------------------------------------------------------}

process (iCLK, iRST)
begin
	if (iRST = '1') then
		wREG_L <= '1';	
	elsif (iCLK'event) and (iCLK = '1') then
		if (wATREG_RD = '1') or (wATREG_WR = '1') then
			case wCF_STATE is
				when CF_IDLE =>
					wREG_L <= '1';
				when ADR_SETUP =>
					wREG_L <= '0';
				when NEGATE_ALL =>
					wREG_L <= '1';
				when others =>
					wREG_L <= wREG_L;
			end case;
		else
			wREG_L <= '1';
		end if;
	end if;
end process;

 

[Maxwell077]

In my opinion, it's a good style..
In FSM, I can give you some tips.. In all states, assign a value to all your outputs in your output logic FSM.
If not, synthesizer implements mux's to decide the output value.The inputs of mux are states and old value of output.
Try to use synchronous reset. Your's is asynchronous reset. Each FF has synchronous reset however they do not have asynchronous. To implement the asynchronous reset, it spends some logical blocks. (From Xilinx training videos)

Good luck
Ilgaz

 

[xtcx]

Yeah! quite easy to understand, but did you check the PAR details?....Try coding all in one process and check the results with this!...I doubt that this way of coding in multiple process and sharing signals may create some routing or performance issues!....Check PAR results and post here...Let us see

 

[kurukuru]

Hi,

Thank you very much Ilgaz and xtcx for your answer.

I have checked PAR of the code shown below according to the advice. The result is there is no different in both number of gate used and maximum frequency. !!!

Ps. I’m still opening for more comments and advices

Thank you.
kurukuru

-- Separate version

--{-----------------------------------------------------------------------------}
--{ Process
--{ Describe : Main
--{-----------------------------------------------------------------------------}

process (iCLK, iRST)
begin
	if (iRST = '1') then
		wCF_STATE <= CF_IDLE;
	elsif (iCLK'event) and (iCLK = '1') then
		case wCF_STATE is
			when CF_IDLE =>
				if (iINPUT = '1') then
					wCF_STATE <= ADR_SETUP;
				else
					wCF_STATE <= CF_IDLE;
				end if;
			when ADR_SETUP =>
				wCF_STATE <= CMD_SETUP;
			when CMD_SETUP =>
				wCF_STATE <= CHK_WAIT;
			when CHK_WAIT =>
				wCF_STATE <= RD_DATA;
			when RD_DATA =>
				wCF_STATE <= ADR_HOLD;
			when ADR_HOLD =>
				wCF_STATE <= SEND_ACK;
			when SEND_ACK =>
				wCF_STATE <= NEGATE_ALL;
			when NEGATE_ALL =>
				wCF_STATE <= CF_IDLE;
		end case;
	end if;
end process;

--{-----------------------------------------------------------------------------}
--{ Process
--{ Describe : Output
--{-----------------------------------------------------------------------------}

process (iCLK, iRST)
begin
	if (iRST = '1') then
		iOUTPUT <= "00000000";
	elsif (iCLK'event) and (iCLK = '1') then
		case wCF_STATE is
			when CF_IDLE =>
				iOUTPUT <= "00000001";
			when ADR_SETUP =>
				iOUTPUT <= "00000010";
			when CMD_SETUP =>
				iOUTPUT <= "00000100";
			when CHK_WAIT =>
				iOUTPUT <= "00001000";
			when RD_DATA =>
				iOUTPUT <= "00010000";
			when ADR_HOLD =>
				iOUTPUT <= "00100000";
			when SEND_ACK =>
				iOUTPUT <= "01000000";
			when NEGATE_ALL =>
				iOUTPUT <= "10000000";
		end case;
	end if;
end process;

-- Integrate version

--{-----------------------------------------------------------------------------}
--{ Process
--{ Describe : Main and output
--{-----------------------------------------------------------------------------}

process (iCLK, iRST)
begin
	if (iRST = '1') then
		wCF_STATE <= CF_IDLE;
		iOUTPUT <= "00000000";
	elsif (iCLK'event) and (iCLK = '1') then
		case wCF_STATE is
			when CF_IDLE =>
				iOUTPUT <= "00000001";
				if (iINPUT = '1') then
					wCF_STATE <= ADR_SETUP;
				else
					wCF_STATE <= CF_IDLE;
				end if;
			when ADR_SETUP =>
				iOUTPUT <= "00000010";
				wCF_STATE <= CMD_SETUP;
			when CMD_SETUP =>
				iOUTPUT <= "00000100";
				wCF_STATE <= CHK_WAIT;
			when CHK_WAIT =>
				iOUTPUT <= "00001000";
				wCF_STATE <= RD_DATA;
			when RD_DATA =>
				iOUTPUT <= "00010000";
				wCF_STATE <= ADR_HOLD;
			when ADR_HOLD =>
				iOUTPUT <= "00100000";
				wCF_STATE <= SEND_ACK;
			when SEND_ACK =>
				iOUTPUT <= "01000000";
				wCF_STATE <= NEGATE_ALL;
			when NEGATE_ALL =>
				iOUTPUT <= "10000000";
				wCF_STATE <= CF_IDLE;
		end case;
	end if;
end process;

 

[xtcx]

Well!, thats ok to go then!...As for as your code is concerned, there appears no problem. But the real problem comes when you handle signals and variables from process to process. Handling variables across different process is not recomended and it has failed in many case for poor performance or design failure as far as xilinx ise tool is concerned..In such cases, you must try not to complicate the design just for the readability!..
Regards