Hi Pavel,
(Dropping Vikram from the email chain; I received "recipient not found"
errors from AMD's mail servers in response to all of my patches)
On 8/20/2024 11:57 PM, Pavel Pisa wrote:
> Hello Doug Brown,
>
> On Friday 16 of August 2024 18:35:02 Doug Brown wrote:
>> - if (frame->flags == QEMU_CAN_FRMF_TYPE_FD) {
>> + if (frame->flags & QEMU_CAN_FRMF_TYPE_FD) {
>
> Reviewed-by: Pavel Pisa <p...@cmp.felk.cvut.cz>
>
> That is a great catch, I have overlooked this in previous
> review of the Xilinx code.
Thank you for reviewing!
> When I look into hw/net/can/xlnx-versal-canfd.c functions
> regs2frame and store_rx_sequential then I see missing
> handling of flags QEMU_CAN_FRMF_ESI and QEMU_CAN_FRMF_BRS.
Nice find. It looks like it would be pretty straightforward to add
support for those flags. As far as I can tell they map directly to
register bits.
> In the function regs2frame is missing even initialization
> of frame->flags = 0 at the start, which I expect should be there.
> The
> frame->flags = QEMU_CAN_FRMF_TYPE_FD;
> should be then
> frame->flags |= QEMU_CAN_FRMF_TYPE_FD;
You're absolutely right. It looks like frame->flags isn't being
initialized at all when it's a non-FD frame. I can also take care of
this. I'll wait and see how the review goes for the other patches, and I
can add another patch to fix these flag issues in the next version of
the series.
> As for the DLC conversion, there are functions
>
> frame->can_dlc = can_dlc2len(xxxx)
> XXX = can_len2dlc(frame->can_dlc);
>
> provided by net/can/can_core.c
Nice! It seems like using these functions could simplify this code quite
a bit, by eliminating the need for canfd_dlc_array. I can add this as
another patch for the next version.
> I am not sure how much competent I am for the rest of the patches,
> because I do not know XilinX IP core so well. Review by Vikram Garhwal
> or somebody else from AMD/XilinX is more valueable there.
> But I can add my ACK there based on rough overview.
Thanks! I also see that Francisco reviewed a couple of the patches --
thanks Francisco! I will wait and see how review goes on the others.
For what it's worth, I was stress testing this in QEMU today and found
another issue with the FIFO read_index and store_index calculations and
the logic that wraps them around to 0. I will submit the fix for this
problem as another patch in the next version of the series (or a new
series if that's more convenient).
Doug
