[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [[Qemu devel] RFC] hw/net: Add Smartfusion2 emac block
From: |
sundeep subbaraya |
Subject: |
Re: [Qemu-devel] [[Qemu devel] RFC] hw/net: Add Smartfusion2 emac block |
Date: |
Wed, 30 May 2018 18:43:12 +0530 |
Hi Peter,
On Tue, May 29, 2018 at 10:13 PM, Peter Maydell
<address@hidden> wrote:
> On 26 May 2018 at 10:51, Subbaraya Sundeep <address@hidden> 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 <address@hidden>
>
> 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 <address@hidden>.
>> + *
>> + * 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