* [Qemu-devel] [PATCH v2 0/4] Add i.MX25 support through the 3DS evaluation board
@ 2013-05-04 14:09 Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver Jean-Christophe DUBOIS
` (3 more replies)
0 siblings, 4 replies; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-04 14:09 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, peter.crosthwaite, peter.chubb, afaerber,
Jean-Christophe DUBOIS
This serie of patches add the support for the i.MX25 processor through the
Freescale 3DS evaluation board.
For now a limited set of devices are supported.
* GPT timers (from i.MX31)
* EPI timers (from i.MX31)
* Serial ports (from i.MX31)
* Ethernet FEC port
* I2C controller
Jean-Christophe DUBOIS (4):
Add i.MX FEC Ethernet driver
Add i.MX I2C controller driver.
Add i.MX25 3DS evaluation board support.
Add qtest support for i.MX I2C device emulation.
default-configs/arm-softmmu.mak | 3 +
hw/arm/Makefile.objs | 1 +
hw/arm/imx25_3ds.c | 258 ++++++++++++++
hw/i2c/Makefile.objs | 1 +
hw/i2c/imx_i2c.c | 383 ++++++++++++++++++++
hw/net/Makefile.objs | 1 +
hw/net/imx_fec.c | 748 ++++++++++++++++++++++++++++++++++++++++
include/hw/arm/imx.h | 1 +
tests/Makefile | 3 +
tests/ds1338-test.c | 64 ++++
tests/libqos/i2c-imx.c | 224 ++++++++++++
tests/libqos/i2c.h | 3 +
12 files changed, 1690 insertions(+)
create mode 100644 hw/arm/imx25_3ds.c
create mode 100644 hw/i2c/imx_i2c.c
create mode 100644 hw/net/imx_fec.c
create mode 100644 tests/ds1338-test.c
create mode 100644 tests/libqos/i2c-imx.c
--
1.8.1.2
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-04 14:09 [Qemu-devel] [PATCH v2 0/4] Add i.MX25 support through the 3DS evaluation board Jean-Christophe DUBOIS
@ 2013-05-04 14:09 ` Jean-Christophe DUBOIS
2013-05-05 3:11 ` Peter Crosthwaite
2013-05-05 17:49 ` Michael S. Tsirkin
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver Jean-Christophe DUBOIS
` (2 subsequent siblings)
3 siblings, 2 replies; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-04 14:09 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, peter.crosthwaite, peter.chubb, afaerber,
Jean-Christophe DUBOIS
This is based on the mcf_fec.c FEC implementation for ColdFire.
* a generic phy was added (borrowed from lan9118).
* The buffer management is also modified as buffers are
slightly different between coldfire and i.MX.
Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
---
default-configs/arm-softmmu.mak | 1 +
hw/net/Makefile.objs | 1 +
hw/net/imx_fec.c | 748 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 750 insertions(+)
create mode 100644 hw/net/imx_fec.c
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 27cbe3d..b3a0207 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -28,6 +28,7 @@ CONFIG_SSI_SD=y
CONFIG_SSI_M25P80=y
CONFIG_LAN9118=y
CONFIG_SMC91C111=y
+CONFIG_IMX_FEC=y
CONFIG_DS1338=y
CONFIG_PFLASH_CFI01=y
CONFIG_PFLASH_CFI02=y
diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
index 951cca3..5c84727 100644
--- a/hw/net/Makefile.objs
+++ b/hw/net/Makefile.objs
@@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
common-obj-$(CONFIG_XGMAC) += xgmac.o
common-obj-$(CONFIG_MIPSNET) += mipsnet.o
common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
+common-obj-$(CONFIG_IMX_FEC) += imx_fec.o
common-obj-$(CONFIG_CADENCE) += cadence_gem.o
common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
new file mode 100644
index 0000000..e894d50
--- /dev/null
+++ b/hw/net/imx_fec.c
@@ -0,0 +1,748 @@
+/*
+ * i.MX Fast Ethernet Controller emulation.
+ *
+ * Copyright (c) 2013 Jean-Christophe Dubois.
+ *
+ * Based on Coldfire Fast Ethernet Controller emulation.
+ *
+ * Copyright (c) 2007 CodeSourcery.
+ *
+ * This code is licensed under the GPL
+ */
+#include "hw/sysbus.h"
+#include "net/net.h"
+#include "hw/devices.h"
+
+/* For crc32 */
+#include <zlib.h>
+
+#include "hw/arm/imx.h"
+
+#ifndef IMX_FEC_DEBUG
+#define IMX_FEC_DEBUG 0
+#endif
+
+#ifndef IMX_PHY_DEBUG
+#define IMX_PHY_DEBUG 0
+#endif
+
+#if IMX_FEC_DEBUG
+#define DPRINTF(fmt, ...) \
+do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#if IMX_PHY_DEBUG
+#define DPPRINTF(fmt, ...) \
+do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define FEC_MAX_FRAME_SIZE 2032
+
+typedef struct {
+ SysBusDevice busdev;
+ NICState *nic;
+ NICConf conf;
+ qemu_irq irq;
+ MemoryRegion iomem;
+
+ uint32_t irq_state;
+ uint32_t eir;
+ uint32_t eimr;
+ uint32_t rx_enabled;
+ uint32_t rx_descriptor;
+ uint32_t tx_descriptor;
+ uint32_t ecr;
+ uint32_t mmfr;
+ uint32_t mscr;
+ uint32_t mibc;
+ uint32_t rcr;
+ uint32_t tcr;
+ uint32_t tfwr;
+ uint32_t frsr;
+ uint32_t erdsr;
+ uint32_t etdsr;
+ uint32_t emrbr;
+ uint32_t miigsk_cfgr;
+ uint32_t miigsk_enr;
+
+ uint32_t phy_status;
+ uint32_t phy_control;
+ uint32_t phy_advertise;
+ uint32_t phy_int;
+ uint32_t phy_int_mask;
+} imx_fec_state;
+
+static const VMStateDescription vmstate_imx_fec = {
+ .name = "fec",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(irq_state, imx_fec_state),
+ VMSTATE_UINT32(eir, imx_fec_state),
+ VMSTATE_UINT32(eimr, imx_fec_state),
+ VMSTATE_UINT32(rx_enabled, imx_fec_state),
+ VMSTATE_UINT32(rx_descriptor, imx_fec_state),
+ VMSTATE_UINT32(tx_descriptor, imx_fec_state),
+ VMSTATE_UINT32(ecr, imx_fec_state),
+ VMSTATE_UINT32(mmfr, imx_fec_state),
+ VMSTATE_UINT32(mscr, imx_fec_state),
+ VMSTATE_UINT32(mibc, imx_fec_state),
+ VMSTATE_UINT32(rcr, imx_fec_state),
+ VMSTATE_UINT32(tcr, imx_fec_state),
+ VMSTATE_UINT32(tfwr, imx_fec_state),
+ VMSTATE_UINT32(frsr, imx_fec_state),
+ VMSTATE_UINT32(erdsr, imx_fec_state),
+ VMSTATE_UINT32(etdsr, imx_fec_state),
+ VMSTATE_UINT32(emrbr, imx_fec_state),
+ VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
+ VMSTATE_UINT32(miigsk_enr, imx_fec_state),
+
+ VMSTATE_UINT32(phy_status, imx_fec_state),
+ VMSTATE_UINT32(phy_control, imx_fec_state),
+ VMSTATE_UINT32(phy_advertise, imx_fec_state),
+ VMSTATE_UINT32(phy_int, imx_fec_state),
+ VMSTATE_UINT32(phy_int_mask, imx_fec_state),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+#define PHY_INT_ENERGYON (1 << 7)
+#define PHY_INT_AUTONEG_COMPLETE (1 << 6)
+#define PHY_INT_FAULT (1 << 5)
+#define PHY_INT_DOWN (1 << 4)
+#define PHY_INT_AUTONEG_LP (1 << 3)
+#define PHY_INT_PARFAULT (1 << 2)
+#define PHY_INT_AUTONEG_PAGE (1 << 1)
+
+static void imx_fec_update(imx_fec_state *s);
+
+/*
+ * The MII phy could raise a GPIO to the processor which in turn
+ * could be handled as an interrpt by the OS.
+ * For now we don't handle any GPIO/interrupt line, so the OS will
+ * have to poll for the PHY status.
+ */
+static void phy_update_irq(imx_fec_state *s)
+{
+ imx_fec_update(s);
+}
+
+static void phy_update_link(imx_fec_state *s)
+{
+ /* Autonegotiation status mirrors link status. */
+ if (qemu_get_queue(s->nic)->link_down) {
+ DPPRINTF("%s: link is down\n", __func__);
+ s->phy_status &= ~0x0024;
+ s->phy_int |= PHY_INT_DOWN;
+ } else {
+ DPPRINTF("%s: link is up\n", __func__);
+ s->phy_status |= 0x0024;
+ s->phy_int |= PHY_INT_ENERGYON;
+ s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
+ }
+ phy_update_irq(s);
+}
+
+static void imx_fec_set_link(NetClientState *nc)
+{
+ phy_update_link(qemu_get_nic_opaque(nc));
+}
+
+static void phy_reset(imx_fec_state *s)
+{
+ DPPRINTF("%s\n", __func__);
+
+ s->phy_status = 0x7809;
+ s->phy_control = 0x3000;
+ s->phy_advertise = 0x01e1;
+ s->phy_int_mask = 0;
+ s->phy_int = 0;
+ phy_update_link(s);
+}
+
+static uint32_t do_phy_read(imx_fec_state *s, int reg)
+{
+ uint32_t val;
+
+ switch (reg) {
+ case 0: /* Basic Control */
+ DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);
+ return s->phy_control;
+ case 1: /* Basic Status */
+ DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
+ return s->phy_status;
+ case 2: /* ID1 */
+ DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
+ return 0x0007;
+ case 3: /* ID2 */
+ DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
+ return 0xc0d1;
+ case 4: /* Auto-neg advertisement */
+ DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
+ return s->phy_advertise;
+ case 5: /* Auto-neg Link Partner Ability */
+ DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
+ return 0x0f71;
+ case 6: /* Auto-neg Expansion */
+ DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
+ return 1;
+ /* TODO 17, 18, 27, 29, 30, 31 */
+ case 29: /* Interrupt source. */
+ val = s->phy_int;
+ s->phy_int = 0;
+ phy_update_irq(s);
+ return val;
+ case 30: /* Interrupt mask */
+ return s->phy_int_mask;
+ default:
+ DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);
+ return 0;
+ }
+}
+
+static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
+{
+ switch (reg) {
+ case 0: /* Basic Control */
+ if (val & 0x8000) {
+ phy_reset(s);
+ } else {
+ s->phy_control = val & 0x7980;
+ /* Complete autonegotiation immediately. */
+ if (val & 0x1000) {
+ s->phy_status |= 0x0020;
+ }
+ }
+ break;
+ case 4: /* Auto-neg advertisement */
+ s->phy_advertise = (val & 0x2d7f) | 0x80;
+ break;
+ /* TODO 17, 18, 27, 31 */
+ case 30: /* Interrupt mask */
+ s->phy_int_mask = val & 0xff;
+ phy_update_irq(s);
+ break;
+ default:
+ DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
+ }
+}
+
+#define FEC_INT_HB (1 << 31)
+#define FEC_INT_BABR (1 << 30)
+#define FEC_INT_BABT (1 << 29)
+#define FEC_INT_GRA (1 << 28)
+#define FEC_INT_TXF (1 << 27)
+#define FEC_INT_TXB (1 << 26)
+#define FEC_INT_RXF (1 << 25)
+#define FEC_INT_RXB (1 << 24)
+#define FEC_INT_MII (1 << 23)
+#define FEC_INT_EBERR (1 << 22)
+#define FEC_INT_LC (1 << 21)
+#define FEC_INT_RL (1 << 20)
+#define FEC_INT_UN (1 << 19)
+
+#define FEC_EN 2
+#define FEC_RESET 1
+
+/* Buffer Descriptor. */
+typedef struct {
+ uint16_t length;
+ uint16_t flags;
+ uint32_t data;
+} imx_fec_bd;
+
+#define FEC_BD_R (1 << 15)
+#define FEC_BD_E (1 << 15)
+#define FEC_BD_O1 (1 << 14)
+#define FEC_BD_W (1 << 13)
+#define FEC_BD_O2 (1 << 12)
+#define FEC_BD_L (1 << 11)
+#define FEC_BD_TC (1 << 10)
+#define FEC_BD_ABC (1 << 9)
+#define FEC_BD_M (1 << 8)
+#define FEC_BD_BC (1 << 7)
+#define FEC_BD_MC (1 << 6)
+#define FEC_BD_LG (1 << 5)
+#define FEC_BD_NO (1 << 4)
+#define FEC_BD_CR (1 << 2)
+#define FEC_BD_OV (1 << 1)
+#define FEC_BD_TR (1 << 0)
+
+static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
+{
+ cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));
+}
+
+static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
+{
+ cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
+}
+
+static void imx_fec_update(imx_fec_state *s)
+{
+ uint32_t active;
+ uint32_t changed;
+
+ active = s->eir & s->eimr;
+ changed = active ^ s->irq_state;
+ qemu_set_irq(s->irq, changed);
+ s->irq_state = active;
+}
+
+static void imx_fec_do_tx(imx_fec_state *s)
+{
+ uint32_t addr;
+ imx_fec_bd bd;
+ int frame_size;
+ int len;
+ uint8_t frame[FEC_MAX_FRAME_SIZE];
+ uint8_t *ptr;
+
+ DPRINTF("%s:\n", __func__);
+ ptr = frame;
+ frame_size = 0;
+ addr = s->tx_descriptor;
+ while (1) {
+ imx_fec_read_bd(&bd, addr);
+ DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
+ __func__, addr, bd.flags, bd.length, bd.data);
+ if ((bd.flags & FEC_BD_R) == 0) {
+ /* Run out of descriptors to transmit. */
+ break;
+ }
+ len = bd.length;
+ if (frame_size + len > FEC_MAX_FRAME_SIZE) {
+ len = FEC_MAX_FRAME_SIZE - frame_size;
+ s->eir |= FEC_INT_BABT;
+ }
+ cpu_physical_memory_read(bd.data, ptr, len);
+ ptr += len;
+ frame_size += len;
+ if (bd.flags & FEC_BD_L) {
+ /* Last buffer in frame. */
+ DPRINTF("Sending packet\n");
+ qemu_send_packet(qemu_get_queue(s->nic), frame, len);
+ ptr = frame;
+ frame_size = 0;
+ s->eir |= FEC_INT_TXF;
+ }
+ s->eir |= FEC_INT_TXB;
+ bd.flags &= ~FEC_BD_R;
+ /* Write back the modified descriptor. */
+ imx_fec_write_bd(&bd, addr);
+ /* Advance to the next descriptor. */
+ if ((bd.flags & FEC_BD_W) != 0) {
+ addr = s->etdsr;
+ } else {
+ addr += 8;
+ }
+ }
+ s->tx_descriptor = addr;
+ imx_fec_update(s);
+}
+
+static void imx_fec_enable_rx(imx_fec_state *s)
+{
+ imx_fec_bd bd;
+
+ imx_fec_read_bd(&bd, s->rx_descriptor);
+ s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);
+ if (!s->rx_enabled) {
+ DPRINTF("%s: RX buffer full\n", __func__);
+ }
+}
+
+static void imx_fec_reset(DeviceState *d)
+{
+ imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
+
+ /* Reset the FEC */
+ s->eir = 0;
+ s->eimr = 0;
+ s->rx_enabled = 0;
+ s->ecr = 0;
+ s->mscr = 0;
+ s->mibc = 0xc0000000;
+ s->rcr = 0x05ee0001;
+ s->tcr = 0;
+ s->tfwr = 0;
+ s->frsr = 0x500;
+ s->miigsk_cfgr = 0;
+ s->miigsk_enr = 0x6;
+
+ /* We also reset the PHY */
+ phy_reset(s);
+}
+
+static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
+{
+ imx_fec_state *s = (imx_fec_state *) opaque;
+
+ DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
+
+ switch (addr & 0x3ff) {
+ case 0x004:
+ return s->eir;
+ case 0x008:
+ return s->eimr;
+ case 0x010:
+ return s->rx_enabled ? (1 << 24) : 0; /* RDAR */
+ case 0x014:
+ return 0; /* TDAR */
+ case 0x024:
+ return s->ecr;
+ case 0x040:
+ return s->mmfr;
+ case 0x044:
+ return s->mscr;
+ case 0x064:
+ return s->mibc; /* MIBC */
+ case 0x084:
+ return s->rcr;
+ case 0x0c4:
+ return s->tcr;
+ case 0x0e4: /* PALR */
+ return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
+ a[1] << 16)
+ | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
+ break;
+ case 0x0e8: /* PAUR */
+ return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
+ a[5] << 16) | 0x8808;
+ case 0x0ec:
+ return 0x10000; /* OPD */
+ case 0x118:
+ return 0;
+ case 0x11c:
+ return 0;
+ case 0x120:
+ return 0;
+ case 0x124:
+ return 0;
+ case 0x144:
+ return s->tfwr;
+ case 0x14c:
+ return 0x600;
+ case 0x150:
+ return s->frsr;
+ case 0x180:
+ return s->erdsr;
+ case 0x184:
+ return s->etdsr;
+ case 0x188:
+ return s->emrbr;
+ case 0x300:
+ return s->miigsk_cfgr;
+ case 0x308:
+ return s->miigsk_enr;
+ default:
+ hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
+ return 0;
+ }
+}
+
+static void imx_fec_write(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size)
+{
+ imx_fec_state *s = (imx_fec_state *) opaque;
+
+ DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
+
+ switch (addr & 0x3ff) {
+ case 0x004: /* EIR */
+ s->eir &= ~value;
+ break;
+ case 0x008: /* EIMR */
+ s->eimr = value;
+ break;
+ case 0x010: /* RDAR */
+ if ((s->ecr & FEC_EN) && !s->rx_enabled) {
+ DPRINTF("RX enable\n");
+ imx_fec_enable_rx(s);
+ }
+ break;
+ case 0x014: /* TDAR */
+ if (s->ecr & FEC_EN) {
+ imx_fec_do_tx(s);
+ }
+ break;
+ case 0x024: /* ECR */
+ s->ecr = value;
+ if (value & FEC_RESET) {
+ DPRINTF("Reset\n");
+ imx_fec_reset(&s->busdev.qdev);
+ }
+ if ((s->ecr & FEC_EN) == 0) {
+ s->rx_enabled = 0;
+ }
+ break;
+ case 0x040: /* MMFR */
+ /* store the value */
+ s->mmfr = value;
+ if (value & (1 << 28)) {
+ DPRINTF("PHY write %d = 0x%04x\n",
+ ((int)value >> 18) & 0x1f, (int)value & 0xffff);
+ do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
+ } else {
+ s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
+ DPRINTF("PHY read %d = 0x%04x\n",
+ ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
+ }
+ /* raise the interrupt as the PHY operation is done */
+ s->eir |= FEC_INT_MII;
+ break;
+ case 0x044: /* MSCR */
+ s->mscr = value & 0xfe;
+ break;
+ case 0x064: /* MIBC */
+ /* TODO: Implement MIB. */
+ s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
+ break;
+ case 0x084: /* RCR */
+ s->rcr = value & 0x07ff003f;
+ /* TODO: Implement LOOP mode. */
+ break;
+ case 0x0c4: /* TCR */
+ /* We transmit immediately, so raise GRA immediately. */
+ s->tcr = value;
+ if (value & 1) {
+ s->eir |= FEC_INT_GRA;
+ }
+ break;
+ case 0x0e4: /* PALR */
+ s->conf.macaddr.a[0] = value >> 24;
+ s->conf.macaddr.a[1] = value >> 16;
+ s->conf.macaddr.a[2] = value >> 8;
+ s->conf.macaddr.a[3] = value;
+ break;
+ case 0x0e8: /* PAUR */
+ s->conf.macaddr.a[4] = value >> 24;
+ s->conf.macaddr.a[5] = value >> 16;
+ break;
+ case 0x0ec: /* OPDR */
+ break;
+ case 0x118: /* IAUR */
+ case 0x11c: /* IALR */
+ case 0x120: /* GAUR */
+ case 0x124: /* GALR */
+ /* TODO: implement MAC hash filtering. */
+ break;
+ case 0x144: /* TFWR */
+ s->tfwr = value & 3;
+ break;
+ case 0x14c: /* FRBR */
+ /* FRBR writes ignored. */
+ break;
+ case 0x150: /* FRSR */
+ s->frsr = (value & 0x3fc) | 0x400;
+ break;
+ case 0x180: /* ERDSR */
+ s->erdsr = value & ~3;
+ s->rx_descriptor = s->erdsr;
+ break;
+ case 0x184: /* ETDSR */
+ s->etdsr = value & ~3;
+ s->tx_descriptor = s->etdsr;
+ break;
+ case 0x188: /* EMRBR */
+ s->emrbr = value & 0x7f0;
+ break;
+ case 0x300: /* MIIGSK_CFGR */
+ s->miigsk_cfgr = value & 0x53;
+ break;
+ case 0x308: /* MIIGSK_ENR */
+ s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
+ break;
+ default:
+ hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
+ }
+ imx_fec_update(s);
+}
+
+static int imx_fec_can_receive(NetClientState *nc)
+{
+ imx_fec_state *s = qemu_get_nic_opaque(nc);
+
+ return s->rx_enabled;
+}
+
+static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
+ size_t len)
+{
+ imx_fec_state *s = qemu_get_nic_opaque(nc);
+ imx_fec_bd bd;
+ uint32_t flags = 0;
+ uint32_t addr;
+ uint32_t crc;
+ uint32_t buf_addr;
+ uint8_t *crc_ptr;
+ unsigned int buf_len;
+ size_t size = len;
+
+ DPRINTF("%s: len %d\n", __func__, (int)size);
+
+ if (!s->rx_enabled) {
+ DPRINTF("%s: Unexpected packet\n", __func__);
+ return 0;
+ }
+ /* 4 bytes for the CRC. */
+ size += 4;
+ crc = cpu_to_be32(crc32(~0, buf, size));
+ crc_ptr = (uint8_t *) &crc;
+ /* Huge frames are truncted. */
+ if (size > FEC_MAX_FRAME_SIZE) {
+ size = FEC_MAX_FRAME_SIZE;
+ flags |= FEC_BD_TR | FEC_BD_LG;
+ }
+ /* Frames larger than the user limit just set error flags. */
+ if (size > (s->rcr >> 16)) {
+ flags |= FEC_BD_LG;
+ }
+ addr = s->rx_descriptor;
+ while (size > 0) {
+ imx_fec_read_bd(&bd, addr);
+ if ((bd.flags & FEC_BD_E) == 0) {
+ /* No descriptors available. Bail out. */
+ /*
+ * FIXME: This is wrong. We should probably either
+ * save the remainder for when more RX buffers are
+ * available, or flag an error.
+ */
+ DPRINTF("%s: Lost end of frame\n", __func__);
+ break;
+ }
+ buf_len = (size <= s->emrbr) ? size : s->emrbr;
+ bd.length = buf_len;
+ size -= buf_len;
+ DPRINTF("rx_bd %x length %d\n", addr, bd.length);
+ /* The last 4 bytes are the CRC. */
+ if (size < 4) {
+ buf_len += size - 4;
+ }
+ buf_addr = bd.data;
+ cpu_physical_memory_write(buf_addr, buf, buf_len);
+ buf += buf_len;
+ if (size < 4) {
+ cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
+ 4 - size);
+ crc_ptr += 4 - size;
+ }
+ bd.flags &= ~FEC_BD_E;
+ if (size == 0) {
+ /* Last buffer in frame. */
+ bd.flags |= flags | FEC_BD_L;
+ DPRINTF("rx frame flags %04x\n", bd.flags);
+ s->eir |= FEC_INT_RXF;
+ } else {
+ s->eir |= FEC_INT_RXB;
+ }
+ imx_fec_write_bd(&bd, addr);
+ /* Advance to the next descriptor. */
+ if ((bd.flags & FEC_BD_W) != 0) {
+ addr = s->erdsr;
+ } else {
+ addr += 8;
+ }
+ }
+ s->rx_descriptor = addr;
+ imx_fec_enable_rx(s);
+ imx_fec_update(s);
+ return len;
+}
+
+static const MemoryRegionOps imx_fec_ops = {
+ .read = imx_fec_read,
+ .write = imx_fec_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void imx_fec_cleanup(NetClientState *nc)
+{
+ imx_fec_state *s = qemu_get_nic_opaque(nc);
+
+ s->nic = NULL;
+}
+
+static NetClientInfo net_imx_fec_info = {
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
+ .size = sizeof(NICState),
+ .can_receive = imx_fec_can_receive,
+ .receive = imx_fec_receive,
+ .cleanup = imx_fec_cleanup,
+ .link_status_changed = imx_fec_set_link,
+};
+
+static int imx_fec_init(SysBusDevice *dev)
+{
+ imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
+
+ memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
+ sysbus_init_mmio(dev, &s->iomem);
+ sysbus_init_irq(dev, &s->irq);
+ qemu_macaddr_default_if_unset(&s->conf.macaddr);
+
+ s->conf.peers.ncs[0] = nd_table[0].netdev;
+
+ s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
+ object_get_typename(OBJECT(dev)), dev->qdev.id,
+ s);
+ qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
+
+ return 0;
+}
+
+static Property imx_fec_properties[] = {
+ DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void imx_fec_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+ k->init = imx_fec_init;
+ dc->reset = imx_fec_reset;
+ dc->vmsd = &vmstate_imx_fec;
+ dc->props = imx_fec_properties;
+}
+
+static const TypeInfo imx_fec_info = {
+ .name = "fec",
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(imx_fec_state),
+ .class_init = imx_fec_class_init,
+};
+
+static void imx_fec_register_types(void)
+{
+ type_register_static(&imx_fec_info);
+}
+
+void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
+{
+ NICInfo *nd;
+ DeviceState *dev;
+ SysBusDevice *s;
+
+ if (nic >= MAX_NICS) {
+ hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
+ nic, MAX_NICS);
+ }
+
+ nd = &nd_table[nic];
+
+ qemu_check_nic_model(nd, "fec");
+ dev = qdev_create(NULL, "fec");
+ qdev_set_nic_properties(dev, nd);
+ qdev_init_nofail(dev);
+ s = SYS_BUS_DEVICE(dev);
+ sysbus_mmio_map(s, 0, base);
+ sysbus_connect_irq(s, 0, irq);
+};
+
+type_init(imx_fec_register_types)
--
1.8.1.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-04 14:09 [Qemu-devel] [PATCH v2 0/4] Add i.MX25 support through the 3DS evaluation board Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver Jean-Christophe DUBOIS
@ 2013-05-04 14:09 ` Jean-Christophe DUBOIS
2013-05-05 3:14 ` Peter Crosthwaite
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 3/4] Add i.MX25 3DS evaluation board support Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 4/4] Add qtest support for i.MX I2C device emulation Jean-Christophe DUBOIS
3 siblings, 1 reply; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-04 14:09 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, peter.crosthwaite, peter.chubb, afaerber,
Jean-Christophe DUBOIS
The slave mode is not implemented.
Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
---
default-configs/arm-softmmu.mak | 2 +
hw/i2c/Makefile.objs | 1 +
hw/i2c/imx_i2c.c | 383 ++++++++++++++++++++++++++++++++++++++++
include/hw/arm/imx.h | 1 +
4 files changed, 387 insertions(+)
create mode 100644 hw/i2c/imx_i2c.c
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index b3a0207..a20f112 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -81,3 +81,5 @@ CONFIG_VERSATILE_PCI=y
CONFIG_VERSATILE_I2C=y
CONFIG_SDHCI=y
+
+CONFIG_IMX_I2C=y
diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 648278e..d27bbaa 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o
common-obj-$(CONFIG_APM) += pm_smbus.o
common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
+common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
obj-$(CONFIG_OMAP) += omap_i2c.o
diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
new file mode 100644
index 0000000..5b0d046
--- /dev/null
+++ b/hw/i2c/imx_i2c.c
@@ -0,0 +1,383 @@
+/*
+ * i.MX I2C Bus Serial Interface Emulation
+ *
+ * Copyright (C) 2013 Jean-Christophe Dubois.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "hw/sysbus.h"
+#include "hw/i2c/i2c.h"
+
+#ifndef IMX_I2C_DEBUG
+#define IMX_I2C_DEBUG 0
+#endif
+
+#define TYPE_IMX_I2C "imx.i2c"
+#define IMX_I2C(obj) \
+ OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C)
+
+/* i.MX I2C memory map */
+#define IMX_I2C_MEM_SIZE 0x14
+#define IADR_ADDR 0x00 /* address register */
+#define IFDR_ADDR 0x04 /* frequency divider register */
+#define I2CR_ADDR 0x08 /* control register */
+#define I2SR_ADDR 0x0c /* status register */
+#define I2DR_ADDR 0x10 /* data register */
+
+#define IADR_MASK 0xFE
+#define IADR_RESET 0
+
+#define IFDR_MASK 0x3F
+#define IFDR_RESET 0
+
+#define I2CR_IEN (1 << 7)
+#define I2CR_IIEN (1 << 6)
+#define I2CR_MSTA (1 << 5)
+#define I2CR_MTX (1 << 4)
+#define I2CR_TXAK (1 << 3)
+#define I2CR_RSTA (1 << 2)
+#define I2CR_MASK 0xFC
+#define I2CR_RESET 0
+
+#define I2SR_ICF (1 << 7)
+#define I2SR_IAAF (1 << 6)
+#define I2SR_IBB (1 << 5)
+#define I2SR_IAL (1 << 4)
+#define I2SR_SRW (1 << 2)
+#define I2SR_IIF (1 << 1)
+#define I2SR_RXAK (1 << 0)
+#define I2SR_MASK 0xE9
+#define I2SR_RESET 0x81
+
+#define I2DR_MASK 0xFF
+#define I2DR_RESET 0
+
+#define ADDR_RESET 0xFF00
+
+#if IMX_I2C_DEBUG
+#define DPRINT(fmt, args...) \
+ do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0)
+
+static const char *imx_i2c_get_regname(unsigned offset)
+{
+ switch (offset) {
+ case IADR_ADDR:
+ return "IADR";
+ case IFDR_ADDR:
+ return "IFDR";
+ case I2CR_ADDR:
+ return "I2CR";
+ case I2SR_ADDR:
+ return "I2SR";
+ case I2DR_ADDR:
+ return "I2DR";
+ default:
+ return "[?]";
+ }
+}
+#else
+#define DPRINT(fmt, args...) do { } while (0)
+#endif
+
+typedef struct imx_i2c_state {
+ SysBusDevice parent_obj;
+ MemoryRegion iomem;
+ i2c_bus *bus;
+ qemu_irq irq;
+
+ uint16_t address;
+
+ uint16_t iadr;
+ uint16_t ifdr;
+ uint16_t i2cr;
+ uint16_t i2sr;
+ uint16_t i2dr_read;
+ uint16_t i2dr_write;
+} imx_i2c_state;
+
+static inline bool imx_i2c_is_enabled(imx_i2c_state *s)
+{
+ return s->i2cr & I2CR_IEN;
+}
+
+static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
+{
+ return s->i2cr & I2CR_IIEN;
+}
+
+static inline bool imx_i2c_is_master(imx_i2c_state *s)
+{
+ return s->i2cr & I2CR_MSTA;
+}
+
+static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
+{
+ return s->i2cr & I2CR_MTX;
+}
+
+static void imx_i2c_reset(DeviceState *dev)
+{
+ imx_i2c_state *s = IMX_I2C(dev);
+
+ if (s->address != ADDR_RESET) {
+ i2c_end_transfer(s->bus);
+ }
+
+ s->address = ADDR_RESET;
+ s->iadr = IADR_RESET;
+ s->ifdr = IFDR_RESET;
+ s->i2cr = I2CR_RESET;
+ s->i2sr = I2SR_RESET;
+ s->i2dr_read = I2DR_RESET;
+ s->i2dr_write = I2DR_RESET;
+}
+
+static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
+{
+ /*
+ * raise an interrupt if the device is enabled and it is configured
+ * to generate some interrupts.
+ */
+ if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
+ s->i2sr |= I2SR_IIF;
+ qemu_irq_raise(s->irq);
+ }
+}
+
+static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+ uint16_t value;
+ imx_i2c_state *s = IMX_I2C(opaque);
+
+ switch (offset) {
+ case IADR_ADDR:
+ value = s->iadr;
+ break;
+ case IFDR_ADDR:
+ value = s->ifdr;
+ break;
+ case I2CR_ADDR:
+ value = s->i2cr;
+ break;
+ case I2SR_ADDR:
+ value = s->i2sr;
+ break;
+ case I2DR_ADDR:
+ value = s->i2dr_read;
+
+ if (imx_i2c_is_master(s)) { /* master mode */
+ int ret = 0xff;
+
+ if (s->address == ADDR_RESET) {
+ /* something is wrong as the address is not set */
+ DPRINT("Trying to read without specifying the slave address\n");
+ } else if (s->i2cr & I2CR_MTX) {
+ DPRINT("Trying to read but MTX is set\n");
+ } else {
+ /* get the next byte */
+ ret = i2c_recv(s->bus);
+
+ if (ret >= 0) {
+ imx_i2c_raise_interrupt(s);
+ } else {
+ DPRINT("read failed for device 0x%02x\n" s->address);
+ ret = 0xff;
+ }
+ }
+
+ s->i2dr_read = ret;
+ }
+ break;
+ default:
+ hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
+ break;
+ }
+
+ DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset),
+ (unsigned int)offset, value);
+
+ return (uint64_t)value;
+}
+
+static void imx_i2c_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned size)
+{
+ imx_i2c_state *s = IMX_I2C(opaque);
+
+ DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset),
+ (unsigned int)offset, (int)value);
+
+ value &= 0xff;
+
+ switch (offset) {
+ case IADR_ADDR:
+ s->iadr = value & IADR_MASK;
+ /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */
+ break;
+ case IFDR_ADDR:
+ s->ifdr = value & IFDR_MASK;
+ break;
+ case I2CR_ADDR:
+ if (imx_i2c_is_enabled(s) && ((value & I2CR_IEN) == 0)) {
+ /* This is a soft reset. IADR is preserved during soft resets */
+ uint16_t iadr = s->iadr;
+ imx_i2c_reset(&s->parent_obj.qdev);
+ s->iadr = iadr;
+ } else { /* normal write */
+ s->i2cr = value & I2CR_MASK;
+
+ if (imx_i2c_is_master(s)) { /* master mode */
+ /* set the bus to busy */
+ s->i2sr |= I2SR_IBB;
+ } else { /* slave mode */
+ /* bus is not busy anymore */
+ s->i2sr &= ~I2SR_IBB;
+
+ /*
+ *if we unset the master mode then it ends the ongoing
+ * transfer if any
+ */
+ if (s->address != ADDR_RESET) {
+ i2c_end_transfer(s->bus);
+ s->address = ADDR_RESET;
+ }
+ }
+
+ if (s->i2cr & I2CR_RSTA) { /* Restart */
+ /* if this is a restart then it ends the ongoing transfer */
+ if (s->address != ADDR_RESET) {
+ i2c_end_transfer(s->bus);
+ s->address = ADDR_RESET;
+ s->i2cr &= ~I2CR_RSTA;
+ }
+ }
+ }
+ break;
+ case I2SR_ADDR:
+ /*
+ * if the user writes 0 to IIF then lower the interrupt and
+ * reset the bit
+ */
+ if ((s->i2sr & I2SR_IIF) && !(value & I2SR_IIF)) {
+ s->i2sr &= ~I2SR_IIF;
+ qemu_irq_lower(s->irq);
+ }
+
+ /*
+ * if the user writes 0 to IAL, reset the bit
+ */
+ if ((s->i2sr & I2SR_IAL) && !(value & I2SR_IAL)) {
+ s->i2sr &= ~I2SR_IAL;
+ }
+
+ break;
+ case I2DR_ADDR:
+ /* if the device is not enabled, nothing to do */
+ if (!imx_i2c_is_enabled(s)) {
+ break;
+ }
+
+ s->i2dr_write = value & I2DR_MASK;
+
+ if (imx_i2c_is_master(s)) { /* master mode */
+ /* If this is the first write cycle then it is the slave addr */
+ if (s->address == ADDR_RESET) {
+ if (i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1,
+ (int)s->i2dr_write & 0x01)) {
+ /* if non zero is returned, the adress is not valid */
+ s->i2sr |= I2SR_RXAK;
+ } else {
+ s->address = s->i2dr_write;
+ s->i2sr &= ~I2SR_RXAK;
+ imx_i2c_raise_interrupt(s);
+ }
+ } else { /* This is a normal data write */
+ if (i2c_send(s->bus, s->i2dr_write)) {
+ /* if the target return non zero then end the transfer */
+ s->i2sr |= I2SR_RXAK;
+ s->address = ADDR_RESET;
+ i2c_end_transfer(s->bus);
+ } else {
+ s->i2sr &= ~I2SR_RXAK;
+ imx_i2c_raise_interrupt(s);
+ }
+ }
+ }
+ break;
+ default:
+ hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
+ break;
+ }
+}
+
+static const MemoryRegionOps imx_i2c_ops = {
+ .read = imx_i2c_read,
+ .write = imx_i2c_write,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 2,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription imx_i2c_vmstate = {
+ .name = TYPE_IMX_I2C,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT16(address, imx_i2c_state),
+ VMSTATE_UINT16(iadr, imx_i2c_state),
+ VMSTATE_UINT16(ifdr, imx_i2c_state),
+ VMSTATE_UINT16(i2cr, imx_i2c_state),
+ VMSTATE_UINT16(i2sr, imx_i2c_state),
+ VMSTATE_UINT16(i2dr_read, imx_i2c_state),
+ VMSTATE_UINT16(i2dr_write, imx_i2c_state),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void imx_i2c_realize(DeviceState *dev, Error **errp)
+{
+ imx_i2c_state *s = IMX_I2C(dev);
+
+ memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C,
+ IMX_I2C_MEM_SIZE);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
+ sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
+ s->bus = i2c_init_bus(&SYS_BUS_DEVICE(dev)->qdev, "i2c");
+}
+
+static void imx_i2c_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->vmsd = &imx_i2c_vmstate;
+ dc->reset = imx_i2c_reset;
+ dc->realize = imx_i2c_realize;
+}
+
+static const TypeInfo imx_i2c_type_info = {
+ .name = TYPE_IMX_I2C,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(imx_i2c_state),
+ .class_init = imx_i2c_class_init,
+};
+
+static void imx_i2c_register_types(void)
+{
+ type_register_static(&imx_i2c_type_info);
+}
+
+type_init(imx_i2c_register_types)
diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h
index ea9e093..a875171 100644
--- a/include/hw/arm/imx.h
+++ b/include/hw/arm/imx.h
@@ -30,5 +30,6 @@ void imx_timerg_create(const hwaddr addr,
qemu_irq irq,
DeviceState *ccm);
+void imx_fec_create(int nic, const hwaddr base, qemu_irq irq);
#endif /* IMX_H */
--
1.8.1.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] Add i.MX25 3DS evaluation board support.
2013-05-04 14:09 [Qemu-devel] [PATCH v2 0/4] Add i.MX25 support through the 3DS evaluation board Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver Jean-Christophe DUBOIS
@ 2013-05-04 14:09 ` Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 4/4] Add qtest support for i.MX I2C device emulation Jean-Christophe DUBOIS
3 siblings, 0 replies; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-04 14:09 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, peter.crosthwaite, peter.chubb, afaerber,
Jean-Christophe DUBOIS
For now we support:
* timers (GPT and EPIT)
* serial ports (only 2 out of 5 possible)
* ethernet (through the newly added FEC driver)
* I2C (through the newly added I2C driver)
A ds1338 I2C RTC chip was added on the first i2c bus to allow
automatic test through qtest. This RTC is not present on the real
board.
Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
---
hw/arm/Makefile.objs | 1 +
hw/arm/imx25_3ds.c | 258 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 259 insertions(+)
create mode 100644 hw/arm/imx25_3ds.c
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 9e3a06f..2f4280d 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -2,6 +2,7 @@ obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
obj-y += omap_sx1.o palm.o pic_cpu.o realview.o spitz.o stellaris.o
obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
+obj-y += imx25_3ds.o
obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
obj-y += omap1.o omap2.o strongarm.o
diff --git a/hw/arm/imx25_3ds.c b/hw/arm/imx25_3ds.c
new file mode 100644
index 0000000..b7ff26d
--- /dev/null
+++ b/hw/arm/imx25_3ds.c
@@ -0,0 +1,258 @@
+/*
+ * Copyright (c) 2013 Jean-Christophe Dubois
+ *
+ * 3Dstack Board System emulation.
+ *
+ * Based on hw/arm/kzm.c
+ *
+ * Copyright (c) 2008 OKL and 2011 NICTA
+ * Written by Hans at OK-Labs
+ * Updated by Peter Chubb.
+ *
+ * This code is licensed under the GPL, version 2 or later.
+ * See the file `COPYING' in the top level directory.
+ *
+ * It (partially) emulates a i.MX25 3D-Stack PDK board
+ */
+
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+#include "hw/hw.h"
+#include "hw/arm/arm.h"
+#include "hw/devices.h"
+#include "net/net.h"
+#include "sysemu/sysemu.h"
+#include "hw/boards.h"
+#include "hw/char/serial.h"
+#include "hw/arm/imx.h"
+#include "hw/i2c/i2c.h"
+
+#include "sysemu/qtest.h"
+
+/* Memory map for 3D-Stack Emulation Baseboard:
+ * 0x00000000-0x00003fff 16k ROM IGNORED
+ * 0x00004000-0x00403fff Reserved IGNORED
+ * 0x00404000-0x00408fff 20k ROM IGNORED
+ * 0x00409000-0x0fffffff Reserved IGNORED
+ * 0x10000000-0x1fffffff Reserved IGNORED
+ * 0x20000000-0x2fffffff Reserved IGNORED
+ * 0x30000000-0x3fffffff Reserved IGNORED
+ * 0x40000000-0x43efffff Reserved IGNORED
+ * 0x43f00000-0x6fffffff I.MX25 Internal Register Space
+ * 0x43f00000 IO_AREA0
+ * 0x43f80000 I2C0 EMULATED
+ * 0x43f84000 I2C2 EMULATED
+ * 0x43f98000 I2C1 EMULATED
+ * 0x43f90000 UART1 EMULATED
+ * 0x43f94000 UART2 EMULATED
+ * 0x43fb0000 UART4 IGNORED
+ * 0x43fb4000 UART5 IGNORED
+ * 0x5000c000 UART3 IGNORED
+ * 0x53f80000 CCM EMULATED
+ * 0x53f84000 GPT 4 EMULATED
+ * 0x53f88000 GPT 3 EMULATED
+ * 0x53f8c000 GPT 2 EMULATED
+ * 0x53f90000 GPT 1 EMULATED
+ * 0x53f94000 PIT 1 EMULATED
+ * 0x53f98000 PIT 2 EMULATED
+ * 0x68000000 ASIC EMULATED
+ * 0x78000000-0x7801ffff SRAM EMULATED
+ * 0x78020000-0x7fffffff SRAM Aliasing EMULATED
+ * 0x80000000-0x87ffffff RAM + Alias EMULATED
+ * 0x90000000-0x9fffffff RAM + Alias EMULATED
+ * 0xa0000000-0xa7ffffff Flash IGNORED
+ * 0xa8000000-0xafffffff Flash IGNORED
+ * 0xb0000000-0xb1ffffff SRAM IGNORED
+ * 0xb2000000-0xb3ffffff SRAM IGNORED
+ * 0xb4000000-0xb5ffffff CS4 IGNORED
+ * 0xb6000000-0xb8000fff Reserved IGNORED
+ * 0xb8001000-0xb8001fff SDRAM CTRL reg IGNORED
+ * 0xb8002000-0xb8002fff WEIM CTRL reg IGNORED
+ * 0xb8003000-0xb8003fff M3IF CTRL reg IGNORED
+ * 0xb8004000-0xb8004fff EMI CTRL reg IGNORED
+ * 0xb8005000-0xbaffffff Reserved IGNORED
+ * 0xbb000000-0xbb000fff NAND flash area buf IGNORED
+ * 0xbb001000-0xbb0011ff NAND flash reserved IGNORED
+ * 0xbb001200-0xbb001dff Reserved IGNORED
+ * 0xbb001e00-0xbb001fff NAN flash CTRL reg IGNORED
+ * 0xbb012000-0xbfffffff Reserved IGNORED
+ * 0xc0000000-0xffffffff Reserved IGNORED
+ */
+
+#define IMX25_SRAM_ADDRESS (0x78000000)
+#define IMX25_SRAMSIZE (128*1024)
+#define IMX25_CS_SRAMSIZE (128*1024*1024)
+#define IMX25_3DS_ADDRESS (0x80000000)
+#define IMX25_CS_RAMSIZE (256*1024*1024)
+
+static struct arm_boot_info imx25_3ds_binfo = {
+ .loader_start = IMX25_3DS_ADDRESS,
+ .board_id = 1771,
+};
+
+static void imx25_3ds_init(QEMUMachineInitArgs *args)
+{
+ int i;
+ ram_addr_t ram_size = args->ram_size;
+ const char *cpu_model = args->cpu_model;
+ const char *kernel_filename = args->kernel_filename;
+ const char *kernel_cmdline = args->kernel_cmdline;
+ const char *initrd_filename = args->initrd_filename;
+ ARMCPU *cpu;
+ MemoryRegion *address_space_mem = get_system_memory();
+ qemu_irq *cpu_pic;
+ DeviceState *pic_dev;
+ DeviceState *ccm;
+ MemoryRegion *sram = g_new(MemoryRegion, 1);
+ MemoryRegion *sram_alias = g_new(MemoryRegion, 1);
+
+ if (!cpu_model) {
+ cpu_model = "arm926";
+ }
+
+ cpu = cpu_arm_init(cpu_model);
+ if (!cpu) {
+ fprintf(stderr, "Unable to find CPU definition\n");
+ exit(1);
+ }
+
+ if (ram_size > (2 * IMX25_CS_RAMSIZE)) {
+ fprintf(stderr, "i.MX25 can support only up to %d MB\n",
+ 2 * IMX25_CS_RAMSIZE / (1024 * 1024));
+ exit(1);
+ }
+
+ /* create our main memory */
+ for (i = 0; i <= (ram_size / IMX25_CS_RAMSIZE); i++) {
+ ram_addr_t blk_size = ram_size - (IMX25_CS_RAMSIZE * i);
+ MemoryRegion *ram;
+ char ram_name[20];
+
+ if (blk_size > IMX25_CS_RAMSIZE) {
+ blk_size = IMX25_CS_RAMSIZE;
+ }
+
+ if (blk_size == 0) {
+ break;
+ }
+
+ sprintf(ram_name, "imx25.ram%d", i);
+
+ ram = g_new(MemoryRegion, 1);
+ memory_region_init_ram(ram, ram_name, blk_size);
+ vmstate_register_ram_global(ram);
+ memory_region_add_subregion(address_space_mem, IMX25_3DS_ADDRESS
+ + (IMX25_CS_RAMSIZE * i), ram);
+
+ /* Add ram alias */
+ if (blk_size < IMX25_CS_RAMSIZE) {
+ MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
+ char alias_name[20];
+
+ sprintf(alias_name, "ram.alias%d", i);
+
+ memory_region_init_alias(ram_alias, alias_name, ram, 0,
+ IMX25_CS_RAMSIZE - blk_size);
+ memory_region_add_subregion(address_space_mem, IMX25_3DS_ADDRESS
+ + (IMX25_CS_RAMSIZE * i) + blk_size,
+ ram_alias);
+ }
+ }
+
+ /* create the sram area */
+ memory_region_init_ram(sram, "imx25.sram", IMX25_SRAMSIZE);
+ vmstate_register_ram_global(sram);
+ memory_region_add_subregion(address_space_mem, IMX25_SRAM_ADDRESS,
+ sram);
+
+ /* add sram alias */
+ memory_region_init_alias(sram_alias, "sram.alias", sram, 0,
+ IMX25_CS_SRAMSIZE - IMX25_SRAMSIZE);
+ memory_region_add_subregion(address_space_mem,
+ IMX25_SRAM_ADDRESS + IMX25_SRAMSIZE,
+ sram_alias);
+
+ /* add the PIC */
+ cpu_pic = arm_pic_init_cpu(cpu);
+ pic_dev = sysbus_create_varargs("imx_avic", 0x68000000,
+ cpu_pic[ARM_PIC_CPU_IRQ],
+ cpu_pic[ARM_PIC_CPU_FIQ], NULL);
+
+ /* add some serial lines */
+ imx_serial_create(0, 0x43f90000, qdev_get_gpio_in(pic_dev, 45));
+ imx_serial_create(1, 0x43f94000, qdev_get_gpio_in(pic_dev, 32));
+ /*
+ * QEMU does not support more than 4 serial ports. Too bad.
+ */
+ /*
+ imx_serial_create(3, 0x5000c000, qdev_get_gpio_in(pic_dev, 18));
+ imx_serial_create(4, 0x43fb0000, qdev_get_gpio_in(pic_dev, 46));
+ imx_serial_create(5, 0x43fb4000, qdev_get_gpio_in(pic_dev, 47));
+ */
+
+ ccm = sysbus_create_simple("imx_ccm", 0x53f80000, NULL);
+
+ /* add gpt timers */
+ imx_timerg_create(0x53f84000, qdev_get_gpio_in(pic_dev, 1), ccm);
+ imx_timerg_create(0x53f88000, qdev_get_gpio_in(pic_dev, 29), ccm);
+ imx_timerg_create(0x53f8c000, qdev_get_gpio_in(pic_dev, 53), ccm);
+ imx_timerg_create(0x53f90000, qdev_get_gpio_in(pic_dev, 54), ccm);
+
+ /* add epit timers */
+ imx_timerp_create(0x53f94000, qdev_get_gpio_in(pic_dev, 28), ccm);
+ imx_timerp_create(0x53f98000, qdev_get_gpio_in(pic_dev, 27), ccm);
+
+ imx_fec_create(0, 0x50038000, qdev_get_gpio_in(pic_dev, 57));
+
+ /*** I2C ***/
+ for (i = 0; i < 3; i++) {
+ static uint32_t addr[] = {0x43F80000, 0x43F98000, 0x43F84000};
+ static uint32_t irq[] = {3, 4, 10};
+ SysBusDevice *busdev;
+ DeviceState *i2c_dev = qdev_create(NULL, "imx.i2c");
+
+ qdev_init_nofail(i2c_dev);
+ busdev = SYS_BUS_DEVICE(i2c_dev);
+ sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(pic_dev, irq[i]));
+ sysbus_mmio_map(busdev, 0, addr[i]);
+
+ if (i == 0) {
+ /*
+ * this I2C device doesn't exits on the real board.
+ * We add it to be able to do a bit of simple qtest.
+ * see "make check" for details
+ */
+ i2c_create_slave((i2c_bus *)qdev_get_child_bus(i2c_dev, "i2c"),
+ "ds1338", 0x68);
+ }
+ }
+
+ imx25_3ds_binfo.ram_size = ram_size;
+ imx25_3ds_binfo.kernel_filename = kernel_filename;
+ imx25_3ds_binfo.kernel_cmdline = kernel_cmdline;
+ imx25_3ds_binfo.initrd_filename = initrd_filename;
+ imx25_3ds_binfo.nb_cpus = 1;
+
+ /*
+ * We test explicitely for qtest here as it is not done (yet?) in
+ * arm_load_kernel(). Without this the "make check" command would
+ * fail.
+ */
+ if (!qtest_enabled()) {
+ arm_load_kernel(cpu, &imx25_3ds_binfo);
+ }
+}
+
+static QEMUMachine imx25_3ds_machine = {
+ .name = "imx25_3ds",
+ .desc = "ARM i.MX25 PDK board (ARM926)",
+ .init = imx25_3ds_init,
+ DEFAULT_MACHINE_OPTIONS,
+};
+
+static void imx25_3ds_machine_init(void)
+{
+ qemu_register_machine(&imx25_3ds_machine);
+}
+
+machine_init(imx25_3ds_machine_init)
--
1.8.1.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] Add qtest support for i.MX I2C device emulation.
2013-05-04 14:09 [Qemu-devel] [PATCH v2 0/4] Add i.MX25 support through the 3DS evaluation board Jean-Christophe DUBOIS
` (2 preceding siblings ...)
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 3/4] Add i.MX25 3DS evaluation board support Jean-Christophe DUBOIS
@ 2013-05-04 14:09 ` Jean-Christophe DUBOIS
2013-05-04 16:53 ` Andreas Färber
3 siblings, 1 reply; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-04 14:09 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, peter.crosthwaite, peter.chubb, afaerber,
Jean-Christophe DUBOIS
This is using a ds1338 RTC chip on the i2c bus. This RTC
chip is nop present on the real board
Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
---
tests/Makefile | 3 +
tests/ds1338-test.c | 64 ++++++++++++++
tests/libqos/i2c-imx.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++++
tests/libqos/i2c.h | 3 +
4 files changed, 294 insertions(+)
create mode 100644 tests/ds1338-test.c
create mode 100644 tests/libqos/i2c-imx.c
diff --git a/tests/Makefile b/tests/Makefile
index bf41d10..5f7a0e0 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -64,6 +64,7 @@ gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y)
gcov-files-sparc-y += hw/m48t59.c
gcov-files-sparc64-y += hw/m48t59.c
check-qtest-arm-y = tests/tmp105-test$(EXESUF)
+check-qtest-arm-y += tests/ds1338-test$(EXESUF)
gcov-files-arm-y += hw/tmp105.c
GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
@@ -123,12 +124,14 @@ libqos-obj-y += tests/libqos/i2c.o
libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o tests/libqos/fw_cfg-pc.o
libqos-pc-obj-y += tests/libqos/malloc-pc.o
libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
+libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
tests/rtc-test$(EXESUF): tests/rtc-test.o
tests/m48t59-test$(EXESUF): tests/m48t59-test.o
tests/fdc-test$(EXESUF): tests/fdc-test.o
tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
+tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
diff --git a/tests/ds1338-test.c b/tests/ds1338-test.c
new file mode 100644
index 0000000..3e3fa0b
--- /dev/null
+++ b/tests/ds1338-test.c
@@ -0,0 +1,64 @@
+/*
+ * QTest testcase for the DS1338 RTC
+ *
+ * Copyright (c) 2013 Jean-Christophe Dubois
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "libqtest.h"
+#include "libqos/i2c.h"
+
+#include <glib.h>
+
+#define IMX25_I2C_0_BASE 0x43F80000
+
+#define DS1338_ADDR 0x68
+
+static I2CAdapter *i2c;
+static uint8_t addr;
+
+#define bcd2bin(x) (((x) & 0x0f) + ((x) >> 4) * 10)
+
+static void send_and_receive(void)
+{
+ uint8_t cmd[1];
+ uint8_t resp[7];
+ time_t now = time(NULL);
+ struct tm *tm_ptr = localtime(&now);
+
+ /* reset the index in the RTC memory */
+ cmd[0] = 0;
+ i2c_send(i2c, addr, cmd, 1);
+
+ /* retrieve the date */
+ i2c_recv(i2c, addr, resp, 7);
+
+ /* check retreived time againt local time */
+ g_assert_cmpuint(bcd2bin(resp[4]), == , tm_ptr->tm_mday);
+ g_assert_cmpuint(bcd2bin(resp[5]), == , 1 + tm_ptr->tm_mon);
+ g_assert_cmpuint(2000 + bcd2bin(resp[6]), == , 1900 + tm_ptr->tm_year);
+}
+
+int main(int argc, char **argv)
+{
+ QTestState *s = NULL;
+ int ret;
+
+ g_test_init(&argc, &argv, NULL);
+
+ s = qtest_start("-display none -machine imx25_3ds");
+ i2c = imx_i2c_create(IMX25_I2C_0_BASE);
+ addr = DS1338_ADDR;
+
+ qtest_add_func("/ds1338/tx-rx", send_and_receive);
+
+ ret = g_test_run();
+
+ if (s) {
+ qtest_quit(s);
+ }
+ g_free(i2c);
+
+ return ret;
+}
diff --git a/tests/libqos/i2c-imx.c b/tests/libqos/i2c-imx.c
new file mode 100644
index 0000000..da7316f
--- /dev/null
+++ b/tests/libqos/i2c-imx.c
@@ -0,0 +1,224 @@
+/*
+ * QTest i.MX I2C driver
+ *
+ * Copyright (c) 2013 Jean-Christophe Dubois
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "libqos/i2c.h"
+
+#include <glib.h>
+#include <string.h>
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "libqtest.h"
+
+enum IMXI2CRegisters {
+ IMX_I2C_IADR = 0x00,
+ IMX_I2C_IFDR = 0x04,
+ IMX_I2C_I2CR = 0x08,
+ IMX_I2C_I2SR = 0x0c,
+ IMX_I2C_I2DR = 0x10,
+};
+
+enum IMXI2CCRBits {
+ IMX_I2C_I2CR_IEN = 1 << 7,
+ IMX_I2C_I2CR_IIEN = 1 << 6,
+ IMX_I2C_I2CR_MSTA = 1 << 5,
+ IMX_I2C_I2CR_MTX = 1 << 4,
+ IMX_I2C_I2CR_TXAK = 1 << 3,
+ IMX_I2C_I2CR_RSTA = 1 << 2,
+};
+
+enum IMXI2CSRBits {
+ IMX_I2C_I2SR_ICF = 1 << 7,
+ IMX_I2C_I2SR_IAAF = 1 << 6,
+ IMX_I2C_I2SR_IBB = 1 << 5,
+ IMX_I2C_I2SR_IAL = 1 << 4,
+ IMX_I2C_I2SR_SRW = 1 << 2,
+ IMX_I2C_I2SR_IIF = 1 << 1,
+ IMX_I2C_I2SR_RXAK = 1 << 0,
+};
+
+enum IMXI2CDirection {
+ IMX_I2C_READ,
+ IMX_I2C_WRITE,
+};
+
+typedef struct IMXI2C {
+ I2CAdapter parent;
+
+ uint64_t addr;
+} IMXI2C;
+
+
+static void imx_i2c_set_slave_addr(IMXI2C *s, uint8_t addr,
+ enum IMXI2CDirection direction)
+{
+ writeb(s->addr + IMX_I2C_I2DR, (addr << 1) |
+ (direction == IMX_I2C_READ ? 1 : 0));
+}
+
+static void imx_i2c_send(I2CAdapter *i2c, uint8_t addr,
+ const uint8_t *buf, uint16_t len)
+{
+ IMXI2C *s = (IMXI2C *)i2c;
+ uint8_t data;
+ uint8_t status;
+ uint16_t size = 0;
+
+ if (!len) {
+ return;
+ }
+
+ /* set the bus for write */
+ data = IMX_I2C_I2CR_IEN |
+ IMX_I2C_I2CR_IIEN |
+ IMX_I2C_I2CR_MSTA |
+ IMX_I2C_I2CR_MTX |
+ IMX_I2C_I2CR_TXAK;
+
+ writeb(s->addr + IMX_I2C_I2CR, data);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IBB) != 0);
+
+ /* set the slave address */
+ imx_i2c_set_slave_addr(s, addr, IMX_I2C_WRITE);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) != 0);
+ g_assert((status & IMX_I2C_I2SR_RXAK) == 0);
+
+ /* ack the interrupt */
+ writeb(s->addr + IMX_I2C_I2SR, 0);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) == 0);
+
+ while (size < len) {
+ /* check we are still busy */
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IBB) != 0);
+
+ /* write the data */
+ writeb(s->addr + IMX_I2C_I2DR, buf[size]);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) != 0);
+ g_assert((status & IMX_I2C_I2SR_RXAK) == 0);
+
+ /* ack the interrupt */
+ writeb(s->addr + IMX_I2C_I2SR, 0);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) == 0);
+
+ size++;
+ }
+
+ /* release the bus */
+ data &= ~(IMX_I2C_I2CR_MSTA | IMX_I2C_I2CR_MTX);
+ writeb(s->addr + IMX_I2C_I2CR, data);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IBB) == 0);
+}
+
+static void imx_i2c_recv(I2CAdapter *i2c, uint8_t addr,
+ uint8_t *buf, uint16_t len)
+{
+ IMXI2C *s = (IMXI2C *)i2c;
+ uint8_t data;
+ uint8_t status;
+ uint16_t size = 0;
+
+ if (!len) {
+ return;
+ }
+
+ /* set the bus for write */
+ data = IMX_I2C_I2CR_IEN |
+ IMX_I2C_I2CR_IIEN |
+ IMX_I2C_I2CR_MSTA |
+ IMX_I2C_I2CR_MTX |
+ IMX_I2C_I2CR_TXAK;
+
+ writeb(s->addr + IMX_I2C_I2CR, data);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IBB) != 0);
+
+ /* set the slave address */
+ imx_i2c_set_slave_addr(s, addr, IMX_I2C_READ);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) != 0);
+ g_assert((status & IMX_I2C_I2SR_RXAK) == 0);
+
+ /* ack the interrupt */
+ writeb(s->addr + IMX_I2C_I2SR, 0);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) == 0);
+
+ /* set the bus for read */
+ data &= ~IMX_I2C_I2CR_MTX;
+ /* if only one byte don't ack */
+ if (len != 1) {
+ data &= ~IMX_I2C_I2CR_TXAK;
+ }
+ writeb(s->addr + IMX_I2C_I2CR, data);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IBB) != 0);
+
+ /* dummy read */
+ readb(s->addr + IMX_I2C_I2DR);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) != 0);
+
+ /* ack the interrupt */
+ writeb(s->addr + IMX_I2C_I2SR, 0);
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) == 0);
+
+ while (size < len) {
+ /* check we are still busy */
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IBB) != 0);
+
+ if (size == (len - 1)) {
+ /* stop the read transaction */
+ data &= ~(IMX_I2C_I2CR_MSTA | IMX_I2C_I2CR_MTX);
+ } else {
+ /* ack the data read */
+ data |= IMX_I2C_I2CR_TXAK;
+ }
+ writeb(s->addr + IMX_I2C_I2CR, data);
+
+ /* read the data */
+ buf[size] = readb(s->addr + IMX_I2C_I2DR);
+
+ if (size != (len - 1)) {
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) != 0);
+
+ /* ack the interrupt */
+ writeb(s->addr + IMX_I2C_I2SR, 0);
+ }
+
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IIF) == 0);
+
+ size++;
+ }
+
+ status = readb(s->addr + IMX_I2C_I2SR);
+ g_assert((status & IMX_I2C_I2SR_IBB) == 0);
+}
+
+I2CAdapter *imx_i2c_create(uint64_t addr)
+{
+ IMXI2C *s = g_malloc0(sizeof(*s));
+ I2CAdapter *i2c = (I2CAdapter *)s;
+
+ s->addr = addr;
+
+ i2c->send = imx_i2c_send;
+ i2c->recv = imx_i2c_recv;
+
+ return i2c;
+}
diff --git a/tests/libqos/i2c.h b/tests/libqos/i2c.h
index 1ce9af4..c21f1dc 100644
--- a/tests/libqos/i2c.h
+++ b/tests/libqos/i2c.h
@@ -27,4 +27,7 @@ void i2c_recv(I2CAdapter *i2c, uint8_t addr,
/* libi2c-omap.c */
I2CAdapter *omap_i2c_create(uint64_t addr);
+/* libi2c-imx.c */
+I2CAdapter *imx_i2c_create(uint64_t addr);
+
#endif
--
1.8.1.2
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] Add qtest support for i.MX I2C device emulation.
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 4/4] Add qtest support for i.MX I2C device emulation Jean-Christophe DUBOIS
@ 2013-05-04 16:53 ` Andreas Färber
2013-05-04 19:02 ` Jean-Christophe DUBOIS
0 siblings, 1 reply; 35+ messages in thread
From: Andreas Färber @ 2013-05-04 16:53 UTC (permalink / raw)
To: Jean-Christophe DUBOIS
Cc: peter.maydell, peter.crosthwaite, peter.chubb, qemu-devel
Am 04.05.2013 16:09, schrieb Jean-Christophe DUBOIS:
> This is using a ds1338 RTC chip on the i2c bus. This RTC
> chip is nop present on the real board
>
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
> tests/Makefile | 3 +
> tests/ds1338-test.c | 64 ++++++++++++++
> tests/libqos/i2c-imx.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++++
> tests/libqos/i2c.h | 3 +
> 4 files changed, 294 insertions(+)
> create mode 100644 tests/ds1338-test.c
> create mode 100644 tests/libqos/i2c-imx.c
[...]
The qtest itself looks fine, thanks.
> diff --git a/tests/libqos/i2c-imx.c b/tests/libqos/i2c-imx.c
> new file mode 100644
> index 0000000..da7316f
> --- /dev/null
> +++ b/tests/libqos/i2c-imx.c
> @@ -0,0 +1,224 @@
> +/*
> + * QTest i.MX I2C driver
> + *
> + * Copyright (c) 2013 Jean-Christophe Dubois
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "libqos/i2c.h"
> +
> +#include <glib.h>
> +#include <string.h>
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bswap.h"
Is this one needed?
> +#include "libqtest.h"
> +
> +enum IMXI2CRegisters {
> + IMX_I2C_IADR = 0x00,
> + IMX_I2C_IFDR = 0x04,
> + IMX_I2C_I2CR = 0x08,
> + IMX_I2C_I2SR = 0x0c,
> + IMX_I2C_I2DR = 0x10,
> +};
> +
> +enum IMXI2CCRBits {
> + IMX_I2C_I2CR_IEN = 1 << 7,
> + IMX_I2C_I2CR_IIEN = 1 << 6,
> + IMX_I2C_I2CR_MSTA = 1 << 5,
> + IMX_I2C_I2CR_MTX = 1 << 4,
> + IMX_I2C_I2CR_TXAK = 1 << 3,
> + IMX_I2C_I2CR_RSTA = 1 << 2,
> +};
> +
> +enum IMXI2CSRBits {
> + IMX_I2C_I2SR_ICF = 1 << 7,
> + IMX_I2C_I2SR_IAAF = 1 << 6,
> + IMX_I2C_I2SR_IBB = 1 << 5,
> + IMX_I2C_I2SR_IAL = 1 << 4,
> + IMX_I2C_I2SR_SRW = 1 << 2,
> + IMX_I2C_I2SR_IIF = 1 << 1,
> + IMX_I2C_I2SR_RXAK = 1 << 0,
> +};
> +
> +enum IMXI2CDirection {
> + IMX_I2C_READ,
> + IMX_I2C_WRITE,
> +};
libqos/i2c-omap.c was a driver for an unmaintained legacy device.
i.MX I2C however is being added by you in 2/4, so it would be better to
put these constants in a header in 2/4 for reuse here (i2c/imx_regs.h?).
Otherwise looking fine!
Regards,
Andreas
> +
> +typedef struct IMXI2C {
> + I2CAdapter parent;
> +
> + uint64_t addr;
> +} IMXI2C;
> +
> +
> +static void imx_i2c_set_slave_addr(IMXI2C *s, uint8_t addr,
> + enum IMXI2CDirection direction)
> +{
> + writeb(s->addr + IMX_I2C_I2DR, (addr << 1) |
> + (direction == IMX_I2C_READ ? 1 : 0));
> +}
> +
> +static void imx_i2c_send(I2CAdapter *i2c, uint8_t addr,
> + const uint8_t *buf, uint16_t len)
> +{
> + IMXI2C *s = (IMXI2C *)i2c;
> + uint8_t data;
> + uint8_t status;
> + uint16_t size = 0;
> +
> + if (!len) {
> + return;
> + }
> +
> + /* set the bus for write */
> + data = IMX_I2C_I2CR_IEN |
> + IMX_I2C_I2CR_IIEN |
> + IMX_I2C_I2CR_MSTA |
> + IMX_I2C_I2CR_MTX |
> + IMX_I2C_I2CR_TXAK;
> +
> + writeb(s->addr + IMX_I2C_I2CR, data);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IBB) != 0);
> +
> + /* set the slave address */
> + imx_i2c_set_slave_addr(s, addr, IMX_I2C_WRITE);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) != 0);
> + g_assert((status & IMX_I2C_I2SR_RXAK) == 0);
> +
> + /* ack the interrupt */
> + writeb(s->addr + IMX_I2C_I2SR, 0);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) == 0);
> +
> + while (size < len) {
> + /* check we are still busy */
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IBB) != 0);
> +
> + /* write the data */
> + writeb(s->addr + IMX_I2C_I2DR, buf[size]);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) != 0);
> + g_assert((status & IMX_I2C_I2SR_RXAK) == 0);
> +
> + /* ack the interrupt */
> + writeb(s->addr + IMX_I2C_I2SR, 0);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) == 0);
> +
> + size++;
> + }
> +
> + /* release the bus */
> + data &= ~(IMX_I2C_I2CR_MSTA | IMX_I2C_I2CR_MTX);
> + writeb(s->addr + IMX_I2C_I2CR, data);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IBB) == 0);
> +}
> +
> +static void imx_i2c_recv(I2CAdapter *i2c, uint8_t addr,
> + uint8_t *buf, uint16_t len)
> +{
> + IMXI2C *s = (IMXI2C *)i2c;
> + uint8_t data;
> + uint8_t status;
> + uint16_t size = 0;
> +
> + if (!len) {
> + return;
> + }
> +
> + /* set the bus for write */
> + data = IMX_I2C_I2CR_IEN |
> + IMX_I2C_I2CR_IIEN |
> + IMX_I2C_I2CR_MSTA |
> + IMX_I2C_I2CR_MTX |
> + IMX_I2C_I2CR_TXAK;
> +
> + writeb(s->addr + IMX_I2C_I2CR, data);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IBB) != 0);
> +
> + /* set the slave address */
> + imx_i2c_set_slave_addr(s, addr, IMX_I2C_READ);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) != 0);
> + g_assert((status & IMX_I2C_I2SR_RXAK) == 0);
> +
> + /* ack the interrupt */
> + writeb(s->addr + IMX_I2C_I2SR, 0);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) == 0);
> +
> + /* set the bus for read */
> + data &= ~IMX_I2C_I2CR_MTX;
> + /* if only one byte don't ack */
> + if (len != 1) {
> + data &= ~IMX_I2C_I2CR_TXAK;
> + }
> + writeb(s->addr + IMX_I2C_I2CR, data);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IBB) != 0);
> +
> + /* dummy read */
> + readb(s->addr + IMX_I2C_I2DR);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) != 0);
> +
> + /* ack the interrupt */
> + writeb(s->addr + IMX_I2C_I2SR, 0);
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) == 0);
> +
> + while (size < len) {
> + /* check we are still busy */
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IBB) != 0);
> +
> + if (size == (len - 1)) {
> + /* stop the read transaction */
> + data &= ~(IMX_I2C_I2CR_MSTA | IMX_I2C_I2CR_MTX);
> + } else {
> + /* ack the data read */
> + data |= IMX_I2C_I2CR_TXAK;
> + }
> + writeb(s->addr + IMX_I2C_I2CR, data);
> +
> + /* read the data */
> + buf[size] = readb(s->addr + IMX_I2C_I2DR);
> +
> + if (size != (len - 1)) {
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) != 0);
> +
> + /* ack the interrupt */
> + writeb(s->addr + IMX_I2C_I2SR, 0);
> + }
> +
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IIF) == 0);
> +
> + size++;
> + }
> +
> + status = readb(s->addr + IMX_I2C_I2SR);
> + g_assert((status & IMX_I2C_I2SR_IBB) == 0);
> +}
> +
> +I2CAdapter *imx_i2c_create(uint64_t addr)
> +{
> + IMXI2C *s = g_malloc0(sizeof(*s));
> + I2CAdapter *i2c = (I2CAdapter *)s;
> +
> + s->addr = addr;
> +
> + i2c->send = imx_i2c_send;
> + i2c->recv = imx_i2c_recv;
> +
> + return i2c;
> +}
> diff --git a/tests/libqos/i2c.h b/tests/libqos/i2c.h
> index 1ce9af4..c21f1dc 100644
> --- a/tests/libqos/i2c.h
> +++ b/tests/libqos/i2c.h
> @@ -27,4 +27,7 @@ void i2c_recv(I2CAdapter *i2c, uint8_t addr,
> /* libi2c-omap.c */
> I2CAdapter *omap_i2c_create(uint64_t addr);
>
> +/* libi2c-imx.c */
> +I2CAdapter *imx_i2c_create(uint64_t addr);
> +
> #endif
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] Add qtest support for i.MX I2C device emulation.
2013-05-04 16:53 ` Andreas Färber
@ 2013-05-04 19:02 ` Jean-Christophe DUBOIS
2013-05-04 19:48 ` [Qemu-devel] compile the latest source in mac error Peter Cheung
0 siblings, 1 reply; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-04 19:02 UTC (permalink / raw)
To: Andreas Färber
Cc: peter.maydell, peter.crosthwaite, peter.chubb, qemu-devel
On 05/04/2013 06:53 PM, Andreas Färber wrote:
> Am 04.05.2013 16:09, schrieb Jean-Christophe DUBOIS:
>
>> +#include "qemu/bswap.h"
> Is this one needed?
No, I will remove it.
>> +enum IMXI2CRegisters {
>> + IMX_I2C_IADR = 0x00,
>> + IMX_I2C_IFDR = 0x04,
>> + IMX_I2C_I2CR = 0x08,
>> + IMX_I2C_I2SR = 0x0c,
>> + IMX_I2C_I2DR = 0x10,
>> +};
>> +
>> +enum IMXI2CCRBits {
>> + IMX_I2C_I2CR_IEN = 1 << 7,
>> + IMX_I2C_I2CR_IIEN = 1 << 6,
>> + IMX_I2C_I2CR_MSTA = 1 << 5,
>> + IMX_I2C_I2CR_MTX = 1 << 4,
>> + IMX_I2C_I2CR_TXAK = 1 << 3,
>> + IMX_I2C_I2CR_RSTA = 1 << 2,
>> +};
>> +
>> +enum IMXI2CSRBits {
>> + IMX_I2C_I2SR_ICF = 1 << 7,
>> + IMX_I2C_I2SR_IAAF = 1 << 6,
>> + IMX_I2C_I2SR_IBB = 1 << 5,
>> + IMX_I2C_I2SR_IAL = 1 << 4,
>> + IMX_I2C_I2SR_SRW = 1 << 2,
>> + IMX_I2C_I2SR_IIF = 1 << 1,
>> + IMX_I2C_I2SR_RXAK = 1 << 0,
>> +};
>> +
>> +enum IMXI2CDirection {
>> + IMX_I2C_READ,
>> + IMX_I2C_WRITE,
>> +};
> libqos/i2c-omap.c was a driver for an unmaintained legacy device.
> i.MX I2C however is being added by you in 2/4, so it would be better to
> put these constants in a header in 2/4 for reuse here (i2c/imx_regs.h?).
>
> Otherwise looking fine!
Will do in next version.
Meanwhile, other comments on the series are welcome.
JC
>
> Regards,
> Andreas
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] compile the latest source in mac error
2013-05-04 19:02 ` Jean-Christophe DUBOIS
@ 2013-05-04 19:48 ` Peter Cheung
2013-05-04 20:51 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Peter Cheung @ 2013-05-04 19:48 UTC (permalink / raw)
To: qemu-devel@nongnu.org
[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]
compile the latest source in mac error
/Users/peter/q/qemu>make installinstall -d -m 0755 "/Users/peter/qemu/share/doc/qemu"install -c -m 0644 qemu-doc.html qemu-tech.html "/Users/peter/qemu/share/doc/qemu"install -c -m 0644 QMP/qmp-commands.txt "/Users/peter/qemu/share/doc/qemu"install -d -m 0755 "/Users/peter/qemu/share/man/man1"install -c -m 0644 qemu.1 qemu-img.1 "/Users/peter/qemu/share/man/man1"install -d -m 0755 "/Users/peter/qemu/share/man/man8"install -c -m 0644 qemu-nbd.8 "/Users/peter/qemu/share/man/man8"install -d -m 0755 "/Users/peter/qemu/share/qemu"install -d -m 0755 "/Users/peter/qemu/etc/qemu"install -c -m 0644 /Users/peter/q/qemu/sysconfigs/target/target-x86_64.conf "/Users/peter/qemu/etc/qemu"install -d -m 0755 "/Users/peter/qemu/bin"libtool --quiet --mode=install install -c -m 0755 qemu-ga qemu-nbd qemu-img qemu-io "/Users/peter/qemu/bin"libtool: unknown option character `-' in: --quietUsage: libtool -static [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-sacLT]Usage: libtool -dynamic [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-o output] [-install_name name] [-compatibility_version #] [-current_version #] [-seg1addr 0x#] [-segs_read_only_addr 0x#] [-segs_read_write_addr 0x#] [-seg_addr_table <filename>] [-seg_addr_table_filename <file_system_path>] [-all_load] [-noall_load]make: *** [install] Error 1
[-- Attachment #2: Type: text/html, Size: 1892 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] compile the latest source in mac error
2013-05-04 19:48 ` [Qemu-devel] compile the latest source in mac error Peter Cheung
@ 2013-05-04 20:51 ` Peter Maydell
0 siblings, 0 replies; 35+ messages in thread
From: Peter Maydell @ 2013-05-04 20:51 UTC (permalink / raw)
To: Peter Cheung; +Cc: qemu-devel@nongnu.org
On 4 May 2013 20:48, Peter Cheung <mcheung63@hotmail.com> wrote:
> compile the latest source in mac error
>
> /Users/peter/q/qemu>make install
> install -d -m 0755 "/Users/peter/qemu/share/doc/qemu"
> install -c -m 0644 qemu-doc.html qemu-tech.html
> "/Users/peter/qemu/share/doc/qemu"
> install -c -m 0644 QMP/qmp-commands.txt "/Users/peter/qemu/share/doc/qemu"
> install -d -m 0755 "/Users/peter/qemu/share/man/man1"
> install -c -m 0644 qemu.1 qemu-img.1 "/Users/peter/qemu/share/man/man1"
> install -d -m 0755 "/Users/peter/qemu/share/man/man8"
> install -c -m 0644 qemu-nbd.8 "/Users/peter/qemu/share/man/man8"
> install -d -m 0755 "/Users/peter/qemu/share/qemu"
> install -d -m 0755 "/Users/peter/qemu/etc/qemu"
> install -c -m 0644 /Users/peter/q/qemu/sysconfigs/target/target-x86_64.conf
> "/Users/peter/qemu/etc/qemu"
> install -d -m 0755 "/Users/peter/qemu/bin"
> libtool --quiet --mode=install install -c -m 0755 qemu-ga qemu-nbd qemu-img
> qemu-io "/Users/peter/qemu/bin"
> libtool: unknown option character `-' in: --quiet
> Usage: libtool -static [-] file [...] [-filelist listfile[,dirname]]
> [-arch_only arch] [-sacLT]
> Usage: libtool -dynamic [-] file [...] [-filelist listfile[,dirname]]
> [-arch_only arch] [-o output] [-install_name name] [-compatibility_version
> #] [-current_version #] [-seg1addr 0x#] [-segs_read_only_addr 0x#]
> [-segs_read_write_addr 0x#] [-seg_addr_table <filename>]
> [-seg_addr_table_filename <file_system_path>] [-all_load] [-noall_load]
> make: *** [install] Error 1
Yep, I can reproduce this. I'd never actually tried to do a
"make install" before :-)
I think the simplest way to fix this is just to improve our configure
test so that it does a basic check that 'libtool' is GNU libtool; the
fallback code for "we don't have libtool" will then work fine.
thanks
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver Jean-Christophe DUBOIS
@ 2013-05-05 3:11 ` Peter Crosthwaite
2013-05-05 11:46 ` Andreas Färber
2013-05-05 13:14 ` Jean-Christophe DUBOIS
2013-05-05 17:49 ` Michael S. Tsirkin
1 sibling, 2 replies; 35+ messages in thread
From: Peter Crosthwaite @ 2013-05-05 3:11 UTC (permalink / raw)
To: Jean-Christophe DUBOIS; +Cc: peter.maydell, peter.chubb, qemu-devel, afaerber
Hi JC,
On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> This is based on the mcf_fec.c FEC implementation for ColdFire.
>
> * a generic phy was added (borrowed from lan9118).
> * The buffer management is also modified as buffers are
> slightly different between coldfire and i.MX.
>
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/net/Makefile.objs | 1 +
> hw/net/imx_fec.c | 748 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 750 insertions(+)
> create mode 100644 hw/net/imx_fec.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 27cbe3d..b3a0207 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -28,6 +28,7 @@ CONFIG_SSI_SD=y
> CONFIG_SSI_M25P80=y
> CONFIG_LAN9118=y
> CONFIG_SMC91C111=y
> +CONFIG_IMX_FEC=y
> CONFIG_DS1338=y
> CONFIG_PFLASH_CFI01=y
> CONFIG_PFLASH_CFI02=y
> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
> index 951cca3..5c84727 100644
> --- a/hw/net/Makefile.objs
> +++ b/hw/net/Makefile.objs
> @@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
> common-obj-$(CONFIG_XGMAC) += xgmac.o
> common-obj-$(CONFIG_MIPSNET) += mipsnet.o
> common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
> +common-obj-$(CONFIG_IMX_FEC) += imx_fec.o
>
> common-obj-$(CONFIG_CADENCE) += cadence_gem.o
> common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> new file mode 100644
> index 0000000..e894d50
> --- /dev/null
> +++ b/hw/net/imx_fec.c
> @@ -0,0 +1,748 @@
> +/*
> + * i.MX Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2013 Jean-Christophe Dubois.
> + *
> + * Based on Coldfire Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2007 CodeSourcery.
> + *
> + * This code is licensed under the GPL
> + */
> +#include "hw/sysbus.h"
> +#include "net/net.h"
> +#include "hw/devices.h"
> +
is devices.h needed? Its a collection of helper init fns for misc
devices pre QOM style.
> +/* For crc32 */
> +#include <zlib.h>
> +
> +#include "hw/arm/imx.h"
> +
> +#ifndef IMX_FEC_DEBUG
> +#define IMX_FEC_DEBUG 0
> +#endif
> +
> +#ifndef IMX_PHY_DEBUG
> +#define IMX_PHY_DEBUG 0
> +#endif
> +
> +#if IMX_FEC_DEBUG
> +#define DPRINTF(fmt, ...) \
> +do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#if IMX_PHY_DEBUG
> +#define DPPRINTF(fmt, ...) \
> +do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define FEC_MAX_FRAME_SIZE 2032
> +
> +typedef struct {
> + SysBusDevice busdev;
parent_obj
> + NICState *nic;
> + NICConf conf;
> + qemu_irq irq;
> + MemoryRegion iomem;
> +
> + uint32_t irq_state;
> + uint32_t eir;
> + uint32_t eimr;
> + uint32_t rx_enabled;
> + uint32_t rx_descriptor;
> + uint32_t tx_descriptor;
> + uint32_t ecr;
> + uint32_t mmfr;
> + uint32_t mscr;
> + uint32_t mibc;
> + uint32_t rcr;
> + uint32_t tcr;
> + uint32_t tfwr;
> + uint32_t frsr;
> + uint32_t erdsr;
> + uint32_t etdsr;
> + uint32_t emrbr;
> + uint32_t miigsk_cfgr;
> + uint32_t miigsk_enr;
> +
> + uint32_t phy_status;
> + uint32_t phy_control;
> + uint32_t phy_advertise;
> + uint32_t phy_int;
> + uint32_t phy_int_mask;
> +} imx_fec_state;
types are CamelCase (IMXFECState)
> +
> +static const VMStateDescription vmstate_imx_fec = {
> + .name = "fec",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(irq_state, imx_fec_state),
> + VMSTATE_UINT32(eir, imx_fec_state),
> + VMSTATE_UINT32(eimr, imx_fec_state),
> + VMSTATE_UINT32(rx_enabled, imx_fec_state),
> + VMSTATE_UINT32(rx_descriptor, imx_fec_state),
> + VMSTATE_UINT32(tx_descriptor, imx_fec_state),
> + VMSTATE_UINT32(ecr, imx_fec_state),
> + VMSTATE_UINT32(mmfr, imx_fec_state),
> + VMSTATE_UINT32(mscr, imx_fec_state),
> + VMSTATE_UINT32(mibc, imx_fec_state),
> + VMSTATE_UINT32(rcr, imx_fec_state),
> + VMSTATE_UINT32(tcr, imx_fec_state),
> + VMSTATE_UINT32(tfwr, imx_fec_state),
> + VMSTATE_UINT32(frsr, imx_fec_state),
> + VMSTATE_UINT32(erdsr, imx_fec_state),
> + VMSTATE_UINT32(etdsr, imx_fec_state),
> + VMSTATE_UINT32(emrbr, imx_fec_state),
> + VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
> + VMSTATE_UINT32(miigsk_enr, imx_fec_state),
> +
> + VMSTATE_UINT32(phy_status, imx_fec_state),
> + VMSTATE_UINT32(phy_control, imx_fec_state),
> + VMSTATE_UINT32(phy_advertise, imx_fec_state),
> + VMSTATE_UINT32(phy_int, imx_fec_state),
> + VMSTATE_UINT32(phy_int_mask, imx_fec_state),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +#define PHY_INT_ENERGYON (1 << 7)
> +#define PHY_INT_AUTONEG_COMPLETE (1 << 6)
> +#define PHY_INT_FAULT (1 << 5)
> +#define PHY_INT_DOWN (1 << 4)
> +#define PHY_INT_AUTONEG_LP (1 << 3)
> +#define PHY_INT_PARFAULT (1 << 2)
> +#define PHY_INT_AUTONEG_PAGE (1 << 1)
> +
> +static void imx_fec_update(imx_fec_state *s);
> +
> +/*
> + * The MII phy could raise a GPIO to the processor which in turn
> + * could be handled as an interrpt by the OS.
"interrupt". Also a bit of a nitpick but s/OS/guest. Guest is the
common term for the software running on a QEMU emulation.
> + * For now we don't handle any GPIO/interrupt line, so the OS will
> + * have to poll for the PHY status.
> + */
> +static void phy_update_irq(imx_fec_state *s)
> +{
> + imx_fec_update(s);
> +}
> +
> +static void phy_update_link(imx_fec_state *s)
> +{
> + /* Autonegotiation status mirrors link status. */
> + if (qemu_get_queue(s->nic)->link_down) {
> + DPPRINTF("%s: link is down\n", __func__);
> + s->phy_status &= ~0x0024;
> + s->phy_int |= PHY_INT_DOWN;
> + } else {
> + DPPRINTF("%s: link is up\n", __func__);
> + s->phy_status |= 0x0024;
> + s->phy_int |= PHY_INT_ENERGYON;
> + s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
> + }
> + phy_update_irq(s);
> +}
> +
> +static void imx_fec_set_link(NetClientState *nc)
> +{
> + phy_update_link(qemu_get_nic_opaque(nc));
> +}
> +
> +static void phy_reset(imx_fec_state *s)
> +{
> + DPPRINTF("%s\n", __func__);
> +
> + s->phy_status = 0x7809;
> + s->phy_control = 0x3000;
> + s->phy_advertise = 0x01e1;
> + s->phy_int_mask = 0;
> + s->phy_int = 0;
> + phy_update_link(s);
> +}
> +
> +static uint32_t do_phy_read(imx_fec_state *s, int reg)
> +{
> + uint32_t val;
> +
> + switch (reg) {
> + case 0: /* Basic Control */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);
Can you factor out these DPRINTFs in some way? I think you had
something going for your I2C with the array of register names and a
%s?
> + return s->phy_control;
> + case 1: /* Basic Status */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
> + return s->phy_status;
> + case 2: /* ID1 */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
> + return 0x0007;
> + case 3: /* ID2 */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
> + return 0xc0d1;
> + case 4: /* Auto-neg advertisement */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
> + return s->phy_advertise;
> + case 5: /* Auto-neg Link Partner Ability */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
> + return 0x0f71;
> + case 6: /* Auto-neg Expansion */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
> + return 1;
> + /* TODO 17, 18, 27, 29, 30, 31 */
This un-implemented functionality? If so, create a case (6 way fall
through), and qemu_log_mask(LOG_UNIMP to allow debugging users to know
that QEMU is ignoring something.
> + case 29: /* Interrupt source. */
> + val = s->phy_int;
> + s->phy_int = 0;
> + phy_update_irq(s);
> + return val;
> + case 30: /* Interrupt mask */
> + return s->phy_int_mask;
> + default:
> + DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);
Is this un-defined register space? if so, qemu_log_mask(LOG_GUEST_ERROR
> + return 0;
> + }
> +}
> +
> +static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
> +{
> + switch (reg) {
> + case 0: /* Basic Control */
> + if (val & 0x8000) {
> + phy_reset(s);
> + } else {
> + s->phy_control = val & 0x7980;
> + /* Complete autonegotiation immediately. */
> + if (val & 0x1000) {
> + s->phy_status |= 0x0020;
> + }
> + }
> + break;
> + case 4: /* Auto-neg advertisement */
> + s->phy_advertise = (val & 0x2d7f) | 0x80;
> + break;
> + /* TODO 17, 18, 27, 31 */
> + case 30: /* Interrupt mask */
> + s->phy_int_mask = val & 0xff;
> + phy_update_irq(s);
> + break;
> + default:
> + DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
> + }
> +}
> +
> +#define FEC_INT_HB (1 << 31)
> +#define FEC_INT_BABR (1 << 30)
> +#define FEC_INT_BABT (1 << 29)
> +#define FEC_INT_GRA (1 << 28)
> +#define FEC_INT_TXF (1 << 27)
> +#define FEC_INT_TXB (1 << 26)
> +#define FEC_INT_RXF (1 << 25)
> +#define FEC_INT_RXB (1 << 24)
> +#define FEC_INT_MII (1 << 23)
> +#define FEC_INT_EBERR (1 << 22)
> +#define FEC_INT_LC (1 << 21)
> +#define FEC_INT_RL (1 << 20)
> +#define FEC_INT_UN (1 << 19)
> +
> +#define FEC_EN 2
> +#define FEC_RESET 1
> +
> +/* Buffer Descriptor. */
> +typedef struct {
> + uint16_t length;
> + uint16_t flags;
> + uint32_t data;
> +} imx_fec_bd;
> +
> +#define FEC_BD_R (1 << 15)
> +#define FEC_BD_E (1 << 15)
> +#define FEC_BD_O1 (1 << 14)
> +#define FEC_BD_W (1 << 13)
> +#define FEC_BD_O2 (1 << 12)
> +#define FEC_BD_L (1 << 11)
> +#define FEC_BD_TC (1 << 10)
> +#define FEC_BD_ABC (1 << 9)
> +#define FEC_BD_M (1 << 8)
> +#define FEC_BD_BC (1 << 7)
> +#define FEC_BD_MC (1 << 6)
> +#define FEC_BD_LG (1 << 5)
> +#define FEC_BD_NO (1 << 4)
> +#define FEC_BD_CR (1 << 2)
> +#define FEC_BD_OV (1 << 1)
> +#define FEC_BD_TR (1 << 0)
> +
> +static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> + cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));
cpu_physical_memory_read/write is deprecated for new code (at least
for doing DMA from device land). Use dma_memory_read/write. pl330.c
has some examples and has the DMAContext bolierplate for setting this
up.
> +}
> +
> +static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> + cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
> +}
> +
> +static void imx_fec_update(imx_fec_state *s)
> +{
> + uint32_t active;
> + uint32_t changed;
> +
> + active = s->eir & s->eimr;
> + changed = active ^ s->irq_state;
> + qemu_set_irq(s->irq, changed);
This looks strange (although I haven't read your devices specs so I am
speculating). You set the IRQ pin on an edge of eir & eimr. Should
this be
if (changed) {
qemu_set_irq(s->irq, active)
}
?
> + s->irq_state = active;
> +}
> +
> +static void imx_fec_do_tx(imx_fec_state *s)
> +{
> + uint32_t addr;
> + imx_fec_bd bd;
> + int frame_size;
> + int len;
> + uint8_t frame[FEC_MAX_FRAME_SIZE];
> + uint8_t *ptr;
> +
> + DPRINTF("%s:\n", __func__);
> + ptr = frame;
> + frame_size = 0;
> + addr = s->tx_descriptor;
> + while (1) {
at least len and bd (maybe other local variables) are local to this
while block so could be defined here. Helps to clarify which variables
have cross iteration life, and which ones do not. (not a blocker just
a suggestion)
> + imx_fec_read_bd(&bd, addr);
> + DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
> + __func__, addr, bd.flags, bd.length, bd.data);
> + if ((bd.flags & FEC_BD_R) == 0) {
> + /* Run out of descriptors to transmit. */
> + break;
> + }
> + len = bd.length;
> + if (frame_size + len > FEC_MAX_FRAME_SIZE) {
> + len = FEC_MAX_FRAME_SIZE - frame_size;
> + s->eir |= FEC_INT_BABT;
> + }
> + cpu_physical_memory_read(bd.data, ptr, len);
> + ptr += len;
> + frame_size += len;
> + if (bd.flags & FEC_BD_L) {
> + /* Last buffer in frame. */
> + DPRINTF("Sending packet\n");
> + qemu_send_packet(qemu_get_queue(s->nic), frame, len);
> + ptr = frame;
> + frame_size = 0;
> + s->eir |= FEC_INT_TXF;
> + }
> + s->eir |= FEC_INT_TXB;
> + bd.flags &= ~FEC_BD_R;
> + /* Write back the modified descriptor. */
> + imx_fec_write_bd(&bd, addr);
> + /* Advance to the next descriptor. */
> + if ((bd.flags & FEC_BD_W) != 0) {
> + addr = s->etdsr;
> + } else {
> + addr += 8;
> + }
> + }
> + s->tx_descriptor = addr;
> + imx_fec_update(s);
> +}
> +
> +static void imx_fec_enable_rx(imx_fec_state *s)
> +{
> + imx_fec_bd bd;
> +
> + imx_fec_read_bd(&bd, s->rx_descriptor);
> + s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);
s->rx_enabled is used to return false from imx_fec_can_recieve. This
means you need to check for queued packets here using
qemu_flush_queued_packets, as 0->1 change of rx_enabled may require a
flush. If your device has implemented packet dropping logic, then you
could instead just always return true from can_recieve, although my
experience is a fully implemented can_recieve path is much better
(although slightly less faithful to the real hardware).
> + if (!s->rx_enabled) {
> + DPRINTF("%s: RX buffer full\n", __func__);
> + }
> +}
> +
> +static void imx_fec_reset(DeviceState *d)
> +{
> + imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
> +
QOM cast macro needed.
> + /* Reset the FEC */
> + s->eir = 0;
> + s->eimr = 0;
> + s->rx_enabled = 0;
> + s->ecr = 0;
> + s->mscr = 0;
> + s->mibc = 0xc0000000;
> + s->rcr = 0x05ee0001;
> + s->tcr = 0;
> + s->tfwr = 0;
> + s->frsr = 0x500;
> + s->miigsk_cfgr = 0;
> + s->miigsk_enr = 0x6;
> +
> + /* We also reset the PHY */
> + phy_reset(s);
> +}
> +
> +static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + imx_fec_state *s = (imx_fec_state *) opaque;
> +
> + DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
> +
> + switch (addr & 0x3ff) {
> + case 0x004:
> + return s->eir;
> + case 0x008:
> + return s->eimr;
> + case 0x010:
> + return s->rx_enabled ? (1 << 24) : 0; /* RDAR */
> + case 0x014:
> + return 0; /* TDAR */
> + case 0x024:
> + return s->ecr;
> + case 0x040:
> + return s->mmfr;
> + case 0x044:
> + return s->mscr;
> + case 0x064:
> + return s->mibc; /* MIBC */
> + case 0x084:
> + return s->rcr;
> + case 0x0c4:
> + return s->tcr;
> + case 0x0e4: /* PALR */
> + return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
> + a[1] << 16)
> + | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
> + break;
> + case 0x0e8: /* PAUR */
> + return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
> + a[5] << 16) | 0x8808;
> + case 0x0ec:
> + return 0x10000; /* OPD */
> + case 0x118:
> + return 0;
> + case 0x11c:
> + return 0;
> + case 0x120:
> + return 0;
> + case 0x124:
> + return 0;
> + case 0x144:
> + return s->tfwr;
> + case 0x14c:
> + return 0x600;
> + case 0x150:
> + return s->frsr;
> + case 0x180:
> + return s->erdsr;
> + case 0x184:
> + return s->etdsr;
> + case 0x188:
> + return s->emrbr;
> + case 0x300:
> + return s->miigsk_cfgr;
> + case 0x308:
> + return s->miigsk_enr;
> + default:
> + hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
> + return 0;
> + }
> +}
> +
> +static void imx_fec_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + imx_fec_state *s = (imx_fec_state *) opaque;
> +
> + DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
> +
> + switch (addr & 0x3ff) {
> + case 0x004: /* EIR */
> + s->eir &= ~value;
> + break;
> + case 0x008: /* EIMR */
> + s->eimr = value;
> + break;
> + case 0x010: /* RDAR */
> + if ((s->ecr & FEC_EN) && !s->rx_enabled) {
> + DPRINTF("RX enable\n");
> + imx_fec_enable_rx(s);
> + }
> + break;
> + case 0x014: /* TDAR */
> + if (s->ecr & FEC_EN) {
> + imx_fec_do_tx(s);
> + }
> + break;
> + case 0x024: /* ECR */
> + s->ecr = value;
> + if (value & FEC_RESET) {
> + DPRINTF("Reset\n");
> + imx_fec_reset(&s->busdev.qdev);
> + }
> + if ((s->ecr & FEC_EN) == 0) {
> + s->rx_enabled = 0;
> + }
> + break;
> + case 0x040: /* MMFR */
> + /* store the value */
> + s->mmfr = value;
> + if (value & (1 << 28)) {
> + DPRINTF("PHY write %d = 0x%04x\n",
> + ((int)value >> 18) & 0x1f, (int)value & 0xffff);
> + do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
> + } else {
> + s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
> + DPRINTF("PHY read %d = 0x%04x\n",
> + ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
> + }
> + /* raise the interrupt as the PHY operation is done */
> + s->eir |= FEC_INT_MII;
> + break;
> + case 0x044: /* MSCR */
> + s->mscr = value & 0xfe;
> + break;
> + case 0x064: /* MIBC */
> + /* TODO: Implement MIB. */
> + s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
> + break;
> + case 0x084: /* RCR */
> + s->rcr = value & 0x07ff003f;
> + /* TODO: Implement LOOP mode. */
> + break;
> + case 0x0c4: /* TCR */
> + /* We transmit immediately, so raise GRA immediately. */
> + s->tcr = value;
> + if (value & 1) {
> + s->eir |= FEC_INT_GRA;
> + }
> + break;
> + case 0x0e4: /* PALR */
> + s->conf.macaddr.a[0] = value >> 24;
> + s->conf.macaddr.a[1] = value >> 16;
> + s->conf.macaddr.a[2] = value >> 8;
> + s->conf.macaddr.a[3] = value;
> + break;
> + case 0x0e8: /* PAUR */
> + s->conf.macaddr.a[4] = value >> 24;
> + s->conf.macaddr.a[5] = value >> 16;
> + break;
> + case 0x0ec: /* OPDR */
> + break;
> + case 0x118: /* IAUR */
> + case 0x11c: /* IALR */
> + case 0x120: /* GAUR */
> + case 0x124: /* GALR */
> + /* TODO: implement MAC hash filtering. */
> + break;
> + case 0x144: /* TFWR */
> + s->tfwr = value & 3;
> + break;
> + case 0x14c: /* FRBR */
> + /* FRBR writes ignored. */
> + break;
> + case 0x150: /* FRSR */
> + s->frsr = (value & 0x3fc) | 0x400;
> + break;
> + case 0x180: /* ERDSR */
> + s->erdsr = value & ~3;
> + s->rx_descriptor = s->erdsr;
> + break;
> + case 0x184: /* ETDSR */
> + s->etdsr = value & ~3;
> + s->tx_descriptor = s->etdsr;
> + break;
> + case 0x188: /* EMRBR */
> + s->emrbr = value & 0x7f0;
> + break;
> + case 0x300: /* MIIGSK_CFGR */
> + s->miigsk_cfgr = value & 0x53;
> + break;
> + case 0x308: /* MIIGSK_ENR */
> + s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
> + break;
> + default:
> + hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
> + }
> + imx_fec_update(s);
> +}
> +
> +static int imx_fec_can_receive(NetClientState *nc)
> +{
> + imx_fec_state *s = qemu_get_nic_opaque(nc);
> +
Andreas, do we care about QOM casts coming from random void* opaques?
> + return s->rx_enabled;
> +}
> +
> +static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> + size_t len)
> +{
> + imx_fec_state *s = qemu_get_nic_opaque(nc);
> + imx_fec_bd bd;
> + uint32_t flags = 0;
> + uint32_t addr;
> + uint32_t crc;
> + uint32_t buf_addr;
> + uint8_t *crc_ptr;
> + unsigned int buf_len;
> + size_t size = len;
> +
> + DPRINTF("%s: len %d\n", __func__, (int)size);
> +
> + if (!s->rx_enabled) {
> + DPRINTF("%s: Unexpected packet\n", __func__);
> + return 0;
> + }
> + /* 4 bytes for the CRC. */
> + size += 4;
> + crc = cpu_to_be32(crc32(~0, buf, size));
> + crc_ptr = (uint8_t *) &crc;
> + /* Huge frames are truncted. */
> + if (size > FEC_MAX_FRAME_SIZE) {
> + size = FEC_MAX_FRAME_SIZE;
> + flags |= FEC_BD_TR | FEC_BD_LG;
> + }
> + /* Frames larger than the user limit just set error flags. */
> + if (size > (s->rcr >> 16)) {
> + flags |= FEC_BD_LG;
> + }
> + addr = s->rx_descriptor;
> + while (size > 0) {
> + imx_fec_read_bd(&bd, addr);
> + if ((bd.flags & FEC_BD_E) == 0) {
> + /* No descriptors available. Bail out. */
> + /*
> + * FIXME: This is wrong. We should probably either
> + * save the remainder for when more RX buffers are
> + * available, or flag an error.
> + */
Wouldn't the real hardware just drops the packet? Does the device have
bits and interrupts for signalling this condition? (most NICs do).
> + DPRINTF("%s: Lost end of frame\n", __func__);
> + break;
> + }
> + buf_len = (size <= s->emrbr) ? size : s->emrbr;
> + bd.length = buf_len;
> + size -= buf_len;
> + DPRINTF("rx_bd %x length %d\n", addr, bd.length);
> + /* The last 4 bytes are the CRC. */
> + if (size < 4) {
> + buf_len += size - 4;
> + }
> + buf_addr = bd.data;
> + cpu_physical_memory_write(buf_addr, buf, buf_len);
> + buf += buf_len;
> + if (size < 4) {
> + cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
> + 4 - size);
> + crc_ptr += 4 - size;
> + }
> + bd.flags &= ~FEC_BD_E;
> + if (size == 0) {
> + /* Last buffer in frame. */
> + bd.flags |= flags | FEC_BD_L;
> + DPRINTF("rx frame flags %04x\n", bd.flags);
> + s->eir |= FEC_INT_RXF;
> + } else {
> + s->eir |= FEC_INT_RXB;
> + }
> + imx_fec_write_bd(&bd, addr);
> + /* Advance to the next descriptor. */
> + if ((bd.flags & FEC_BD_W) != 0) {
> + addr = s->erdsr;
> + } else {
> + addr += 8;
> + }
> + }
> + s->rx_descriptor = addr;
> + imx_fec_enable_rx(s);
> + imx_fec_update(s);
> + return len;
> +}
> +
> +static const MemoryRegionOps imx_fec_ops = {
> + .read = imx_fec_read,
> + .write = imx_fec_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void imx_fec_cleanup(NetClientState *nc)
> +{
> + imx_fec_state *s = qemu_get_nic_opaque(nc);
> +
> + s->nic = NULL;
> +}
> +
> +static NetClientInfo net_imx_fec_info = {
> + .type = NET_CLIENT_OPTIONS_KIND_NIC,
> + .size = sizeof(NICState),
> + .can_receive = imx_fec_can_receive,
> + .receive = imx_fec_receive,
> + .cleanup = imx_fec_cleanup,
> + .link_status_changed = imx_fec_set_link,
> +};
> +
> +static int imx_fec_init(SysBusDevice *dev)
> +{
> + imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
> +
> + memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
> + sysbus_init_mmio(dev, &s->iomem);
> + sysbus_init_irq(dev, &s->irq);
> + qemu_macaddr_default_if_unset(&s->conf.macaddr);
> +
> + s->conf.peers.ncs[0] = nd_table[0].netdev;
> +
> + s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
> + object_get_typename(OBJECT(dev)), dev->qdev.id,
> + s);
> + qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> +
> + return 0;
> +}
> +
> +static Property imx_fec_properties[] = {
> + DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void imx_fec_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = imx_fec_init;
> + dc->reset = imx_fec_reset;
> + dc->vmsd = &vmstate_imx_fec;
> + dc->props = imx_fec_properties;
> +}
> +
> +static const TypeInfo imx_fec_info = {
> + .name = "fec",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(imx_fec_state),
> + .class_init = imx_fec_class_init,
> +};
> +
> +static void imx_fec_register_types(void)
> +{
> + type_register_static(&imx_fec_info);
> +}
> +
> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
does this compile with Werror? I thought there was a requirement that
all non-static functions must have a fn prototype defined beforehand
which I cant see in this patch.
Regards,
Peter
> +{
> + NICInfo *nd;
> + DeviceState *dev;
> + SysBusDevice *s;
> +
> + if (nic >= MAX_NICS) {
> + hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
> + nic, MAX_NICS);
> + }
> +
> + nd = &nd_table[nic];
> +
> + qemu_check_nic_model(nd, "fec");
> + dev = qdev_create(NULL, "fec");
> + qdev_set_nic_properties(dev, nd);
> + qdev_init_nofail(dev);
> + s = SYS_BUS_DEVICE(dev);
> + sysbus_mmio_map(s, 0, base);
> + sysbus_connect_irq(s, 0, irq);
> +};
> +
> +type_init(imx_fec_register_types)
> --
> 1.8.1.2
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver Jean-Christophe DUBOIS
@ 2013-05-05 3:14 ` Peter Crosthwaite
2013-05-05 3:58 ` Jean-Christophe DUBOIS
2013-05-05 11:34 ` Andreas Färber
0 siblings, 2 replies; 35+ messages in thread
From: Peter Crosthwaite @ 2013-05-05 3:14 UTC (permalink / raw)
To: Jean-Christophe DUBOIS; +Cc: peter.maydell, peter.chubb, qemu-devel, afaerber
Hi JC,
Thanks for actioning the comments.
On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> The slave mode is not implemented.
>
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
As a general rule you need to indicate changes between the last
version and this version (changlog). My personal preference is to do
it here - below the line on individual patch emails, although others
do their changlogs in cover letters for entire series. Either system
is acceptable.
> default-configs/arm-softmmu.mak | 2 +
> hw/i2c/Makefile.objs | 1 +
> hw/i2c/imx_i2c.c | 383 ++++++++++++++++++++++++++++++++++++++++
> include/hw/arm/imx.h | 1 +
> 4 files changed, 387 insertions(+)
> create mode 100644 hw/i2c/imx_i2c.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index b3a0207..a20f112 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -81,3 +81,5 @@ CONFIG_VERSATILE_PCI=y
> CONFIG_VERSATILE_I2C=y
>
> CONFIG_SDHCI=y
> +
> +CONFIG_IMX_I2C=y
> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> index 648278e..d27bbaa 100644
> --- a/hw/i2c/Makefile.objs
> +++ b/hw/i2c/Makefile.objs
> @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o
> common-obj-$(CONFIG_APM) += pm_smbus.o
> common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
> common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
> +common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
> obj-$(CONFIG_OMAP) += omap_i2c.o
> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
> new file mode 100644
> index 0000000..5b0d046
> --- /dev/null
> +++ b/hw/i2c/imx_i2c.c
> @@ -0,0 +1,383 @@
> +/*
> + * i.MX I2C Bus Serial Interface Emulation
> + *
> + * Copyright (C) 2013 Jean-Christophe Dubois.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/i2c/i2c.h"
> +
> +#ifndef IMX_I2C_DEBUG
> +#define IMX_I2C_DEBUG 0
> +#endif
> +
> +#define TYPE_IMX_I2C "imx.i2c"
> +#define IMX_I2C(obj) \
> + OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C)
> +
> +/* i.MX I2C memory map */
> +#define IMX_I2C_MEM_SIZE 0x14
> +#define IADR_ADDR 0x00 /* address register */
> +#define IFDR_ADDR 0x04 /* frequency divider register */
> +#define I2CR_ADDR 0x08 /* control register */
> +#define I2SR_ADDR 0x0c /* status register */
> +#define I2DR_ADDR 0x10 /* data register */
> +
> +#define IADR_MASK 0xFE
> +#define IADR_RESET 0
> +
> +#define IFDR_MASK 0x3F
> +#define IFDR_RESET 0
> +
> +#define I2CR_IEN (1 << 7)
> +#define I2CR_IIEN (1 << 6)
> +#define I2CR_MSTA (1 << 5)
> +#define I2CR_MTX (1 << 4)
> +#define I2CR_TXAK (1 << 3)
> +#define I2CR_RSTA (1 << 2)
> +#define I2CR_MASK 0xFC
> +#define I2CR_RESET 0
> +
> +#define I2SR_ICF (1 << 7)
> +#define I2SR_IAAF (1 << 6)
> +#define I2SR_IBB (1 << 5)
> +#define I2SR_IAL (1 << 4)
> +#define I2SR_SRW (1 << 2)
> +#define I2SR_IIF (1 << 1)
> +#define I2SR_RXAK (1 << 0)
> +#define I2SR_MASK 0xE9
> +#define I2SR_RESET 0x81
> +
> +#define I2DR_MASK 0xFF
> +#define I2DR_RESET 0
> +
> +#define ADDR_RESET 0xFF00
> +
> +#if IMX_I2C_DEBUG
> +#define DPRINT(fmt, args...) \
> + do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0)
> +
> +static const char *imx_i2c_get_regname(unsigned offset)
> +{
> + switch (offset) {
> + case IADR_ADDR:
> + return "IADR";
> + case IFDR_ADDR:
> + return "IFDR";
> + case I2CR_ADDR:
> + return "I2CR";
> + case I2SR_ADDR:
> + return "I2SR";
> + case I2DR_ADDR:
> + return "I2DR";
> + default:
> + return "[?]";
> + }
> +}
> +#else
> +#define DPRINT(fmt, args...) do { } while (0)
> +#endif
> +
> +typedef struct imx_i2c_state {
types should be in CamelCase IMXI2CState
> + SysBusDevice parent_obj;
> + MemoryRegion iomem;
> + i2c_bus *bus;
> + qemu_irq irq;
> +
> + uint16_t address;
> +
> + uint16_t iadr;
> + uint16_t ifdr;
> + uint16_t i2cr;
> + uint16_t i2sr;
> + uint16_t i2dr_read;
> + uint16_t i2dr_write;
> +} imx_i2c_state;
> +
> +static inline bool imx_i2c_is_enabled(imx_i2c_state *s)
> +{
> + return s->i2cr & I2CR_IEN;
> +}
> +
> +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
> +{
> + return s->i2cr & I2CR_IIEN;
> +}
> +
> +static inline bool imx_i2c_is_master(imx_i2c_state *s)
> +{
> + return s->i2cr & I2CR_MSTA;
> +}
> +
> +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
> +{
> + return s->i2cr & I2CR_MTX;
> +}
> +
> +static void imx_i2c_reset(DeviceState *dev)
> +{
> + imx_i2c_state *s = IMX_I2C(dev);
> +
> + if (s->address != ADDR_RESET) {
> + i2c_end_transfer(s->bus);
> + }
> +
> + s->address = ADDR_RESET;
> + s->iadr = IADR_RESET;
> + s->ifdr = IFDR_RESET;
> + s->i2cr = I2CR_RESET;
> + s->i2sr = I2SR_RESET;
> + s->i2dr_read = I2DR_RESET;
> + s->i2dr_write = I2DR_RESET;
> +}
> +
> +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
> +{
> + /*
> + * raise an interrupt if the device is enabled and it is configured
> + * to generate some interrupts.
> + */
> + if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
> + s->i2sr |= I2SR_IIF;
> + qemu_irq_raise(s->irq);
> + }
> +}
> +
> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> + uint16_t value;
> + imx_i2c_state *s = IMX_I2C(opaque);
> +
> + switch (offset) {
> + case IADR_ADDR:
> + value = s->iadr;
> + break;
> + case IFDR_ADDR:
> + value = s->ifdr;
> + break;
> + case I2CR_ADDR:
> + value = s->i2cr;
> + break;
> + case I2SR_ADDR:
> + value = s->i2sr;
> + break;
> + case I2DR_ADDR:
> + value = s->i2dr_read;
> +
> + if (imx_i2c_is_master(s)) { /* master mode */
> + int ret = 0xff;
> +
> + if (s->address == ADDR_RESET) {
> + /* something is wrong as the address is not set */
> + DPRINT("Trying to read without specifying the slave address\n");
> + } else if (s->i2cr & I2CR_MTX) {
> + DPRINT("Trying to read but MTX is set\n");
> + } else {
> + /* get the next byte */
> + ret = i2c_recv(s->bus);
> +
> + if (ret >= 0) {
> + imx_i2c_raise_interrupt(s);
> + } else {
> + DPRINT("read failed for device 0x%02x\n" s->address);
> + ret = 0xff;
> + }
> + }
> +
> + s->i2dr_read = ret;
> + }
> + break;
> + default:
> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
> + break;
> + }
> +
> + DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset)
General question for the list, are we encouraging the use or PRIx in
new code to remove the needs for casts from uint_XXt to unsigned in
printfery?
,
> + (unsigned int)offset, value);
> +
> + return (uint64_t)value;
> +}
> +
> +static void imx_i2c_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + imx_i2c_state *s = IMX_I2C(opaque);
> +
> + DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset),
> + (unsigned int)offset, (int)value);
> +
> + value &= 0xff;
> +
> + switch (offset) {
> + case IADR_ADDR:
> + s->iadr = value & IADR_MASK;
> + /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */
> + break;
> + case IFDR_ADDR:
> + s->ifdr = value & IFDR_MASK;
> + break;
> + case I2CR_ADDR:
> + if (imx_i2c_is_enabled(s) && ((value & I2CR_IEN) == 0)) {
> + /* This is a soft reset. IADR is preserved during soft resets */
> + uint16_t iadr = s->iadr;
> + imx_i2c_reset(&s->parent_obj.qdev);
> + s->iadr = iadr;
> + } else { /* normal write */
> + s->i2cr = value & I2CR_MASK;
> +
> + if (imx_i2c_is_master(s)) { /* master mode */
> + /* set the bus to busy */
> + s->i2sr |= I2SR_IBB;
> + } else { /* slave mode */
> + /* bus is not busy anymore */
> + s->i2sr &= ~I2SR_IBB;
> +
> + /*
> + *if we unset the master mode then it ends the ongoing
> + * transfer if any
> + */
> + if (s->address != ADDR_RESET) {
> + i2c_end_transfer(s->bus);
> + s->address = ADDR_RESET;
> + }
> + }
> +
> + if (s->i2cr & I2CR_RSTA) { /* Restart */
> + /* if this is a restart then it ends the ongoing transfer */
> + if (s->address != ADDR_RESET) {
> + i2c_end_transfer(s->bus);
> + s->address = ADDR_RESET;
> + s->i2cr &= ~I2CR_RSTA;
> + }
> + }
> + }
> + break;
> + case I2SR_ADDR:
> + /*
> + * if the user writes 0 to IIF then lower the interrupt and
> + * reset the bit
> + */
> + if ((s->i2sr & I2SR_IIF) && !(value & I2SR_IIF)) {
> + s->i2sr &= ~I2SR_IIF;
> + qemu_irq_lower(s->irq);
> + }
> +
> + /*
> + * if the user writes 0 to IAL, reset the bit
> + */
> + if ((s->i2sr & I2SR_IAL) && !(value & I2SR_IAL)) {
> + s->i2sr &= ~I2SR_IAL;
> + }
> +
> + break;
> + case I2DR_ADDR:
> + /* if the device is not enabled, nothing to do */
> + if (!imx_i2c_is_enabled(s)) {
> + break;
> + }
> +
> + s->i2dr_write = value & I2DR_MASK;
> +
> + if (imx_i2c_is_master(s)) { /* master mode */
> + /* If this is the first write cycle then it is the slave addr */
> + if (s->address == ADDR_RESET) {
> + if (i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1,
You could use an extract macro. extract32 is generally considered
better than shift and & logicvfor multi-bit fields. Ill have to look
up if extract16 exists (im away from tree for this review)
> + (int)s->i2dr_write & 0x01)) {
> + /* if non zero is returned, the adress is not valid */
> + s->i2sr |= I2SR_RXAK;
> + } else {
> + s->address = s->i2dr_write;
> + s->i2sr &= ~I2SR_RXAK;
> + imx_i2c_raise_interrupt(s);
> + }
> + } else { /* This is a normal data write */
> + if (i2c_send(s->bus, s->i2dr_write)) {
> + /* if the target return non zero then end the transfer */
> + s->i2sr |= I2SR_RXAK;
> + s->address = ADDR_RESET;
> + i2c_end_transfer(s->bus);
> + } else {
> + s->i2sr &= ~I2SR_RXAK;
> + imx_i2c_raise_interrupt(s);
> + }
> + }
> + }
> + break;
> + default:
> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps imx_i2c_ops = {
> + .read = imx_i2c_read,
> + .write = imx_i2c_write,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 2,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const VMStateDescription imx_i2c_vmstate = {
> + .name = TYPE_IMX_I2C,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT16(address, imx_i2c_state),
> + VMSTATE_UINT16(iadr, imx_i2c_state),
> + VMSTATE_UINT16(ifdr, imx_i2c_state),
> + VMSTATE_UINT16(i2cr, imx_i2c_state),
> + VMSTATE_UINT16(i2sr, imx_i2c_state),
> + VMSTATE_UINT16(i2dr_read, imx_i2c_state),
> + VMSTATE_UINT16(i2dr_write, imx_i2c_state),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void imx_i2c_realize(DeviceState *dev, Error **errp)
> +{
> + imx_i2c_state *s = IMX_I2C(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
to avoid the repeated casts below (and dereferencing of inline macros
although that has another solution i've mentioned below). This rule is
a little hot off the press and Andreas only just updated the
QOMConventions page this week to clarify this better.
> +
> + memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C,
> + IMX_I2C_MEM_SIZE);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> + s->bus = i2c_init_bus(&SYS_BUS_DEVICE(dev)->qdev, "i2c");
The &foo->qdev is the old style dereferencing downcast which we are
trying to eliminate. Instead this line should actually just be:
s->bus = i2c_init_bus(DEVICE(dev), "i2c");
Consider the ->qdev (or parent_obj) field poisoned globally for all QOM objects.
Regards,
Peter
> +}
> +
> +static void imx_i2c_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->vmsd = &imx_i2c_vmstate;
> + dc->reset = imx_i2c_reset;
> + dc->realize = imx_i2c_realize;
> +}
> +
> +static const TypeInfo imx_i2c_type_info = {
> + .name = TYPE_IMX_I2C,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(imx_i2c_state),
> + .class_init = imx_i2c_class_init,
> +};
> +
> +static void imx_i2c_register_types(void)
> +{
> + type_register_static(&imx_i2c_type_info);
> +}
> +
> +type_init(imx_i2c_register_types)
> diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h
> index ea9e093..a875171 100644
> --- a/include/hw/arm/imx.h
> +++ b/include/hw/arm/imx.h
> @@ -30,5 +30,6 @@ void imx_timerg_create(const hwaddr addr,
> qemu_irq irq,
> DeviceState *ccm);
>
> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq);
>
> #endif /* IMX_H */
> --
> 1.8.1.2
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 3:14 ` Peter Crosthwaite
@ 2013-05-05 3:58 ` Jean-Christophe DUBOIS
2013-05-05 10:47 ` Peter Maydell
2013-05-05 11:34 ` Andreas Färber
1 sibling, 1 reply; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-05 3:58 UTC (permalink / raw)
To: Peter Crosthwaite; +Cc: peter.maydell, peter.chubb, qemu-devel, afaerber
On 05/05/2013 05:14 AM, Peter Crosthwaite wrote:
> Hi JC,
>
> Thanks for actioning the comments.
>
> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
>> The slave mode is not implemented.
>>
>> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
>> ---
> As a general rule you need to indicate changes between the last
> version and this version (changlog). My personal preference is to do
> it here - below the line on individual patch emails, although others
> do their changlogs in cover letters for entire series. Either system
> is acceptable.
I'll do
>
>> default-configs/arm-softmmu.mak | 2 +
>> hw/i2c/Makefile.objs | 1 +
>> hw/i2c/imx_i2c.c | 383 ++++++++++++++++++++++++++++++++++++++++
>> include/hw/arm/imx.h | 1 +
>> 4 files changed, 387 insertions(+)
>> create mode 100644 hw/i2c/imx_i2c.c
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index b3a0207..a20f112 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -81,3 +81,5 @@ CONFIG_VERSATILE_PCI=y
>> CONFIG_VERSATILE_I2C=y
>>
>> CONFIG_SDHCI=y
>> +
>> +CONFIG_IMX_I2C=y
>> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
>> index 648278e..d27bbaa 100644
>> --- a/hw/i2c/Makefile.objs
>> +++ b/hw/i2c/Makefile.objs
>> @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o
>> common-obj-$(CONFIG_APM) += pm_smbus.o
>> common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
>> common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
>> +common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
>> obj-$(CONFIG_OMAP) += omap_i2c.o
>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
>> new file mode 100644
>> index 0000000..5b0d046
>> --- /dev/null
>> +++ b/hw/i2c/imx_i2c.c
>> @@ -0,0 +1,383 @@
>> +/*
>> + * i.MX I2C Bus Serial Interface Emulation
>> + *
>> + * Copyright (C) 2013 Jean-Christophe Dubois.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>> + * for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#include "hw/sysbus.h"
>> +#include "hw/i2c/i2c.h"
>> +
>> +#ifndef IMX_I2C_DEBUG
>> +#define IMX_I2C_DEBUG 0
>> +#endif
>> +
>> +#define TYPE_IMX_I2C "imx.i2c"
>> +#define IMX_I2C(obj) \
>> + OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C)
>> +
>> +/* i.MX I2C memory map */
>> +#define IMX_I2C_MEM_SIZE 0x14
>> +#define IADR_ADDR 0x00 /* address register */
>> +#define IFDR_ADDR 0x04 /* frequency divider register */
>> +#define I2CR_ADDR 0x08 /* control register */
>> +#define I2SR_ADDR 0x0c /* status register */
>> +#define I2DR_ADDR 0x10 /* data register */
>> +
>> +#define IADR_MASK 0xFE
>> +#define IADR_RESET 0
>> +
>> +#define IFDR_MASK 0x3F
>> +#define IFDR_RESET 0
>> +
>> +#define I2CR_IEN (1 << 7)
>> +#define I2CR_IIEN (1 << 6)
>> +#define I2CR_MSTA (1 << 5)
>> +#define I2CR_MTX (1 << 4)
>> +#define I2CR_TXAK (1 << 3)
>> +#define I2CR_RSTA (1 << 2)
>> +#define I2CR_MASK 0xFC
>> +#define I2CR_RESET 0
>> +
>> +#define I2SR_ICF (1 << 7)
>> +#define I2SR_IAAF (1 << 6)
>> +#define I2SR_IBB (1 << 5)
>> +#define I2SR_IAL (1 << 4)
>> +#define I2SR_SRW (1 << 2)
>> +#define I2SR_IIF (1 << 1)
>> +#define I2SR_RXAK (1 << 0)
>> +#define I2SR_MASK 0xE9
>> +#define I2SR_RESET 0x81
>> +
>> +#define I2DR_MASK 0xFF
>> +#define I2DR_RESET 0
>> +
>> +#define ADDR_RESET 0xFF00
>> +
>> +#if IMX_I2C_DEBUG
>> +#define DPRINT(fmt, args...) \
>> + do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0)
>> +
>> +static const char *imx_i2c_get_regname(unsigned offset)
>> +{
>> + switch (offset) {
>> + case IADR_ADDR:
>> + return "IADR";
>> + case IFDR_ADDR:
>> + return "IFDR";
>> + case I2CR_ADDR:
>> + return "I2CR";
>> + case I2SR_ADDR:
>> + return "I2SR";
>> + case I2DR_ADDR:
>> + return "I2DR";
>> + default:
>> + return "[?]";
>> + }
>> +}
>> +#else
>> +#define DPRINT(fmt, args...) do { } while (0)
>> +#endif
>> +
>> +typedef struct imx_i2c_state {
> types should be in CamelCase IMXI2CState
OK
>
>> + SysBusDevice parent_obj;
>> + MemoryRegion iomem;
>> + i2c_bus *bus;
>> + qemu_irq irq;
>> +
>> + uint16_t address;
>> +
>> + uint16_t iadr;
>> + uint16_t ifdr;
>> + uint16_t i2cr;
>> + uint16_t i2sr;
>> + uint16_t i2dr_read;
>> + uint16_t i2dr_write;
>> +} imx_i2c_state;
>> +
>> +static inline bool imx_i2c_is_enabled(imx_i2c_state *s)
>> +{
>> + return s->i2cr & I2CR_IEN;
>> +}
>> +
>> +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
>> +{
>> + return s->i2cr & I2CR_IIEN;
>> +}
>> +
>> +static inline bool imx_i2c_is_master(imx_i2c_state *s)
>> +{
>> + return s->i2cr & I2CR_MSTA;
>> +}
>> +
>> +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
>> +{
>> + return s->i2cr & I2CR_MTX;
>> +}
>> +
>> +static void imx_i2c_reset(DeviceState *dev)
>> +{
>> + imx_i2c_state *s = IMX_I2C(dev);
>> +
>> + if (s->address != ADDR_RESET) {
>> + i2c_end_transfer(s->bus);
>> + }
>> +
>> + s->address = ADDR_RESET;
>> + s->iadr = IADR_RESET;
>> + s->ifdr = IFDR_RESET;
>> + s->i2cr = I2CR_RESET;
>> + s->i2sr = I2SR_RESET;
>> + s->i2dr_read = I2DR_RESET;
>> + s->i2dr_write = I2DR_RESET;
>> +}
>> +
>> +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
>> +{
>> + /*
>> + * raise an interrupt if the device is enabled and it is configured
>> + * to generate some interrupts.
>> + */
>> + if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
>> + s->i2sr |= I2SR_IIF;
>> + qemu_irq_raise(s->irq);
>> + }
>> +}
>> +
>> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
>> + unsigned size)
>> +{
>> + uint16_t value;
>> + imx_i2c_state *s = IMX_I2C(opaque);
>> +
>> + switch (offset) {
>> + case IADR_ADDR:
>> + value = s->iadr;
>> + break;
>> + case IFDR_ADDR:
>> + value = s->ifdr;
>> + break;
>> + case I2CR_ADDR:
>> + value = s->i2cr;
>> + break;
>> + case I2SR_ADDR:
>> + value = s->i2sr;
>> + break;
>> + case I2DR_ADDR:
>> + value = s->i2dr_read;
>> +
>> + if (imx_i2c_is_master(s)) { /* master mode */
>> + int ret = 0xff;
>> +
>> + if (s->address == ADDR_RESET) {
>> + /* something is wrong as the address is not set */
>> + DPRINT("Trying to read without specifying the slave address\n");
>> + } else if (s->i2cr & I2CR_MTX) {
>> + DPRINT("Trying to read but MTX is set\n");
>> + } else {
>> + /* get the next byte */
>> + ret = i2c_recv(s->bus);
>> +
>> + if (ret >= 0) {
>> + imx_i2c_raise_interrupt(s);
>> + } else {
>> + DPRINT("read failed for device 0x%02x\n" s->address);
>> + ret = 0xff;
>> + }
>> + }
>> +
>> + s->i2dr_read = ret;
>> + }
>> + break;
>> + default:
>> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
>> + break;
>> + }
>> +
>> + DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset)
> General question for the list, are we encouraging the use or PRIx in
> new code to remove the needs for casts from uint_XXt to unsigned in
> printfery?
> ,
>> + (unsigned int)offset, value);
>> +
>> + return (uint64_t)value;
>> +}
>> +
>> +static void imx_i2c_write(void *opaque, hwaddr offset,
>> + uint64_t value, unsigned size)
>> +{
>> + imx_i2c_state *s = IMX_I2C(opaque);
>> +
>> + DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset),
>> + (unsigned int)offset, (int)value);
>> +
>> + value &= 0xff;
>> +
>> + switch (offset) {
>> + case IADR_ADDR:
>> + s->iadr = value & IADR_MASK;
>> + /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */
>> + break;
>> + case IFDR_ADDR:
>> + s->ifdr = value & IFDR_MASK;
>> + break;
>> + case I2CR_ADDR:
>> + if (imx_i2c_is_enabled(s) && ((value & I2CR_IEN) == 0)) {
>> + /* This is a soft reset. IADR is preserved during soft resets */
>> + uint16_t iadr = s->iadr;
>> + imx_i2c_reset(&s->parent_obj.qdev);
>> + s->iadr = iadr;
>> + } else { /* normal write */
>> + s->i2cr = value & I2CR_MASK;
>> +
>> + if (imx_i2c_is_master(s)) { /* master mode */
>> + /* set the bus to busy */
>> + s->i2sr |= I2SR_IBB;
>> + } else { /* slave mode */
>> + /* bus is not busy anymore */
>> + s->i2sr &= ~I2SR_IBB;
>> +
>> + /*
>> + *if we unset the master mode then it ends the ongoing
>> + * transfer if any
>> + */
>> + if (s->address != ADDR_RESET) {
>> + i2c_end_transfer(s->bus);
>> + s->address = ADDR_RESET;
>> + }
>> + }
>> +
>> + if (s->i2cr & I2CR_RSTA) { /* Restart */
>> + /* if this is a restart then it ends the ongoing transfer */
>> + if (s->address != ADDR_RESET) {
>> + i2c_end_transfer(s->bus);
>> + s->address = ADDR_RESET;
>> + s->i2cr &= ~I2CR_RSTA;
>> + }
>> + }
>> + }
>> + break;
>> + case I2SR_ADDR:
>> + /*
>> + * if the user writes 0 to IIF then lower the interrupt and
>> + * reset the bit
>> + */
>> + if ((s->i2sr & I2SR_IIF) && !(value & I2SR_IIF)) {
>> + s->i2sr &= ~I2SR_IIF;
>> + qemu_irq_lower(s->irq);
>> + }
>> +
>> + /*
>> + * if the user writes 0 to IAL, reset the bit
>> + */
>> + if ((s->i2sr & I2SR_IAL) && !(value & I2SR_IAL)) {
>> + s->i2sr &= ~I2SR_IAL;
>> + }
>> +
>> + break;
>> + case I2DR_ADDR:
>> + /* if the device is not enabled, nothing to do */
>> + if (!imx_i2c_is_enabled(s)) {
>> + break;
>> + }
>> +
>> + s->i2dr_write = value & I2DR_MASK;
>> +
>> + if (imx_i2c_is_master(s)) { /* master mode */
>> + /* If this is the first write cycle then it is the slave addr */
>> + if (s->address == ADDR_RESET) {
>> + if (i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1,
> You could use an extract macro. extract32 is generally considered
> better than shift and & logicvfor multi-bit fields. Ill have to look
> up if extract16 exists (im away from tree for this review)
no extract16() macro spotted.
Should one be added?
>
>> + (int)s->i2dr_write & 0x01)) {
>> + /* if non zero is returned, the adress is not valid */
>> + s->i2sr |= I2SR_RXAK;
>> + } else {
>> + s->address = s->i2dr_write;
>> + s->i2sr &= ~I2SR_RXAK;
>> + imx_i2c_raise_interrupt(s);
>> + }
>> + } else { /* This is a normal data write */
>> + if (i2c_send(s->bus, s->i2dr_write)) {
>> + /* if the target return non zero then end the transfer */
>> + s->i2sr |= I2SR_RXAK;
>> + s->address = ADDR_RESET;
>> + i2c_end_transfer(s->bus);
>> + } else {
>> + s->i2sr &= ~I2SR_RXAK;
>> + imx_i2c_raise_interrupt(s);
>> + }
>> + }
>> + }
>> + break;
>> + default:
>> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
>> + break;
>> + }
>> +}
>> +
>> +static const MemoryRegionOps imx_i2c_ops = {
>> + .read = imx_i2c_read,
>> + .write = imx_i2c_write,
>> + .valid.min_access_size = 1,
>> + .valid.max_access_size = 2,
>> + .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static const VMStateDescription imx_i2c_vmstate = {
>> + .name = TYPE_IMX_I2C,
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT16(address, imx_i2c_state),
>> + VMSTATE_UINT16(iadr, imx_i2c_state),
>> + VMSTATE_UINT16(ifdr, imx_i2c_state),
>> + VMSTATE_UINT16(i2cr, imx_i2c_state),
>> + VMSTATE_UINT16(i2sr, imx_i2c_state),
>> + VMSTATE_UINT16(i2dr_read, imx_i2c_state),
>> + VMSTATE_UINT16(i2dr_write, imx_i2c_state),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static void imx_i2c_realize(DeviceState *dev, Error **errp)
>> +{
>> + imx_i2c_state *s = IMX_I2C(dev);
> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>
> to avoid the repeated casts below (and dereferencing of inline macros
> although that has another solution i've mentioned below). This rule is
> a little hot off the press and Andreas only just updated the
> QOMConventions page this week to clarify this better.
OK
>
>> +
>> + memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C,
>> + IMX_I2C_MEM_SIZE);
>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>> + s->bus = i2c_init_bus(&SYS_BUS_DEVICE(dev)->qdev, "i2c");
> The &foo->qdev is the old style dereferencing downcast which we are
> trying to eliminate. Instead this line should actually just be:
>
> s->bus = i2c_init_bus(DEVICE(dev), "i2c");
>
> Consider the ->qdev (or parent_obj) field poisoned globally for all QOM objects.
OK
>
> Regards,
> Peter
>
>> +}
>> +
>> +static void imx_i2c_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->vmsd = &imx_i2c_vmstate;
>> + dc->reset = imx_i2c_reset;
>> + dc->realize = imx_i2c_realize;
>> +}
>> +
>> +static const TypeInfo imx_i2c_type_info = {
>> + .name = TYPE_IMX_I2C,
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(imx_i2c_state),
>> + .class_init = imx_i2c_class_init,
>> +};
>> +
>> +static void imx_i2c_register_types(void)
>> +{
>> + type_register_static(&imx_i2c_type_info);
>> +}
>> +
>> +type_init(imx_i2c_register_types)
>> diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h
>> index ea9e093..a875171 100644
>> --- a/include/hw/arm/imx.h
>> +++ b/include/hw/arm/imx.h
>> @@ -30,5 +30,6 @@ void imx_timerg_create(const hwaddr addr,
>> qemu_irq irq,
>> DeviceState *ccm);
>>
>> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq);
>>
>> #endif /* IMX_H */
>> --
>> 1.8.1.2
>>
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 3:58 ` Jean-Christophe DUBOIS
@ 2013-05-05 10:47 ` Peter Maydell
2013-05-05 10:53 ` Peter Crosthwaite
0 siblings, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2013-05-05 10:47 UTC (permalink / raw)
To: Jean-Christophe DUBOIS
Cc: Peter Crosthwaite, peter.chubb, qemu-devel, afaerber
On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> On 05/05/2013 05:14 AM, Peter Crosthwaite wrote:
>> You could use an extract macro. extract32 is generally considered
>> better than shift and & logicvfor multi-bit fields. Ill have to look
>> up if extract16 exists (im away from tree for this review)
>
>
> no extract16() macro spotted.
> Should one be added?
There's no need for one -- just use extract32. The only
reason for having a separate extract64 is to avoid doing
64 bit arithmetic when we don't have to, I think.
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 10:47 ` Peter Maydell
@ 2013-05-05 10:53 ` Peter Crosthwaite
2013-05-05 11:41 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Peter Crosthwaite @ 2013-05-05 10:53 UTC (permalink / raw)
To: Peter Maydell; +Cc: peter.chubb, qemu-devel, afaerber, Jean-Christophe DUBOIS
Hi Peter,
On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>> On 05/05/2013 05:14 AM, Peter Crosthwaite wrote:
>>> You could use an extract macro. extract32 is generally considered
>>> better than shift and & logicvfor multi-bit fields. Ill have to look
>>> up if extract16 exists (im away from tree for this review)
>>
>>
>> no extract16() macro spotted.
>> Should one be added?
>
> There's no need for one -- just use extract32. The only
> reason for having a separate extract64 is to avoid doing
> 64 bit arithmetic when we don't have to, I think.
>
perhaps a quick:
#define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c))
would keep the 16bit device code explicit and clean? Save on
dummy casts in certain situations as well (but not this one).
Regards,
Peter
> -- PMM
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 3:14 ` Peter Crosthwaite
2013-05-05 3:58 ` Jean-Christophe DUBOIS
@ 2013-05-05 11:34 ` Andreas Färber
2013-05-05 12:34 ` Jean-Christophe DUBOIS
1 sibling, 1 reply; 35+ messages in thread
From: Andreas Färber @ 2013-05-05 11:34 UTC (permalink / raw)
To: Peter Crosthwaite, Jean-Christophe DUBOIS
Cc: peter.maydell, peter.chubb, qemu-devel
Am 05.05.2013 05:14, schrieb Peter Crosthwaite:
> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
>> new file mode 100644
>> index 0000000..5b0d046
>> --- /dev/null
>> +++ b/hw/i2c/imx_i2c.c
[...]
>> +typedef struct imx_i2c_state {
>
> types should be in CamelCase IMXI2CState
>
>> + SysBusDevice parent_obj;
While at it, please add a white line here. Background is that this
parent field will pretty likely go away once we switch to a better
object-orientation framework - if it were in a header we would annotate
it as private and thus hidden from documentation.
>> + MemoryRegion iomem;
>> + i2c_bus *bus;
Please rather use i2c_bus bus; and qbus_create_inline() in instance_init.
>> + qemu_irq irq;
>> +
>> + uint16_t address;
>> +
>> + uint16_t iadr;
>> + uint16_t ifdr;
>> + uint16_t i2cr;
>> + uint16_t i2sr;
>> + uint16_t i2dr_read;
>> + uint16_t i2dr_write;
>> +} imx_i2c_state;
[...]
>> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
>> + unsigned size)
>> +{
>> + uint16_t value;
>> + imx_i2c_state *s = IMX_I2C(opaque);
>> +
>> + switch (offset) {
>> + case IADR_ADDR:
>> + value = s->iadr;
>> + break;
>> + case IFDR_ADDR:
>> + value = s->ifdr;
>> + break;
>> + case I2CR_ADDR:
>> + value = s->i2cr;
>> + break;
>> + case I2SR_ADDR:
>> + value = s->i2sr;
>> + break;
>> + case I2DR_ADDR:
>> + value = s->i2dr_read;
>> +
>> + if (imx_i2c_is_master(s)) { /* master mode */
>> + int ret = 0xff;
>> +
>> + if (s->address == ADDR_RESET) {
>> + /* something is wrong as the address is not set */
>> + DPRINT("Trying to read without specifying the slave address\n");
>> + } else if (s->i2cr & I2CR_MTX) {
>> + DPRINT("Trying to read but MTX is set\n");
>> + } else {
>> + /* get the next byte */
>> + ret = i2c_recv(s->bus);
>> +
>> + if (ret >= 0) {
>> + imx_i2c_raise_interrupt(s);
>> + } else {
>> + DPRINT("read failed for device 0x%02x\n" s->address);
>> + ret = 0xff;
>> + }
>> + }
>> +
>> + s->i2dr_read = ret;
>> + }
>> + break;
>> + default:
>> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
>> + break;
>> + }
>> +
>> + DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset)
>
> General question for the list, are we encouraging the use or PRIx in
> new code to remove the needs for casts from uint_XXt to unsigned in
> printfery?
> ,
>> + (unsigned int)offset, value);
I believe so, I've been asked to use HWADDR_PRIx macro in ppc patches.
>> +
>> + return (uint64_t)value;
>> +}
[snip]
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 10:53 ` Peter Crosthwaite
@ 2013-05-05 11:41 ` Peter Maydell
2013-05-05 11:53 ` Peter Crosthwaite
0 siblings, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2013-05-05 11:41 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: peter.chubb, qemu-devel, afaerber, Jean-Christophe DUBOIS
On 5 May 2013 11:53, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>>> no extract16() macro spotted.
>>> Should one be added?
>>
>> There's no need for one -- just use extract32. The only
>> reason for having a separate extract64 is to avoid doing
>> 64 bit arithmetic when we don't have to, I think.
>>
>
> perhaps a quick:
>
> #define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c))
>
> would keep the 16bit device code explicit and clean? Save on
> dummy casts in certain situations as well (but not this one).
Hmm, what situations would ever require a cast of the result
or the input of extract32?
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 3:11 ` Peter Crosthwaite
@ 2013-05-05 11:46 ` Andreas Färber
2013-05-05 11:59 ` Peter Crosthwaite
2013-05-05 13:14 ` Jean-Christophe DUBOIS
1 sibling, 1 reply; 35+ messages in thread
From: Andreas Färber @ 2013-05-05 11:46 UTC (permalink / raw)
To: Peter Crosthwaite, Jean-Christophe DUBOIS
Cc: peter.maydell, peter.chubb, qemu-devel
Am 05.05.2013 05:11, schrieb Peter Crosthwaite:
> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
>> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
>
> does this compile with Werror? I thought there was a requirement that
> all non-static functions must have a fn prototype defined beforehand
> which I cant see in this patch.
It went into 2/4 accidentally. :)
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 11:41 ` Peter Maydell
@ 2013-05-05 11:53 ` Peter Crosthwaite
2013-05-05 12:28 ` Jean-Christophe DUBOIS
0 siblings, 1 reply; 35+ messages in thread
From: Peter Crosthwaite @ 2013-05-05 11:53 UTC (permalink / raw)
To: Peter Maydell; +Cc: Jean-Christophe DUBOIS, peter.chubb, qemu-devel, afaerber
Hi Peter,
On Sun, May 5, 2013 at 9:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 May 2013 11:53, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>>>> no extract16() macro spotted.
>>>> Should one be added?
>>>
>>> There's no need for one -- just use extract32. The only
>>> reason for having a separate extract64 is to avoid doing
>>> 64 bit arithmetic when we don't have to, I think.
>>>
>>
>> perhaps a quick:
>>
>> #define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c))
>>
>> would keep the 16bit device code explicit and clean? Save on
>> dummy casts in certain situations as well (but not this one).
>
> Hmm, what situations would ever require a cast of the result
> or the input of extract32?
>
The one off the top of my head:
fprintf(stderr, "your 16 field is :%" PRIx16 "\n", extract16(foo, bar, baz));
Otherwise you would have to match PRIx32 to extract 16 which is clumsy.
But aren't there there some other varargs situations that may require
casts as well?
As for the input cast, I got nothin, I just put it in there for
completeness. Meet you half way and drop the input cast and keep the
output?
Regards,
Peter
> -- PMM
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 11:46 ` Andreas Färber
@ 2013-05-05 11:59 ` Peter Crosthwaite
2013-05-05 12:41 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Peter Crosthwaite @ 2013-05-05 11:59 UTC (permalink / raw)
To: Andreas Färber
Cc: peter.maydell, peter.chubb, qemu-devel, Jean-Christophe DUBOIS
Hi Andreas,
On Sun, May 5, 2013 at 9:46 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 05.05.2013 05:11, schrieb Peter Crosthwaite:
>> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net> wrote:
>>> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
>>
>> does this compile with Werror? I thought there was a requirement that
>> all non-static functions must have a fn prototype defined beforehand
>> which I cant see in this patch.
>
> It went into 2/4 accidentally. :)
>
Thanks,
While your here I have an on topic question, do we want creation
helpers like this? I notice that we progressively moving towards the
state where machine models talk to QOM directly without wrapper init
fns such as this.. Are functions like this compulsory, optional or
deprecated?
Regards,
Peter
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 11:53 ` Peter Crosthwaite
@ 2013-05-05 12:28 ` Jean-Christophe DUBOIS
0 siblings, 0 replies; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-05 12:28 UTC (permalink / raw)
To: Peter Crosthwaite; +Cc: Peter Maydell, peter.chubb, qemu-devel, afaerber
On 05/05/2013 01:53 PM, Peter Crosthwaite wrote:
> Hi Peter,
>
> On Sun, May 5, 2013 at 9:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 May 2013 11:53, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>> On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>>>>> no extract16() macro spotted.
>>>>> Should one be added?
>>>> There's no need for one -- just use extract32. The only
>>>> reason for having a separate extract64 is to avoid doing
>>>> 64 bit arithmetic when we don't have to, I think.
>>>>
>>> perhaps a quick:
>>>
>>> #define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c))
>>>
>>> would keep the 16bit device code explicit and clean? Save on
>>> dummy casts in certain situations as well (but not this one).
>> Hmm, what situations would ever require a cast of the result
>> or the input of extract32?
>>
> The one off the top of my head:
>
> fprintf(stderr, "your 16 field is :%" PRIx16 "\n", extract16(foo, bar, baz));
>
> Otherwise you would have to match PRIx32 to extract 16 which is clumsy.
>
> But aren't there there some other varargs situations that may require
> casts as well?
>
> As for the input cast, I got nothin, I just put it in there for
> completeness. Meet you half way and drop the input cast and keep the
> output?
>
> Regards,
> Peter
I will use extract32() for now.
JC
>
>> -- PMM
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.
2013-05-05 11:34 ` Andreas Färber
@ 2013-05-05 12:34 ` Jean-Christophe DUBOIS
0 siblings, 0 replies; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-05 12:34 UTC (permalink / raw)
To: Andreas Färber
Cc: peter.maydell, Peter Crosthwaite, peter.chubb, qemu-devel
On 05/05/2013 01:34 PM, Andreas Färber wrote:
> Am 05.05.2013 05:14, schrieb Peter Crosthwaite:
>> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net> wrote:
>>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
>>> new file mode 100644
>>> index 0000000..5b0d046
>>> --- /dev/null
>>> +++ b/hw/i2c/imx_i2c.c
> [...]
>>> +typedef struct imx_i2c_state {
>> types should be in CamelCase IMXI2CState
>>
>>> + SysBusDevice parent_obj;
> While at it, please add a white line here. Background is that this
> parent field will pretty likely go away once we switch to a better
> object-orientation framework - if it were in a header we would annotate
> it as private and thus hidden from documentation.
Will do.
>
>>> + MemoryRegion iomem;
>>> + i2c_bus *bus;
> Please rather use i2c_bus bus; and qbus_create_inline() in instance_init.
I am using i2c_init_bus() which itself uses qbus_create().
Do you mean we should not use i2c_init_bus() but instead reimplement
locally based on qbus_create_inline()?
JC
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 11:59 ` Peter Crosthwaite
@ 2013-05-05 12:41 ` Peter Maydell
0 siblings, 0 replies; 35+ messages in thread
From: Peter Maydell @ 2013-05-05 12:41 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: qemu-devel, peter.chubb, Andreas Färber,
Jean-Christophe DUBOIS
On 5 May 2013 12:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> While your here I have an on topic question, do we want creation
> helpers like this? I notice that we progressively moving towards the
> state where machine models talk to QOM directly without wrapper init
> fns such as this.. Are functions like this compulsory, optional or
> deprecated?
I don't think there's a consensus. Personally I think that the
ideal would be that the helper functions are unnecessary (they
really indicate areas where our QOM instantiation syntax is
unbearably clunky and in an ideal world we'd improve it somehow).
My personal suggestion would be:
* if you have to have them they should be static inline in a
header somewhere (if you stick them in the .c file then there
is no compiler guard against the temptation to have them directly
access bits of the internals of the device)
* err on the side of not having them
In this case I think I'd go for "not necessary".
[Aside: I really need to make some time to look more closely at NIC
init for embedded boards. I have a feeling that recent-ish changes
to move away from deprecated '-net' syntax for PC don't really
work for the embedded boards. (The idea for the PC is that you
create your NIC with "-device whatever,id=thingy", which of course
doesn't work for onboard NICs which you don't need to and in fact
can't create with a command line option.) I'm not sure how much
sense board-level manipulation of nd_table[] still makes, since
it's totally tied to old style -net options.]
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 3:11 ` Peter Crosthwaite
2013-05-05 11:46 ` Andreas Färber
@ 2013-05-05 13:14 ` Jean-Christophe DUBOIS
2013-05-05 13:31 ` Andreas Färber
1 sibling, 1 reply; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-05 13:14 UTC (permalink / raw)
To: Peter Crosthwaite; +Cc: peter.maydell, peter.chubb, qemu-devel, afaerber
On 05/05/2013 05:11 AM, Peter Crosthwaite wrote:
> Hi JC,
>
> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
>> This is based on the mcf_fec.c FEC implementation for ColdFire.
>>
>> * a generic phy was added (borrowed from lan9118).
>> * The buffer management is also modified as buffers are
>> slightly different between coldfire and i.MX.
>>
>> Signed-off-by: Jean-Christophe DUBOIS<jcd@tribudubois.net>
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/net/Makefile.objs | 1 +
>> hw/net/imx_fec.c | 748 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 750 insertions(+)
>> create mode 100644 hw/net/imx_fec.c
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 27cbe3d..b3a0207 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -28,6 +28,7 @@ CONFIG_SSI_SD=y
>> CONFIG_SSI_M25P80=y
>> CONFIG_LAN9118=y
>> CONFIG_SMC91C111=y
>> +CONFIG_IMX_FEC=y
>> CONFIG_DS1338=y
>> CONFIG_PFLASH_CFI01=y
>> CONFIG_PFLASH_CFI02=y
>> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
>> index 951cca3..5c84727 100644
>> --- a/hw/net/Makefile.objs
>> +++ b/hw/net/Makefile.objs
>> @@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
>> common-obj-$(CONFIG_XGMAC) += xgmac.o
>> common-obj-$(CONFIG_MIPSNET) += mipsnet.o
>> common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
>> +common-obj-$(CONFIG_IMX_FEC) += imx_fec.o
>>
>> common-obj-$(CONFIG_CADENCE) += cadence_gem.o
>> common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> new file mode 100644
>> index 0000000..e894d50
>> --- /dev/null
>> +++ b/hw/net/imx_fec.c
>> @@ -0,0 +1,748 @@
>> +/*
>> + * i.MX Fast Ethernet Controller emulation.
>> + *
>> + * Copyright (c) 2013 Jean-Christophe Dubois.
>> + *
>> + * Based on Coldfire Fast Ethernet Controller emulation.
>> + *
>> + * Copyright (c) 2007 CodeSourcery.
>> + *
>> + * This code is licensed under the GPL
>> + */
>> +#include "hw/sysbus.h"
>> +#include "net/net.h"
>> +#include "hw/devices.h"
>> +
> is devices.h needed? Its a collection of helper init fns for misc
> devices pre QOM style.
No, I'll remove it. It was part of mcf_fec.c
>> +/* For crc32 */
>> +#include <zlib.h>
>> +
>> +#include "hw/arm/imx.h"
>> +
>> +#ifndef IMX_FEC_DEBUG
>> +#define IMX_FEC_DEBUG 0
>> +#endif
>> +
>> +#ifndef IMX_PHY_DEBUG
>> +#define IMX_PHY_DEBUG 0
>> +#endif
>> +
>> +#if IMX_FEC_DEBUG
>> +#define DPRINTF(fmt, ...) \
>> +do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#if IMX_PHY_DEBUG
>> +#define DPPRINTF(fmt, ...) \
>> +do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#define FEC_MAX_FRAME_SIZE 2032
>> +
>> +typedef struct {
>> + SysBusDevice busdev;
> parent_obj
OK
>> + NICState *nic;
>> + NICConf conf;
>> + qemu_irq irq;
>> + MemoryRegion iomem;
>> +
>> + uint32_t irq_state;
>> + uint32_t eir;
>> + uint32_t eimr;
>> + uint32_t rx_enabled;
>> + uint32_t rx_descriptor;
>> + uint32_t tx_descriptor;
>> + uint32_t ecr;
>> + uint32_t mmfr;
>> + uint32_t mscr;
>> + uint32_t mibc;
>> + uint32_t rcr;
>> + uint32_t tcr;
>> + uint32_t tfwr;
>> + uint32_t frsr;
>> + uint32_t erdsr;
>> + uint32_t etdsr;
>> + uint32_t emrbr;
>> + uint32_t miigsk_cfgr;
>> + uint32_t miigsk_enr;
>> +
>> + uint32_t phy_status;
>> + uint32_t phy_control;
>> + uint32_t phy_advertise;
>> + uint32_t phy_int;
>> + uint32_t phy_int_mask;
>> +} imx_fec_state;
> types are CamelCase (IMXFECState)
OK
>> +
>> +static const VMStateDescription vmstate_imx_fec = {
>> + .name = "fec",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32(irq_state, imx_fec_state),
>> + VMSTATE_UINT32(eir, imx_fec_state),
>> + VMSTATE_UINT32(eimr, imx_fec_state),
>> + VMSTATE_UINT32(rx_enabled, imx_fec_state),
>> + VMSTATE_UINT32(rx_descriptor, imx_fec_state),
>> + VMSTATE_UINT32(tx_descriptor, imx_fec_state),
>> + VMSTATE_UINT32(ecr, imx_fec_state),
>> + VMSTATE_UINT32(mmfr, imx_fec_state),
>> + VMSTATE_UINT32(mscr, imx_fec_state),
>> + VMSTATE_UINT32(mibc, imx_fec_state),
>> + VMSTATE_UINT32(rcr, imx_fec_state),
>> + VMSTATE_UINT32(tcr, imx_fec_state),
>> + VMSTATE_UINT32(tfwr, imx_fec_state),
>> + VMSTATE_UINT32(frsr, imx_fec_state),
>> + VMSTATE_UINT32(erdsr, imx_fec_state),
>> + VMSTATE_UINT32(etdsr, imx_fec_state),
>> + VMSTATE_UINT32(emrbr, imx_fec_state),
>> + VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
>> + VMSTATE_UINT32(miigsk_enr, imx_fec_state),
>> +
>> + VMSTATE_UINT32(phy_status, imx_fec_state),
>> + VMSTATE_UINT32(phy_control, imx_fec_state),
>> + VMSTATE_UINT32(phy_advertise, imx_fec_state),
>> + VMSTATE_UINT32(phy_int, imx_fec_state),
>> + VMSTATE_UINT32(phy_int_mask, imx_fec_state),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +#define PHY_INT_ENERGYON (1 << 7)
>> +#define PHY_INT_AUTONEG_COMPLETE (1 << 6)
>> +#define PHY_INT_FAULT (1 << 5)
>> +#define PHY_INT_DOWN (1 << 4)
>> +#define PHY_INT_AUTONEG_LP (1 << 3)
>> +#define PHY_INT_PARFAULT (1 << 2)
>> +#define PHY_INT_AUTONEG_PAGE (1 << 1)
>> +
>> +static void imx_fec_update(imx_fec_state *s);
>> +
>> +/*
>> + * The MII phy could raise a GPIO to the processor which in turn
>> + * could be handled as an interrpt by the OS.
> "interrupt". Also a bit of a nitpick but s/OS/guest. Guest is the
> common term for the software running on a QEMU emulation.
OK
>> + * For now we don't handle any GPIO/interrupt line, so the OS will
>> + * have to poll for the PHY status.
>> + */
>> +static void phy_update_irq(imx_fec_state *s)
>> +{
>> + imx_fec_update(s);
>> +}
>> +
>> +static void phy_update_link(imx_fec_state *s)
>> +{
>> + /* Autonegotiation status mirrors link status. */
>> + if (qemu_get_queue(s->nic)->link_down) {
>> + DPPRINTF("%s: link is down\n", __func__);
>> + s->phy_status &= ~0x0024;
>> + s->phy_int |= PHY_INT_DOWN;
>> + } else {
>> + DPPRINTF("%s: link is up\n", __func__);
>> + s->phy_status |= 0x0024;
>> + s->phy_int |= PHY_INT_ENERGYON;
>> + s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
>> + }
>> + phy_update_irq(s);
>> +}
>> +
>> +static void imx_fec_set_link(NetClientState *nc)
>> +{
>> + phy_update_link(qemu_get_nic_opaque(nc));
>> +}
>> +
>> +static void phy_reset(imx_fec_state *s)
>> +{
>> + DPPRINTF("%s\n", __func__);
>> +
>> + s->phy_status = 0x7809;
>> + s->phy_control = 0x3000;
>> + s->phy_advertise = 0x01e1;
>> + s->phy_int_mask = 0;
>> + s->phy_int = 0;
>> + phy_update_link(s);
>> +}
>> +
>> +static uint32_t do_phy_read(imx_fec_state *s, int reg)
>> +{
>> + uint32_t val;
>> +
>> + switch (reg) {
>> + case 0: /* Basic Control */
>> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);
> Can you factor out these DPRINTFs in some way? I think you had
> something going for your I2C with the array of register names and a
> %s?
This is a copy/paste of the lan9118 code. But I'll change it.
>> + return s->phy_control;
>> + case 1: /* Basic Status */
>> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
>> + return s->phy_status;
>> + case 2: /* ID1 */
>> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
>> + return 0x0007;
>> + case 3: /* ID2 */
>> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
>> + return 0xc0d1;
>> + case 4: /* Auto-neg advertisement */
>> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
>> + return s->phy_advertise;
>> + case 5: /* Auto-neg Link Partner Ability */
>> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
>> + return 0x0f71;
>> + case 6: /* Auto-neg Expansion */
>> + DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
>> + return 1;
>> + /* TODO 17, 18, 27, 29, 30, 31 */
> This un-implemented functionality? If so, create a case (6 way fall
> through), and qemu_log_mask(LOG_UNIMP to allow debugging users to know
> that QEMU is ignoring something.
Will do
>> + case 29: /* Interrupt source. */
>> + val = s->phy_int;
>> + s->phy_int = 0;
>> + phy_update_irq(s);
>> + return val;
>> + case 30: /* Interrupt mask */
>> + return s->phy_int_mask;
>> + default:
>> + DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);
> Is this un-defined register space? if so, qemu_log_mask(LOG_GUEST_ERROR
Will do
>> + return 0;
>> + }
>> +}
>> +
>> +static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
>> +{
>> + switch (reg) {
>> + case 0: /* Basic Control */
>> + if (val & 0x8000) {
>> + phy_reset(s);
>> + } else {
>> + s->phy_control = val & 0x7980;
>> + /* Complete autonegotiation immediately. */
>> + if (val & 0x1000) {
>> + s->phy_status |= 0x0020;
>> + }
>> + }
>> + break;
>> + case 4: /* Auto-neg advertisement */
>> + s->phy_advertise = (val & 0x2d7f) | 0x80;
>> + break;
>> + /* TODO 17, 18, 27, 31 */
>> + case 30: /* Interrupt mask */
>> + s->phy_int_mask = val & 0xff;
>> + phy_update_irq(s);
>> + break;
>> + default:
>> + DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
>> + }
>> +}
>> +
>> +#define FEC_INT_HB (1 << 31)
>> +#define FEC_INT_BABR (1 << 30)
>> +#define FEC_INT_BABT (1 << 29)
>> +#define FEC_INT_GRA (1 << 28)
>> +#define FEC_INT_TXF (1 << 27)
>> +#define FEC_INT_TXB (1 << 26)
>> +#define FEC_INT_RXF (1 << 25)
>> +#define FEC_INT_RXB (1 << 24)
>> +#define FEC_INT_MII (1 << 23)
>> +#define FEC_INT_EBERR (1 << 22)
>> +#define FEC_INT_LC (1 << 21)
>> +#define FEC_INT_RL (1 << 20)
>> +#define FEC_INT_UN (1 << 19)
>> +
>> +#define FEC_EN 2
>> +#define FEC_RESET 1
>> +
>> +/* Buffer Descriptor. */
>> +typedef struct {
>> + uint16_t length;
>> + uint16_t flags;
>> + uint32_t data;
>> +} imx_fec_bd;
>> +
>> +#define FEC_BD_R (1 << 15)
>> +#define FEC_BD_E (1 << 15)
>> +#define FEC_BD_O1 (1 << 14)
>> +#define FEC_BD_W (1 << 13)
>> +#define FEC_BD_O2 (1 << 12)
>> +#define FEC_BD_L (1 << 11)
>> +#define FEC_BD_TC (1 << 10)
>> +#define FEC_BD_ABC (1 << 9)
>> +#define FEC_BD_M (1 << 8)
>> +#define FEC_BD_BC (1 << 7)
>> +#define FEC_BD_MC (1 << 6)
>> +#define FEC_BD_LG (1 << 5)
>> +#define FEC_BD_NO (1 << 4)
>> +#define FEC_BD_CR (1 << 2)
>> +#define FEC_BD_OV (1 << 1)
>> +#define FEC_BD_TR (1 << 0)
>> +
>> +static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
>> +{
>> + cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));
> cpu_physical_memory_read/write is deprecated for new code (at least
> for doing DMA from device land). Use dma_memory_read/write. pl330.c
> has some examples and has the DMAContext bolierplate for setting this
> up.
OK
>> +}
>> +
>> +static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
>> +{
>> + cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
>> +}
>> +
>> +static void imx_fec_update(imx_fec_state *s)
>> +{
>> + uint32_t active;
>> + uint32_t changed;
>> +
>> + active = s->eir & s->eimr;
>> + changed = active ^ s->irq_state;
>> + qemu_set_irq(s->irq, changed);
> This looks strange (although I haven't read your devices specs so I am
> speculating). You set the IRQ pin on an edge of eir & eimr. Should
> this be
>
> if (changed) {
> qemu_set_irq(s->irq, active)
> }
>
> ?
You are right.
>> + s->irq_state = active;
>> +}
>> +
>> +static void imx_fec_do_tx(imx_fec_state *s)
>> +{
>> + uint32_t addr;
>> + imx_fec_bd bd;
>> + int frame_size;
>> + int len;
>> + uint8_t frame[FEC_MAX_FRAME_SIZE];
>> + uint8_t *ptr;
>> +
>> + DPRINTF("%s:\n", __func__);
>> + ptr = frame;
>> + frame_size = 0;
>> + addr = s->tx_descriptor;
>> + while (1) {
> at least len and bd (maybe other local variables) are local to this
> while block so could be defined here. Helps to clarify which variables
> have cross iteration life, and which ones do not. (not a blocker just
> a suggestion)
OK
>> + imx_fec_read_bd(&bd, addr);
>> + DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
>> + __func__, addr, bd.flags, bd.length, bd.data);
>> + if ((bd.flags & FEC_BD_R) == 0) {
>> + /* Run out of descriptors to transmit. */
>> + break;
>> + }
>> + len = bd.length;
>> + if (frame_size + len > FEC_MAX_FRAME_SIZE) {
>> + len = FEC_MAX_FRAME_SIZE - frame_size;
>> + s->eir |= FEC_INT_BABT;
>> + }
>> + cpu_physical_memory_read(bd.data, ptr, len);
>> + ptr += len;
>> + frame_size += len;
>> + if (bd.flags & FEC_BD_L) {
>> + /* Last buffer in frame. */
>> + DPRINTF("Sending packet\n");
>> + qemu_send_packet(qemu_get_queue(s->nic), frame, len);
>> + ptr = frame;
>> + frame_size = 0;
>> + s->eir |= FEC_INT_TXF;
>> + }
>> + s->eir |= FEC_INT_TXB;
>> + bd.flags &= ~FEC_BD_R;
>> + /* Write back the modified descriptor. */
>> + imx_fec_write_bd(&bd, addr);
>> + /* Advance to the next descriptor. */
>> + if ((bd.flags & FEC_BD_W) != 0) {
>> + addr = s->etdsr;
>> + } else {
>> + addr += 8;
>> + }
>> + }
>> + s->tx_descriptor = addr;
>> + imx_fec_update(s);
>> +}
>> +
>> +static void imx_fec_enable_rx(imx_fec_state *s)
>> +{
>> + imx_fec_bd bd;
>> +
>> + imx_fec_read_bd(&bd, s->rx_descriptor);
>> + s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);
> s->rx_enabled is used to return false from imx_fec_can_recieve. This
> means you need to check for queued packets here using
> qemu_flush_queued_packets, as 0->1 change of rx_enabled may require a
> flush. If your device has implemented packet dropping logic, then you
> could instead just always return true from can_recieve, although my
> experience is a fully implemented can_recieve path is much better
> (although slightly less faithful to the real hardware).
Will do
>
>> + if (!s->rx_enabled) {
>> + DPRINTF("%s: RX buffer full\n", __func__);
>> + }
>> +}
>> +
>> +static void imx_fec_reset(DeviceState *d)
>> +{
>> + imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
>> +
> QOM cast macro needed.
OK
>> + /* Reset the FEC */
>> + s->eir = 0;
>> + s->eimr = 0;
>> + s->rx_enabled = 0;
>> + s->ecr = 0;
>> + s->mscr = 0;
>> + s->mibc = 0xc0000000;
>> + s->rcr = 0x05ee0001;
>> + s->tcr = 0;
>> + s->tfwr = 0;
>> + s->frsr = 0x500;
>> + s->miigsk_cfgr = 0;
>> + s->miigsk_enr = 0x6;
>> +
>> + /* We also reset the PHY */
>> + phy_reset(s);
>> +}
>> +
>> +static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> + imx_fec_state *s = (imx_fec_state *) opaque;
>> +
>> + DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
>> +
>> + switch (addr & 0x3ff) {
>> + case 0x004:
>> + return s->eir;
>> + case 0x008:
>> + return s->eimr;
>> + case 0x010:
>> + return s->rx_enabled ? (1 << 24) : 0; /* RDAR */
>> + case 0x014:
>> + return 0; /* TDAR */
>> + case 0x024:
>> + return s->ecr;
>> + case 0x040:
>> + return s->mmfr;
>> + case 0x044:
>> + return s->mscr;
>> + case 0x064:
>> + return s->mibc; /* MIBC */
>> + case 0x084:
>> + return s->rcr;
>> + case 0x0c4:
>> + return s->tcr;
>> + case 0x0e4: /* PALR */
>> + return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
>> + a[1] << 16)
>> + | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
>> + break;
>> + case 0x0e8: /* PAUR */
>> + return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
>> + a[5] << 16) | 0x8808;
>> + case 0x0ec:
>> + return 0x10000; /* OPD */
>> + case 0x118:
>> + return 0;
>> + case 0x11c:
>> + return 0;
>> + case 0x120:
>> + return 0;
>> + case 0x124:
>> + return 0;
>> + case 0x144:
>> + return s->tfwr;
>> + case 0x14c:
>> + return 0x600;
>> + case 0x150:
>> + return s->frsr;
>> + case 0x180:
>> + return s->erdsr;
>> + case 0x184:
>> + return s->etdsr;
>> + case 0x188:
>> + return s->emrbr;
>> + case 0x300:
>> + return s->miigsk_cfgr;
>> + case 0x308:
>> + return s->miigsk_enr;
>> + default:
>> + hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
>> + return 0;
>> + }
>> +}
>> +
>> +static void imx_fec_write(void *opaque, hwaddr addr,
>> + uint64_t value, unsigned size)
>> +{
>> + imx_fec_state *s = (imx_fec_state *) opaque;
>> +
>> + DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
>> +
>> + switch (addr & 0x3ff) {
>> + case 0x004: /* EIR */
>> + s->eir &= ~value;
>> + break;
>> + case 0x008: /* EIMR */
>> + s->eimr = value;
>> + break;
>> + case 0x010: /* RDAR */
>> + if ((s->ecr & FEC_EN) && !s->rx_enabled) {
>> + DPRINTF("RX enable\n");
>> + imx_fec_enable_rx(s);
>> + }
>> + break;
>> + case 0x014: /* TDAR */
>> + if (s->ecr & FEC_EN) {
>> + imx_fec_do_tx(s);
>> + }
>> + break;
>> + case 0x024: /* ECR */
>> + s->ecr = value;
>> + if (value & FEC_RESET) {
>> + DPRINTF("Reset\n");
>> + imx_fec_reset(&s->busdev.qdev);
>> + }
>> + if ((s->ecr & FEC_EN) == 0) {
>> + s->rx_enabled = 0;
>> + }
>> + break;
>> + case 0x040: /* MMFR */
>> + /* store the value */
>> + s->mmfr = value;
>> + if (value & (1 << 28)) {
>> + DPRINTF("PHY write %d = 0x%04x\n",
>> + ((int)value >> 18) & 0x1f, (int)value & 0xffff);
>> + do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
>> + } else {
>> + s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
>> + DPRINTF("PHY read %d = 0x%04x\n",
>> + ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
>> + }
>> + /* raise the interrupt as the PHY operation is done */
>> + s->eir |= FEC_INT_MII;
>> + break;
>> + case 0x044: /* MSCR */
>> + s->mscr = value & 0xfe;
>> + break;
>> + case 0x064: /* MIBC */
>> + /* TODO: Implement MIB. */
>> + s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
>> + break;
>> + case 0x084: /* RCR */
>> + s->rcr = value & 0x07ff003f;
>> + /* TODO: Implement LOOP mode. */
>> + break;
>> + case 0x0c4: /* TCR */
>> + /* We transmit immediately, so raise GRA immediately. */
>> + s->tcr = value;
>> + if (value & 1) {
>> + s->eir |= FEC_INT_GRA;
>> + }
>> + break;
>> + case 0x0e4: /* PALR */
>> + s->conf.macaddr.a[0] = value >> 24;
>> + s->conf.macaddr.a[1] = value >> 16;
>> + s->conf.macaddr.a[2] = value >> 8;
>> + s->conf.macaddr.a[3] = value;
>> + break;
>> + case 0x0e8: /* PAUR */
>> + s->conf.macaddr.a[4] = value >> 24;
>> + s->conf.macaddr.a[5] = value >> 16;
>> + break;
>> + case 0x0ec: /* OPDR */
>> + break;
>> + case 0x118: /* IAUR */
>> + case 0x11c: /* IALR */
>> + case 0x120: /* GAUR */
>> + case 0x124: /* GALR */
>> + /* TODO: implement MAC hash filtering. */
>> + break;
>> + case 0x144: /* TFWR */
>> + s->tfwr = value & 3;
>> + break;
>> + case 0x14c: /* FRBR */
>> + /* FRBR writes ignored. */
>> + break;
>> + case 0x150: /* FRSR */
>> + s->frsr = (value & 0x3fc) | 0x400;
>> + break;
>> + case 0x180: /* ERDSR */
>> + s->erdsr = value & ~3;
>> + s->rx_descriptor = s->erdsr;
>> + break;
>> + case 0x184: /* ETDSR */
>> + s->etdsr = value & ~3;
>> + s->tx_descriptor = s->etdsr;
>> + break;
>> + case 0x188: /* EMRBR */
>> + s->emrbr = value & 0x7f0;
>> + break;
>> + case 0x300: /* MIIGSK_CFGR */
>> + s->miigsk_cfgr = value & 0x53;
>> + break;
>> + case 0x308: /* MIIGSK_ENR */
>> + s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
>> + break;
>> + default:
>> + hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
>> + }
>> + imx_fec_update(s);
>> +}
>> +
>> +static int imx_fec_can_receive(NetClientState *nc)
>> +{
>> + imx_fec_state *s = qemu_get_nic_opaque(nc);
>> +
> Andreas, do we care about QOM casts coming from random void* opaques?
OK, I am going to do a QOM cast anyway.
>> + return s->rx_enabled;
>> +}
>> +
>> +static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
>> + size_t len)
>> +{
>> + imx_fec_state *s = qemu_get_nic_opaque(nc);
>> + imx_fec_bd bd;
>> + uint32_t flags = 0;
>> + uint32_t addr;
>> + uint32_t crc;
>> + uint32_t buf_addr;
>> + uint8_t *crc_ptr;
>> + unsigned int buf_len;
>> + size_t size = len;
>> +
>> + DPRINTF("%s: len %d\n", __func__, (int)size);
>> +
>> + if (!s->rx_enabled) {
>> + DPRINTF("%s: Unexpected packet\n", __func__);
>> + return 0;
>> + }
>> + /* 4 bytes for the CRC. */
>> + size += 4;
>> + crc = cpu_to_be32(crc32(~0, buf, size));
>> + crc_ptr = (uint8_t *) &crc;
>> + /* Huge frames are truncted. */
>> + if (size > FEC_MAX_FRAME_SIZE) {
>> + size = FEC_MAX_FRAME_SIZE;
>> + flags |= FEC_BD_TR | FEC_BD_LG;
>> + }
>> + /* Frames larger than the user limit just set error flags. */
>> + if (size > (s->rcr >> 16)) {
>> + flags |= FEC_BD_LG;
>> + }
>> + addr = s->rx_descriptor;
>> + while (size > 0) {
>> + imx_fec_read_bd(&bd, addr);
>> + if ((bd.flags & FEC_BD_E) == 0) {
>> + /* No descriptors available. Bail out. */
>> + /*
>> + * FIXME: This is wrong. We should probably either
>> + * save the remainder for when more RX buffers are
>> + * available, or flag an error.
>> + */
> Wouldn't the real hardware just drops the packet? Does the device have
> bits and interrupts for signalling this condition? (most NICs do).
This is the ColdFire FEC code (in mcf_fec.c). I did not try to improve
it on this side.
>> + DPRINTF("%s: Lost end of frame\n", __func__);
>> + break;
>> + }
>> + buf_len = (size <= s->emrbr) ? size : s->emrbr;
>> + bd.length = buf_len;
>> + size -= buf_len;
>> + DPRINTF("rx_bd %x length %d\n", addr, bd.length);
>> + /* The last 4 bytes are the CRC. */
>> + if (size < 4) {
>> + buf_len += size - 4;
>> + }
>> + buf_addr = bd.data;
>> + cpu_physical_memory_write(buf_addr, buf, buf_len);
>> + buf += buf_len;
>> + if (size < 4) {
>> + cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
>> + 4 - size);
>> + crc_ptr += 4 - size;
>> + }
>> + bd.flags &= ~FEC_BD_E;
>> + if (size == 0) {
>> + /* Last buffer in frame. */
>> + bd.flags |= flags | FEC_BD_L;
>> + DPRINTF("rx frame flags %04x\n", bd.flags);
>> + s->eir |= FEC_INT_RXF;
>> + } else {
>> + s->eir |= FEC_INT_RXB;
>> + }
>> + imx_fec_write_bd(&bd, addr);
>> + /* Advance to the next descriptor. */
>> + if ((bd.flags & FEC_BD_W) != 0) {
>> + addr = s->erdsr;
>> + } else {
>> + addr += 8;
>> + }
>> + }
>> + s->rx_descriptor = addr;
>> + imx_fec_enable_rx(s);
>> + imx_fec_update(s);
>> + return len;
>> +}
>> +
>> +static const MemoryRegionOps imx_fec_ops = {
>> + .read = imx_fec_read,
>> + .write = imx_fec_write,
>> + .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static void imx_fec_cleanup(NetClientState *nc)
>> +{
>> + imx_fec_state *s = qemu_get_nic_opaque(nc);
>> +
>> + s->nic = NULL;
>> +}
>> +
>> +static NetClientInfo net_imx_fec_info = {
>> + .type = NET_CLIENT_OPTIONS_KIND_NIC,
>> + .size = sizeof(NICState),
>> + .can_receive = imx_fec_can_receive,
>> + .receive = imx_fec_receive,
>> + .cleanup = imx_fec_cleanup,
>> + .link_status_changed = imx_fec_set_link,
>> +};
>> +
>> +static int imx_fec_init(SysBusDevice *dev)
>> +{
>> + imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
>> +
>> + memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
>> + sysbus_init_mmio(dev, &s->iomem);
>> + sysbus_init_irq(dev, &s->irq);
>> + qemu_macaddr_default_if_unset(&s->conf.macaddr);
>> +
>> + s->conf.peers.ncs[0] = nd_table[0].netdev;
>> +
>> + s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
>> + object_get_typename(OBJECT(dev)), dev->qdev.id,
>> + s);
>> + qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>> +
>> + return 0;
>> +}
>> +
>> +static Property imx_fec_properties[] = {
>> + DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void imx_fec_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> + k->init = imx_fec_init;
>> + dc->reset = imx_fec_reset;
>> + dc->vmsd = &vmstate_imx_fec;
>> + dc->props = imx_fec_properties;
>> +}
>> +
>> +static const TypeInfo imx_fec_info = {
>> + .name = "fec",
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(imx_fec_state),
>> + .class_init = imx_fec_class_init,
>> +};
>> +
>> +static void imx_fec_register_types(void)
>> +{
>> + type_register_static(&imx_fec_info);
>> +}
>> +
>> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
> does this compile with Werror? I thought there was a requirement that
> all non-static functions must have a fn prototype defined beforehand
> which I cant see in this patch.
It is part of imx.h which was (wrongly) added to another patch.
> Regards,
> Peter
>
>
>> +{
>> + NICInfo *nd;
>> + DeviceState *dev;
>> + SysBusDevice *s;
>> +
>> + if (nic >= MAX_NICS) {
>> + hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
>> + nic, MAX_NICS);
>> + }
>> +
>> + nd = &nd_table[nic];
>> +
>> + qemu_check_nic_model(nd, "fec");
>> + dev = qdev_create(NULL, "fec");
>> + qdev_set_nic_properties(dev, nd);
>> + qdev_init_nofail(dev);
>> + s = SYS_BUS_DEVICE(dev);
>> + sysbus_mmio_map(s, 0, base);
>> + sysbus_connect_irq(s, 0, irq);
>> +};
>> +
>> +type_init(imx_fec_register_types)
>> --
>> 1.8.1.2
>>
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 13:14 ` Jean-Christophe DUBOIS
@ 2013-05-05 13:31 ` Andreas Färber
2013-05-05 14:05 ` Jean-Christophe DUBOIS
0 siblings, 1 reply; 35+ messages in thread
From: Andreas Färber @ 2013-05-05 13:31 UTC (permalink / raw)
To: Jean-Christophe DUBOIS, Peter Crosthwaite
Cc: peter.maydell, peter.chubb, qemu-devel
Am 05.05.2013 15:14, schrieb Jean-Christophe DUBOIS:
> On 05/05/2013 05:11 AM, Peter Crosthwaite wrote:
>> Hi JC,
>>
>> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net> wrote:
>>> This is based on the mcf_fec.c FEC implementation for ColdFire.
Note that ColdFire is one of the least maintained targets in QEMU...
>>> +static int imx_fec_can_receive(NetClientState *nc)
>>> +{
>>> + imx_fec_state *s = qemu_get_nic_opaque(nc);
>>> +
>> Andreas, do we care about QOM casts coming from random void* opaques?
Generally no. If we ever switch to C++ or some other OO language we'll
have to touch casts anyway.
Peter, please note that I usually don't have time to read through all
patches - noticed this inline question incidentally only.
> OK, I am going to do a QOM cast anyway.
You should check how frequently this function is called - the current
OBJECT_CHECK() implementation does at least one string comparison, so
I'd suggest to avoid it here. That is, if the type passed in as opaque
matches the type assigned here (thinking of interfaces).
>>> + return s->rx_enabled;
>>> +}
[snip]
Not knowing this piece of hardware, might it make sense to call it
"imx-fec" when there's another FEC for ColdFire? Or are they the same
thing and should be unified?
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 13:31 ` Andreas Färber
@ 2013-05-05 14:05 ` Jean-Christophe DUBOIS
0 siblings, 0 replies; 35+ messages in thread
From: Jean-Christophe DUBOIS @ 2013-05-05 14:05 UTC (permalink / raw)
To: Andreas Färber
Cc: peter.maydell, Peter Crosthwaite, peter.chubb, qemu-devel
On 05/05/2013 03:31 PM, Andreas Färber wrote:
> Am 05.05.2013 15:14, schrieb Jean-Christophe DUBOIS:
>> On 05/05/2013 05:11 AM, Peter Crosthwaite wrote:
>>> Hi JC,
>>>
>>> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
>>> <jcd@tribudubois.net> wrote:
>>>> This is based on the mcf_fec.c FEC implementation for ColdFire.
> Note that ColdFire is one of the least maintained targets in QEMU...
Well, that's too bad.
I actually believe the ColdFire FEC implementation would not work in
actual QEMU (but I don't plan to build a ColdFire Kernel and RootFS to
find out).
>
>>>> +static int imx_fec_can_receive(NetClientState *nc)
>>>> +{
>>>> + imx_fec_state *s = qemu_get_nic_opaque(nc);
>>>> +
>>> Andreas, do we care about QOM casts coming from random void* opaques?
> Generally no. If we ever switch to C++ or some other OO language we'll
> have to touch casts anyway.
>
> Peter, please note that I usually don't have time to read through all
> patches - noticed this inline question incidentally only.
>
>> OK, I am going to do a QOM cast anyway.
> You should check how frequently this function is called - the current
> OBJECT_CHECK() implementation does at least one string comparison, so
> I'd suggest to avoid it here. That is, if the type passed in as opaque
> matches the type assigned here (thinking of interfaces).
Hum, ... you are going to be unhappy with my new patch.
>
>>>> + return s->rx_enabled;
>>>> +}
> [snip]
>
> Not knowing this piece of hardware, might it make sense to call it
> "imx-fec" when there's another FEC for ColdFire?
I am calling it "imx.fec" in the new version of the file (to be
equivalent to "imx.i2c" I did for I2C).
> Or are they the same
> thing and should be unified?
Well, they are certainly very similar but are different on a few points.
In particular the "buffer descriptors" are reversed between i.MX and
Coldfire. Linux does a compile time decision about these buffer
descriptors but I don't know how this would work with Qemu (can you make
a different compile time decision for 2 different targets?).
We could also implement a more cumbersome run time decision but I wanted
to keep simple.
And anyway I don't have Coldfire (cross compilers, rootfs, ...) setup
around to test.
>
> Regards,
> Andreas
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver Jean-Christophe DUBOIS
2013-05-05 3:11 ` Peter Crosthwaite
@ 2013-05-05 17:49 ` Michael S. Tsirkin
2013-05-05 18:01 ` Peter Maydell
1 sibling, 1 reply; 35+ messages in thread
From: Michael S. Tsirkin @ 2013-05-05 17:49 UTC (permalink / raw)
To: Jean-Christophe DUBOIS
Cc: peter.maydell, peter.crosthwaite, peter.chubb, qemu-devel,
afaerber
On Sat, May 04, 2013 at 04:09:11PM +0200, Jean-Christophe DUBOIS wrote:
> This is based on the mcf_fec.c FEC implementation for ColdFire.
>
> * a generic phy was added (borrowed from lan9118).
> * The buffer management is also modified as buffers are
> slightly different between coldfire and i.MX.
>
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/net/Makefile.objs | 1 +
> hw/net/imx_fec.c | 748 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 750 insertions(+)
> create mode 100644 hw/net/imx_fec.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 27cbe3d..b3a0207 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -28,6 +28,7 @@ CONFIG_SSI_SD=y
> CONFIG_SSI_M25P80=y
> CONFIG_LAN9118=y
> CONFIG_SMC91C111=y
> +CONFIG_IMX_FEC=y
> CONFIG_DS1338=y
> CONFIG_PFLASH_CFI01=y
> CONFIG_PFLASH_CFI02=y
> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
> index 951cca3..5c84727 100644
> --- a/hw/net/Makefile.objs
> +++ b/hw/net/Makefile.objs
> @@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
> common-obj-$(CONFIG_XGMAC) += xgmac.o
> common-obj-$(CONFIG_MIPSNET) += mipsnet.o
> common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
> +common-obj-$(CONFIG_IMX_FEC) += imx_fec.o
>
> common-obj-$(CONFIG_CADENCE) += cadence_gem.o
> common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> new file mode 100644
> index 0000000..e894d50
> --- /dev/null
> +++ b/hw/net/imx_fec.c
> @@ -0,0 +1,748 @@
> +/*
> + * i.MX Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2013 Jean-Christophe Dubois.
> + *
> + * Based on Coldfire Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2007 CodeSourcery.
> + *
> + * This code is licensed under the GPL
Let's do as we did in other places like this,
and say 'Contributions after XYZ are licensed under the terms of the
GNU GPL, version 2 or (at your option) any later version.'
> + */
> +#include "hw/sysbus.h"
> +#include "net/net.h"
> +#include "hw/devices.h"
> +
> +/* For crc32 */
> +#include <zlib.h>
> +
> +#include "hw/arm/imx.h"
> +
> +#ifndef IMX_FEC_DEBUG
> +#define IMX_FEC_DEBUG 0
> +#endif
> +
> +#ifndef IMX_PHY_DEBUG
> +#define IMX_PHY_DEBUG 0
> +#endif
> +
> +#if IMX_FEC_DEBUG
> +#define DPRINTF(fmt, ...) \
> +do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#if IMX_PHY_DEBUG
> +#define DPPRINTF(fmt, ...) \
> +do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define FEC_MAX_FRAME_SIZE 2032
> +
> +typedef struct {
> + SysBusDevice busdev;
> + NICState *nic;
> + NICConf conf;
> + qemu_irq irq;
> + MemoryRegion iomem;
> +
> + uint32_t irq_state;
> + uint32_t eir;
> + uint32_t eimr;
> + uint32_t rx_enabled;
> + uint32_t rx_descriptor;
> + uint32_t tx_descriptor;
> + uint32_t ecr;
> + uint32_t mmfr;
> + uint32_t mscr;
> + uint32_t mibc;
> + uint32_t rcr;
> + uint32_t tcr;
> + uint32_t tfwr;
> + uint32_t frsr;
> + uint32_t erdsr;
> + uint32_t etdsr;
> + uint32_t emrbr;
> + uint32_t miigsk_cfgr;
> + uint32_t miigsk_enr;
> +
> + uint32_t phy_status;
> + uint32_t phy_control;
> + uint32_t phy_advertise;
> + uint32_t phy_int;
> + uint32_t phy_int_mask;
> +} imx_fec_state;
> +
> +static const VMStateDescription vmstate_imx_fec = {
> + .name = "fec",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(irq_state, imx_fec_state),
> + VMSTATE_UINT32(eir, imx_fec_state),
> + VMSTATE_UINT32(eimr, imx_fec_state),
> + VMSTATE_UINT32(rx_enabled, imx_fec_state),
> + VMSTATE_UINT32(rx_descriptor, imx_fec_state),
> + VMSTATE_UINT32(tx_descriptor, imx_fec_state),
> + VMSTATE_UINT32(ecr, imx_fec_state),
> + VMSTATE_UINT32(mmfr, imx_fec_state),
> + VMSTATE_UINT32(mscr, imx_fec_state),
> + VMSTATE_UINT32(mibc, imx_fec_state),
> + VMSTATE_UINT32(rcr, imx_fec_state),
> + VMSTATE_UINT32(tcr, imx_fec_state),
> + VMSTATE_UINT32(tfwr, imx_fec_state),
> + VMSTATE_UINT32(frsr, imx_fec_state),
> + VMSTATE_UINT32(erdsr, imx_fec_state),
> + VMSTATE_UINT32(etdsr, imx_fec_state),
> + VMSTATE_UINT32(emrbr, imx_fec_state),
> + VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
> + VMSTATE_UINT32(miigsk_enr, imx_fec_state),
> +
> + VMSTATE_UINT32(phy_status, imx_fec_state),
> + VMSTATE_UINT32(phy_control, imx_fec_state),
> + VMSTATE_UINT32(phy_advertise, imx_fec_state),
> + VMSTATE_UINT32(phy_int, imx_fec_state),
> + VMSTATE_UINT32(phy_int_mask, imx_fec_state),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +#define PHY_INT_ENERGYON (1 << 7)
> +#define PHY_INT_AUTONEG_COMPLETE (1 << 6)
> +#define PHY_INT_FAULT (1 << 5)
> +#define PHY_INT_DOWN (1 << 4)
> +#define PHY_INT_AUTONEG_LP (1 << 3)
> +#define PHY_INT_PARFAULT (1 << 2)
> +#define PHY_INT_AUTONEG_PAGE (1 << 1)
> +
> +static void imx_fec_update(imx_fec_state *s);
> +
> +/*
> + * The MII phy could raise a GPIO to the processor which in turn
> + * could be handled as an interrpt by the OS.
> + * For now we don't handle any GPIO/interrupt line, so the OS will
> + * have to poll for the PHY status.
> + */
> +static void phy_update_irq(imx_fec_state *s)
> +{
> + imx_fec_update(s);
> +}
> +
> +static void phy_update_link(imx_fec_state *s)
> +{
> + /* Autonegotiation status mirrors link status. */
> + if (qemu_get_queue(s->nic)->link_down) {
> + DPPRINTF("%s: link is down\n", __func__);
> + s->phy_status &= ~0x0024;
> + s->phy_int |= PHY_INT_DOWN;
> + } else {
> + DPPRINTF("%s: link is up\n", __func__);
> + s->phy_status |= 0x0024;
> + s->phy_int |= PHY_INT_ENERGYON;
> + s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
> + }
> + phy_update_irq(s);
> +}
> +
> +static void imx_fec_set_link(NetClientState *nc)
> +{
> + phy_update_link(qemu_get_nic_opaque(nc));
> +}
> +
> +static void phy_reset(imx_fec_state *s)
> +{
> + DPPRINTF("%s\n", __func__);
> +
> + s->phy_status = 0x7809;
> + s->phy_control = 0x3000;
> + s->phy_advertise = 0x01e1;
> + s->phy_int_mask = 0;
> + s->phy_int = 0;
> + phy_update_link(s);
> +}
> +
> +static uint32_t do_phy_read(imx_fec_state *s, int reg)
> +{
> + uint32_t val;
> +
> + switch (reg) {
> + case 0: /* Basic Control */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);
> + return s->phy_control;
> + case 1: /* Basic Status */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
> + return s->phy_status;
> + case 2: /* ID1 */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
> + return 0x0007;
> + case 3: /* ID2 */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
> + return 0xc0d1;
> + case 4: /* Auto-neg advertisement */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
> + return s->phy_advertise;
> + case 5: /* Auto-neg Link Partner Ability */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
> + return 0x0f71;
> + case 6: /* Auto-neg Expansion */
> + DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
> + return 1;
> + /* TODO 17, 18, 27, 29, 30, 31 */
> + case 29: /* Interrupt source. */
> + val = s->phy_int;
> + s->phy_int = 0;
> + phy_update_irq(s);
> + return val;
> + case 30: /* Interrupt mask */
> + return s->phy_int_mask;
> + default:
> + DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);
> + return 0;
> + }
> +}
> +
> +static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
> +{
> + switch (reg) {
> + case 0: /* Basic Control */
> + if (val & 0x8000) {
> + phy_reset(s);
> + } else {
> + s->phy_control = val & 0x7980;
> + /* Complete autonegotiation immediately. */
> + if (val & 0x1000) {
> + s->phy_status |= 0x0020;
> + }
> + }
> + break;
> + case 4: /* Auto-neg advertisement */
> + s->phy_advertise = (val & 0x2d7f) | 0x80;
> + break;
> + /* TODO 17, 18, 27, 31 */
> + case 30: /* Interrupt mask */
> + s->phy_int_mask = val & 0xff;
> + phy_update_irq(s);
> + break;
> + default:
> + DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
> + }
> +}
> +
> +#define FEC_INT_HB (1 << 31)
> +#define FEC_INT_BABR (1 << 30)
> +#define FEC_INT_BABT (1 << 29)
> +#define FEC_INT_GRA (1 << 28)
> +#define FEC_INT_TXF (1 << 27)
> +#define FEC_INT_TXB (1 << 26)
> +#define FEC_INT_RXF (1 << 25)
> +#define FEC_INT_RXB (1 << 24)
> +#define FEC_INT_MII (1 << 23)
> +#define FEC_INT_EBERR (1 << 22)
> +#define FEC_INT_LC (1 << 21)
> +#define FEC_INT_RL (1 << 20)
> +#define FEC_INT_UN (1 << 19)
> +
> +#define FEC_EN 2
> +#define FEC_RESET 1
> +
> +/* Buffer Descriptor. */
> +typedef struct {
> + uint16_t length;
> + uint16_t flags;
> + uint32_t data;
> +} imx_fec_bd;
> +
> +#define FEC_BD_R (1 << 15)
> +#define FEC_BD_E (1 << 15)
> +#define FEC_BD_O1 (1 << 14)
> +#define FEC_BD_W (1 << 13)
> +#define FEC_BD_O2 (1 << 12)
> +#define FEC_BD_L (1 << 11)
> +#define FEC_BD_TC (1 << 10)
> +#define FEC_BD_ABC (1 << 9)
> +#define FEC_BD_M (1 << 8)
> +#define FEC_BD_BC (1 << 7)
> +#define FEC_BD_MC (1 << 6)
> +#define FEC_BD_LG (1 << 5)
> +#define FEC_BD_NO (1 << 4)
> +#define FEC_BD_CR (1 << 2)
> +#define FEC_BD_OV (1 << 1)
> +#define FEC_BD_TR (1 << 0)
> +
> +static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> + cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));
> +}
> +
> +static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> + cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
> +}
> +
> +static void imx_fec_update(imx_fec_state *s)
> +{
> + uint32_t active;
> + uint32_t changed;
> +
> + active = s->eir & s->eimr;
> + changed = active ^ s->irq_state;
> + qemu_set_irq(s->irq, changed);
> + s->irq_state = active;
> +}
> +
> +static void imx_fec_do_tx(imx_fec_state *s)
> +{
> + uint32_t addr;
> + imx_fec_bd bd;
> + int frame_size;
> + int len;
> + uint8_t frame[FEC_MAX_FRAME_SIZE];
> + uint8_t *ptr;
> +
> + DPRINTF("%s:\n", __func__);
> + ptr = frame;
> + frame_size = 0;
> + addr = s->tx_descriptor;
> + while (1) {
> + imx_fec_read_bd(&bd, addr);
> + DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
> + __func__, addr, bd.flags, bd.length, bd.data);
> + if ((bd.flags & FEC_BD_R) == 0) {
> + /* Run out of descriptors to transmit. */
> + break;
> + }
> + len = bd.length;
> + if (frame_size + len > FEC_MAX_FRAME_SIZE) {
> + len = FEC_MAX_FRAME_SIZE - frame_size;
> + s->eir |= FEC_INT_BABT;
> + }
> + cpu_physical_memory_read(bd.data, ptr, len);
> + ptr += len;
> + frame_size += len;
> + if (bd.flags & FEC_BD_L) {
> + /* Last buffer in frame. */
> + DPRINTF("Sending packet\n");
> + qemu_send_packet(qemu_get_queue(s->nic), frame, len);
> + ptr = frame;
> + frame_size = 0;
> + s->eir |= FEC_INT_TXF;
> + }
> + s->eir |= FEC_INT_TXB;
> + bd.flags &= ~FEC_BD_R;
> + /* Write back the modified descriptor. */
> + imx_fec_write_bd(&bd, addr);
> + /* Advance to the next descriptor. */
> + if ((bd.flags & FEC_BD_W) != 0) {
> + addr = s->etdsr;
> + } else {
> + addr += 8;
> + }
> + }
> + s->tx_descriptor = addr;
> + imx_fec_update(s);
> +}
> +
> +static void imx_fec_enable_rx(imx_fec_state *s)
> +{
> + imx_fec_bd bd;
> +
> + imx_fec_read_bd(&bd, s->rx_descriptor);
> + s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);
> + if (!s->rx_enabled) {
> + DPRINTF("%s: RX buffer full\n", __func__);
> + }
> +}
> +
> +static void imx_fec_reset(DeviceState *d)
> +{
> + imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
> +
> + /* Reset the FEC */
> + s->eir = 0;
> + s->eimr = 0;
> + s->rx_enabled = 0;
> + s->ecr = 0;
> + s->mscr = 0;
> + s->mibc = 0xc0000000;
> + s->rcr = 0x05ee0001;
> + s->tcr = 0;
> + s->tfwr = 0;
> + s->frsr = 0x500;
> + s->miigsk_cfgr = 0;
> + s->miigsk_enr = 0x6;
> +
> + /* We also reset the PHY */
> + phy_reset(s);
> +}
> +
> +static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + imx_fec_state *s = (imx_fec_state *) opaque;
> +
> + DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
> +
> + switch (addr & 0x3ff) {
> + case 0x004:
> + return s->eir;
> + case 0x008:
> + return s->eimr;
> + case 0x010:
> + return s->rx_enabled ? (1 << 24) : 0; /* RDAR */
> + case 0x014:
> + return 0; /* TDAR */
> + case 0x024:
> + return s->ecr;
> + case 0x040:
> + return s->mmfr;
> + case 0x044:
> + return s->mscr;
> + case 0x064:
> + return s->mibc; /* MIBC */
> + case 0x084:
> + return s->rcr;
> + case 0x0c4:
> + return s->tcr;
> + case 0x0e4: /* PALR */
> + return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
> + a[1] << 16)
> + | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
> + break;
> + case 0x0e8: /* PAUR */
> + return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
> + a[5] << 16) | 0x8808;
> + case 0x0ec:
> + return 0x10000; /* OPD */
> + case 0x118:
> + return 0;
> + case 0x11c:
> + return 0;
> + case 0x120:
> + return 0;
> + case 0x124:
> + return 0;
> + case 0x144:
> + return s->tfwr;
> + case 0x14c:
> + return 0x600;
> + case 0x150:
> + return s->frsr;
> + case 0x180:
> + return s->erdsr;
> + case 0x184:
> + return s->etdsr;
> + case 0x188:
> + return s->emrbr;
> + case 0x300:
> + return s->miigsk_cfgr;
> + case 0x308:
> + return s->miigsk_enr;
> + default:
> + hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
> + return 0;
> + }
> +}
> +
> +static void imx_fec_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + imx_fec_state *s = (imx_fec_state *) opaque;
> +
> + DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
> +
> + switch (addr & 0x3ff) {
> + case 0x004: /* EIR */
> + s->eir &= ~value;
> + break;
> + case 0x008: /* EIMR */
> + s->eimr = value;
> + break;
> + case 0x010: /* RDAR */
> + if ((s->ecr & FEC_EN) && !s->rx_enabled) {
> + DPRINTF("RX enable\n");
> + imx_fec_enable_rx(s);
> + }
> + break;
> + case 0x014: /* TDAR */
> + if (s->ecr & FEC_EN) {
> + imx_fec_do_tx(s);
> + }
> + break;
> + case 0x024: /* ECR */
> + s->ecr = value;
> + if (value & FEC_RESET) {
> + DPRINTF("Reset\n");
> + imx_fec_reset(&s->busdev.qdev);
> + }
> + if ((s->ecr & FEC_EN) == 0) {
> + s->rx_enabled = 0;
> + }
> + break;
> + case 0x040: /* MMFR */
> + /* store the value */
> + s->mmfr = value;
> + if (value & (1 << 28)) {
> + DPRINTF("PHY write %d = 0x%04x\n",
> + ((int)value >> 18) & 0x1f, (int)value & 0xffff);
> + do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
> + } else {
> + s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
> + DPRINTF("PHY read %d = 0x%04x\n",
> + ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
> + }
> + /* raise the interrupt as the PHY operation is done */
> + s->eir |= FEC_INT_MII;
> + break;
> + case 0x044: /* MSCR */
> + s->mscr = value & 0xfe;
> + break;
> + case 0x064: /* MIBC */
> + /* TODO: Implement MIB. */
> + s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
> + break;
> + case 0x084: /* RCR */
> + s->rcr = value & 0x07ff003f;
> + /* TODO: Implement LOOP mode. */
> + break;
> + case 0x0c4: /* TCR */
> + /* We transmit immediately, so raise GRA immediately. */
> + s->tcr = value;
> + if (value & 1) {
> + s->eir |= FEC_INT_GRA;
> + }
> + break;
> + case 0x0e4: /* PALR */
> + s->conf.macaddr.a[0] = value >> 24;
> + s->conf.macaddr.a[1] = value >> 16;
> + s->conf.macaddr.a[2] = value >> 8;
> + s->conf.macaddr.a[3] = value;
> + break;
> + case 0x0e8: /* PAUR */
> + s->conf.macaddr.a[4] = value >> 24;
> + s->conf.macaddr.a[5] = value >> 16;
> + break;
> + case 0x0ec: /* OPDR */
> + break;
> + case 0x118: /* IAUR */
> + case 0x11c: /* IALR */
> + case 0x120: /* GAUR */
> + case 0x124: /* GALR */
> + /* TODO: implement MAC hash filtering. */
> + break;
> + case 0x144: /* TFWR */
> + s->tfwr = value & 3;
> + break;
> + case 0x14c: /* FRBR */
> + /* FRBR writes ignored. */
> + break;
> + case 0x150: /* FRSR */
> + s->frsr = (value & 0x3fc) | 0x400;
> + break;
> + case 0x180: /* ERDSR */
> + s->erdsr = value & ~3;
> + s->rx_descriptor = s->erdsr;
> + break;
> + case 0x184: /* ETDSR */
> + s->etdsr = value & ~3;
> + s->tx_descriptor = s->etdsr;
> + break;
> + case 0x188: /* EMRBR */
> + s->emrbr = value & 0x7f0;
> + break;
> + case 0x300: /* MIIGSK_CFGR */
> + s->miigsk_cfgr = value & 0x53;
> + break;
> + case 0x308: /* MIIGSK_ENR */
> + s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
> + break;
> + default:
> + hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
> + }
> + imx_fec_update(s);
> +}
> +
> +static int imx_fec_can_receive(NetClientState *nc)
> +{
> + imx_fec_state *s = qemu_get_nic_opaque(nc);
> +
> + return s->rx_enabled;
> +}
> +
> +static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> + size_t len)
> +{
> + imx_fec_state *s = qemu_get_nic_opaque(nc);
> + imx_fec_bd bd;
> + uint32_t flags = 0;
> + uint32_t addr;
> + uint32_t crc;
> + uint32_t buf_addr;
> + uint8_t *crc_ptr;
> + unsigned int buf_len;
> + size_t size = len;
> +
> + DPRINTF("%s: len %d\n", __func__, (int)size);
> +
> + if (!s->rx_enabled) {
> + DPRINTF("%s: Unexpected packet\n", __func__);
> + return 0;
> + }
> + /* 4 bytes for the CRC. */
> + size += 4;
> + crc = cpu_to_be32(crc32(~0, buf, size));
> + crc_ptr = (uint8_t *) &crc;
> + /* Huge frames are truncted. */
> + if (size > FEC_MAX_FRAME_SIZE) {
> + size = FEC_MAX_FRAME_SIZE;
> + flags |= FEC_BD_TR | FEC_BD_LG;
> + }
> + /* Frames larger than the user limit just set error flags. */
> + if (size > (s->rcr >> 16)) {
> + flags |= FEC_BD_LG;
> + }
> + addr = s->rx_descriptor;
> + while (size > 0) {
> + imx_fec_read_bd(&bd, addr);
> + if ((bd.flags & FEC_BD_E) == 0) {
> + /* No descriptors available. Bail out. */
> + /*
> + * FIXME: This is wrong. We should probably either
> + * save the remainder for when more RX buffers are
> + * available, or flag an error.
> + */
> + DPRINTF("%s: Lost end of frame\n", __func__);
> + break;
> + }
> + buf_len = (size <= s->emrbr) ? size : s->emrbr;
> + bd.length = buf_len;
> + size -= buf_len;
> + DPRINTF("rx_bd %x length %d\n", addr, bd.length);
> + /* The last 4 bytes are the CRC. */
> + if (size < 4) {
> + buf_len += size - 4;
> + }
> + buf_addr = bd.data;
> + cpu_physical_memory_write(buf_addr, buf, buf_len);
> + buf += buf_len;
> + if (size < 4) {
> + cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
> + 4 - size);
> + crc_ptr += 4 - size;
> + }
> + bd.flags &= ~FEC_BD_E;
> + if (size == 0) {
> + /* Last buffer in frame. */
> + bd.flags |= flags | FEC_BD_L;
> + DPRINTF("rx frame flags %04x\n", bd.flags);
> + s->eir |= FEC_INT_RXF;
> + } else {
> + s->eir |= FEC_INT_RXB;
> + }
> + imx_fec_write_bd(&bd, addr);
> + /* Advance to the next descriptor. */
> + if ((bd.flags & FEC_BD_W) != 0) {
> + addr = s->erdsr;
> + } else {
> + addr += 8;
> + }
> + }
> + s->rx_descriptor = addr;
> + imx_fec_enable_rx(s);
> + imx_fec_update(s);
> + return len;
> +}
> +
> +static const MemoryRegionOps imx_fec_ops = {
> + .read = imx_fec_read,
> + .write = imx_fec_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void imx_fec_cleanup(NetClientState *nc)
> +{
> + imx_fec_state *s = qemu_get_nic_opaque(nc);
> +
> + s->nic = NULL;
> +}
> +
> +static NetClientInfo net_imx_fec_info = {
> + .type = NET_CLIENT_OPTIONS_KIND_NIC,
> + .size = sizeof(NICState),
> + .can_receive = imx_fec_can_receive,
> + .receive = imx_fec_receive,
> + .cleanup = imx_fec_cleanup,
> + .link_status_changed = imx_fec_set_link,
> +};
> +
> +static int imx_fec_init(SysBusDevice *dev)
> +{
> + imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
> +
> + memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
> + sysbus_init_mmio(dev, &s->iomem);
> + sysbus_init_irq(dev, &s->irq);
> + qemu_macaddr_default_if_unset(&s->conf.macaddr);
> +
> + s->conf.peers.ncs[0] = nd_table[0].netdev;
> +
> + s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
> + object_get_typename(OBJECT(dev)), dev->qdev.id,
> + s);
> + qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> +
> + return 0;
> +}
> +
> +static Property imx_fec_properties[] = {
> + DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void imx_fec_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = imx_fec_init;
> + dc->reset = imx_fec_reset;
> + dc->vmsd = &vmstate_imx_fec;
> + dc->props = imx_fec_properties;
> +}
> +
> +static const TypeInfo imx_fec_info = {
> + .name = "fec",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(imx_fec_state),
> + .class_init = imx_fec_class_init,
> +};
> +
> +static void imx_fec_register_types(void)
> +{
> + type_register_static(&imx_fec_info);
> +}
> +
> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
> +{
> + NICInfo *nd;
> + DeviceState *dev;
> + SysBusDevice *s;
> +
> + if (nic >= MAX_NICS) {
> + hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
> + nic, MAX_NICS);
> + }
> +
> + nd = &nd_table[nic];
> +
> + qemu_check_nic_model(nd, "fec");
> + dev = qdev_create(NULL, "fec");
> + qdev_set_nic_properties(dev, nd);
> + qdev_init_nofail(dev);
Let's not bother with legacy -net support
for new devices.
Anyone who wants it can create it with the new style
-device flag.
> + s = SYS_BUS_DEVICE(dev);
> + sysbus_mmio_map(s, 0, base);
> + sysbus_connect_irq(s, 0, irq);
> +};
> +
> +type_init(imx_fec_register_types)
> --
> 1.8.1.2
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 17:49 ` Michael S. Tsirkin
@ 2013-05-05 18:01 ` Peter Maydell
2013-05-05 21:15 ` Michael S. Tsirkin
0 siblings, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2013-05-05 18:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: peter.crosthwaite, peter.chubb, qemu-devel, afaerber,
Jean-Christophe DUBOIS
On 5 May 2013 18:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> Let's not bother with legacy -net support
> for new devices.
>
> Anyone who wants it can create it with the new style
> -device flag.
Sorry, you can't say this until we've sorted out the mess
that is new-style networking options in a machine which
creates embedded network controllers.
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 18:01 ` Peter Maydell
@ 2013-05-05 21:15 ` Michael S. Tsirkin
2013-05-05 22:00 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Michael S. Tsirkin @ 2013-05-05 21:15 UTC (permalink / raw)
To: Peter Maydell
Cc: peter.crosthwaite, Jean-Christophe DUBOIS, peter.chubb,
qemu-devel, afaerber
On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
> On 5 May 2013 18:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Let's not bother with legacy -net support
> > for new devices.
> >
> > Anyone who wants it can create it with the new style
> > -device flag.
>
> Sorry, you can't say this until we've sorted out the mess
> that is new-style networking options in a machine which
> creates embedded network controllers.
>
> -- PMM
What is missing exactly?
Could you please give some examples of the problems
that -netdev + -device has but -net does not have?
--
MST
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 21:15 ` Michael S. Tsirkin
@ 2013-05-05 22:00 ` Peter Maydell
2013-05-06 8:51 ` Michael S. Tsirkin
0 siblings, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2013-05-05 22:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: peter.crosthwaite, Jean-Christophe DUBOIS, peter.chubb,
qemu-devel, afaerber
On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
>> Sorry, you can't say this until we've sorted out the mess
>> that is new-style networking options in a machine which
>> creates embedded network controllers.
> What is missing exactly?
> Could you please give some examples of the problems
> that -netdev + -device has but -net does not have?
-netdev + -device is fine (unsurprisingly since that's the
PC usecase); -netdev + a device that's preinstantiated by the
board is not so fine. And you can't use -device to instantiate
most embedded network controllers because there's no way to
wire up the IRQs and MMIOs.
There's probably a nasty workaround involving '-global', but:
* that requires the user to know the device name for the
onboard NIC for the board, which is a regression from
the -net situation
* it's not clear how it works if the board has two NICs
of the same type
* if we claim -global is the right approach we need to actually
document it (and document all the board NIC names, yuck)
* we need to fix existing boards which do the "don't instantiate
NIC unless the user said -net nic" trick by looking at
nd_table[]
* we need to make the board code pass the right NIC properties
in both the "legacy -net option" and "new style" cases (at the
moment, for instance, lan911_init() insists on having a
NICInfo* passed to it)
-net nic works for these use cases because it will operate on
the NICs created by the machine models, because the machine
models look at the nd_table[] when they create the NICs.
thanks
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-05 22:00 ` Peter Maydell
@ 2013-05-06 8:51 ` Michael S. Tsirkin
2013-05-06 9:08 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Michael S. Tsirkin @ 2013-05-06 8:51 UTC (permalink / raw)
To: Peter Maydell
Cc: peter.crosthwaite, Jean-Christophe DUBOIS, peter.chubb,
qemu-devel, afaerber
On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
> >> Sorry, you can't say this until we've sorted out the mess
> >> that is new-style networking options in a machine which
> >> creates embedded network controllers.
>
> > What is missing exactly?
> > Could you please give some examples of the problems
> > that -netdev + -device has but -net does not have?
>
> -netdev + -device is fine (unsurprisingly since that's the
> PC usecase); -netdev + a device that's preinstantiated by the
> board is not so fine. And you can't use -device to instantiate
> most embedded network controllers because there's no way to
> wire up the IRQs and MMIOs.
Can't board code look for instanciated controllers
and wire them up?
>
> There's probably a nasty workaround involving '-global', but:
> * that requires the user to know the device name for the
> onboard NIC for the board, which is a regression from
> the -net situation
> * it's not clear how it works if the board has two NICs
> of the same type
How does it work now?
I am guessing each -net nic gets mapped to a random device.
At some level that's worse than documenting about internal names,
we are teaching users to learn order of initialization
by trial and error and then rely on this.
> * if we claim -global is the right approach we need to actually
> document it (and document all the board NIC names, yuck)
> * we need to fix existing boards which do the "don't instantiate
> NIC unless the user said -net nic" trick by looking at
> nd_table[]
> * we need to make the board code pass the right NIC properties
> in both the "legacy -net option" and "new style" cases (at the
> moment, for instance, lan911_init() insists on having a
> NICInfo* passed to it)
>
> -net nic works for these use cases because it will operate on
> the NICs created by the machine models, because the machine
> models look at the nd_table[] when they create the NICs.
>
> thanks
> -- PMM
--
MST
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-06 8:51 ` Michael S. Tsirkin
@ 2013-05-06 9:08 ` Peter Maydell
2013-05-06 9:24 ` Michael S. Tsirkin
0 siblings, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2013-05-06 9:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, peter.crosthwaite,
Jean-Christophe DUBOIS, peter.chubb, afaerber
[cc'd Anthony since this has drifted into a more general topic]
On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
>> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
>> >> Sorry, you can't say this until we've sorted out the mess
>> >> that is new-style networking options in a machine which
>> >> creates embedded network controllers.
>>
>> > What is missing exactly?
>> > Could you please give some examples of the problems
>> > that -netdev + -device has but -net does not have?
>>
>> -netdev + -device is fine (unsurprisingly since that's the
>> PC usecase); -netdev + a device that's preinstantiated by the
>> board is not so fine. And you can't use -device to instantiate
>> most embedded network controllers because there's no way to
>> wire up the IRQs and MMIOs.
>
> Can't board code look for instanciated controllers
> and wire them up?
I don't think this will work, because -device does both
'instance_init' and 'realize', and some of the things the
board needs to set and wire up must be done before 'realize'.
>> There's probably a nasty workaround involving '-global', but:
>> * that requires the user to know the device name for the
>> onboard NIC for the board, which is a regression from
>> the -net situation
>> * it's not clear how it works if the board has two NICs
>> of the same type
>
> How does it work now?
> I am guessing each -net nic gets mapped to a random device.
> At some level that's worse than documenting about internal names,
> we are teaching users to learn order of initialization
> by trial and error and then rely on this.
Well, it gets mapped to a specific device (hopefully we pick
the same order as the kernel so first nic is eth0, second
is eth1, and so on). This isn't a question of initialization
order, because you can happily initialize the NIC corresponding
to nd_table[1] before the one for nd_table[0] if you like.
It's just a matter of picking which bit of hardware we call
the "first" ethernet device, in the same way that we pick
one of two serial ports to call the "first" serial port.
thanks
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-06 9:08 ` Peter Maydell
@ 2013-05-06 9:24 ` Michael S. Tsirkin
2013-05-06 12:01 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Michael S. Tsirkin @ 2013-05-06 9:24 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, qemu-devel, peter.crosthwaite,
Jean-Christophe DUBOIS, peter.chubb, afaerber
On Mon, May 06, 2013 at 10:08:42AM +0100, Peter Maydell wrote:
> [cc'd Anthony since this has drifted into a more general topic]
>
> On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
> >> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
> >> >> Sorry, you can't say this until we've sorted out the mess
> >> >> that is new-style networking options in a machine which
> >> >> creates embedded network controllers.
> >>
> >> > What is missing exactly?
> >> > Could you please give some examples of the problems
> >> > that -netdev + -device has but -net does not have?
> >>
> >> -netdev + -device is fine (unsurprisingly since that's the
> >> PC usecase); -netdev + a device that's preinstantiated by the
> >> board is not so fine. And you can't use -device to instantiate
> >> most embedded network controllers because there's no way to
> >> wire up the IRQs and MMIOs.
> >
> > Can't board code look for instanciated controllers
> > and wire them up?
>
> I don't think this will work, because -device does both
> 'instance_init' and 'realize', and some of the things the
> board needs to set and wire up must be done before 'realize'.
Well let's add a flag that tells QM to delay realize then?
It's not "abstract" but maybe "embedded" type?
> >> There's probably a nasty workaround involving '-global', but:
> >> * that requires the user to know the device name for the
> >> onboard NIC for the board, which is a regression from
> >> the -net situation
> >> * it's not clear how it works if the board has two NICs
> >> of the same type
> >
> > How does it work now?
> > I am guessing each -net nic gets mapped to a random device.
> > At some level that's worse than documenting about internal names,
> > we are teaching users to learn order of initialization
> > by trial and error and then rely on this.
>
> Well, it gets mapped to a specific device (hopefully we pick
> the same order as the kernel so first nic is eth0, second
> is eth1, and so on). This isn't a question of initialization
> order, because you can happily initialize the NIC corresponding
> to nd_table[1] before the one for nd_table[0] if you like.
> It's just a matter of picking which bit of hardware we call
> the "first" ethernet device, in the same way that we pick
> one of two serial ports to call the "first" serial port.
>
> thanks
> -- PMM
In other words, it's an undocumented hack :(
Scary as it sounds, for this case I like documenting
internal names better.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-06 9:24 ` Michael S. Tsirkin
@ 2013-05-06 12:01 ` Peter Maydell
2013-05-07 0:39 ` Peter Crosthwaite
0 siblings, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2013-05-06 12:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, peter.crosthwaite,
Jean-Christophe DUBOIS, peter.chubb, afaerber
On 6 May 2013 10:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, May 06, 2013 at 10:08:42AM +0100, Peter Maydell wrote:
>> On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
>> >> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
>> > Can't board code look for instanciated controllers
>> > and wire them up?
>>
>> I don't think this will work, because -device does both
>> 'instance_init' and 'realize', and some of the things the
>> board needs to set and wire up must be done before 'realize'.
>
> Well let's add a flag that tells QM to delay realize then?
> It's not "abstract" but maybe "embedded" type?
This still requires users to know what their board's NIC
happens to be, and how do you match up the half-finished
thing created with -device to the device that the board
creates later?
>> >> There's probably a nasty workaround involving '-global', but:
>> >> * that requires the user to know the device name for the
>> >> onboard NIC for the board, which is a regression from
>> >> the -net situation
>> >> * it's not clear how it works if the board has two NICs
>> >> of the same type
>> >
>> > How does it work now?
>> > I am guessing each -net nic gets mapped to a random device.
>> > At some level that's worse than documenting about internal names,
>> > we are teaching users to learn order of initialization
>> > by trial and error and then rely on this.
>>
>> Well, it gets mapped to a specific device (hopefully we pick
>> the same order as the kernel so first nic is eth0, second
>> is eth1, and so on). This isn't a question of initialization
>> order, because you can happily initialize the NIC corresponding
>> to nd_table[1] before the one for nd_table[0] if you like.
>> It's just a matter of picking which bit of hardware we call
>> the "first" ethernet device, in the same way that we pick
>> one of two serial ports to call the "first" serial port.
> In other words, it's an undocumented hack :(
> Scary as it sounds, for this case I like documenting
> internal names better.
How does that work when both internal NICs are the same kind
of device?
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-06 12:01 ` Peter Maydell
@ 2013-05-07 0:39 ` Peter Crosthwaite
2013-05-07 9:03 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Peter Crosthwaite @ 2013-05-07 0:39 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, Michael S. Tsirkin, qemu-devel,
Jean-Christophe DUBOIS, peter.chubb, afaerber
Hi Peter, Michael,
On Mon, May 6, 2013 at 10:01 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 May 2013 10:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Mon, May 06, 2013 at 10:08:42AM +0100, Peter Maydell wrote:
>>> On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> > On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
>>> >> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> >> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
>
>>> > Can't board code look for instanciated controllers
>>> > and wire them up?
>>>
>>> I don't think this will work, because -device does both
>>> 'instance_init' and 'realize', and some of the things the
>>> board needs to set and wire up must be done before 'realize'.
>>
>> Well let's add a flag that tells QM to delay realize then?
>> It's not "abstract" but maybe "embedded" type?
>
This seems fundamentally flawed to me. -device should create a new
device to the users specification, whereas this flow will create a new
device to user specification but then let a machine model modify as it
sees fit.
> This still requires users to know what their board's NIC
> happens to be,
Which is ugly detail the user should not have to care about.
> and how do you match up the half-finished
> thing created with -device to the device that the board
> creates later?
>
There may also be cases where machine model want to create a NIC
regardless of whether its used or not. Relevant for sysbus NICs as we
don't have the luxury of a PCI probe process so a generic guest (e.g.
a kernel and its pre-canned dtb) may assume a NIC exists and crash if
the sysbus device is not there. I'm half tempted to pull out the
nb_nics conditionals on Zynqs NIC creation for this very reason.
Bottom line is we shouldn't have to rely on a -device or -net arg at
all to get a NIC.
>>> >> There's probably a nasty workaround involving '-global', but:
>>> >> * that requires the user to know the device name for the
>>> >> onboard NIC for the board, which is a regression from
>>> >> the -net situation
>>> >> * it's not clear how it works if the board has two NICs
>>> >> of the same type
>>> >
>>> > How does it work now?
>>> > I am guessing each -net nic gets mapped to a random device.
>>> > At some level that's worse than documenting about internal names,
>>> > we are teaching users to learn order of initialization
>>> > by trial and error and then rely on this.
>>>
>>> Well, it gets mapped to a specific device (hopefully we pick
>>> the same order as the kernel so first nic is eth0, second
>>> is eth1, and so on). This isn't a question of initialization
>>> order, because you can happily initialize the NIC corresponding
>>> to nd_table[1] before the one for nd_table[0] if you like.
>>> It's just a matter of picking which bit of hardware we call
>>> the "first" ethernet device, in the same way that we pick
>>> one of two serial ports to call the "first" serial port.
>
>> In other words, it's an undocumented hack :(
>> Scary as it sounds, for this case I like documenting
>> internal names better.
+1 and give machine-model created NICs a reasonable naming scheme.
Could we also expose the names to the monitor somehow so they can be
looked up easily?
>
> How does that work when both internal NICs are the same kind
> of device?
>
Sanitize the naming scheme:
candence_gem.0 and cadence_gem.1 or something for Zynqs two NICs.
Regards,
Peter
> -- PMM
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver
2013-05-07 0:39 ` Peter Crosthwaite
@ 2013-05-07 9:03 ` Peter Maydell
0 siblings, 0 replies; 35+ messages in thread
From: Peter Maydell @ 2013-05-07 9:03 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Anthony Liguori, Michael S. Tsirkin, qemu-devel,
Jean-Christophe DUBOIS, peter.chubb, afaerber
On 7 May 2013 01:39, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> There may also be cases where machine model want to create a NIC
> regardless of whether its used or not. Relevant for sysbus NICs as we
> don't have the luxury of a PCI probe process so a generic guest (e.g.
> a kernel and its pre-canned dtb) may assume a NIC exists and crash if
> the sysbus device is not there. I'm half tempted to pull out the
> nb_nics conditionals on Zynqs NIC creation for this very reason.
> Bottom line is we shouldn't have to rely on a -device or -net arg at
> all to get a NIC.
I agree here -- we should just always create all the
hardware the embedded board has. I think the nb_nics conditional
stuff is legacy (not all board models do it).
>>> In other words, it's an undocumented hack :(
>>> Scary as it sounds, for this case I like documenting
>>> internal names better.
>
> +1 and give machine-model created NICs a reasonable naming scheme.
> Could we also expose the names to the monitor somehow so they can be
> looked up easily?
This is basically asking for -global to work on instance
names rather than class names, I think. Sounds like a
reasonable idea.
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2013-05-07 9:16 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-04 14:09 [Qemu-devel] [PATCH v2 0/4] Add i.MX25 support through the 3DS evaluation board Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 1/4] Add i.MX FEC Ethernet driver Jean-Christophe DUBOIS
2013-05-05 3:11 ` Peter Crosthwaite
2013-05-05 11:46 ` Andreas Färber
2013-05-05 11:59 ` Peter Crosthwaite
2013-05-05 12:41 ` Peter Maydell
2013-05-05 13:14 ` Jean-Christophe DUBOIS
2013-05-05 13:31 ` Andreas Färber
2013-05-05 14:05 ` Jean-Christophe DUBOIS
2013-05-05 17:49 ` Michael S. Tsirkin
2013-05-05 18:01 ` Peter Maydell
2013-05-05 21:15 ` Michael S. Tsirkin
2013-05-05 22:00 ` Peter Maydell
2013-05-06 8:51 ` Michael S. Tsirkin
2013-05-06 9:08 ` Peter Maydell
2013-05-06 9:24 ` Michael S. Tsirkin
2013-05-06 12:01 ` Peter Maydell
2013-05-07 0:39 ` Peter Crosthwaite
2013-05-07 9:03 ` Peter Maydell
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver Jean-Christophe DUBOIS
2013-05-05 3:14 ` Peter Crosthwaite
2013-05-05 3:58 ` Jean-Christophe DUBOIS
2013-05-05 10:47 ` Peter Maydell
2013-05-05 10:53 ` Peter Crosthwaite
2013-05-05 11:41 ` Peter Maydell
2013-05-05 11:53 ` Peter Crosthwaite
2013-05-05 12:28 ` Jean-Christophe DUBOIS
2013-05-05 11:34 ` Andreas Färber
2013-05-05 12:34 ` Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 3/4] Add i.MX25 3DS evaluation board support Jean-Christophe DUBOIS
2013-05-04 14:09 ` [Qemu-devel] [PATCH v2 4/4] Add qtest support for i.MX I2C device emulation Jean-Christophe DUBOIS
2013-05-04 16:53 ` Andreas Färber
2013-05-04 19:02 ` Jean-Christophe DUBOIS
2013-05-04 19:48 ` [Qemu-devel] compile the latest source in mac error Peter Cheung
2013-05-04 20:51 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).