User field of TL Channels does not work (No connection for user fields in Xbar)

[ksungkeun84]

Background Work

• Yes, I searched the mailing list
• Yes, I searched prior issues
• Yes, I searched the documentation

Chipyard Version and Hash

Release: 1.11.0
Hash: ac58f38

OS Setup

uname -a
Linux sk84 6.5.0-15-generic #15 SMP PREEMPT_DYNAMIC Fri Jan 12 18:54:30 UTC 2 x86_64 GNU/Linux

lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 22.04.3 LTS
Release: 22.04
Codename: jammy

Other Setup

Bug description

I have made my own custom bundle fields (MyUserField) in TLBundleA and a MMIO device (TLUserFieldReader) to service a read request (Get) from DCache along with the custom user field over Tile Link (Channel A).

I couldn't simple share the code changes as As the modifications are across in both chipyard and rocket-chip. Therefore, I have attached the diff file here.

Then, I run a test program on a verilator simulation. I've attached the test program as well.

TLUserField_Chipyard.patch
TLUserField_Rocket.patch

Changes in Chipyard repo

diff --git a/generators/chipyard/src/main/scala/DigitalTop.scala b/generators/chipyard/src/main/scala/DigitalTop.scala
index bd82585b..a696a990 100644
--- a/generators/chipyard/src/main/scala/DigitalTop.scala
+++ b/generators/chipyard/src/main/scala/DigitalTop.scala
@@ -31,6 +31,7 @@ class DigitalTop(implicit p: Parameters) extends ChipyardSystem
   with icenet.CanHavePeripheryIceNIC // Enables optionally adding the IceNIC for FireSim
   with chipyard.example.CanHavePeripheryInitZero // Enables optionally adding the initzero example widget
   with chipyard.example.CanHavePeripheryGCD // Enables optionally adding the GCD example widget
+  with chipyard.example.CanHavePeripheryTLUserFieldReader // Enables optionally adding TLUserFieldReader
   with chipyard.example.CanHavePeripheryStreamingFIR // Enables optionally adding the DSPTools FIR example widget
   with chipyard.example.CanHavePeripheryStreamingPassthrough // Enables optionally adding the DSPTools streaming-passthrough example widget
   with nvidia.blocks.dla.CanHavePeripheryNVDLA // Enables optionally having an NVDLA
diff --git a/generators/chipyard/src/main/scala/config/TLUserFieldConfig.scala b/generators/chipyard/src/main/scala/config/TLUserFieldConfig.scala
new file mode 100644
index 00000000..af56af06
--- /dev/null
+++ b/generators/chipyard/src/main/scala/config/TLUserFieldConfig.scala
@@ -0,0 +1,8 @@
+package chipyard
+
+import org.chipsalliance.cde.config.{Config}
+import freechips.rocketchip.diplomacy.{AsynchronousCrossing}
+
+class TLUserFieldReaderConfig extends Config(
+  new chipyard.RocketConfig ++
+  new chipyard.example.WithTLUserFieldReader)
diff --git a/generators/chipyard/src/main/scala/example/TLUserFieldReader.scala b/generators/chipyard/src/main/scala/example/TLUserFieldReader.scala
new file mode 100644
index 00000000..a13a6152
--- /dev/null
+++ b/generators/chipyard/src/main/scala/example/TLUserFieldReader.scala
@@ -0,0 +1,86 @@
+package chipyard.example
+
+import chisel3._
+import org.chipsalliance.cde.config.{Field, Parameters, Config}
+import freechips.rocketchip.subsystem._
+import freechips.rocketchip.diplomacy._
+import freechips.rocketchip.tilelink._
+import freechips.rocketchip.util._
+
+case class TLUserFieldReaderParams(
+  address: BigInt = 0x30000,
+  size: Int = 0x10000)
+
+class TLUserFieldReader(val base: BigInt, val size: Int, val beatBytes: Int = 1)(implicit p: Parameters) extends LazyModule
+{
+  val resources: Seq[Resource]= new SimpleDevice("tluserfield", Seq("sifive,tluserfield-0")).reg("mem")
+  val node = TLManagerNode(
+    Seq(
+      TLSlavePortParameters.v1(
+        Seq(
+          TLSlaveParameters.v1(
+            address     = List(AddressSet(base, size-1)),
+            resources   = resources,
+            regionType  = RegionType.UNCACHED,
+            executable  = false,
+            supportsGet = TransferSizes(1, beatBytes),
+            fifoId      = Some(0)
+          )
+        ),
+        beatBytes = beatBytes,
+        requestKeys = Seq(MyUserFieldKey)
+      )
+    )
+  )
+
+  lazy val module = new TLUserFieldReaderModuleImpl(this)
+}
+  class TLUserFieldReaderModuleImpl(outer: TLUserFieldReader) extends LazyModuleImp(outer) {
+
+    val (in, edge) = outer.node.in(0)
+
+    in.d.valid := in.a.valid
+    in.a.ready := in.d.ready
+
+    val myfield = Wire(UInt(8.W))
+    in.a.bits.user.lift(MyUserFieldKey).foreach { x =>
+      myfield := x.myfield
+    }
+    in.d.bits := edge.AccessAck(in.a.bits, myfield)
+
+    // Tie off unused channels
+    in.b.valid := false.B
+    in.c.ready := true.B
+    in.e.ready := true.B
+  }
+
+case object TLUserFieldReaderKey extends Field[Option[TLUserFieldReaderParams]](None)
+
+trait CanHavePeripheryTLUserFieldReader { this: BaseSubsystem =>
+  private val portName = "TLUserFieldReader"
+
+  val tluserfield_busy = p(TLUserFieldReaderKey) match {
+    case Some(params) => {
+      val tluserfield = pbus { LazyModule(new TLUserFieldReader(params.address, params.size, pbus.beatBytes)(p)) }
+      pbus.coupleTo(portName) { tluserfield.node := TLFragmenter(pbus.beatBytes, pbus.blockBytes) := _ }
+      val pbus_io = pbus { InModuleBody {
+        val busy = IO(Output(Bool()))
+        busy := false.B
+        busy
+      }}
+      val tluserfield_busy = InModuleBody {
+        val busy = IO(Output(Bool())).suggestName("tluserfieldreader_busy")
+        busy := pbus_io
+        busy
+      }
+      Some(tluserfield_busy)
+    }
+    case None => {
+      None
+    }
+  }
+}
+
+class WithTLUserFieldReader extends Config((site, here, up) => {
+  case TLUserFieldReaderKey => Some(TLUserFieldReaderParams(address = 0x30000, size = 0x10000))
+})
diff --git a/tests/Makefile b/tests/Makefile
index 1c6df31b..616b0178 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -29,7 +29,7 @@ include libgloss.mk
 
 PROGRAMS = pwm blkdev accum charcount nic-loopback big-blkdev pingd \
            streaming-passthrough streaming-fir nvdla spiflashread spiflashwrite fft gcd \
