Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snoopy #320

Closed
wants to merge 12 commits into from
Closed

Snoopy #320

wants to merge 12 commits into from

Conversation

Snoopy87
Copy link
Member

  • Rework AhbInterconnect (Decode + arbitrer)

  • Add AHB tests for arbitrer/decoder/interconnect

  • Add PhaseInitReg (Init all register uninitiated )

  • You can add comments of a component in the rtf file generated

.gitignore Outdated Show resolved Hide resolved
@jijingg
Copy link
Collaborator

jijingg commented May 27, 2020

Add PhaseInitReg (Init all register uninitiated )

hi @Snoopy87 i have question ,dose this will automatic init all register uninitiated? or just a manually option

@Snoopy87
Copy link
Member Author

Snoopy87 commented Jun 3, 2020

Add PhaseInitReg (Init all register uninitiated )

hi @Snoopy87 i have question ,dose this will automatic init all register uninitiated? or just a manually option

It is a manually option. In the SpinalConfig you can add transfomration phases
SpinalConfig( mode = VHDL, transformationPhases = ArrayBuffer(new PhaseInitReg()) ).generate(newTopLevel)

@jijingg
Copy link
Collaborator

jijingg commented Jun 7, 2020

@Snoopy87, manually option is ok . if you can add report log of uninitiated register it 'will be great .
because for some team, they prefer to remove reset on data-path for purpose of saving area(there is no functional risk), but on control path must with reset logic, if spinal give report log of uninitiated, the designer can find out which control path without uninitiated.

@Snoopy87
Copy link
Member Author

Snoopy87 commented Jun 9, 2020

@Snoopy87, manually option is ok . if you can add report log of uninitiated register it 'will be great .
because for some team, they prefer to remove reset on data-path for purpose of saving area(there is no functional risk), but on control path must with reset logic, if spinal give report log of uninitiated, the designer can find out which control path without uninitiated.

I have added messages in the PhaseInitReg

Comment on lines -64 to +98
}
when(io.output.HREADY & io.output.last && !io.output.HMASTLOCK) { // End of burst and no lock
locked := False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much dislike this code, both in its old and in its new form. AMBA 3 AHB-Lite specification, Section 3.5.2 states that

Although masters are not permitted to terminate a burst request early, slaves must be designed to work correctly if the burst is not completed.

This means that decoding the last information is quite worthless and leading to wrong conclusions in unlocking from a burst. IMHO, the only condition for unlocking is checking for HMASTLOCK, and whether HTRANS is either IDLE or NONSEQ. I am aware of the implications on timing, but relying on last is improper.

@@ -138,7 +138,7 @@ case class AhbLite3(config: AhbLite3Config) extends Bundle with IMasterSlave {
//return true when the current transaction is the last one of the current burst
def last(): Bool = {
val beatCounter = Reg(UInt(4 bits)) init(0)
val isLast = Vec(U"0000", U"0011", U"0111", U"1111")(U(HBURST >> 1)) === beatCounter || (HREADY && ERROR)
val isLast = Vec(U"0000", U"0011", U"0111", U"1111")(U(HBURST >> 1)) === beatCounter || ERROR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLast doesn’t indicate properly a burst of type INCR, i.e. a burst of undefined length. In its current form, it takes a burst of type INCR just the same like the type SINGLE which is quite the opposite of INCR.

Edit: Also, according to Section 3.5.2 of the AMBA 3 AHB-Lite specification, an ERROR response doesn’t implicitly terminate a burst. A master is allowed to continue a burst, and a slave must be prepared for this situation.

However, see my other note on using last() in the AhbLite3Arbiter for why I see this method useless in the first place.

@numero-744 numero-744 closed this Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants