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

Ability to remap address for BusSlaveFactory #1354

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

KireinaHoro
Copy link
Contributor

This draft PR is intended more as a discussion point; I don't think it can be merged as is.

Context, Motivation & Description

I'm working on a slave design that involves aliasing a resource into multiple places; this is necessary for Mem for example, since calling readMemSync multiple times would create multiple memory ports.

I've implemented a simple remap functionality on Axi4SlaveFactory for this purpose; it works for my test case so far. However, I feel like such a feature would belong in the general BusSlaveFactory through duplicating the relevant the job elements. I'm not so familiar with the internals with the general version, so I'm not sure if it's actually doable there.

@jiegec since you're the original author of Axi4SlaveFactory.

Checklist

  • Unit tests were added
  • API changes are or will be documented:

@KireinaHoro
Copy link
Contributor Author

A use-case can be seen here: https://github.com/KireinaHoro/enzian-pio-nic/blob/69c8548e905e72ae899f2b0b10dd52b6d75f737f/hw/spinal/pionic/eci/EciDecoupledRxTxProtocol.scala#L57-L63

I remap 0x40 of the bus to 0xc0, in order to allow aliasing the two memory regions filled by memory read.

@Dolu1990
Copy link
Member

Hi,

That's interresting ^^
Hmm i don't know if that should realy be in the bus slave factory, as it may try to cook too many different feature in the same plate.
I mean that fondamentaly, it is an address signal manipulation, so it could also be done in a transformation on the memory bus before the bus slave factory, right ? Some kind of bus preprocessing.

But at the same time, having it there in the bus slave factory base would allow to have a portable aproache. So, why not ?

@KireinaHoro
Copy link
Contributor Author

KireinaHoro commented Mar 15, 2024

Yeah I agree. So what happened is that I tried doing it outside of the bus slave factory first by first unbursting the AXI bus and then remapping it, but for some reason it didn't work (I didn't investigate why).

The approach of doing it in the slave factory has the benefit of not having to worry about bus details. For example, AXI4-MM has bursts, so simply remapping part of the address space will not work: a master can issue a burst read that does not start directly at the remapped address (AR is only transmitted once and the beat addresses are calculated by burst mode).

I'll put together a more general version next week and hopefully see how we can test it with some other bus protocols.

@KireinaHoro
Copy link
Contributor Author

After some backend P&R work, it became apparent that if I do something complicated in remap, I need a pipeline stage between remap and the downstream operators. I pushed a new commit for this. New questions @Dolu1990:

  • MultiData.mapElement and Stream[MultiData].mapPayloadElement are useful for remapping. I've put them here but would it be useful to add them to lib?
  • With the pipeline stage feature, it's more unclear how can I integrate this into the general bus slave factory. Should the pipeline option also in the base implementation (i.e. BusSlaveFactoryDelayed)?

def mapElement[E <: Data](locator: T => E)(f: E => E): T = {
val ret = b.clone.asInstanceOf[T].allowOverride()
ret := b
locator(ret) := f(locator(b))
Copy link
Member

Choose a reason for hiding this comment

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

locator(ret).removeAssignments() := f(locator(b))

Would remove the need of allowOverride and avoid the double assignement.

def mapElement[E <: Data](locator: T => E)(f: E => E): T = {
val ret = b.clone.asInstanceOf[T].allowOverride()
ret := b
locator(ret) := f(locator(b))
Copy link
Member

Choose a reason for hiding this comment

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

locator(ret).removeAssignments() := f(locator(b))

Would remove the need of allowOverride and avoid the double assignement.

implicit class RichMultiData[T <: MultiData](b: T) {
def mapElement[E <: Data](locator: T => E)(f: E => E): T = {
val ret = b.clone.asInstanceOf[T].allowOverride()
ret := b
Copy link
Member

Choose a reason for hiding this comment

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

val ret = CombInit(b).allowOverride()

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the difference between CombInit and clone?

Copy link
Member

Choose a reason for hiding this comment

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

clone => create a new signal of the same kind => type copy
CombInit => create a new signal of the same kind and assign it with the provided argument aswell => type + value copy

@Dolu1990
Copy link
Member

Hi,

MultiData.mapElement and Stream[MultiData].mapPayloadElement are useful for remapping. I've put them here but would it be useful to add them to lib?

The one issue i have with it, is may push people to chain mapElement to modify multiple elements.

In general, i'm doing things more like :

val miaou = CombInit(original)
miaou.x.removeAssignements() := 0x42
miaou.y.removeAssignements() := 0x42

So maybe a more sane aproache overall would be to have a dedicated assignement operator ?

val miaou = CombInit(original)
val miaou.x :=!! 0x42 //Override value of x completly

I need a pipeline stage between remap and the downstream operators.

For timing reasons ?

@KireinaHoro
Copy link
Contributor Author

KireinaHoro commented Apr 22, 2024

Thanks for the review!

So maybe a more sane aproache overall would be to have a dedicated assignement operator ?

val miaou = CombInit(original)
val miaou.x :=!! 0x42 //Override value of x completly

This I agree, mapElement on MultiData is in principle equivalent to this and I think this syntax is clearer. mapPayloadElement is just a shorthand of translateWith and the override assign, but I find it a pattern quite frequent so I just added a shorthand. One can argue that it's not that long to keep as is...

I need a pipeline stage between remap and the downstream operators.

For timing reasons ?

Yes. In the AXI slave factory originally the unburst and downstream actions are in the same clock cycle and it was still passable; chaining in remapping the address made the comb path very long (>15 stages).

@Dolu1990
Copy link
Member

One can argue that it's not that long to keep as is..

at the same time short hands are good <3
Hmmm, i don't know. Have you seen in the stream library, there kinda a similar short hand. Stream.map (that is a bad name)
But isn't targeting a single element of the payload.

What's about :

val wuff = miaou.combInit(_.x := miaou.y + 0x42)

chaining in remapping the address made the comb path very long (>15 stages).

Hmmm, else an alternative is to RegNext the result of the remap, and systematicaly block the access first cycle (half throughput)

@KireinaHoro
Copy link
Contributor Author

What's about :

val wuff = miaou.combInit(_.x := miaou.y + 0x42)

This would then become:

val remapped = awChan.map { aw =>
  aw.combInit(_.addr := aw.addr + 0x42)
}

Feels quite concise already, I feel like I can take this. It has the advantage of not having to specialise on Stream[T <: MultiData].

Hmmm, else an alternative is to RegNext the result of the remap, and systematicaly block the access first cycle (half throughput)

Yeah but that would require changing everywhere readAddress is used, in the generic BusSlaveFactory, no? That feels way more invasive...

@Dolu1990
Copy link
Member

Yeah but that would require changing everywhere readAddress is used, in the generic BusSlaveFactory, no? That feels way more invasive...

Hmm wouldn't it just be a specific implementation in the frontend of Axi4SlaveFactory ?
I don't know.

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

2 participants