-           hello mt-hello symmetric
+           hello mt-hello symmetric tluserfield
 
 
 .DEFAULT_GOAL := default
diff --git a/tests/tluserfield.c b/tests/tluserfield.c
new file mode 100644
index 00000000..f392c076
--- /dev/null
+++ b/tests/tluserfield.c
@@ -0,0 +1,11 @@
+#include <stdio.h>
+#include "mmio.h"
+
+int main(void)
+{
+  printf("Start TLUserField Test...\n");
+  printf("Read %#x, result: %#x\n", 0x30008, reg_read32(0x30008));
+  printf("Read %#x, result: %#x\n", 0x30010, reg_read32(0x30010));
+  return 0;
+}
+// DOC include end: GCD test

Changes in rocket-chip repo

diff --git a/src/main/scala/rocket/DCache.scala b/src/main/scala/rocket/DCache.scala
index 4d9195cf9..45f937ce4 100644
--- a/src/main/scala/rocket/DCache.scala
+++ b/src/main/scala/rocket/DCache.scala
@@ -620,6 +620,10 @@ class DCacheModule(outer: DCache) extends HellaCacheModule(outer) {
     x.secure      := true.B
   }
 
+  tl_out_a.bits.user.lift(MyUserFieldKey).foreach { x =>
+    x.myfield := tl_out_a.bits.address
+  }
+
   // Set pending bits for outstanding TileLink transaction
   val a_sel = UIntToOH(a_source, maxUncachedInFlight+mmioOffset) >> mmioOffset
   when (tl_out_a.fire) {
diff --git a/src/main/scala/rocket/HellaCache.scala b/src/main/scala/rocket/HellaCache.scala
index 15cb4b6ac..380855ed4 100644
--- a/src/main/scala/rocket/HellaCache.scala
+++ b/src/main/scala/rocket/HellaCache.scala
@@ -207,7 +207,7 @@ abstract class HellaCache(tileId: Int)(implicit p: Parameters) extends LazyModul
   val node = TLClientNode(Seq(TLMasterPortParameters.v1(
     clients = cacheClientParameters ++ mmioClientParameters,
     minLatency = 1,
-    requestFields = tileParams.core.useVM.option(Seq()).getOrElse(Seq(AMBAProtField())))))
+    requestFields = tileParams.core.useVM.option(Seq(MyUserField(8))).getOrElse(Seq(AMBAProtField())))))
 
   val hartIdSinkNodeOpt = cfg.scratch.map(_ => BundleBridgeSink[UInt]())
   val mmioAddressPrefixSinkNodeOpt = cfg.scratch.map(_ => BundleBridgeSink[UInt]())
diff --git a/src/main/scala/util/BundleMap.scala b/src/main/scala/util/BundleMap.scala
index 2b8dc0a19..6ebcfbea4 100644
--- a/src/main/scala/util/BundleMap.scala
+++ b/src/main/scala/util/BundleMap.scala
@@ -87,6 +87,14 @@ sealed class BundleKey[T <: Data](val name: String) extends BundleKeyBase
 abstract class ControlKey[T <: Data](name: String) extends BundleKey[T](name) with IsControlKey
 abstract class DataKey   [T <: Data](name: String) extends BundleKey[T](name) with IsDataKey
 
