Re: [PATCH] hw/net/can: Add mcp25625 model
On Tue, Jan 17, 2023 at 07:16:35PM +0100, Pavel Pisa wrote:
> Dear Ben,
>
> sorry for longer response times...
I think we've both dropped the ball on this one, just got reminded about
this set and found it got deleted from work email.
We've done review upates and will try and get some branches out today and
maybe a new patch out for inclusion.
> On Tuesday 17 of January 2023 14:32:29 Ben Dooks wrote:
> > On 04/01/2023 12:22, Ben Dooks wrote:
> > > From: Ben Dooks
> > >
> > > Add support for Microchip MCP25625 SPI based CAN controller which is
> > > very similar to the MCP2515 (and covered by the same Linux driver).
> ...
> > Has anyone had chance to review this, it would be great to get
> > this moving along.
>
> Generally, I am happy that you consider use and extend our work.
>
> I have looked at the code. But even that implementation of CAN
> subsystem in QEMU was my idea and I led studnets working on
> it and sometimes heavily rewritten code to be acceptable,
> I am not QEMU expert and I have not studied its SSI subsystem
> so for comment from QEMU code requiremts I would be happy
> for some other to step in.
>
> I would like to test the peripheral. Please, can you try to elaborate
> and prepare description how to config QEMU for RPi emulation with
> mcp2515 overlay?
>
>
> https://github.com/raspberrypi/linux/blob/rpi-6.1.y/arch/arm/boot/dts/overlays/mcp2515-can0-overlay.dts
>
> I have RPi images and I have experience with this hardware.
> Even that I consider mcp2515 as really unfortunate solution
> and we have spent lot of time to help colleagues to enhance a little
> latencies of this solution when they chose that HW for serious
> wok instead of some NXP, TI, Xilinx or other sane SoC with CAN.
I had a quick look at setting an pi3b emulation but it does not currently
have an SSI port available by default. I will try and post the branch I
used with the SiFive Unmatched board later.
>
> https://dspace.cvut.cz/bitstream/handle/10467/68605/F3-DP-2017-Prudek-Martin-Dp_2017_prudek_martin.pdf
>
> But yes, people are using this chip a lot so it would worth to have
> emulation in QEMU. If the SPI connection is required then mcp251xfd
> seems to have chance for lower SPI transfer count overhead nd performance.
> But real SoC bus connected controllers are much better for serious
> project, if you design chip there are more cores available M-CAN,
> GRCAN and even our own CTU CAN FD which has already emulation in QEMU.
>
> Back to MCP25x1x. I have gone through code and try to understand
> the function. There is lot of connected to the locations in the
> registers maps in the chip
>
> > s->ssi_addr = 0x31;
A lot of these where basically used in one place and we map the regs
into internal data structs. Nazar and I have been through and tried
to tidy some of this up and make it more explicit what is being done.
> I have took manual but I think that it would help to add there comments
> with registers names or even use defines for these. But may it be,
> that for the easy ampping in the table and increment logic numbers
> are reasonable option... But comments with register symbolic names
> would help.
>
> The code does define only single property ("canbus") to select CAN
> bus to connect controller to. Mapping to the SPI peripheral is
> provided by device tree on QEMU side or by some other machine specific
> glue code? Please, can you provide more information for intended
> target use and RPi option to use for testing?
Yes, at the moment it seems that command-line mapping of SSI is not
easy, or well documented? Currently, other than the CS line that is
generally done with the controller, the only ouput needed is the IRQ
line. We do have the RXB0 and RXB1 outputs but they're not really needed
and only there for people to try if they want.
> As for the code, I have read it the first time and for full check
> I would need to spent more time with it. But I expect that
> functionality check with the respect to mcp25x1x datasheet has been
> done mainly by you so the full check bit by bit is not necessary.
> If there is some omitted case it would be (hopefully) found during
> code use. As for generic code style and redability, I see no problem.
> I expect that you have checked for QEMU style and if there has been
> some problem you have propably received QEMU CI and static analysis
> feedback.
>
> > +static void mcp25625_rx_into_buf(MCP25625State *s,
> > + const qemu_can_frame *frame,
> > + unsigned buffnr, int filthit)
> > +{
> > +struct rxbuff *rxbuff = >rxbuffs[buffnr];
> > +qemu_canid_t e_id, sidl, id, q_id = frame->can_id;
> > +unsigned len = frame->can_dlc;
> > +
>
> I would suggest to check for can_dlc > 8 in this function or in
> its caller (mcp25625_can_receive), to ensure that QEMU cannot be
> attacked by some malformed CAN message from the host kernel
> or other VM...
Ok, added.
> > +static ssize_t
Re: [PATCH] hw/net/can: Add mcp25625 model
Dear Ben,
sorry for longer response times...
On Tuesday 17 of January 2023 14:32:29 Ben Dooks wrote:
> On 04/01/2023 12:22, Ben Dooks wrote:
> > From: Ben Dooks
> >
> > Add support for Microchip MCP25625 SPI based CAN controller which is
> > very similar to the MCP2515 (and covered by the same Linux driver).
...
> Has anyone had chance to review this, it would be great to get
> this moving along.
Generally, I am happy that you consider use and extend our work.
I have looked at the code. But even that implementation of CAN
subsystem in QEMU was my idea and I led studnets working on
it and sometimes heavily rewritten code to be acceptable,
I am not QEMU expert and I have not studied its SSI subsystem
so for comment from QEMU code requiremts I would be happy
for some other to step in.
I would like to test the peripheral. Please, can you try to elaborate
and prepare description how to config QEMU for RPi emulation with
mcp2515 overlay?
https://github.com/raspberrypi/linux/blob/rpi-6.1.y/arch/arm/boot/dts/overlays/mcp2515-can0-overlay.dts
I have RPi images and I have experience with this hardware.
Even that I consider mcp2515 as really unfortunate solution
and we have spent lot of time to help colleagues to enhance a little
latencies of this solution when they chose that HW for serious
wok instead of some NXP, TI, Xilinx or other sane SoC with CAN.
https://dspace.cvut.cz/bitstream/handle/10467/68605/F3-DP-2017-Prudek-Martin-Dp_2017_prudek_martin.pdf
But yes, people are using this chip a lot so it would worth to have
emulation in QEMU. If the SPI connection is required then mcp251xfd
seems to have chance for lower SPI transfer count overhead nd performance.
But real SoC bus connected controllers are much better for serious
project, if you design chip there are more cores available M-CAN,
GRCAN and even our own CTU CAN FD which has already emulation in QEMU.
Back to MCP25x1x. I have gone through code and try to understand
the function. There is lot of connected to the locations in the
registers maps in the chip
> s->ssi_addr = 0x31;
I have took manual but I think that it would help to add there comments
with registers names or even use defines for these. But may it be,
that for the easy ampping in the table and increment logic numbers
are reasonable option... But comments with register symbolic names
would help.
The code does define only single property ("canbus") to select CAN
bus to connect controller to. Mapping to the SPI peripheral is
provided by device tree on QEMU side or by some other machine specific
glue code? Please, can you provide more information for intended
target use and RPi option to use for testing?
As for the code, I have read it the first time and for full check
I would need to spent more time with it. But I expect that
functionality check with the respect to mcp25x1x datasheet has been
done mainly by you so the full check bit by bit is not necessary.
If there is some omitted case it would be (hopefully) found during
code use. As for generic code style and redability, I see no problem.
I expect that you have checked for QEMU style and if there has been
some problem you have propably received QEMU CI and static analysis
feedback.
> +static void mcp25625_rx_into_buf(MCP25625State *s,
> + const qemu_can_frame *frame,
> + unsigned buffnr, int filthit)
> +{
> +struct rxbuff *rxbuff = >rxbuffs[buffnr];
> +qemu_canid_t e_id, sidl, id, q_id = frame->can_id;
> +unsigned len = frame->can_dlc;
> +
I would suggest to check for can_dlc > 8 in this function or in
its caller (mcp25625_can_receive), to ensure that QEMU cannot be
attacked by some malformed CAN message from the host kernel
or other VM...
> +static ssize_t mcp25625_can_receive(CanBusClientState *client,
> +const qemu_can_frame *buf,
> +size_t frames_cnt)
> +{
> +MCP25625State *s = client_to_mcp(client);
> +int ret;
> +
> +/* support receiving only one frame at a time */
> +if (frames_cnt != 1) {
> +return -1;
> +}
> +
> +/* we don't support error frames or buf->flags */
> +if (buf->can_id & QEMU_CAN_ERR_FLAG || buf->flags != 0) {
> +return -1;
> +}
I see as next step that you provide description/patch etc
for some target that I can test the code with CAN tools on my
side. I would prefer connection to some ARM Cortex-A target
where I have some images t run ready. RISC-V is really interesting
for me too, so if there is some option to run something
small, I can try that too. I prefer minimal setup with
self compilled bysybox in initramfs and the mapping of some
development directories through virtfs into system.
If you plan to visit FOSDEM 2023, we can meet there in person
at RISC-V devroom and I want to take tour for automotive and other
areas. Another chance is Embedded World where we plan to show
CAN latency tester demo
Re: [PATCH] hw/net/can: Add mcp25625 model
On 04/01/2023 12:22, Ben Dooks wrote: From: Ben Dooks Add support for Microchip MCP25625 SPI based CAN controller which is very similar to the MCP2515 (and covered by the same Linux driver). This can be added to any machine with SPI support in the machine model file. Example for using this when configured into a machine: -object can-bus,id=canbus0 \ -object can-host-socketcan,id=canhost0,if=vcan0,canbus=canbus0 \ -global driver=mcp25625,property=canbus,value=canbus0 There is tracing support with --trace "*mcp25*" Signed-off-by: Ben Dooks Co-developed-by: Nazar Kazakov Signed-off-by: Nazar Kazakov Co-developed-by: Lawrence Hunter Signed-off-by: Lawrence Hunter Reviewed-by: Frank Chang Has anyone had chance to review this, it would be great to get this moving along. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html
Re: [PATCH] hw/net/can: Add mcp25625 model
On 04/01/2023 12:22, Ben Dooks wrote: From: Ben Dooks Add support for Microchip MCP25625 SPI based CAN controller which is very similar to the MCP2515 (and covered by the same Linux driver). This can be added to any machine with SPI support in the machine model file. Example for using this when configured into a machine: -object can-bus,id=canbus0 \ -object can-host-socketcan,id=canhost0,if=vcan0,canbus=canbus0 \ -global driver=mcp25625,property=canbus,value=canbus0 There is tracing support with --trace "*mcp25*" Signed-off-by: Ben Dooks Co-developed-by: Nazar Kazakov Signed-off-by: Nazar Kazakov Co-developed-by: Lawrence Hunter Signed-off-by: Lawrence Hunter Reviewed-by: Frank Chang [snip] OOPS, just noticed I forgot to send user.name and user.email in this copy of the git repo and sent from @codethink instead of @sifive. Will fix this if needed. -- Ben
[PATCH] hw/net/can: Add mcp25625 model
From: Ben Dooks
Add support for Microchip MCP25625 SPI based CAN controller which is
very similar to the MCP2515 (and covered by the same Linux driver).
This can be added to any machine with SPI support in the machine
model file.
Example for using this when configured into a machine:
-object can-bus,id=canbus0 \
-object can-host-socketcan,id=canhost0,if=vcan0,canbus=canbus0 \
-global driver=mcp25625,property=canbus,value=canbus0
There is tracing support with --trace "*mcp25*"
Signed-off-by: Ben Dooks
Co-developed-by: Nazar Kazakov
Signed-off-by: Nazar Kazakov
Co-developed-by: Lawrence Hunter
Signed-off-by: Lawrence Hunter
Reviewed-by: Frank Chang
---
hw/net/Kconfig|5 +
hw/net/can/can_mcp25625.c | 1317 +
hw/net/can/meson.build|1 +
hw/net/can/trace-events | 13 +
include/hw/net/can_mcp25625.h | 10 +
5 files changed, 1346 insertions(+)
create mode 100644 hw/net/can/can_mcp25625.c
create mode 100644 include/hw/net/can_mcp25625.h
diff --git a/hw/net/Kconfig b/hw/net/Kconfig
index 6d795ec752..e058b97c23 100644
--- a/hw/net/Kconfig
+++ b/hw/net/Kconfig
@@ -132,6 +132,11 @@ config ROCKER
config CAN_BUS
bool
+config CAN_MCP25625
+bool
+default y if SSI
+select CAN_BUS
+
config CAN_SJA1000
bool
default y if PCI_DEVICES
diff --git a/hw/net/can/can_mcp25625.c b/hw/net/can/can_mcp25625.c
new file mode 100644
index 00..8a95208ee2
--- /dev/null
+++ b/hw/net/can/can_mcp25625.c
@@ -0,0 +1,1317 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * CAN device - MCP25625 chip model
+ *
+ * Copyright (c) 2022 SiFive, Inc.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "chardev/char.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "net/can_emu.h"
+#include "hw/ssi/ssi.h"
+#include "hw/qdev-properties.h"
+#include "hw/net/can_mcp25625.h"
+
+#include "trace.h"
+
+/*
+ * note: filter registers read back 0 unless in config mode
+ */
+#define OFF_RXFSIDH (0x0)
+#define OFF_RXFSIDL (0x1)
+#define OFF_RXFEID8 (0x2)
+#define OFF_RXFEID0 (0x3)
+
+#define RXFSIDL_EXIDE (1 << 3)
+#define RXFSIDL_WRITEMASK (0xE0 | RXFSIDL_EXIDE | 0x3)
+
+struct rxfilter {
+uint8_t data[4];
+};
+
+#define BFPCTRL_WRITEMASK (0x3F)
+#define BFPCTRL_B0BFM (1 << 0)
+#define BFPCTRL_B0BFE (1 << 2)
+#define BFPCTRL_B0BFS (1 << 4)
+
+#define TXRTSCTRL_WRITEMASK (0x7)
+
+/* rx mask register offsets (read back 0 unless in config mode) */
+#define OFF_RXMSIDH (0x0)
+#define OFF_RXMSIDL (0x1)
+#define OFF_RXMEID8 (0x2)
+#define OFF_RXMEID0 (0x3)
+
+#define RXMSIDL_WRITEMASK (0xE3)
+
+struct rxmask {
+uint8_t data[4];
+};
+
+#define CNF3_WRITEMASK (0xC7)
+
+#define OFF_TXBCTRL (0x0)
+#define TXBCTRL_ABTF(1 << 6)
+#define TXBCTRL_MLOA(1 << 5)
+#define TXBCTRL_TXREQ (1 << 3)
+#define TXBCTRL_TXP1(1 << 1)
+#define TXBCTRL_TXP0(1 << 0)
+#define TXBCTRL_TXP (TXBCTRL_TXP1 | TXBCTRL_TXP0)
+#define TXBCTRL_WRITEMASK (TXBCTRL_TXREQ | TXBCTRL_TXP)
+
+#define OFF_TXBSIDH (0x1)
+
+#define OFF_TXBSIDL (0x2)
+#define TXBSIDL_EXIDE (1 << 3)
+/* bits 7..5 are SID[2:0], bits 1..0 are EID[17:16] */
+#define TXBSIDL_WRITEMASK (0xE0 | TXBSIDL_EXIDE | 0x3)
+
+#define OFF_TXBEID8 (0x3) /* this is EID[15:8] */
+#define OFF_TXBEID0 (0x4) /* this is EID[7:0] */
+
+#define OFF_TXBDLC (0x5) /* bits 3..0 are DLC */
+#define TXBDLC_RTR (1 << 6)
+#define TXBDLC_WRITEMASK(TXBDLC_RTR | 0xF)
+
+#define OFF_TXBDATA (0x6)
+
+struct txbuff {
+uint8_t data[14];
+};
+
+#define OFF_RXBCTRL (0x0)
+#define RXBCTRL_RXM_ANY (3 << 5)
+#define RXBCTRL_RXM_VALID (0 << 5)
+#define RXBCTRL_RXM_MASK(3 << 5)
+
+#define RXBCTRL_RXRTR (1 << 3)
+#define RXBCTRL_BUKT(1 << 2)
+#define RXBCTRL_BUKT1 (1 << 1)
+/* RXBCTRL0, bit0 = filter hit for message */
+/* RXBCTRL1, bits 2..0 show filter hit */
+#define RXBCTRL0_WRITEMASK (RXBCTRL_RXM_MASK | RXBCTRL_BUKT)
+#define RXBCTRL1_WRITEMASK (RXBCTRL_RXM_MASK)
+
+#define OFF_RXBSIDH (0x1)
+/* bits 7..0 = SID[10:3] */
+
+#define OFF_RXBSIDL (0x2)
+#define RXBSIDL_SRR (1 << 4)
+#define RXBSIDL_IDE (1 << 3)
+/* bits 7..5 = SID[2:0], bits 1..0 = EID[17:16] */
+
+#define OFF_RXBEID8 (0x3) /* bits 7..0 = EID[15:8] */
+#define OFF_RXBEID0 (0x4) /* bits 7..0 = EID[7:0] */
+
+#define OFF_RXBDLC (0x5)
+#define RXBDLC_RTR (1 << 6) /* bits 3..0 = number of bytes */
+
+struct rxbuff {
+uint8_t data[14];
+};
+
+/*
