|
From: | Vikram Garhwal |
Subject: | Re: [QEMU][PATCH 2/5] hw/net/can: Introduce Xilinx Versal CANFD controller |
Date: | Fri, 21 Oct 2022 15:36:28 -0700 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 |
Hi Peter, On 9/22/22 7:46 AM, Peter Maydell wrote:
On Sat, 10 Sept 2022 at 07:13, Vikram Garhwal <vikram.garhwal@amd.com> wrote:The Xilinx Versal CANFD controller is developed based on SocketCAN, QEMU CAN bus implementation. Bus connection and socketCAN connection for each CAN module can be set through command lines. Example for using connecting CANFD0 and CANFD1 to same bus: -machine xlnx-versal-virt -object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus To create virtual CAN on the host machine, please check the QEMU CAN docs: https://github.com/qemu/qemu/blob/master/docs/can.txtThat link is a 404. You could just give the relative path to the docs in the repo, which is docs/system/devices/can.rst For the machine specifics, you should include (either in the patch 4 where you add this to the xlnx-versal-virt board, or in a separate patch if it seems too big) updates to docs/system/arm/xlnx-versal-virt.rst which document the new functionality, including, if it's useful to users, some documentation of how to use it.
Sorry for delay in reply. I was waiting for other to review before sending v2. But it's been a few weeks so I will send v2 today EoD or tomorrow.
I will move the documentation part in xlnx-versal-virt.rst file.
+/* To avoid the build issues on Windows machines. */ +#undef ERRORWhat ?
The ERROR field of the INTERRUPT_STATUS_REGISTER colides with a macro in the Windows build enviorment. Build fails due to incorrect size of ERROR on Windows systems.
No, both are very different controllers. Versal SoCs CANFD has lot more registers, RX/TX FIFOs compare to ZynqMP CAN device. Interrupt handling, overflow conditions are also different. I will recheck if can find some common function and can use for both with making the code more complex.+static void canfd_config_mode(XlnxVersalCANFDState *s) +{ + register_reset(&s->reg_info[R_ERROR_COUNTER_REGISTER]); + register_reset(&s->reg_info[R_ERROR_STATUS_REGISTER]); + register_reset(&s->reg_info[R_STATUS_REGISTER]); + + /* Put XlnxVersalCANFDState in configuration mode. */ + ARRAY_FIELD_DP32(s->regs, STATUS_REGISTER, CONFIG, 1); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, WKUP, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, SLP, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, BSOFF, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, ERROR, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXFOFLW, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXFOFLW_1, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXOK, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, TXOK, 0); + ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, ARBLST, 0); + + /* Clear the time stamp. */ + ptimer_transaction_begin(s->canfd_timer); + ptimer_set_count(s->canfd_timer, 0); + ptimer_transaction_commit(s->canfd_timer); + + canfd_update_irq(s); +} +A lot of this looks like it's just copy-and-pasted code from the existing hw/net/can/xlnx-zynqmp-can.c. Is this just an updated/extra-features version of that device? Is there some way we can share the code rather than duplicating 2000-odd lines ?
+#ifndef HW_CANFD_XILINX_H +#define HW_CANFD_XILINX_H + +#include "hw/register.h" +#include "hw/ptimer.h" +#include "net/can_emu.h" +#include "hw/qdev-clock.h" + +#define TYPE_XILINX_CANFD "xlnx.versal-canfd"Should this be a dot or a comma? The codebase has examples of both for xlnx devices :-(
You are right about dot or comma comment. We need to harmonize these across all Xilinx devices. For Versal board, recently introduced machine has all dots, so I am following that. Let me check with team here and perhaps we can send another series to address the dot/comma variations.
+ +#define XILINX_CANFD(obj) \ + OBJECT_CHECK(XlnxVersalCANFDState, (obj), TYPE_XILINX_CANFD)Please use OBJECT_DECLARE_SIMPLE_TYPE() rather than defining the cast macro by hand.+ +#define NUM_REGS_PER_MSG_SPACE 18 +#define MAX_NUM_RX 64 +#define CANFD_TIMER_MAX 0XFFFFULDon't use capital X in the 0x hex prefix, please.+#define CANFD_DEFAULT_CLOCK (24 * 1000 * 1000) + +/* 0x4144/4 + 1 + (64 - 1) * 18 + 1. */This comment isn't very informative. The #define itself is much better because it uses symbolic constants. What is the magic number 0x4144. It should either be defined via some kind of symbolic constant, or if that's not possible at least explained in a comment.
Rest of the comments will be resolved in v2.
+#define XLNX_VERSAL_CANFD_R_MAX (0x4144 / 4 + \ + ((MAX_NUM_RX - 1) * NUM_REGS_PER_MSG_SPACE) + 1)thanks -- PMM
[Prev in Thread] | Current Thread | [Next in Thread] |