CanAttachTile.crossingParams is inappropriately used.

[DecodeTheEncoded]

The RocketTile has servial connections with the context:

    connectMasterPorts(domain, context)
    connectSlavePorts(domain, context)
    connectInterrupts(domain, context)
    connectPRC(domain, context)
    connectOutputNotifications(domain, context)
    connectInputConstants(domain, context)
    connectTrace(domain, context)

But there is only one crossingParams.crossingType to control the CDC between boundries. This is inappropriate and may lead to bugs in some scenarios, for example: The CLINT is at CBUS the code below made assumption that the crossingType is the crossingType between CBUS and the Tile:

    // 2. The CLINT and PLIC output interrupts are synchronous to the CLINT/PLIC respectively,
    //    so might need to be synchronized depending on the Tile's crossing type.

    //    From CLINT: "msip" and "mtip"
    context.msipDomain {
      domain.crossIntIn(crossingParams.crossingType, domain.element.intInwardNode) :=
        context.msipNodes(domain.element.tileId)
    }

However, when connecting the tile and the MasterPort, the code made assumption that the crossingType should indicate the crossing between Tile And SBUS:

  /** Connect the port where the tile is the master to a TileLink interconnect. */
  def connectMasterPorts(domain: TilePRCIDomain[TileType], context: Attachable): Unit = {
    implicit val p = context.p
    val dataBus = context.locateTLBusWrapper(crossingParams.master.where)
    dataBus.coupleFrom(tileParams.baseName) { bus =>
      bus :=* crossingParams.master.injectNode(context) :=* domain.crossMasterPort(crossingParams.crossingType)
    }
  }

This works fine if all buses are at the same clock domain which is the default configuration in simulation, but in real world scenarios, this may lead to bugs. Especially when using RationalCrossing.SlowToFast or RationalCrossing.FastToSlow configurations.
Can anyone in the team confirm my understandings, if I am right, I will make a PR and fix this, Thanks.
@sequencer @jerryrzhao @tianrui-wei

[jerryz123]

This seems likes a bug to me.

I believe allowing the cbus and sbus to be on different clock domains is the real unnecessary feature that adds unnecessary complexity.

I'd prefer a solution that fixes the cbus domain to the sbus domain.

[DecodeTheEncoded]

@jerryz123 Thanks for you reply, Jerry. I agree that the CBUS should be in the same clock domain with SBUS, there are ways to do it, maybe using a ClockGroupCombiner in a mandatory way.
Also, Do you think we should do this for FBUS, I don't think FBUS should be necessarily in the same clock domain with CBUS(SBUS). But code below in the Debug Module seems assume that the the CBUS and FBUS are in the same domain(The Inner part of Debug Module is at CBUS domain, and the SBA feature if enabled will master FBUS, therefore this may be a problem).

    tlDM.dmInner.dmInner.sb2tlOpt.foreach { sb2tl  =>
      locateTLBusWrapper(p(ExportDebug).masterWhere).coupleFrom("debug_sb") {
        _ := TLWidthWidget(1) := sb2tl.node
      }
    }

[jerryz123]

That does look like a bug to me as well. Let's compartmentalize the changes.

To disallow cbus/sbus crossings, I think its best to replace all sbusToCbusXType with NoCrossing in this file: https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/subsystem/BusTopology.scala.

[DecodeTheEncoded]

That does look like a bug to me as well. Let's compartmentalize the changes.

To disallow cbus/sbus crossings, I think its best to replace all sbusToCbusXType with NoCrossing in this file: https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/subsystem/BusTopology.scala.

This will be a very convinient change, I think it's also appropriate to add some require to make sure users do not specify other crossing types, things will go wired if they do.
Thanks for your reply, Jerry 👍