hw/arm/sabrelite: Add FlexCAN support
Subsystems (2)
v4 cover hw/arm/sabrelite: Add FlexCAN support
This series adds emulation of the FlexCAN CAN controller, version 2, found in NXP i.MX6 series SoCs. The controller is integrated into fsl-imx6 and the Sabrelite ARM board.
The chip has two FlexCAN controllers, but the Linux Sabrelite device tree enables only one by default. Linux kernel with both controllers enabled has been tested to work properly (using a custom device tree).
See docs/system/devices/can.rst for an example of QEMU command line invocation.
More information about the implementation can be found in my bachelor thesis [1].
Changes from v3:
- corrected attribution for definitions borrowed from the Linux kernel
- rebased onto latest master
- Link to v3: https://lore.kernel.org/qemu-devel/[email protected]/
Changes from v2 to v3:
- moved variable declarations to the top-of-the-block
- cleanup
- removed redundant license boilerplate
- rebased onto latest master
- Link to v2: https://lore.kernel.org/qemu-devel/[email protected]/
Changes from v1 to v2:
- removed DPRINTF (turned into traces or guest errors)
- removed gotos and unnecessary continues
- register memory region shortened to actual length w/o padding
- make flexcan_* fns private
- used DEVICE(s)->canonical_path instead of allocating new strings
- changed DEVICE_NATIVE_ENDIAN to DEVICE_LITTLE_ENDIAN
- removed ugly USE macro
- used enums instead of define sequences in flexcan.c
- fixed FlexCAN KConfig conf (fixes build on non-arm-softmmu)
- fixed style and other issues from Bernhard's review
- better wording in docs
- rebased onto latest master
- Link to v1: https://lore.kernel.org/qemu-devel/[email protected]
Thanks Bernhard Beschow for the review and his patience. I borrowed some fixes from his flexcan branch at [2].
[1] http://dspace.cvut.cz/bitstream/handle/10467/122654/F3-BP-2025-Bobek-Matyas-BP_Bobek_FlexCAN_final_4.pdf
[2] https://github.com/shentok/qemu/tree/flexcan
Matyáš Bobek (7):
hw/arm/sabrelite: Open code DEFINE_MACHINE_ARM hw/arm/sabrelite: Introduce class SabreliteMachineState hw/misc/imx6_ccm: Add PLL3 and CAN clock hw/net/can/flexcan: NXP FlexCAN core emulation hw/arm: Plug FlexCAN into FSL_IMX6 and Sabrelite
tests: Add qtests for FlexCAN docs/arm/sabrelite: Mention FlexCAN support
MAINTAINERS | 9 + docs/system/arm/sabrelite.rst | 1 + docs/system/devices/can.rst | 24 + hw/arm/Kconfig | 1 + hw/arm/fsl-imx6.c | 29 + hw/arm/sabrelite.c | 68 +- hw/misc/imx6_ccm.c | 24 + hw/misc/trace-events | 2 + hw/net/Kconfig | 5 + hw/net/can/flexcan.c | 1395 +++++++++++++++++++++++++++++++++ hw/net/can/flexcan_regs.h | 196 +++++ hw/net/can/meson.build | 1 + hw/net/can/trace-events | 18 + include/hw/arm/fsl-imx6.h | 6 + include/hw/misc/imx6_ccm.h | 4 + include/hw/misc/imx_ccm.h | 1 + include/hw/net/flexcan.h | 144 ++++ tests/qtest/flexcan-test.c | 421 ++++++++++ tests/qtest/meson.build | 1 + 19 files changed, 2340 insertions(+), 10 deletions(-) create mode 100644 hw/net/can/flexcan.c create mode 100644 hw/net/can/flexcan_regs.h create mode 100644 include/hw/net/flexcan.h create mode 100644 tests/qtest/flexcan-test.c
Signature · 1 line
2.54.0
PATCH v4 1/7 hw/arm/sabrelite: Open code DEFINE_MACHINE_ARM
Open code the DEFINE_MACHINE_ARM macro in preparation of creating SabreliteMachineState class.
--- hw/arm/sabrelite.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
hw/arm/sabrelite.c +18 -2
@@ -104,8 +104,10 @@ static void sabrelite_init(MachineState *machine) } } -static void sabrelite_machine_init (MachineClass *mc) +static void sabrelite_machine_class_init (ObjectClass *oc,const void *data) { +MachineClass *mc =MACHINE_CLASS (oc); + mc->desc ="Freescale i.MX6 Quad SABRE Lite Board (Cortex-A9)" ; mc->init = sabrelite_init; mc->max_cpus =FSL_IMX6_NUM_CPUS ;
@@ -114,4 +116,18 @@ static void sabrelite_machine_init(MachineClass *mc) mc->auto_create_sdcard = true; } -DEFINE_MACHINE_ARM ("sabrelite" , sabrelite_machine_init) +static const TypeInfo sabrelite_machine_init_typeinfo = { + .name =MACHINE_TYPE_NAME ("sabrelite" ), + .parent =TYPE_MACHINE , + .class_init = sabrelite_machine_class_init, + .instance_size =sizeof (MachineState), + .abstract = false, + .interfaces = arm_machine_interfaces, +}; + +static void sabrelite_machine_init_register_types (void ) +{ +type_register_static (&sabrelite_machine_init_typeinfo); +} + +type_init (sabrelite_machine_init_register_types)
Signature · 1 line
2.54.0
PATCH v4 5/7 hw/arm: Plug FlexCAN into FSL_IMX6 and Sabrelite
In reply to Matyáš BobekFlexcanState is added to the FslIMX6State struct like other peripherals.
Add two new machine properties to Sabrelite machine for linking the embedded FlexCAN instances to QEMU CAN buses by name. No other machine uses FslIMX6State.
--- hw/arm/Kconfig | 1 + hw/arm/fsl-imx6.c | 29 +++++++++++++++++++++++++++++ hw/arm/sabrelite.c | 24 ++++++++++++++++++++++++ include/hw/arm/fsl-imx6.h | 6 ++++++ 4 files changed, 60 insertions(+)
hw/arm/Kconfig +1 -0
@@ -519,6 +519,7 @@ config FSL_IMX6 select IMX_FEC select IMX_I2C select IMX_USBPHY + select CAN_FLEXCAN select WDT_IMX2 select PL310 # cache controller select PCI_EXPRESS_DESIGNWARE
hw/arm/fsl-imx6.c +29 -0
@@ -90,6 +90,10 @@ static void fsl_imx6_init(Object *obj)snprintf (name,NAME_SIZE ,"spi%d" , i +1 );object_initialize_child (obj, name, &s->spi[i],TYPE_IMX_SPI ); } +for (i =0 ; i <FSL_IMX6_NUM_CANS ; i++) { +snprintf (name,NAME_SIZE ,"flexcan%d" , i +1 ); +object_initialize_child (obj, name, &s->can[i],TYPE_CAN_FLEXCAN ); + }for (i =0 ; i <FSL_IMX6_NUM_WDTS ; i++) {snprintf (name,NAME_SIZE ,"wdt%d" , i);object_initialize_child (obj, name, &s->wdt[i],TYPE_IMX2_WDT );
@@ -377,6 +381,27 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp) qdev_get_gpio_in(gic, spi_table[i].irq)); } +/* Initialize all FLEXCANs */ +for (i =0 ; i <FSL_IMX6_NUM_CANS ; i++) { +static const struct { +hwaddr addr; +unsigned int irq; + } flexcan_table[FSL_IMX6_NUM_CANS ] = { + {FSL_IMX6_CAN1_ADDR ,FSL_IMX6_FLEXCAN1_IRQ }, + {FSL_IMX6_CAN2_ADDR ,FSL_IMX6_FLEXCAN2_IRQ }, + }; + + s->can[i].ccm =IMX_CCM (&s->ccm); +object_property_set_link (OBJECT (&s->can[i]),"canbus" , +OBJECT (s->canbus[i]), &error_abort); + +sysbus_realize (SYS_BUS_DEVICE (&s->can[i]), &error_abort); + +sysbus_mmio_map (SYS_BUS_DEVICE (&s->can[i]),0 , flexcan_table[i].addr); +sysbus_connect_irq (SYS_BUS_DEVICE (&s->can[i]),0 , +qdev_get_gpio_in (gic, flexcan_table[i].irq)); + } +object_property_set_uint (OBJECT (&s->eth),"phy-num" , s->phy_num, &error_abort);qemu_configure_nic_device (DEVICE (&s->eth), true,NULL );
@@ -480,6 +505,10 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)static const Property fsl_imx6_properties[] = {DEFINE_PROP_UINT32 ("fec-phy-num" , FslIMX6State, phy_num,0 ), +DEFINE_PROP_LINK ("canbus0" , FslIMX6State, canbus[0 ],TYPE_CAN_BUS , + CanBusState *), +DEFINE_PROP_LINK ("canbus1" , FslIMX6State, canbus[1 ],TYPE_CAN_BUS , + CanBusState *), };static void fsl_imx6_class_init (ObjectClass *oc,const void *data)
hw/arm/sabrelite.c +24 -0
@@ -24,6 +24,7 @@ struct SabreliteMachineState {MachineState parent_obj;FslIMX6State soc; +CanBusState *canbus[FSL_IMX6_NUM_CANS ]; };#define TYPE_SABRELITE_MACHINE MACHINE_TYPE_NAME("sabrelite")
@@ -64,6 +65,13 @@ static void sabrelite_init(MachineState *machine)/* Ethernet PHY address is 6 */ object_property_set_int (OBJECT (&s->soc),"fec-phy-num" ,6 , &error_fatal); +for (int i =0 ; i <FSL_IMX6_NUM_CANS ; i++) { + g_autofree char *bus_name = g_strdup_printf("canbus%d", i); + +object_property_set_link (OBJECT (&s->soc), bus_name, +OBJECT (s->canbus[i]), &error_fatal); + } +qdev_realize (DEVICE (&s->soc),NULL , &error_fatal); memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
@@ -112,6 +120,21 @@ static void sabrelite_init(MachineState *machine) } } +static void sabrelite_machine_instance_init (Object *obj) +{ +SabreliteMachineState *s =SABRELITE_MACHINE (obj); + +object_property_add_link (obj,"canbus0" ,TYPE_CAN_BUS , + (Object **)&s->canbus[0 ], + object_property_allow_set_link, +0 ); + +object_property_add_link (obj,"canbus1" ,TYPE_CAN_BUS , + (Object **)&s->canbus[1 ], + object_property_allow_set_link, +0 ); +} +static void sabrelite_machine_class_init (ObjectClass *oc,const void *data) {MachineClass *mc =MACHINE_CLASS (oc);
@@ -128,6 +151,7 @@ static const TypeInfo sabrelite_machine_init_typeinfo = { .name = TYPE_SABRELITE_MACHINE, .parent = TYPE_MACHINE, .class_init = sabrelite_machine_class_init, + .instance_init = sabrelite_machine_instance_init, .instance_size = sizeof(SabreliteMachineState), .abstract = false, .interfaces = arm_machine_interfaces,
include/hw/arm/fsl-imx6.h +6 -0
@@ -30,12 +30,14 @@#include "hw/sd/sdhci.h" #include "hw/ssi/imx_spi.h" #include "hw/net/imx_fec.h" +#include "hw/net/flexcan.h" #include "hw/usb/chipidea.h" #include "hw/usb/imx-usb-phy.h" #include "hw/pci-host/designware.h" #include "hw/core/or-irq.h" #include "system/memory.h" #include "target/arm/cpu.h" +#include "net/can_emu.h" #include "qom/object.h" #define TYPE_FSL_IMX6 "fsl-imx6"
@@ -51,6 +53,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(FslIMX6State, FSL_IMX6)#define FSL_IMX6_NUM_WDTS 2#define FSL_IMX6_NUM_USB_PHYS 2#define FSL_IMX6_NUM_USBS 4 +#define FSL_IMX6_NUM_CANS 2struct FslIMX6State {/*< private >*/
@@ -73,6 +76,7 @@ struct FslIMX6State {IMXUSBPHYState usbphy[FSL_IMX6_NUM_USB_PHYS ];ChipideaState usb[FSL_IMX6_NUM_USBS ];IMXFECState eth; +FlexcanState can[FSL_IMX6_NUM_CANS ];DesignwarePCIEHost pcie;OrIRQState pcie4_msi_irq;MemoryRegion rom;
@@ -80,6 +84,8 @@ struct FslIMX6State {MemoryRegion ocram;MemoryRegion ocram_alias;uint32_t phy_num; + +CanBusState *canbus[FSL_IMX6_NUM_CANS ]; };
Signature · 1 line
2.54.0
PATCH v4 2/7 hw/arm/sabrelite: Introduce class SabreliteMachineState
In reply to Matyáš BobekCreate full class SabreliteMachineState extending MachineState.
Previously, Sabrelite board MachineState was declared indirectly by the DEFINE_MACHINE_ARM macro. FslIMX6State was only instantiated in the sabrelite_init function. For FlexCAN device, machine properties will be used to select QEMU CAN buses. A custom class is therefore required, so the CanBusState "connections" can then be added as fields.
--- hw/arm/sabrelite.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
hw/arm/sabrelite.c +18 -10
@@ -20,6 +20,15 @@#include "qemu/error-report.h" #include "system/qtest.h" +struct SabreliteMachineState { +MachineState parent_obj; + +FslIMX6State soc; +}; + +#define TYPE_SABRELITE_MACHINE MACHINE_TYPE_NAME("sabrelite") +OBJECT_DECLARE_SIMPLE_TYPE(SabreliteMachineState, SABRELITE_MACHINE) +static struct arm_boot_info sabrelite_binfo = {/* DDR memory start */ .loader_start =FSL_IMX6_MMDC_ADDR ,
@@ -41,7 +50,7 @@ static void sabrelite_reset_secondary(ARMCPU *cpu,static void sabrelite_init (MachineState *machine) { -FslIMX6State *s; +SabreliteMachineState *s =SABRELITE_MACHINE (machine);/* Check the amount of memory is compatible with the SOC */ if (machine->ram_size >FSL_IMX6_MMDC_SIZE ) {
@@ -50,13 +59,12 @@ static void sabrelite_init(MachineState *machine)exit (1 ); } - s =FSL_IMX6 (object_new (TYPE_FSL_IMX6 )); -object_property_add_child (OBJECT (machine),"soc" ,OBJECT (s)); +object_initialize_child (OBJECT (machine),"soc" , &s->soc,TYPE_FSL_IMX6 );/* Ethernet PHY address is 6 */ -object_property_set_int (OBJECT (s),"fec-phy-num" ,6 , &error_fatal); +object_property_set_int (OBJECT (&s->soc),"fec-phy-num" ,6 , &error_fatal); -qdev_realize (DEVICE (s),NULL , &error_fatal); +qdev_realize (DEVICE (&s->soc),NULL , &error_fatal);memory_region_add_subregion (get_system_memory (),FSL_IMX6_MMDC_ADDR , machine->ram);
@@ -70,7 +78,7 @@ static void sabrelite_init(MachineState *machine)/* Add the sst25vf016b NOR FLASH memory to first SPI */ Object *spi_dev; - spi_dev =object_resolve_path_component (OBJECT (s),"spi1" ); + spi_dev =object_resolve_path_component (OBJECT (&s->soc),"spi1" );if (spi_dev) {SSIBus *spi_bus;
@@ -89,7 +97,7 @@ static void sabrelite_init(MachineState *machine)qdev_realize_and_unref (flash_dev,BUS (spi_bus), &error_fatal); cs_line =qdev_get_gpio_in_named (flash_dev,SSI_GPIO_CS ,0 ); -qdev_connect_gpio_out (DEVICE (&s->gpio[2 ]),19 , cs_line); +qdev_connect_gpio_out (DEVICE (&s->soc.gpio[2 ]),19 , cs_line); } } }
@@ -100,7 +108,7 @@ static void sabrelite_init(MachineState *machine) sabrelite_binfo.secondary_cpu_reset_hook = sabrelite_reset_secondary;if (!qtest_enabled ()) { -arm_load_kernel (&s->cpu[0 ], machine, &sabrelite_binfo); +arm_load_kernel (&s->soc.cpu[0 ], machine, &sabrelite_binfo); } }
@@ -117,10 +125,10 @@ static void sabrelite_machine_class_init(ObjectClass *oc, const void *data) }static const TypeInfo sabrelite_machine_init_typeinfo = { - .name =MACHINE_TYPE_NAME ("sabrelite" ), + .name =TYPE_SABRELITE_MACHINE , .parent =TYPE_MACHINE , .class_init = sabrelite_machine_class_init, - .instance_size =sizeof (MachineState), + .instance_size =sizeof (SabreliteMachineState), .abstract = false, .interfaces = arm_machine_interfaces, };
Signature · 1 line
2.54.0
PATCH v4 7/7 docs/arm/sabrelite: Mention FlexCAN support
In reply to Matyáš BobekAlso added example command line usage of the Sabrelite board with FlexCAN controllers.
--- docs/system/arm/sabrelite.rst | 1 + docs/system/devices/can.rst | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+)
docs/system/arm/sabrelite.rst +1 -0
@@ -24,6 +24,7 @@ The SABRE Lite machine supports the following devices: * 4 SDHC storage controllers * 4 USB 2.0 host controllers * 5 ECSPI controllers + * 2 FlexCAN CAN controllers * 1 SST 25VF016B flash Please note above list is a complete superset the QEMU SABRE Lite machine can
docs/system/devices/can.rst +24 -0
@@ -173,6 +173,30 @@ The test can also be run the other way around, generating messages in the guest system and capturing them in the host system. Other combinations are also possible. +Examples on how to use CAN emulation for FlexCAN on SabreLite board +------------------------------------------------------------------- +FlexCANs are connected to QEMU CAN buses by passing the bus IDs as machine +properties: + +* property ``canbus0`` for connecting ``flexcan1`` +* property ``canbus1`` for connecting ``flexcan2`` + +Note that upstream Linux SabreLite DTs have only a single FlexCAN (``flexcan1``) +enabled. + +An example command to run QEMU emulating a Sabrelite development board +with both FlexCANs connected to a single QEMU CAN bus (called ``qcan0``), +bridged to host system ``can0`` interface:: + + qemu-system-arm -M sabrelite -smp 4 -m 1G \ + -object can-bus,id=qcan0 \ + -machine canbus0=qcan0 -machine canbus1=qcan0 \ + -object can-host-socketcan,if=can0,canbus=qcan0,id=qcan0-socketcan \ + -kernel ... -dtb ... -initrd ... + +Note that in the Linux guest, bitrate for the FlexCAN device is ignored, +but needs to be set via the ``ip`` command. + Links to other resources ------------------------
Signature · 1 line
2.54.0
PATCH v4 6/7 tests: Add qtests for FlexCAN
In reply to Matyáš BobekThe tests do not test all of the FlexCAN emulator functionality.
--- MAINTAINERS | 1 + tests/qtest/flexcan-test.c | 421 +++++++++++++++++++++++++++++++++++++ tests/qtest/meson.build | 1 + 3 files changed, 423 insertions(+) create mode 100644 tests/qtest/flexcan-test.c
MAINTAINERS +1 -0
@@ -2108,6 +2108,7 @@ S: Maintained F: hw/net/can/flexcan.c F: hw/net/can/flexcan_regs.h F: include/hw/net/flexcan.h +F: tests/qtest/flexcan-test.c EDU M: Jiri Slaby <[email protected]>
tests/qtest/flexcan-test.c new file +421 -0
421 lines not on this page · raw message
tests/qtest/meson.build +1 -0
@@ -249,6 +249,7 @@ qtests_arm = \ (config_all_devices.has_key('CONFIG_MICROBIT') ? ['microbit-test'] : []) + \ (config_all_devices.has_key('CONFIG_STM32L4X5_SOC') ? qtests_stm32l4x5 : []) + \ (config_all_devices.has_key('CONFIG_FSI_APB2OPB_ASPEED') ? ['aspeed_fsi-test'] : []) + \ + (config_all_devices.has_key('CONFIG_CAN_FLEXCAN') ? ['flexcan-test'] : []) + \ (config_all_devices.has_key('CONFIG_STM32L4X5_SOC') and config_all_devices.has_key('CONFIG_DM163')? ['dm163-test'] : []) + \ ['arm-cpu-features',
Signature · 1 line
2.54.0
PATCH v4 4/7 hw/net/can/flexcan: NXP FlexCAN core emulation
In reply to Matyáš BobekAdded the FlexCAN2 emulator implementation core, with CAN_FLEXCAN Kconfig flag and MAINTAINERS entry.
FlexCAN2 version can be found in i.MX6 SoCs and others.
More information about the implementation can be found in [1].
Some macro and struct defintions were borrowed from the Linux kernel. The original authors agreed with relicensing them to GPL-2.0-or-later on the qemu-devel mailing list.
--- MAINTAINERS | 8 + hw/net/Kconfig | 5 + hw/net/can/flexcan.c | 1395 +++++++++++++++++++++++++++++++++++++ hw/net/can/flexcan_regs.h | 196 ++++++ hw/net/can/meson.build | 1 + hw/net/can/trace-events | 18 + include/hw/net/flexcan.h | 144 ++++ 7 files changed, 1767 insertions(+) create mode 100644 hw/net/can/flexcan.c create mode 100644 hw/net/can/flexcan_regs.h create mode 100644 include/hw/net/flexcan.h
MAINTAINERS +8 -0
@@ -2101,6 +2101,14 @@ F: hw/net/can/xlnx-* F: include/hw/net/xlnx-* F: tests/qtest/xlnx-can*-test* +FlexCAN +M: Matyas Bobek <[email protected]> +M: Pavel Pisa <[email protected]> +S: Maintained +F: hw/net/can/flexcan.c +F: hw/net/can/flexcan_regs.h +F: include/hw/net/flexcan.h + EDU M: Jiri Slaby <[email protected]> S: Maintained
hw/net/Kconfig +5 -0
@@ -157,3 +157,8 @@ config CAN_CTUCANFD_PCI default y if PCI_DEVICES depends on PCI && CAN_CTUCANFD select CAN_BUS + +config CAN_FLEXCAN + bool + depends on IMX + select CAN_BUS
hw/net/can/flexcan.c new file +1395 -0
1,395 lines not on this page · raw message
hw/net/can/flexcan_regs.h new file +196 -0
196 lines not on this page · raw message
hw/net/can/meson.build +1 -0
@@ -6,3 +6,4 @@ system_ss.add(when: 'CONFIG_CAN_CTUCANFD', if_true: files('ctucan_core.c')) system_ss.add(when: 'CONFIG_CAN_CTUCANFD_PCI', if_true: files('ctucan_pci.c')) system_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: files('xlnx-zynqmp-can.c')) system_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files('xlnx-versal-canfd.c')) +system_ss.add(when: 'CONFIG_CAN_FLEXCAN', if_true: files('flexcan.c'))
hw/net/can/trace-events +18 -0
@@ -1,3 +1,21 @@ +# flexcan.c +flexcan_irq_update(const char *inst, uint32_t mb_irqs1, uint32_t mb_irqs2, int setting) "%s: irqs1 0x%08x irqs2 0x%08x request %i" +flexcan_set_mcr(const char *inst, const char *enabled, const char *freeze, const char *fifo, const char *rdy, const char *sync) "%s: %s %s %s %s %s" +flexcan_mb_write(const char *inst, int mbidx, const char *code, int is_mailbox, uint32_t ctrl, uint32_t id) "%s: mbidx %i code %s is_mailbox %i ctrl 0x%08x id 0x%08x" +flexcan_mb_lock(const char *inst, int mbidx, int had_rx_code) "%s: mbidx %i had_rx_code %i" +flexcan_mb_unlock(const char *inst, int mbidx, const char *pending_frame) "%s: mbidx %i%s" +flexcan_fifo_pop(const char *inst, int non_empty_before, int non_empty_after) "%s: non_empty before %i non_empty_after %i" +flexcan_fifo_push(const char *inst, int n_occupied) "%s: n_slots_occupied %i" +flexcan_reset(const char *inst) "%s: resetting" +flexcan_mem_op(const char *inst, const char *op, uint32_t v, int offset, const char *reg_name, int size) "%s: %s 0x%08x at offset %i register %s size %i" +flexcan_get_timestamp(const char *inst, int64_t time_elapsed_ms, uint32_t bitrate, uint64_t cycles, uint32_t shift, uint32_t timestamp) "%s: time_elapsed %" PRIi64 "ms bitrate %ub/s cycles %" PRIu64 " shift %u timestamp 0x%04x" +flexcan_get_bitrate(const char *inst, uint32_t pe_freq, uint32_t prediv, uint32_t s_freq, uint32_t tseg1, uint32_t tseg2, uint32_t quata_per_bit, uint32_t bitrate) "%s: pe_freq %uHz prescaler %u s_freq %uHz tseg1 %uq tseg2 %uq total %uq/b bitrate %ub/s" +flexcan_timer_start(const char *inst, uint32_t bitrate, uint32_t value) "%s: bitrate %ub/s value 0x%04x" +flexcan_timer_stop(const char *inst, uint32_t bitrate, uint32_t value) "%s: bitrate %ub/s value 0x%04x" +flexcan_timer_overflow(const char *inst, int64_t current_time, int64_t timer_start, int64_t elapsed_ns) "%s: current_time %" PRIi64 "timer_start %" PRIi64 "elapsed_ns %" PRIi64 +flexcan_mb_rx_check_mb(const char *inst, int mbidx, const char *code, int is_matched, int is_ftr, int is_serviced, int is_locked) "%s: checking mb %i code %s is_matched %i is_free_to_receive %i is_serviced %i is_locked %i" +flexcan_receive(const char *inst, size_t n_frames) "%s: received %zu frames" + # xlnx-zynqmp-can.c xlnx_can_update_irq(uint32_t isr, uint32_t ier, uint32_t irq) "ISR: 0x%08x IER: 0x%08x IRQ: 0x%08x" xlnx_can_reset(uint32_t val) "Resetting controller with value = 0x%08x"
include/hw/net/flexcan.h new file +144 -0
144 lines not on this page · raw message
Signature · 1 line
2.54.0
PATCH v4 3/7 hw/misc/imx6_ccm: Add PLL3 and CAN clock
In reply to Matyáš BobekAdd fixed frequency (480 MHz) PLL3, of which the FlexCAN clock is derived, and compute FlexCAN frequency based on the divider configuration (CCM_CSCMR2).
The clock frequency will be used for computing timestamps in FlexCAN emulator.
--- hw/misc/imx6_ccm.c | 24 ++++++++++++++++++++++++ hw/misc/trace-events | 2 ++ include/hw/misc/imx6_ccm.h | 4 ++++ include/hw/misc/imx_ccm.h | 1 + 4 files changed, 31 insertions(+)
hw/misc/imx6_ccm.c +24 -0
@@ -257,6 +257,15 @@ static uint64_t imx6_analog_get_pll2_clk(IMX6CCMState *dev)return freq; } +static uint64_t imx6_analog_get_pll3_clk (IMX6CCMState *dev) +{ +uint64_t freq =480000000 ; + +trace_imx6_analog_get_pll3_clk (freq); + +return freq; +} +static uint64_t imx6_analog_get_pll2_pfd0_clk (IMX6CCMState *dev) {uint64_t freq =0 ;
@@ -344,6 +353,18 @@ static uint64_t imx6_ccm_get_per_clk(IMX6CCMState *dev)return freq; } +static uint64_t imx6_ccm_get_can_clk (IMX6CCMState *dev) +{ +uint64_t freq =0 ; + + freq =imx6_analog_get_pll3_clk (dev) /8 ; + freq /= (1 +EXTRACT (dev->ccm[CCM_CSCMR2 ],CAN_CLK_PODF )); + +trace_imx6_ccm_get_can_clk (freq); + +return freq; +} +static uint32_t imx6_ccm_get_clock_frequency (IMXCCMState *dev,IMXClk clock) {uint32_t freq =0 ;
@@ -358,6 +379,9 @@ static uint32_t imx6_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock)case CLK_IPG_HIGH : freq =imx6_ccm_get_per_clk (s);break ; +case CLK_CAN : + freq =imx6_ccm_get_can_clk (s); +break ;case CLK_32k: freq =CKIL_FREQ ;break ;
hw/misc/trace-events +2 -0
@@ -242,11 +242,13 @@ imx6_analog_get_periph_clk(uint32_t freq) "freq = %u Hz" imx6_analog_get_pll2_clk(uint32_t freq) "freq = %u Hz" imx6_analog_get_pll2_pfd0_clk(uint32_t freq) "freq = %u Hz" imx6_analog_get_pll2_pfd2_clk(uint32_t freq) "freq = %u Hz" +imx6_analog_get_pll3_clk(uint32_t freq) "freq = %u Hz" imx6_analog_read(const char *reg, uint32_t value) "reg[%s] => 0x%" PRIx32 imx6_analog_write(const char *reg, uint32_t value) "reg[%s] <= 0x%" PRIx32 imx6_ccm_get_ahb_clk(uint32_t freq) "freq = %u Hz" imx6_ccm_get_ipg_clk(uint32_t freq) "freq = %u Hz" imx6_ccm_get_per_clk(uint32_t freq) "freq = %u Hz" +imx6_ccm_get_can_clk(uint32_t freq) "freq = %u Hz" imx6_ccm_get_clock_frequency(unsigned clock, uint32_t freq) "(Clock = %d) = %u" imx6_ccm_read(const char *reg, uint32_t value) "reg[%s] => 0x%" PRIx32 imx6_ccm_reset(void) ""
include/hw/misc/imx6_ccm.h +4 -0
@@ -164,6 +164,10 @@#define PERCLK_PODF_SHIFT (0)#define PERCLK_PODF_LENGTH (6) +/* CCM_CSCMR2 */ +#define CAN_CLK_PODF_SHIFT (2) +#define CAN_CLK_PODF_LENGTH (6) +/* CCM_ANALOG_PFD_528 */ #define PFD0_FRAC_SHIFT (0)#define PFD0_FRAC_LENGTH (6)
include/hw/misc/imx_ccm.h +1 -0
@@ -46,6 +46,7 @@ typedef enum {CLK_EXT ,CLK_HIGH_DIV ,CLK_HIGH , +CLK_CAN , } IMXClk; struct IMXCCMClass {
Signature · 1 line
2.54.0
Re: [PATCH v4 4/7] hw/net/can/flexcan: NXP FlexCAN core emulation
In reply to Matyáš BobekQuoted 22 lines · Matyáš Bobek
Added the FlexCAN2 emulator implementation core, with
CAN_FLEXCANKconfigflag andMAINTAINERSentry.FlexCAN2 version can be found in i.MX6 SoCs and others.
More information about the implementation can be found in [1].
Some macro and struct defintions were borrowed from the Linux kernel. The original authors agreed with relicensing them to GPL-2.0-or-later on the qemu-devel mailing list.
Signed-off-by Matyáš Bobek <[email protected]>Signed-off-by Pavel Pisa <[email protected]>Tested-by Pavel Pisa <[email protected]>Reviewed-by Bernhard Beschow <[email protected]>Reviewed-by Pavel Pisa <[email protected]>
Quoted 7 lines
+/**
+ * The following macros were originally written for the Linux kernel by:+ * Andrey Volkov <[email protected]> + * Sascha Hauer <[email protected]> + * Marc Kleine-Budde <[email protected]> + * David Jander <[email protected]>+ */
For clarity, we should add a line to this comment:
"and they have agreed to license them under GPL-2.0-or-later".
thanks
-- PMM
Quoted 11 lines · Matyáš Bobek
This series adds emulation of the FlexCAN CAN controller, version 2, found in NXP i.MX6 series SoCs. The controller is integrated into fsl-imx6 and the Sabrelite ARM board.
The chip has two FlexCAN controllers, but the Linux Sabrelite device tree enables only one by default. Linux kernel with both controllers enabled has been tested to work properly (using a custom device tree).
Seedocs/system/devices/can.rstfor an example of QEMU command line invocation.
If I run "make check" under a build with the clang sanitizers enabled, the test fails with an array out of bounds access:
test: qtest+qtest-arm - qemu:qtest-arm/flexcan-test start time: 08:46:34 duration: 3.22s result: killed by signal 6 SIGABRT command: QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
QTEST_QEMU_VNC_BINARY=./tools/qemu-vnc/qemu-vnc MESON_TEST_ITERATION=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_BINARY=./qemu-system-arm RUST_BACKTRACE=1 PYTHON=/home/pm215/qemu/build/arm-clang/pyvenv/bin/python3 MALLOC_PERTURB_=136 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 G_TEST_DBUS_DAEMON=/home/pm215/qemu/tests/dbus-daemon.sh QTEST_QEMU_IMG=./qemu-img ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 /home/pm215/qemu/build/arm-clang/tests/qtest/flexcan-test --tap -k ----------------------------------- stdout ----------------------------------- TAP version 13
# random seed: R02S02bc6ca307d8afe2f7478a86cd8f0c6a
1..5
# Start of arm tests
# Start of net tests
# Start of flexcan tests
# starting QEMU: exec ./qemu-system-arm -qtest
unix:/tmp/qtest-3845367.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-3845367.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -run-with
exit-with-parent=on -nographic -M sabrelite -object can-bus,id=qcan0
-machine canbus0=qcan0 -machine canbus1=qcan0 -accel qtest
ok 1 /arm/net/flexcan/linux_probe
# starting QEMU: exec ./qemu-system-arm -qtest
unix:/tmp/qtest-3845367.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-3845367.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -run-with
exit-with-parent=on -nographic -M sabrelite -object can-bus,id=qcan0
-machine canbus0=qcan0 -machine canbus1=qcan0 -accel qtest
ok 2 /arm/net/flexcan/freeze_disable_interaction
# starting QEMU: exec ./qemu-system-arm -qtest
unix:/tmp/qtest-3845367.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-3845367.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -run-with
exit-with-parent=on -nographic -M sabrelite -object can-bus,id=qcan0
-machine canbus0=qcan0 -machine canbus1=qcan0 -accel qtest
----------------------------------- stderr -----------------------------------
../../hw/net/can/flexcan.c:305:24: runtime error: index 80 out of
bounds for type 'FlexcanRegsMessageBuffer[64]' (aka 'struct
FlexcanRegsMessageBuffer[64]')
#0 0x5b5c6378e3c9 in flexcan_get_last_enabled_mailbox
/home/pm215/qemu/build/arm-clang/../../hw/net/can/flexcan.c:305:24
#1 0x5b5c6378a9a1 in flexcan_mb_write
/home/pm215/qemu/build/arm-clang/../../hw/net/can/flexcan.c:700:30
#2 0x5b5c6378a9a1 in flexcan_mem_write
/home/pm215/qemu/build/arm-clang/../../hw/net/can/flexcan.c:1232:17
#3 0x5b5c63c6de46 in memory_region_write_accessor
/home/pm215/qemu/build/arm-clang/../../system/memory.c:492:5
#4 0x5b5c63c6d3cd in access_with_adjusted_size
/home/pm215/qemu/build/arm-clang/../../system/memory.c:568:18
#5 0x5b5c63c6ce92 in memory_region_dispatch_write
/home/pm215/qemu/build/arm-clang/../../system/memory.c
#6 0x5b5c63ca7008 in flatview_write_continue_step
/home/pm215/qemu/build/arm-clang/../../system/physmem.c:3263:18
#7 0x5b5c63c9e5b6 in flatview_write_continue
/home/pm215/qemu/build/arm-clang/../../system/physmem.c:3293:19
#8 0x5b5c63c9e5b6 in flatview_write
/home/pm215/qemu/build/arm-clang/../../system/physmem.c:3324:12
#9 0x5b5c63c9e4a8 in address_space_write
/home/pm215/qemu/build/arm-clang/../../system/physmem.c:3444:18
#10 0x5b5c63caf690 in qtest_process_command
/home/pm215/qemu/build/arm-clang/../../system/qtest.c
#11 0x5b5c63cacb8d in qtest_process_inbuf
/home/pm215/qemu/build/arm-clang/../../system/qtest.c:778:9
#12 0x5b5c646888b7 in tcp_chr_read
/home/pm215/qemu/build/arm-clang/../../chardev/char-socket.c:511:13
#13 0x7d3004bd245d
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5d45d) (BuildId:
116e142b9b52c8a4dfd403e759e71ab8f95d8bb3)
#14 0x7d3004bd26cf in g_main_context_dispatch
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5d6cf) (BuildId:
116e142b9b52c8a4dfd403e759e71ab8f95d8bb3)
#15 0x5b5c64866a4b in glib_pollfds_poll
/home/pm215/qemu/build/arm-clang/../../util/main-loop.c:290:9
#16 0x5b5c64866a4b in os_host_main_loop_wait
/home/pm215/qemu/build/arm-clang/../../util/main-loop.c:313:5
#17 0x5b5c64866a4b in main_loop_wait
/home/pm215/qemu/build/arm-clang/../../util/main-loop.c:592:11
#18 0x5b5c63cb5cd6 in qemu_main_loop
/home/pm215/qemu/build/arm-clang/../../system/runstate.c:950:9
#19 0x5b5c647026bb in qemu_default_main
/home/pm215/qemu/build/arm-clang/../../system/main.c:50:14
#20 0x5b5c64702693 in main
/home/pm215/qemu/build/arm-clang/../../system/main.c:93:9
#21 0x7d300282a1c9 in __libc_start_call_main
csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#23 0x5b5c63077684 in _start
(/home/pm215/qemu/build/arm-clang/qemu-system-arm+0x1546684) (BuildId:
46de6576c6cf821195d15a70a05dd4e75e97f22d)SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../hw/net/can/flexcan.c:305:24
Broken pipe../../tests/qtest/libqtest.c:210: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
You can do a sanitizer enabled build by running configure with options like:
'--cc=clang' '--cxx=clang++' '--enable-ubsan'
and then run 'make check' as normal.
You can also do a build with the address sanitizer enabled as well, but it's a little bit more involved:
'../../configure' '--cc=clang' '--cxx=clang++' '--enable-ubsan' '--enable-asan' '--target-list=arm-softmmu,arm-linux-user,aarch64-softmmu,aarch64-linux-user'
For an asan build, you will need to set ASAN_OPTIONS and LSAN_OPTIONSwhen running make check, like this:
TIMEOUT_MULTIPLIER=3 ASAN_OPTIONS="fast_unwind_on_malloc=0" LSAN_OPTIONS="suppressions=$PWD/scripts/lsan_suppressions.txt" make -C build/arm-san/ -j15 check
(Current head-of-git has a leak in the tpm-tis device which is fixed by this patch:
https://patchew.org/QEMU/[email protected]/
That should get upstream shortly, but if you don't have it yet you'll also see leak reports about that.)
thanks
-- PMM
Re: [PATCH v4 5/7] hw/arm: Plug FlexCAN into FSL_IMX6 and Sabrelite
In reply to Matyáš BobekHi Matyáš, Pavel,
Some small remarks regarding the use of hard-coded constants such as FSL_IMX6_NUM_WDTS in loops, IMHO, it would be better to use |ARRAY_SIZE()|instead:
- for (i = 0; i < FSL_IMX6_NUM_CANS; i++) {
+ for (i = 0; i < ARRAY_SIZE(s->can); i++) {
BTW, it would be also more logical to combine |CanBusState|and |FlexcanState|into a single structure (possibly anonymous), for example:struct FslIMX6State { ...
struct {
CanBusState *state;
FlexcanState flexcan;} can[FSL_IMX6_NUM_CANS];
...
};
As a matter of personal preference, I would also rename "|can"|to "|flexcan"/"fcan"|, since these objects represent FlexCAN controllers rather than CAN2.0 ones.
Regards,
Andrey VOLKOV
Le 28/05/2026 à 19:17, Matyáš Bobek a écrit :
Quoted 184 lines
FlexcanState is added to the FslIMX6State struct like other peripherals.
Add two new machine properties to Sabrelite machine for linking the embedded FlexCAN instances to QEMU CAN buses by name. No other machine uses FslIMX6State.
Signed-off-by Matyáš Bobek <[email protected]>Signed-off-by Pavel Pisa <[email protected]>Tested-by Pavel Pisa <[email protected]>Reviewed-by Bernhard Beschow <[email protected]>Reviewed-by Pavel Pisa <[email protected]>--- hw/arm/Kconfig | 1 + hw/arm/fsl-imx6.c | 29 +++++++++++++++++++++++++++++ hw/arm/sabrelite.c | 24 ++++++++++++++++++++++++ include/hw/arm/fsl-imx6.h | 6 ++++++ 4 files changed, 60 insertions(+)hw/arm/Kconfig +1
-0@@ -519,6 +519,7 @@ config FSL_IMX6 select IMX_FEC select IMX_I2C select IMX_USBPHY + select CAN_FLEXCAN select WDT_IMX2 select PL310 # cache controller select PCI_EXPRESS_DESIGNWAREhw/arm/fsl-imx6.c +29
-0@@ -90,6 +90,10 @@ static void fsl_imx6_init(Object *obj)snprintf (name,NAME_SIZE ,"spi%d" , i +1 );object_initialize_child (obj, name, &s->spi[i],TYPE_IMX_SPI ); } +for (i =0 ; i <FSL_IMX6_NUM_CANS ; i++) { +snprintf (name,NAME_SIZE ,"flexcan%d" , i +1 ); +object_initialize_child (obj, name, &s->can[i],TYPE_CAN_FLEXCAN ); + }for (i =0 ; i <FSL_IMX6_NUM_WDTS ; i++) {snprintf (name,NAME_SIZE ,"wdt%d" , i);object_initialize_child (obj, name, &s->wdt[i],TYPE_IMX2_WDT );@@ -377,6 +381,27 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp) qdev_get_gpio_in(gic, spi_table[i].irq)); } +/* Initialize all FLEXCANs */ +for (i =0 ; i <FSL_IMX6_NUM_CANS ; i++) { +static const struct { +hwaddr addr; +unsigned int irq; + } flexcan_table[FSL_IMX6_NUM_CANS ] = { + {FSL_IMX6_CAN1_ADDR ,FSL_IMX6_FLEXCAN1_IRQ }, + {FSL_IMX6_CAN2_ADDR ,FSL_IMX6_FLEXCAN2_IRQ }, + }; + + s->can[i].ccm =IMX_CCM (&s->ccm); +object_property_set_link (OBJECT (&s->can[i]),"canbus" , +OBJECT (s->canbus[i]), &error_abort); + +sysbus_realize (SYS_BUS_DEVICE (&s->can[i]), &error_abort); + +sysbus_mmio_map (SYS_BUS_DEVICE (&s->can[i]),0 , flexcan_table[i].addr); +sysbus_connect_irq (SYS_BUS_DEVICE (&s->can[i]),0 , +qdev_get_gpio_in (gic, flexcan_table[i].irq)); + } +object_property_set_uint (OBJECT (&s->eth),"phy-num" , s->phy_num, &error_abort);qemu_configure_nic_device (DEVICE (&s->eth), true,NULL );@@ -480,6 +505,10 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)static const Property fsl_imx6_properties[] = {DEFINE_PROP_UINT32 ("fec-phy-num" , FslIMX6State, phy_num,0 ), +DEFINE_PROP_LINK ("canbus0" , FslIMX6State, canbus[0 ],TYPE_CAN_BUS , + CanBusState *), +DEFINE_PROP_LINK ("canbus1" , FslIMX6State, canbus[1 ],TYPE_CAN_BUS , + CanBusState *), };static void fsl_imx6_class_init (ObjectClass *oc,const void *data)hw/arm/sabrelite.c +24
-0@@ -24,6 +24,7 @@ struct SabreliteMachineState {MachineState parent_obj;FslIMX6State soc; +CanBusState *canbus[FSL_IMX6_NUM_CANS ]; };#define TYPE_SABRELITE_MACHINE MACHINE_TYPE_NAME("sabrelite")@@ -64,6 +65,13 @@ static void sabrelite_init(MachineState *machine)/* Ethernet PHY address is 6 */ object_property_set_int (OBJECT (&s->soc),"fec-phy-num" ,6 , &error_fatal); +for (int i =0 ; i <FSL_IMX6_NUM_CANS ; i++) { + g_autofree char *bus_name = g_strdup_printf("canbus%d", i); + +object_property_set_link (OBJECT (&s->soc), bus_name, +OBJECT (s->canbus[i]), &error_fatal); + } +qdev_realize (DEVICE (&s->soc),NULL , &error_fatal); memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,@@ -112,6 +120,21 @@ static void sabrelite_init(MachineState *machine) } } +static void sabrelite_machine_instance_init (Object *obj) +{ +SabreliteMachineState *s =SABRELITE_MACHINE (obj); + +object_property_add_link (obj,"canbus0" ,TYPE_CAN_BUS , + (Object **)&s->canbus[0 ], + object_property_allow_set_link, +0 ); + +object_property_add_link (obj,"canbus1" ,TYPE_CAN_BUS , + (Object **)&s->canbus[1 ], + object_property_allow_set_link, +0 ); +} +static void sabrelite_machine_class_init (ObjectClass *oc,const void *data) {MachineClass *mc =MACHINE_CLASS (oc);@@ -128,6 +151,7 @@ static const TypeInfo sabrelite_machine_init_typeinfo = { .name = TYPE_SABRELITE_MACHINE, .parent = TYPE_MACHINE, .class_init = sabrelite_machine_class_init, + .instance_init = sabrelite_machine_instance_init, .instance_size = sizeof(SabreliteMachineState), .abstract = false, .interfaces = arm_machine_interfaces,include/hw/arm/fsl-imx6.h +6
-0@@ -30,12 +30,14 @@#include "hw/sd/sdhci.h" #include "hw/ssi/imx_spi.h" #include "hw/net/imx_fec.h" +#include "hw/net/flexcan.h" #include "hw/usb/chipidea.h" #include "hw/usb/imx-usb-phy.h" #include "hw/pci-host/designware.h" #include "hw/core/or-irq.h" #include "system/memory.h" #include "target/arm/cpu.h" +#include "net/can_emu.h" #include "qom/object.h" #define TYPE_FSL_IMX6 "fsl-imx6"@@ -51,6 +53,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(FslIMX6State, FSL_IMX6)#define FSL_IMX6_NUM_WDTS 2#define FSL_IMX6_NUM_USB_PHYS 2#define FSL_IMX6_NUM_USBS 4 +#define FSL_IMX6_NUM_CANS 2struct FslIMX6State {/*< private >*/ @@ -73,6 +76,7 @@ struct FslIMX6State {IMXUSBPHYState usbphy[FSL_IMX6_NUM_USB_PHYS ];ChipideaState usb[FSL_IMX6_NUM_USBS ];IMXFECState eth; +FlexcanState can[FSL_IMX6_NUM_CANS ];DesignwarePCIEHost pcie;OrIRQState pcie4_msi_irq;MemoryRegion rom;@@ -80,6 +84,8 @@ struct FslIMX6State {MemoryRegion ocram;MemoryRegion ocram_alias;uint32_t phy_num; + +CanBusState *canbus[FSL_IMX6_NUM_CANS ]; };
Re: [PATCH v4 5/7] hw/arm: Plug FlexCAN into FSL_IMX6 and Sabrelite
Dear Andrey and others,
thanks for license resolution and feedback to the updated v4 series. As for the suggestions, I suggest to postpone at least some of them after mainlining which is again delayed. The opinion why to postpone some of changes based on the suggestions described inline
Review
Quoted 7 lines · On Friday 29 of May 2026 13:35:39 Andrey VOLKOV wrote:
Some small remarks regarding the use of hard-coded constants such as
FSL_IMX6_NUM_WDTSin loops, IMHO, it would be better to use|ARRAY_SIZE()|instead:
(quoted diff) +1
-1- for (i = 0; i < FSL_IMX6_NUM_CANS; i++) { + for (i = 0; i < ARRAY_SIZE(s->can); i++) {
I think that it is worse to change one/newly introduced peripheral declaration to new style which makes it inconsistent with the rest of peripherals than keep the style of other peripherals
https://gitlab.com/qemu-project/qemu/-/blob/master/include/hw/arm/fsl-imx6.h#L44
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/arm/fsl-imx6.c#L60
for (i = 0; i < FSL_IMX6_NUM_USBS; i++) {
snprintf(name, NAME_SIZE, "usb%d", i);
object_initialize_child(obj, name, &s->usb[i], TYPE_CHIPIDEA);
}
for (i = 0; i < FSL_IMX6_NUM_ECSPIS; i++) {
snprintf(name, NAME_SIZE, "spi%d", i + 1);
object_initialize_child(obj, name, &s->spi[i], TYPE_IMX_SPI);
}
for (i = 0; i < FSL_IMX6_NUM_WDTS; i++) {
snprintf(name, NAME_SIZE, "wdt%d", i);
object_initialize_child(obj, name, &s->wdt[i], TYPE_IMX2_WDT);
}I understand the generic idea of higher safety of the ARRAY_SIZE to prevent mishmash, but in this specific case, I think that if the change should be done then on all peripherals setup at least in whole fsl-imx6.c.
Quoted 6 lines
BTW, it would be also more logical to combine |CanBusState|and
|FlexcanState|into a single structure (possibly anonymous), for
example:struct FslIMX6State { ...
Quoted 7 lines
struct { CanBusState *state; FlexcanState flexcan; } can[FSL_IMX6_NUM_CANS];...
};
This is probably for more thinking and considerations. The property
hw/arm/fsl-imx6.c:
static const TypeInfo fsl_imx6_type_info = {
.name = TYPE_FSL_IMX6,
.parent = TYPE_DEVICE,
...
hw/arm/sabrelite.c:
static const TypeInfo sabrelite_machine_init_typeinfo = {
.name = TYPE_SABRELITE_MACHINE,
.parent = TYPE_MACHINE,
...
The property and possibility to set "canbus0" and "canbus1" have to be accessible when machine is selected and setup. The fsl_imx6 SoC is not directly parent of sabrelite_machine but only one of its components so the properties are not directly inherited. This means that if the properties are not defined on the level of sabrelite_machine, current code
object_property_add_link(obj, "canbus0",
then there have to be added aliases or some other mechanism to propagate/make visible the required mapping to CAN bus objects (their names) from machine to the SoC.
Quoted 3 lines
As a matter of personal preference, I would also rename "|can"|to "|flexcan"/"fcan"|, since these objects represent FlexCAN controllers rather than CAN2.0 ones.
Matyas Bobek has already updated his patch series
https://gitlab.fel.cvut.cz/bobekmat/qemu-flexcan/-/commits/flexcan-series-v6.1
to reflect some of yours suggestions including more specific naming even that it seems that for most peripherals the SoC style is to keep names as simple as "eth" which could be questioned for possible hidding of some other mame
struct FslIMX6State {
...
ChipideaState usb[FSL_IMX6_NUM_USBS];
IMXFECState eth;
+ FlexcanState flexcan[FSL_IMX6_NUM_CANS];
DesignwarePCIEHost pcie;
OrIRQState pcie4_msi_irq;
MemoryRegion rom;So I agree that there are more thing which could be discussed, changed in direction to implement CAN FD capable FlexCAN variant, etc., but I think that we should get to the point where this iMX6 version functional with Linux kernel is included in the mainline as the level change.
Best wishes,
Pavel Pisa
phone: +420 603531357
e-mail: [email protected]
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