BusSlaveFactory driveFlow does not honor byteEnable

[KireinaHoro]

It seems like the driveFlow function in BusSlaveFactory does not honour byteEnable:

SpinalHDL/lib/src/main/scala/spinal/lib/bus/misc/BusSlaveFactory.scala

Lines 512 to 516 in 68b6158

	if (wordCount == 1){
	that.valid := False
	onWrite(address){ that.valid := True }
	nonStopWrite(that.payload, bitOffset)
	}else{

I had the impression that the valid bit in the flow should not be on when byteEnable was all zero for the width of the flow. This scenario is tricky however, since it's not clear how to properly handle partial byteEnables on a Flow:

• do we trigger the Flow but with false data on those that the byte is not enabled (similar to current situation), or
• do we drop the write completely?

A real-scenario where this would matter: I am using a Axi4SlaveFactory after an AXI width converter. For a wide write request with some bytes disabled with strb, the AXI width converter issues requests that contains beats with zero strb. This then falsely triggers the downstream Flow.

[Dolu1990]

Flow them self aren't made for byteenable, could add additional versions which would work on specific payload type.

A real-scenario where this would matter: I am using a Axi4SlaveFactory after an AXI width converter. For a wide write request with some bytes disabled with strb, the AXI width converter issues requests that contains beats with zero strb. This then falsely triggers the downstream Flow.

Yes you are right, all bytemask being low could / should be used to not have valid = '1'

which transactions make the axi width converter make all strb going low in some transactions (example) ?

[KireinaHoro]

I'm not actually using the built-in width converter / unburstifier / ..., but an external IP instead. An example of the all low strb transaction is as follows in a downsize from 512 to 64:

You can see that s_axi_wstrb is 0xff000000, which gets translated to the 4th beat having m_axi_wstrb being 0xff. The other beats have wready && wvalid but wstrb == 0.

[Dolu1990]

I would say, you may get peripherals which trigger actions when a register is written and on which byte mask make no sense, for instance the PLIC interrupt reservation.
So there is a few way to fix the situation i would say :

• patching every peripherals to not trigger side effect when byte mask is 0, but that seems the worst option to me
• but more fondamentaly, i think the main issue is the s_axi transaction we can see on the wave, with its awsize of 64 bytes.

What kind of master generates it ?

[KireinaHoro]

The offending request comes from the Xilinx XDMA bridge; it issues narrow writes with 64B AWSIZE but only partial WSTRB (in this case 8 bytes). Per my understanding this is allowed by AXI (see this thread: WSTRB is allowed to go low during a transaction to signal only some bytes are valid).

Regarding a possible solution, I agree that it's bad to change behaviour of existing peripherals that expect the current behaviour. I'm working on a pull request to add an optional parameter to the method (that defaults to the old behaviour); what do you think?

[Dolu1990]

Ok for me ^^

Else, a general fix would be to implement an AXI bridge which filter this kind of accesses, so you would just have to put the bridge in between the width adapter and peripherals.