Hi,
On Sun, Nov 19, 2023 at 11:51:01PM +0100, Philippe Mathieu-Daudé wrote:
> Per https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Message-Format
>
> Message Format
>
> The same message format is used for RXFIFO, TXFIFO, and TXHPB.
> Each message includes four words (16 bytes). Software must read
> and write all four words regardless of the actual number of data
> bytes and valid fields in the message.
>
> There is no mention in this reference manual about what the
> hardware does when not all four words are written. To fix the
> reported underflow behavior when DATA2 register is written,
> I choose to fill the data with the previous content of the
> ID / DLC / DATA1 registers, which is how I expect hardware
> would do.
>
> Note there is no hardware flag raised under such condition.
>
> Reported-by: Qiang Liu <cyruscy...@gmail.com>
> Fixes: 98e5d7a2b7 ("hw/net/can: Introduce Xilinx ZynqMP CAN controller")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1425
> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> Francisco Iglesias <francisco.igles...@amd.com>
> ---
> hw/net/can/xlnx-zynqmp-can.c | 49 +++++++++++++++++++++++++++++++++---
> 1 file changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
> index e93e6c5e19..58938b574e 100644
> --- a/hw/net/can/xlnx-zynqmp-can.c
> +++ b/hw/net/can/xlnx-zynqmp-can.c
> @@ -434,6 +434,51 @@ static bool tx_ready_check(XlnxZynqMPCANState *s)
> return true;
> }
>
> +static void read_tx_frame(XlnxZynqMPCANState *s, Fifo32 *fifo, uint32_t
> *data)
> +{
> + unsigned used = fifo32_num_used(fifo);
> + bool is_txhpb = fifo == &s->txhpb_fifo;
> +
> + assert(used > 0);
> + used %= CAN_FRAME_SIZE;
> +
> + /*
> + * Frame Message Format
> + *
> + * Each frame includes four words (16 bytes). Software must read and
> write
> + * all four words regardless of the actual number of data bytes and valid
> + * fields in the message.
> + * If software misbehave (not writting all four words), we use the
> previous
%s/writting/writing
> + * registers content to initialize each missing word.
> + */
> + if (used > 0) {
> + /* ID, DLC, DATA1 missing */
Code is correct but i feel This comment is confusing. ID is missing for sure
if user > 0 but same is not the case for DLC and DATA1.
> + data[0] = s->regs[is_txhpb ? R_TXHPB_ID : R_TXFIFO_ID];
> + } else {
> + data[0] = fifo32_pop(fifo);
> + }
> + if (used == 1 || used == 2) {
> + /* DLC, DATA1 missing */
Same here DLC is missing for sure but DATA1 is not for used == 2.
> + data[1] = s->regs[is_txhpb ? R_TXHPB_DLC : R_TXFIFO_DLC];
> + } else {
> + data[1] = fifo32_pop(fifo);
> + }
> + if (used == 1) {
> + /* DATA1 missing */
May be we remove all these individual comments and write a common comment before
the first check(if(used > 0)). Something like this:
/*
* If used is 1 then ID, DLC and DATA1 are missing.
*
* if used is 2 then ID and DLC are missing.
*
* if used is 3 then only ID is missing.
*/
Code looks correct to me. So, With above minor changes in code comments:
Reviewed-by: Vikram Garhwal <vikram.garh...@amd.com>
> + data[2] = s->regs[is_txhpb ? R_TXHPB_DATA1 : R_TXFIFO_DATA1];
> + } else {
> + data[2] = fifo32_pop(fifo);
> + }
> + /* DATA2 triggered the transfer thus is always available */
> + data[3] = fifo32_pop(fifo);
> +
> + if (used) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Incomplete CAN frame (only %u/%u slots used)\n",
> + TYPE_XLNX_ZYNQMP_CAN, used, CAN_FRAME_SIZE);
> + }
> +}
> +
> static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo)
> {
> qemu_can_frame frame;
> @@ -451,9 +496,7 @@ static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32
> *fifo)
> }
>
> while (!fifo32_is_empty(fifo)) {
> - for (i = 0; i < CAN_FRAME_SIZE; i++) {
> - data[i] = fifo32_pop(fifo);
> - }
> + read_tx_frame(s, fifo, data);
>
> if (ARRAY_FIELD_EX32(s->regs, STATUS_REGISTER, LBACK)) {
> /*
> --
> 2.41.0
>
