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>
---
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.1Hi 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
>
On Tuesday 27 of August 2024 05:49:27 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>
Generally, I agree that index should wrap up for cyclic FIFO
implementation and change looks logical to me but I do not
see and studied all consequences related to emulated HW.
If that is confirmed or corrected by somebody from AMD/XilinX,
it would be better. I can find more time to do deeper analysis
if no other looks into the whole code.
Best wishes,
Pavel
--
Pavel Pisa
phone: +420 603531357
e-mail: pisa@cmp.felk.cvut.cz
Department of Control Engineering FEE CVUT
Karlovo namesti 13, 121 35, Prague 2
university: http://control.fel.cvut.cz/
personal: http://cmp.felk.cvut.cz/~pisa
social: https://social.kernel.org/ppisa
projects: https://www.openhub.net/accounts/ppisa
CAN related:http://canbus.pages.fel.cvut.cz/
RISC-V education: https://comparch.edu.cvut.cz/
Open Technologies Research Education and Exchange Services
https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home
Hi Pavel, On 8/29/2024 6:24 AM, Pavel Pisa wrote: > Generally, I agree that index should wrap up for cyclic FIFO > implementation and change looks logical to me but I do not > see and studied all consequences related to emulated HW. > > If that is confirmed or corrected by somebody from AMD/XilinX, > it would be better. I can find more time to do deeper analysis > if no other looks into the whole code. Thank you for your reviews! I agree. I don't know how the hardware is actually implemented internally so it would be great if someone from AMD could confirm that this change 100% matches the hardware. Unfortunately Versal dev boards are very expensive so I can't test hardware myself. What I can say is that before I implemented the index calculation fixes in this patch, the Linux driver in the Xilinx kernel was seeing incorrect messages. Now it works fine even with heavy bus load. Doug
© 2016 - 2024 Red Hat, Inc.