+class MyUserFieldBundle(width: Int) extends Bundle {
+  val myfield= UInt(width.W)
+}
+case object MyUserFieldKey extends ControlKey[MyUserFieldBundle]("myuserfield")
+case class MyUserField(width: Int) extends BundleField[MyUserFieldBundle](MyUserFieldKey, Output((new MyUserFieldBundle(width))), x => {
+  x.myfield:= 0x0.U
+})
+
 /* Signals can be further categorized in a request-response protocol:
  *  - request fields flow from master to slave
  *  - response fields flow from slave to master

Current Behavior

The test program reads addresses at 0x3008, 0x3010, etc. which are mapped to my device (TLUserFieldReader). Then, the device simply read the user field (MyUserField) and put it to the data field of the response.
In DCache where the read request is created, lower one byte of target address is stored in the user field.

Therefore, I expected for the program to print 0x8 and 0x10 but the current results are both 0x0.

Test Result

[UART] UART0 is here (stdin/stdout).
Start TLUserField Test...
Read 0x30008, result: 0x0
Read 0x30010, result: 0x00

Expected Behavior

See the current behavior section.

Other Information

I debugged my self and figure out the reason. In Xbar (Xbar.scala), user fields of in and out are not connected even if there are custom user field.
My suggestion is that Xbar connects user field if there are list of the custom user field.
I've attached the patch that I'd like to contribute.

Please review my bug report and solution.
If you like my device (TLUserFieldReader) and related codes, it would good to be part of chipyard examples.

There might be misunderstanding something about this issue. It would be appreciated if you recommend a better way to fix it.
Xbar.patch

Bug Fix in Xbar

diff --git a/src/main/scala/tilelink/Xbar.scala b/src/main/scala/tilelink/Xbar.scala
index 1d6a82bbd..e6c16d944 100644
--- a/src/main/scala/tilelink/Xbar.scala
+++ b/src/main/scala/tilelink/Xbar.scala
@@ -159,6 +159,12 @@ object TLXbar
       if (connectAIO(i).exists(x=>x)) {
         in(i).a.squeezeAll.waiveAll :<>= io_in(i).a.squeezeAll.waiveAll
         in(i).a.bits.user := DontCare
+        // If there are user defined bundles in both sides, connect them.
+        io_in(i).a.bits.user.keydata.foreach { case (io_in_key, io_in_bundle) =>
+          in(i).a.bits.user.lift(io_in_key).foreach { in_bundle =>
+            in_bundle <> io_in_bundle
+          }
+        }
         in(i).a.bits.source := io_in(i).a.bits.source | r.start.U
       } else {
         in(i).a := DontCare
@@ -180,6 +186,12 @@ object TLXbar
       if (connectCIO(i).exists(x=>x)) {
         in(i).c.squeezeAll.waiveAll :<>= io_in(i).c.squeezeAll.waiveAll
         in(i).c.bits.user := DontCare
+        // If there are user defined bundles in both sides, connect them.
+        io_in(i).c.bits.user.keydata.foreach { case (io_in_key, io_in_bundle) =>
+          in(i).c.bits.user.lift(io_in_key).foreach { in_bundle =>
+            in_bundle <> io_in_bundle
+          }
+        }
         in(i).c.bits.source := io_in(i).c.bits.source | r.start.U
       } else {
         in(i).c := DontCare
@@ -216,6 +228,12 @@ object TLXbar
       if (connectAOI(o).exists(x=>x)) {
         io_out(o).a.squeezeAll.waiveAll :<>= out(o).a.squeezeAll.waiveAll
         out(o).a.bits.user := DontCare
+        // If there are user defined bundles in both sides, connect them.
+        out(o).a.bits.user.keydata.foreach { case (out_key, out_bundle) =>
+          io_out(o).a.bits.user.lift(out_key).foreach { io_out_bundle =>
+            io_out_bundle <> out_bundle
+          }
+        }
       } else {
         out(o).a := DontCare
         io_out(o).a := DontCare
@@ -235,6 +253,12 @@ object TLXbar
       if (connectCOI(o).exists(x=>x)) {
         io_out(o).c.squeezeAll.waiveAll :<>= out(o).c.squeezeAll.waiveAll
         out(o).c.bits.user := DontCare
+        // If there are user defined bundles in both sides, connect them.
+        out(o).c.bits.user.keydata.foreach { case (out_key, out_bundle) =>
+          io_out(o).c.bits.user.lift(out_key).foreach { io_out_bundle =>
+            io_out_bundle <> out_bundle
+          }
+        }
       } else {
         out(o).c  := DontCare
         io_out(o).c  := DontCare

Test result after bug fix

[UART] UART0 is here (stdin/stdout).
Start TLUserField Test...
Read 0x30008, result: 0x8
Read 0x30010, result: 0x10

[jerryz123]

Can you open a PR with this fix in the rocket-chip repo?

[ksungkeun84]

@jerryz123 Sure I'll make a PR in the rocket-chip repo. Thanks for your response :D

[ksungkeun84]

@jerryz123 Hi Jerry, I've made a PR in the rocket-chip. Could you please review it?
chipsalliance/rocket-chip#3637