Re: [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues

[Francisco Iglesias]

Hi Doug,

On Mon, Aug 26, 2024 at 08:49:27PM -0700, Doug Brown wrote:
> The read index should not be changed when storing a new message into the
> RX or TX FIFO. Changing it at this point will cause the reader to get
> out of sync. The wrapping of the read index is already handled by the
> pre-write functions for the FIFO status registers anyway.
> 
> Additionally, the calculation for wrapping the store index was off by
> one, which caused new messages to be written to the wrong location in
> the FIFO. This caused incorrect messages to be delivered.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

Thanks a lot for the fixes and sorry for the delayed review!

Best regards,
Francisco


> ---
>  hw/net/can/xlnx-versal-canfd.c | 36 +++-------------------------------
>  1 file changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
> index 589c21db69..c291a0272b 100644
> --- a/hw/net/can/xlnx-versal-canfd.c
> +++ b/hw/net/can/xlnx-versal-canfd.c
> @@ -1144,18 +1144,8 @@ static void update_rx_sequential(XlnxVersalCANFDState 
> *s,
>              read_index = ARRAY_FIELD_EX32(s->regs, RX_FIFO_STATUS_REGISTER, 
> RI);
>              store_index = read_index + fill_level;
>  
> -            if (read_index == s->cfg.rx0_fifo - 1) {
> -                /*
> -                 * When ri is s->cfg.rx0_fifo - 1 i.e. max, it goes cyclic 
> that
> -                 * means we reset the ri to 0x0.
> -                 */
> -                read_index = 0;
> -                ARRAY_FIELD_DP32(s->regs, RX_FIFO_STATUS_REGISTER, RI,
> -                                 read_index);
> -            }
> -
>              if (store_index > s->cfg.rx0_fifo - 1) {
> -                store_index -= s->cfg.rx0_fifo - 1;
> +                store_index -= s->cfg.rx0_fifo;
>              }
>  
>              store_location = R_RB_ID_REGISTER +
> @@ -1172,18 +1162,8 @@ static void update_rx_sequential(XlnxVersalCANFDState 
> *s,
>                                            RI_1);
>              store_index = read_index + fill_level;
>  
> -            if (read_index == s->cfg.rx1_fifo - 1) {
> -                /*
> -                 * When ri is s->cfg.rx1_fifo - 1 i.e. max, it goes cyclic 
> that
> -                 * means we reset the ri to 0x0.
> -                 */
> -                read_index = 0;
> -                ARRAY_FIELD_DP32(s->regs, RX_FIFO_STATUS_REGISTER, RI_1,
> -                                 read_index);
> -            }
> -
>              if (store_index > s->cfg.rx1_fifo - 1) {
> -                store_index -= s->cfg.rx1_fifo - 1;
> +                store_index -= s->cfg.rx1_fifo;
>              }
>  
>              store_location = R_RB_ID_REGISTER_1 +
> @@ -1265,18 +1245,8 @@ static void tx_fifo_stamp(XlnxVersalCANFDState *s, 
> uint32_t tb0_regid)
>                            " Discarding the message\n");
>              ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, TXEOFLW, 1);
>          } else {
> -            if (read_index == s->cfg.tx_fifo - 1) {
> -                /*
> -                 * When ri is s->cfg.tx_fifo - 1 i.e. max, it goes cyclic 
> that
> -                 * means we reset the ri to 0x0.
> -                 */
> -                read_index = 0;
> -                ARRAY_FIELD_DP32(s->regs, TX_EVENT_FIFO_STATUS_REGISTER, 
> TXE_RI,
> -                                 read_index);
> -            }
> -
>              if (store_index > s->cfg.tx_fifo - 1) {
> -                store_index -= s->cfg.tx_fifo - 1;
> +                store_index -= s->cfg.tx_fifo;
>              }
>  
>              assert(store_index < s->cfg.tx_fifo);
> -- 
> 2.34.1
>