Hi Peter, On Tue, May 29, 2018 at 10:13 PM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 26 May 2018 at 10:51, Subbaraya Sundeep <sundeep.l...@gmail.com> wrote: >> Modelled Ethernet MAC of Smartfusion2 SoC. >> Micrel KSZ8051 PHY is present on Emcraft's SOM kit hence same >> PHY is emulated. >> >> Signed-off-by: Subbaraya Sundeep <sundeep.l...@gmail.com> > > Hi; thanks for this patch. I have some review comments, but they're > fairly minor things; this generally looks good. I think you could > drop the RFC tag for your next version. > >> --- >> hw/arm/msf2-soc.c | 21 +- >> hw/net/Makefile.objs | 1 + >> hw/net/mss-emac.c | 544 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/arm/msf2-soc.h | 3 + >> include/hw/net/mss-emac.h | 23 ++ > > I think it would be better to split this into two patches, where > patch 1 implements the new device, and then patch 2 adds an > instance of that device to the msf2-soc. (For multi-patch sets, > remember to include a cover letter.)
Ok I will split into multiple patches. > >> 5 files changed, 591 insertions(+), 1 deletion(-) >> create mode 100644 hw/net/mss-emac.c >> create mode 100644 include/hw/net/mss-emac.h >> >> diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c >> index 75c44ad..ed3d0f5 100644 >> --- a/hw/arm/msf2-soc.c >> +++ b/hw/arm/msf2-soc.c >> @@ -35,6 +35,7 @@ >> >> #define MSF2_TIMER_BASE 0x40004000 >> #define MSF2_SYSREG_BASE 0x40038000 >> +#define MSF2_EMAC_BASE 0x40041000 >> >> #define ENVM_BASE_ADDRESS 0x60000000 >> >> @@ -55,6 +56,7 @@ static const uint32_t uart_addr[MSF2_NUM_UARTS] = { >> 0x40000000 , 0x40010000 }; >> static const int spi_irq[MSF2_NUM_SPIS] = { 2, 3 }; >> static const int uart_irq[MSF2_NUM_UARTS] = { 10, 11 }; >> static const int timer_irq[MSF2_NUM_TIMERS] = { 14, 15 }; >> +static const int emac_irq[MSF2_NUM_EMACS] = { 12 }; >> >> static void do_sys_reset(void *opaque, int n, int level) >> { >> @@ -82,6 +84,13 @@ static void m2sxxx_soc_initfn(Object *obj) >> TYPE_MSS_SPI); >> qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default()); >> } >> + >> + object_initialize(&s->emac, sizeof(s->emac), TYPE_MSS_EMAC); >> + qdev_set_parent_bus(DEVICE(&s->emac), sysbus_get_default()); >> + if (nd_table[0].used) { >> + qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC); >> + qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]); >> + } >> } >> >> static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) >> @@ -192,6 +201,17 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, >> Error **errp) >> g_free(bus_name); >> } >> >> + dev = DEVICE(&s->emac); >> + object_property_set_bool(OBJECT(&s->emac), true, "realized", &err); >> + if (err != NULL) { >> + error_propagate(errp, err); >> + return; >> + } >> + busdev = SYS_BUS_DEVICE(dev); >> + sysbus_mmio_map(busdev, 0, MSF2_EMAC_BASE); >> + sysbus_connect_irq(busdev, 0, >> + qdev_get_gpio_in(armv7m, emac_irq[0])); >> + >> /* Below devices are not modelled yet. */ >> create_unimplemented_device("i2c_0", 0x40002000, 0x1000); >> create_unimplemented_device("dma", 0x40003000, 0x1000); >> @@ -202,7 +222,6 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, >> Error **errp) >> create_unimplemented_device("can", 0x40015000, 0x1000); >> create_unimplemented_device("rtc", 0x40017000, 0x1000); >> create_unimplemented_device("apb_config", 0x40020000, 0x10000); >> - create_unimplemented_device("emac", 0x40041000, 0x1000); >> create_unimplemented_device("usb", 0x40043000, 0x1000); >> } >> >> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs >> index ab22968..d9b4cae 100644 >> --- a/hw/net/Makefile.objs >> +++ b/hw/net/Makefile.objs >> @@ -48,3 +48,4 @@ common-obj-$(CONFIG_ROCKER) += rocker/rocker.o >> rocker/rocker_fp.o \ >> obj-$(call lnot,$(CONFIG_ROCKER)) += rocker/qmp-norocker.o >> >> common-obj-$(CONFIG_CAN_BUS) += can/ >> +common-obj-$(CONFIG_MSF2) += mss-emac.o >> diff --git a/hw/net/mss-emac.c b/hw/net/mss-emac.c >> new file mode 100644 >> index 0000000..a9588c0 >> --- /dev/null >> +++ b/hw/net/mss-emac.c >> @@ -0,0 +1,544 @@ >> +/* >> + * QEMU model of the Smartfusion2 Ethernet MAC. >> + * >> + * Copyright (c) 2018 Subbaraya Sundeep <sundeep.l...@gmail.com>. >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> copy >> + * of this software and associated documentation files (the "Software"), to >> deal >> + * in the Software without restriction, including without limitation the >> rights >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell >> + * copies of the Software, and to permit persons to whom the Software is >> + * furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be included >> in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >> + * THE SOFTWARE. >> + */ > > Is there a datasheet for this hardware? If so, this is a good place for a > comment giving a URL and title for it. > Ok I will add URL for datasheet. >> + >> +#include "qemu/osdep.h" >> +#include "qemu-common.h" >> +#include "qemu/log.h" >> +#include "hw/net/mss-emac.h" >> +#include "hw/net/mii.h" >> + >> +#define R_CFG1 (0x0 / 4) >> +#define R_CFG2 (0x4 / 4) >> +#define R_IFG (0x8 / 4) >> +#define R_HALF_DUPLEX (0xc / 4) >> +#define R_FRM_LEN (0x10 / 4) >> +#define R_MII_CFG (0x20 / 4) >> +#define R_MII_CMD (0x24 / 4) >> +#define R_MII_ADDR (0x28 / 4) >> +#define R_MII_CTL (0x2c / 4) >> +#define R_MII_STS (0x30 / 4) >> +#define R_MII_IND (0x34 / 4) >> +#define R_STA1 (0x40 / 4) >> +#define R_STA2 (0x44 / 4) >> +#define R_FIFO_CFG0 (0x48 / 4) >> + >> +#define R_DMA_TX_CTL (0x180 / 4) >> +#define R_DMA_TX_DESC (0x184 / 4) >> +#define R_DMA_TX_STATUS (0x188 / 4) >> +#define R_DMA_RX_CTL (0x18c / 4) >> +#define R_DMA_RX_DESC (0x190 / 4) >> +#define R_DMA_RX_STATUS (0x194 / 4) >> +#define R_DMA_IRQ_MASK (0x198 / 4) >> +#define R_DMA_IRQ (0x19c / 4) >> + >> +#define R_DMA_PKTCNT_MASK 0xFF0000 >> +#define R_DMA_PKTCNT_SHIFT 16 >> +#define R_DMA_PKT_TXRX (1 << 0) >> +#define DMA_TX_UNDERRUN (1 << 1) >> +#define DMA_RX_OVERFLOW (1 << 2) >> + >> +#define EMPTY_MASK (1 << 31) >> +#define PKT_SIZE 0x7FF >> + >> +#define CFG1_RESET (1 << 31) >> + >> +#define FIFO_CFG0_FTFENRPLY (1 << 20) >> +#define FIFO_CFG0_STFENRPLY (1 << 19) >> +#define FIFO_CFG0_FRFENRPLY (1 << 18) >> +#define FIFO_CFG0_SRFENRPLY (1 << 17) >> +#define FIFO_CFG0_WTMENRPLY (1 << 16) >> +#define FIFO_CFG0_FTFENREQ (1 << 12) >> +#define FIFO_CFG0_STFENREQ (1 << 11) >> +#define FIFO_CFG0_FRFENREQ (1 << 10) >> +#define FIFO_CFG0_SRFENREQ (1 << 9) >> +#define FIFO_CFG0_WTMENREQ (1 << 8) >> + >> +#define DMA_TX_CTL_EN (1 << 0) >> +#define DMA_RX_CTL_EN (1 << 0) >> + >> +#define MII_CMD_READ (1 << 0) >> + >> +/* We emulate PHY at address 0x1 */ >> +#define PHYADDR 0x1 >> +#define MII_ADDR_MASK 0x1F >> +#define PHY_ADDR_SHIFT 8 >> + >> +typedef struct { >> + uint32_t PktAddr; >> + uint32_t PktSize; >> + uint32_t Next; > > Our coding standard suggests that struct field names > should be lowercase. > Ok will change. >> +} EmacDesc; >> + >> +static uint32_t emac_get_isr(MSSEmacState *s) >> +{ >> + uint32_t ier = s->regs[R_DMA_IRQ_MASK]; >> + uint32_t tx = s->regs[R_DMA_TX_STATUS] & 0xF; >> + uint32_t rx = s->regs[R_DMA_RX_STATUS] & 0xF; >> + uint32_t isr = (rx << 4) | tx; >> + >> + s->regs[R_DMA_IRQ] = ier & isr; >> + return s->regs[R_DMA_IRQ]; >> +} >> + >> +static void emac_update_irq(MSSEmacState *s) >> +{ >> + bool intr = !!emac_get_isr(s); > > Since you're assigning to a bool the !! is unnecessary here. > Ok. >> + qemu_set_irq(s->irq, intr); >> +} >> + >> +static void emac_load_desc(EmacDesc *d, hwaddr desc) >> +{ >> + cpu_physical_memory_read(desc, d, sizeof *d); > > The address_space_* functions are preferable to > cpu_physical_memory_* here: > * there is likely to be behaviour you need to model correctly > if the read or write fails > * you should really be performing operations on your > own AddressSpace, rather than on the system address space > > (Ideally the device has a property which accepts a MemoryRegion > which it uses to create an AddressSpace for its DMA operations, > which the SoC that uses it can wire up appropriately. Less > ideally, you can use &address_space_memory, which at least lets > you get the read/write failure behaviour right.) > Ok I will look into this. >> + /* Convert from LE into host endianness. */ >> + d->PktAddr = le32_to_cpu(d->PktAddr); >> + d->PktSize = le32_to_cpu(d->PktSize); >> + d->Next = le32_to_cpu(d->Next); >> +} >> + >> +static void emac_store_desc(EmacDesc *d, hwaddr desc) >> +{ >> + /* Convert from host endianness into LE. */ >> + d->PktAddr = cpu_to_le32(d->PktAddr); >> + d->PktSize = cpu_to_le32(d->PktSize); >> + d->Next = cpu_to_le32(d->Next); >> + cpu_physical_memory_write(desc, d, sizeof *d); > > Similarly this should be using the address_space_write() function. > Ok >> +} >> + >> +static void mss_dma_tx(MSSEmacState *s) >> +{ >> + EmacDesc d; >> + uint8_t buf[2 * 1024]; > > It's better to avoid hardcoded constants for things like array > sizes. Should this be something related to PKT_SIZE ? > Ok will define PKT_SIZE. >> + int size; >> + uint8_t pktcnt; >> + uint32_t status; >> + >> + while (1) { >> + emac_load_desc(&d, s->tx_desc); >> + if (d.PktSize & EMPTY_MASK) { >> + break; >> + } >> + size = d.PktSize & PKT_SIZE; >> + cpu_physical_memory_read(d.PktAddr, buf, size); >> + qemu_send_packet(qemu_get_queue(s->nic), buf, size); >> + d.PktSize |= EMPTY_MASK; >> + emac_store_desc(&d, s->tx_desc); >> + /* update sent packets count */ >> + status = s->regs[R_DMA_TX_STATUS]; >> + pktcnt = extract32(status, R_DMA_PKTCNT_SHIFT, 8); >> + pktcnt++; >> + s->regs[R_DMA_TX_STATUS] = ((status & ~R_DMA_PKTCNT_MASK) | >> + (pktcnt << R_DMA_PKTCNT_SHIFT)); >> + s->regs[R_DMA_TX_STATUS] |= R_DMA_PKT_TXRX; >> + s->tx_desc = d.Next; >> + } >> + s->regs[R_DMA_TX_STATUS] |= DMA_TX_UNDERRUN; >> + s->regs[R_DMA_TX_CTL] &= ~DMA_TX_CTL_EN; >> +} >> + >> +static void mss_phy_update_link(MSSEmacState *s) >> +{ >> + /* Autonegotiation status mirrors link status. */ >> + if (qemu_get_queue(s->nic)->link_down) { >> + s->phy_regs[MII_BMSR] &= ~(MII_BMSR_AN_COMP | >> + MII_BMSR_LINK_ST); >> + } else { >> + s->phy_regs[MII_BMSR] |= (MII_BMSR_AN_COMP | >> + MII_BMSR_LINK_ST); >> + } >> +} >> + >> +static void mss_phy_reset(MSSEmacState *s) >> +{ >> + memset(&s->phy_regs[0], 0, sizeof(s->phy_regs)); >> + s->phy_regs[MII_BMSR] = MII_BMSR_AUTONEG | MII_BMSR_MFPS | >> + MII_BMSR_10T_HD | MII_BMSR_10T_FD | >> + MII_BMSR_100TX_HD | MII_BMSR_100TX_FD; >> + /* Micrel KSZ8051 PHY */ >> + s->phy_regs[MII_PHYID1] = 0x0022; >> + s->phy_regs[MII_PHYID2] = 0x1550; >> + >> + s->phy_regs[MII_ANAR] = MII_ANAR_CSMACD | MII_ANLPAR_10 | >> + MII_ANLPAR_10FD | MII_ANLPAR_TX | >> MII_ANLPAR_TXFD; >> + mss_phy_update_link(s); >> +} >> + >> +static void write_to_phy(MSSEmacState *s) >> +{ >> + uint8_t reg_addr = s->regs[R_MII_ADDR] & MII_ADDR_MASK; >> + uint8_t phy_addr = (s->regs[R_MII_ADDR] >> PHY_ADDR_SHIFT) & >> MII_ADDR_MASK; >> + uint16_t data = s->regs[R_MII_CTL] & 0xFFFF; >> + >> + if (phy_addr != PHYADDR) { >> + return; >> + } >> + >> + switch (reg_addr) { >> + case MII_BMCR: >> + if (data & MII_BMCR_RESET) { >> + /* Phy reset */ >> + mss_phy_reset(s); >> + data &= ~MII_BMCR_RESET; >> + } >> + if (data & MII_BMCR_AUTOEN) { >> + /* Complete autonegotiation immediately */ >> + data &= ~MII_BMCR_AUTOEN; >> + s->phy_regs[MII_BMSR] |= MII_BMSR_AN_COMP; >> + } >> + break; >> + } >> + >> + s->phy_regs[reg_addr] = data; >> +} >> + >> +static uint16_t read_from_phy(MSSEmacState *s) >> +{ >> + uint8_t reg_addr = s->regs[R_MII_ADDR] & MII_ADDR_MASK; >> + uint8_t phy_addr = (s->regs[R_MII_ADDR] >> PHY_ADDR_SHIFT) & >> MII_ADDR_MASK; >> + >> + if (phy_addr == PHYADDR) { >> + return s->phy_regs[reg_addr]; >> + } else { >> + return 0xFFFF; >> + } >> +} >> + >> +static void mss_emac_do_reset(MSSEmacState *s) >> +{ >> + memset(&s->regs[0], 0, sizeof(s->regs)); >> + s->regs[R_CFG1] = 0x80000000; >> + s->regs[R_CFG2] = 0x00007000; >> + s->regs[R_IFG] = 0x40605060; >> + s->regs[R_HALF_DUPLEX] = 0x00A1F037; >> + s->regs[R_FRM_LEN] = 0x00000600; >> + s->rx_enabled = false; >> + >> + mss_phy_reset(s); >> +} >> + >> +static uint64_t emac_read(void *opaque, hwaddr addr, unsigned int size) >> +{ >> + MSSEmacState *s = opaque; >> + uint32_t r = 0; >> + >> + addr >>= 2; >> + >> + switch (addr) { >> + case R_DMA_IRQ: >> + r = emac_get_isr(s); >> + break; >> + default: >> + if (addr < ARRAY_SIZE(s->regs)) { >> + r = s->regs[addr]; >> + } else { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, >> + addr * 4); >> + return r; >> + } >> + break; >> + } >> + return r; >> +} >> + >> +static void emac_write(void *opaque, hwaddr addr, uint64_t val64, >> + unsigned int size) >> +{ >> + MSSEmacState *s = opaque; >> + uint32_t value = val64; >> + uint8_t pktcnt; >> + >> + addr >>= 2; >> + switch (addr) { >> + case R_DMA_TX_CTL: >> + s->regs[addr] = value; >> + if (value & DMA_TX_CTL_EN) { >> + mss_dma_tx(s); >> + } >> + break; >> + case R_CFG1: >> + if (value & CFG1_RESET) { >> + mss_emac_do_reset(s); >> + } >> + case R_FIFO_CFG0: >> + if (value & FIFO_CFG0_FTFENREQ) { >> + value |= FIFO_CFG0_FTFENRPLY; >> + } > > Assuming I've found the right datasheet, I don't think this is > the correct behaviour for the *ENRPLY bits: > (1) it allows the guest to write 1 to the *ENRPLY bits, which > should be read-only > (2) if the guest writes 0 to the appropriate *ENREQ bit, this > should result in the *ENRPLY bit going to 0 > > You can fix both of these by using > /* For our implementation, turning on modules is instantaneous, > * so the states requested via the *ENREQ bits appear in the > * *ENRPLY bits immediately. > */ > enreqbits = extract32(value, 8, 5); > value = deposit32(value, 16, 5, enreqbits); > instead of this set of if statements. > deposit32() is nice. I will use it. >> + if (value & FIFO_CFG0_STFENREQ) { >> + value |= FIFO_CFG0_STFENRPLY; >> + } >> + if (value & FIFO_CFG0_FRFENREQ) { >> + value |= FIFO_CFG0_FRFENRPLY; >> + } >> + if (value & FIFO_CFG0_SRFENREQ) { >> + value |= FIFO_CFG0_SRFENRPLY; >> + } >> + if (value & FIFO_CFG0_WTMENREQ) { >> + value |= FIFO_CFG0_WTMENRPLY; >> + } >> + s->regs[addr] = value; > > Aren't there reset bits at the bottom of this register? > There are reset bits for sub modules in HW like MAC, FIFO etc., I dont think we need to emulate those. As a whole, reset is done via R_CFG1 register. I will take a look into this and implement resets here if needed. >> + break; >> + case R_DMA_TX_DESC: >> + if (value & 0x3) { >> + qemu_log_mask(LOG_GUEST_ERROR, "Tx Descriptor address should be" >> + " 32 bit aligned\n"); >> + } >> + /* Ignore [1:0] bits */ >> + s->regs[addr] = value & 0xFFFFFFFC; >> + s->tx_desc = s->regs[addr]; > > Why store this register value in two places? > > There are two styles you can use for register state in a QEMU device: > > option 1 gives each register its own named struct field, so > s->tx_desc, s->dma_tx_ctl, s->cfg1, s_>fifo_cfg0, etc > > option 2 puts all the registers in a regs[] array > > You should pick one and stick to it; if you want to use a regs > array then always refer to the tx desc reg via s->regs[R_DMA_TX_DESC]. > You don't need a second copy in its own named struct field. > > Similarly with rx_desc. >DMA_TX_CTL_ENDMA_TX_CTL_EN tx_desc and rx_desc are variables I used for implementation not registers. Controller keep using next descriptor after current descriptor transfer until register reg[R_DMA_TX_DESC] is updated. The current descriptor is stored in tx_desc. >> + break; >> + case R_DMA_RX_DESC: >> + if (value & 0x3) { >> + qemu_log_mask(LOG_GUEST_ERROR, "Rx Descriptor address should be" >> + " 32 bit aligned\n"); >> + } >> + /* Ignore [1:0] bits */ >> + s->regs[addr] = value & 0xFFFFFFFC; >> + s->rx_desc = s->regs[addr]; >> + break; >> + case R_DMA_TX_STATUS: >> + if (value & DMA_TX_UNDERRUN) { >> + s->regs[addr] &= ~DMA_TX_UNDERRUN; >> + } >> + if (value & R_DMA_PKT_TXRX) { >> + pktcnt = extract32(s->regs[addr], R_DMA_PKTCNT_SHIFT, 8); >> + pktcnt--; >> + s->regs[addr] = ((s->regs[addr] & ~R_DMA_PKTCNT_MASK) | >> + (pktcnt << R_DMA_PKTCNT_SHIFT)); >> + if (pktcnt == 0) { >> + s->regs[addr] &= ~R_DMA_PKT_TXRX; >> + } >> + } >> + break; >> + case R_DMA_RX_STATUS: >> + if (value & DMA_RX_OVERFLOW) { >> + s->regs[addr] &= ~DMA_RX_OVERFLOW; >> + } >> + if (value & R_DMA_PKT_TXRX) { >> + pktcnt = extract32(s->regs[addr], R_DMA_PKTCNT_SHIFT, 8); >> + pktcnt--; >> + s->regs[addr] = ((s->regs[addr] & ~R_DMA_PKTCNT_MASK) | >> + (pktcnt << R_DMA_PKTCNT_SHIFT)); > > You can use deposit32() for this: > s->regs[addr] = deposit32(s->regs[addr], R_DMA_PKTCNT_SHIFT, 8, pktcnt); > Ok > Also consider having a #define for the field size to avoid the > hardcoded '8'. The FIELD() macro in include/hw/registerfields.h > will define R_DMA_PKTCNT_SHIFT/MASK/LENGTH for you: > > FIELD(DMA, PKTCNT, 16, 8) > Ok I will convert #defines to FIELD >> + if (pktcnt == 0) { >> + s->regs[addr] &= ~R_DMA_PKT_TXRX; >> + } >> + } >> + break; >> + case R_DMA_IRQ: >> + break; >> + case R_MII_CMD: >> + if (value & MII_CMD_READ) { >> + s->regs[R_MII_STS] = read_from_phy(s); >> + } >> + break; >> + case R_MII_CTL: >> + s->regs[addr] = value; >> + write_to_phy(s); >> + break; >> + default: >> + if (addr < ARRAY_SIZE(s->regs)) { >> + s->regs[addr] = value; >> + } else { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, >> + addr * 4); >> + return; >> + } >> + break; >> + } >> + emac_update_irq(s); >> +} >> + >> +static const MemoryRegionOps emac_ops = { >> + .read = emac_read, >> + .write = emac_write, >> + .endianness = DEVICE_NATIVE_ENDIAN, >> + .valid = { >> + .min_access_size = 4, >> + .max_access_size = 4 >> + } >> +}; >> + >> +static int emac_can_rx(NetClientState *nc) >> +{ >> + MSSEmacState *s = qemu_get_nic_opaque(nc); >> + >> + return !!(s->regs[R_DMA_RX_CTL] & DMA_RX_CTL_EN); > > When you use device state to decide whether to return false > from your can_receive method, you have to also tell QEMU's > generic net code when the device state has changed and you > can now receive data again. That is, when this bit in > R_DMA_RX_CTL is changed, you need to call > qemu_flush_queued_packets(). (See other network device > emulation for examples.) > Thanks. To be honest I did not get when to use qemu_flush_queued_packets(). I will use it. >> +} >> + >> +static bool match_addr(MSSEmacState *s, const uint8_t *buf) >> +{ >> + /* >> + * R_STA1 [31:24] : octet 1 of mac address >> + * R_STA1 [23:16] : octet 2 of mac address >> + . >> + . > > These lines are missing their leading '*'. > Ok >> + * R_STA2 [31:24] : octet 5 of mac address >> + * R_STA2 [23:16] : octet 6 of mac address >> + */ >> + uint32_t addr1 = bswap32(s->regs[R_STA1]); >> + uint32_t addr2 = s->regs[R_STA2]; >> + uint8_t *firstword = (uint8_t *)&addr1; >> + /* The broadcast MAC address: FF:FF:FF:FF:FF:FF */ >> + static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, >> + 0xFF, 0xFF }; >> + >> + if (!memcmp(buf, broadcast_addr, 6)) { > > You can use sizeof(broadcast_addr) rather than '6' here. > Ok >> + return true; >> + } >> + if (memcmp(firstword, buf, 4)) { >> + return false; >> + } >> + if (buf[4] != ((addr2 >> 24) & 0xff) || >> + buf[5] != ((addr2 >> 16) & 0xff)) { >> + return false; >> + } > > I think this would be a bit clearer written as > uint8_t addr[6]; > stl_be_p(addr, s->regs[R_STA1]); > stw_be_p(addr + 4, s->regs[R_STA2] >> 16); > > and then > if (!memcmp(buf, addr, sizeof(addr))) { > return false; > } > Ok >> + >> + return true; >> +} >> + >> +static ssize_t emac_rx(NetClientState *nc, const uint8_t *buf, size_t size) >> +{ >> + MSSEmacState *s = qemu_get_nic_opaque(nc); >> + EmacDesc d; >> + uint8_t pktcnt; >> + uint32_t status; >> + >> + if (!emac_can_rx(nc)) { >> + return -1; >> + } >> + if (size > (s->regs[R_FRM_LEN] & 0xFFFF)) { >> + return -1; >> + } > > I don't think you want to return -1 for "ethernet device discarded > the packet because it was oversized", but we don't seem to actually > document the required API for NetClientInfo receive functions. Jason? > > Does this device have an error bit or something for oversize packets? > I am working on stats currently will check and implement oversize packets too. >> + if (!match_addr(s, buf)) { >> + return -1; >> + } >> + >> + emac_load_desc(&d, s->rx_desc); >> + >> + if (d.PktSize & EMPTY_MASK) { >> + cpu_physical_memory_write(d.PktAddr, buf, size & PKT_SIZE); >> + d.PktSize = size & PKT_SIZE; >> + emac_store_desc(&d, s->rx_desc); >> + /* update recieved packets count */ > > "received" > Ok >> + status = s->regs[R_DMA_RX_STATUS]; >> + pktcnt = extract32(status, R_DMA_PKTCNT_SHIFT, 8); >> + pktcnt++; >> + s->regs[R_DMA_RX_STATUS] = ((status & ~R_DMA_PKTCNT_MASK) | >> + (pktcnt << R_DMA_PKTCNT_SHIFT)); > > Again, you can use deposit32() here. > Ok >> + s->regs[R_DMA_RX_STATUS] |= R_DMA_PKT_TXRX; >> + s->rx_desc = d.Next; >> + } else { >> + s->regs[R_DMA_RX_CTL] &= ~DMA_RX_CTL_EN; >> + s->regs[R_DMA_RX_STATUS] |= DMA_RX_OVERFLOW; >> + } >> + emac_update_irq(s); >> + return size; >> +} >> + >> +static void mss_emac_reset(DeviceState *dev) >> +{ >> + MSSEmacState *s = MSS_EMAC(dev); >> + >> + mss_emac_do_reset(s); >> +} >> + >> +static void emac_set_link(NetClientState *nc) >> +{ >> + MSSEmacState *s = qemu_get_nic_opaque(nc); >> + >> + mss_phy_update_link(s); >> +} >> + >> +static NetClientInfo net_mss_emac_info = { >> + .type = NET_CLIENT_DRIVER_NIC, >> + .size = sizeof(NICState), >> + .can_receive = emac_can_rx, >> + .receive = emac_rx, >> + .link_status_changed = emac_set_link, >> +}; >> + >> +static void mss_emac_realize(DeviceState *dev, Error **errp) >> +{ >> + MSSEmacState *s = MSS_EMAC(dev); >> + >> + qemu_macaddr_default_if_unset(&s->conf.macaddr); >> + s->nic = qemu_new_nic(&net_mss_emac_info, &s->conf, >> + object_get_typename(OBJECT(dev)), dev->id, s); >> + qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a); >> +} >> + >> +static void mss_emac_init(Object *obj) >> +{ >> + MSSEmacState *s = MSS_EMAC(obj); >> + >> + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq); >> + >> + memory_region_init_io(&s->mmio, obj, &emac_ops, s, >> + "mss-emac", R_MAX * 4); >> + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio); >> +} >> + >> +static Property mss_emac_properties[] = { >> + DEFINE_NIC_PROPERTIES(MSSEmacState, conf), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static const VMStateDescription vmstate_mss_emac = { >> + .name = TYPE_MSS_EMAC, >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_BOOL(rx_enabled, MSSEmacState), >> + VMSTATE_UINT32(rx_desc, MSSEmacState), >> + VMSTATE_UINT16_ARRAY(phy_regs, MSSEmacState, 32), >> + VMSTATE_UINT32_ARRAY(regs, MSSEmacState, R_MAX), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +static void mss_emac_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->realize = mss_emac_realize; >> + dc->reset = mss_emac_reset; >> + dc->props = mss_emac_properties; >> + dc->vmsd = &vmstate_mss_emac; >> +} >> + >> +static const TypeInfo mss_emac_info = { >> + .name = TYPE_MSS_EMAC, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(MSSEmacState), >> + .instance_init = mss_emac_init, >> + .class_init = mss_emac_class_init, >> +}; >> + >> +static void mss_emac_register_types(void) >> +{ >> + type_register_static(&mss_emac_info); >> +} >> + >> +type_init(mss_emac_register_types) >> diff --git a/include/hw/arm/msf2-soc.h b/include/hw/arm/msf2-soc.h >> index 3cfe5c7..3e3f968 100644 >> --- a/include/hw/arm/msf2-soc.h >> +++ b/include/hw/arm/msf2-soc.h >> @@ -29,12 +29,14 @@ >> #include "hw/timer/mss-timer.h" >> #include "hw/misc/msf2-sysreg.h" >> #include "hw/ssi/mss-spi.h" >> +#include "hw/net/mss-emac.h" >> >> #define TYPE_MSF2_SOC "msf2-soc" >> #define MSF2_SOC(obj) OBJECT_CHECK(MSF2State, (obj), TYPE_MSF2_SOC) >> >> #define MSF2_NUM_SPIS 2 >> #define MSF2_NUM_UARTS 2 >> +#define MSF2_NUM_EMACS 1 >> >> /* >> * System timer consists of two programmable 32-bit >> @@ -62,6 +64,7 @@ typedef struct MSF2State { >> MSF2SysregState sysreg; >> MSSTimerState timer; >> MSSSpiState spi[MSF2_NUM_SPIS]; >> + MSSEmacState emac; >> } MSF2State; >> >> #endif >> diff --git a/include/hw/net/mss-emac.h b/include/hw/net/mss-emac.h >> new file mode 100644 >> index 0000000..d1931e8 >> --- /dev/null >> +++ b/include/hw/net/mss-emac.h >> @@ -0,0 +1,23 @@ > > As Philippe says, this new file should start with a copyright/license comment. > Ok >> +#include "hw/sysbus.h" >> +#include "net/net.h" >> + >> +#define TYPE_MSS_EMAC "mss-emac" >> +#define MSS_EMAC(obj) \ >> + OBJECT_CHECK(MSSEmacState, (obj), TYPE_MSS_EMAC) >> + >> +#define R_MAX (0x1a0 / 4) >> + >> +typedef struct MSSEmacState { >> + SysBusDevice parent; >> + >> + MemoryRegion mmio; >> + qemu_irq irq; >> + NICState *nic; >> + NICConf conf; >> + uint32_t tx_desc; >> + uint32_t rx_desc; >> + bool rx_enabled; > > The rx_enabled field doesn't seem to be used anywhere ? > Yes not used but forgot to delete it. >> + uint16_t phy_regs[32]; > > A #define for the number of phy regs could be used here and in the > vmstate struct definition. > Ok Thank you, Sundeep >> + >> + uint32_t regs[R_MAX]; >> +} MSSEmacState; >> -- >> 2.5.0 >> > > thanks > -- PMM