On 12 July 2016 at 00:20, Alistair Francis <alistair.fran...@xilinx.com> wrote: > Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com> > --- > > There is a indentation error in this patch in the gem_transmit function. > I have written it like that to make it easier to see the changes. It is > fixed in the next patch. > > hw/net/cadence_gem.c | 157 > ++++++++++++++++++++++++++++++++----------- > include/hw/net/cadence_gem.h | 2 +- > 2 files changed, 118 insertions(+), 41 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > index 2dabad5..c80e833 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -141,6 +141,29 @@ > #define GEM_DESCONF6 (0x00000294/4) > #define GEM_DESCONF7 (0x00000298/4) > > +#define GEM_INT_Q1_STATUS (0x00000400 / 4) > + > +#define GEM_TRANSMIT_Q1_PTR (0x00000440 / 4) > +#define GEM_TRANSMIT_Q15_PTR (GEM_TRANSMIT_Q1_PTR + 14) > + > +#define GEM_RECEIVE_Q1_PTR (0x00000480 / 4) > +#define GEM_RECEIVE_Q15_PTR (GEM_RECEIVE_Q1_PTR + 14) > + > +#define GEM_INT_Q1_ENABLE (0x00000600 / 4) > +#define GEM_INT_Q7_ENABLE (GEM_INT_Q1_ENABLE + 6) > +#define GEM_INT_Q8_ENABLE (0x00000660 / 4) > +#define GEM_INT_Q15_ENABLE (GEM_INT_Q8_ENABLE + 7) > + > +#define GEM_INT_Q1_DISABLE (0x00000620 / 4) > +#define GEM_INT_Q7_DISABLE (GEM_INT_Q1_DISABLE + 6) > +#define GEM_INT_Q8_DISABLE (0x00000680 / 4) > +#define GEM_INT_Q15_DISABLE (GEM_INT_Q8_DISABLE + 7) > + > +#define GEM_INT_Q1_MASK (0x00000640 / 4) > +#define GEM_INT_Q7_MASK (GEM_INT_Q1_MASK + 6) > +#define GEM_INT_Q8_MASK (0x000006A0 / 4) > +#define GEM_INT_Q15_MASK (GEM_INT_Q8_MASK + 7) > + > /*****************************************/ > #define GEM_NWCTRL_TXSTART 0x00000200 /* Transmit Enable */ > #define GEM_NWCTRL_TXENA 0x00000008 /* Transmit Enable */ > @@ -284,9 +307,9 @@ static inline unsigned tx_desc_get_length(unsigned *desc) > return desc[1] & DESC_1_LENGTH; > } > > -static inline void print_gem_tx_desc(unsigned *desc) > +static inline void print_gem_tx_desc(unsigned *desc, uint8_t queue) > { > - DB_PRINT("TXDESC:\n"); > + DB_PRINT("TXDESC (queue %" PRId8 "):\n", queue); > DB_PRINT("bufaddr: 0x%08x\n", *desc); > DB_PRINT("used_hw: %d\n", tx_desc_get_used(desc)); > DB_PRINT("wrap: %d\n", tx_desc_get_wrap(desc)); > @@ -416,6 +439,7 @@ static void phy_update_link(CadenceGEMState *s) > static int gem_can_receive(NetClientState *nc) > { > CadenceGEMState *s; > + int i; > > s = qemu_get_nic_opaque(nc); > > @@ -428,18 +452,20 @@ static int gem_can_receive(NetClientState *nc) > return 0; > } > > - if (rx_desc_get_ownership(s->rx_desc[0]) == 1) { > - if (s->can_rx_state != 2) { > - s->can_rx_state = 2; > - DB_PRINT("can't receive - busy buffer descriptor 0x%x\n", > - s->rx_desc_addr[0]); > + for (i = 0; i < s->num_priority_queues; i++) { > + if (rx_desc_get_ownership(s->rx_desc[i]) == 1) { > + if (s->can_rx_state != 2) { > + s->can_rx_state = 2; > + DB_PRINT("can't receive - busy buffer descriptor (q%d) > 0x%x\n", > + i, s->rx_desc_addr[i]); > + } > + return 0; > } > - return 0; > } > > if (s->can_rx_state != 0) { > s->can_rx_state = 0; > - DB_PRINT("can receive 0x%x\n", s->rx_desc_addr[0]); > + DB_PRINT("can receive\n"); > } > return 1; > } > @@ -450,9 +476,16 @@ static int gem_can_receive(NetClientState *nc) > */ > static void gem_update_int_status(CadenceGEMState *s) > { > - if (s->regs[GEM_ISR]) { > - DB_PRINT("asserting int. (0x%08x)\n", s->regs[GEM_ISR]); > - qemu_set_irq(s->irq[0], 1); > + int i; > + > + for (i = 0; i < s->num_priority_queues; ++i) { > + /* There seems to be no sane way to see which queue triggered the > + * interrupt. > + */ > + if (s->regs[GEM_ISR]) { > + DB_PRINT("asserting int. q=%d)\n", i); > + qemu_set_irq(s->irq[i], 1);
I don't understand this. The hardware surely can't trigger every IRQ line simultaneously and identically ? > + } > } > } > > @@ -601,17 +634,18 @@ static int gem_mac_address_filter(CadenceGEMState *s, > const uint8_t *packet) > return GEM_RX_REJECT; > } > > -static void gem_get_rx_desc(CadenceGEMState *s) > +static void gem_get_rx_desc(CadenceGEMState *s, int q) > { > - DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[0]); > + DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]); > /* read current descriptor */ > cpu_physical_memory_read(s->rx_desc_addr[0], > (uint8_t *)s->rx_desc[0], > sizeof(s->rx_desc[0])); > > /* Descriptor owned by software ? */ > - if (rx_desc_get_ownership(s->rx_desc[0]) == 1) { > + if (rx_desc_get_ownership(s->rx_desc[q]) == 1 && > + s->num_priority_queues == 1) { Why only if num_priority_queues is 1? (This is one of those "looks a bit odd, is the h/w really like this?" questions; it could be right.) > DB_PRINT("descriptor 0x%x owned by sw.\n", > - (unsigned)s->rx_desc_addr[0]); > + (unsigned)s->rx_desc_addr[q]); > s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF; > s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]); > /* Handle interrupt consequences */ > @@ -632,6 +666,7 @@ static ssize_t gem_receive(NetClientState *nc, const > uint8_t *buf, size_t size) > uint8_t *rxbuf_ptr; > bool first_desc = true; > int maf; > + int q = 0; Shouldn't we be doing something for each queue, rather than just having a variable that's always 0? > > s = qemu_get_nic_opaque(nc); > > @@ -729,31 +764,31 @@ static ssize_t gem_receive(NetClientState *nc, const > uint8_t *buf, size_t size) > > /* Update the descriptor. */ > if (first_desc) { > - rx_desc_set_sof(s->rx_desc[0]); > + rx_desc_set_sof(s->rx_desc[q]); > first_desc = false; > } > if (bytes_to_copy == 0) { > - rx_desc_set_eof(s->rx_desc[0]); > - rx_desc_set_length(s->rx_desc[0], size); > + rx_desc_set_eof(s->rx_desc[q]); > + rx_desc_set_length(s->rx_desc[q], size); > } > - rx_desc_set_ownership(s->rx_desc[0]); > + rx_desc_set_ownership(s->rx_desc[q]); > > switch (maf) { > case GEM_RX_PROMISCUOUS_ACCEPT: > break; > case GEM_RX_BROADCAST_ACCEPT: > - rx_desc_set_broadcast(s->rx_desc[0]); > + rx_desc_set_broadcast(s->rx_desc[q]); > break; > case GEM_RX_UNICAST_HASH_ACCEPT: > - rx_desc_set_unicast_hash(s->rx_desc[0]); > + rx_desc_set_unicast_hash(s->rx_desc[q]); > break; > case GEM_RX_MULTICAST_HASH_ACCEPT: > - rx_desc_set_multicast_hash(s->rx_desc[0]); > + rx_desc_set_multicast_hash(s->rx_desc[q]); > break; > case GEM_RX_REJECT: > abort(); > default: /* SAR */ > - rx_desc_set_sar(s->rx_desc[0], maf); > + rx_desc_set_sar(s->rx_desc[q], maf); > } > > /* Descriptor write-back. */ > @@ -762,14 +797,15 @@ static ssize_t gem_receive(NetClientState *nc, const > uint8_t *buf, size_t size) > sizeof(s->rx_desc[0])); > > /* Next descriptor */ > - if (rx_desc_get_wrap(s->rx_desc[0])) { > + if (rx_desc_get_wrap(s->rx_desc[q])) { > DB_PRINT("wrapping RX descriptor list\n"); > s->rx_desc_addr[0] = s->regs[GEM_RXQBASE]; > } else { > DB_PRINT("incrementing RX descriptor list\n"); > s->rx_desc_addr[0] += 8; > } > - gem_get_rx_desc(s); > + > + gem_get_rx_desc(s, q); > } > > /* Count it */ > @@ -841,6 +877,7 @@ static void gem_transmit(CadenceGEMState *s) > uint8_t tx_packet[2048]; > uint8_t *p; > unsigned total_bytes; > + int8_t q; Why int8_t here but int everywhere else? > > /* Do nothing if transmit is not enabled. */ > if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) { > @@ -856,8 +893,9 @@ static void gem_transmit(CadenceGEMState *s) > p = tx_packet; > total_bytes = 0; > > + for (q = s->num_priority_queues - 1; q >= 0; q--) { > /* read current descriptor */ > - packet_desc_addr = s->tx_desc_addr[0]; > + packet_desc_addr = s->tx_desc_addr[q]; > > DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr); > cpu_physical_memory_read(packet_desc_addr, > @@ -869,7 +907,7 @@ static void gem_transmit(CadenceGEMState *s) > if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) { > return; > } > - print_gem_tx_desc(desc); > + print_gem_tx_desc(desc, q); > > /* The real hardware would eat this (and possibly crash). > * For QEMU let's lend a helping hand. > @@ -904,18 +942,18 @@ static void gem_transmit(CadenceGEMState *s) > /* Modify the 1st descriptor of this packet to be owned by > * the processor. > */ > - cpu_physical_memory_read(s->tx_desc_addr[0], (uint8_t > *)desc_first, > + cpu_physical_memory_read(s->tx_desc_addr[q], (uint8_t > *)desc_first, > sizeof(desc_first)); > tx_desc_set_used(desc_first); > - cpu_physical_memory_write(s->tx_desc_addr[0], (uint8_t > *)desc_first, > + cpu_physical_memory_write(s->tx_desc_addr[q], (uint8_t > *)desc_first, > sizeof(desc_first)); > /* Advance the hardware current descriptor past this packet */ > if (tx_desc_get_wrap(desc)) { > - s->tx_desc_addr[0] = s->regs[GEM_TXQBASE]; > + s->tx_desc_addr[q] = s->regs[GEM_TXQBASE]; > } else { > - s->tx_desc_addr[0] = packet_desc_addr + 8; > + s->tx_desc_addr[q] = packet_desc_addr + 8; > } > - DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[0]); > + DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[q]); > > s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL; > s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]); > @@ -961,6 +999,7 @@ static void gem_transmit(CadenceGEMState *s) > s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]); > gem_update_int_status(s); > } > + } > } > > static void gem_phy_reset(CadenceGEMState *s) > @@ -1067,7 +1106,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, > unsigned size) > { > CadenceGEMState *s; > uint32_t retval; > - > + int i; > s = (CadenceGEMState *)opaque; > > offset >>= 2; > @@ -1077,8 +1116,10 @@ static uint64_t gem_read(void *opaque, hwaddr offset, > unsigned size) > > switch (offset) { > case GEM_ISR: > - DB_PRINT("lowering irq on ISR read\n"); > - qemu_set_irq(s->irq[0], 0); > + DB_PRINT("lowering irqs on ISR read\n"); > + for (i = 0; i < s->num_priority_queues; ++i) { > + qemu_set_irq(s->irq[i], 0); > + } > break; > case GEM_PHYMNTNC: > if (retval & GEM_PHYMNTNC_OP_R) { > @@ -1103,6 +1144,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, > unsigned size) > retval &= ~(s->regs_wo[offset]); > > DB_PRINT("0x%08x\n", retval); > + gem_update_int_status(s); > return retval; > } > > @@ -1115,6 +1157,7 @@ static void gem_write(void *opaque, hwaddr offset, > uint64_t val, > { > CadenceGEMState *s = (CadenceGEMState *)opaque; > uint32_t readonly; > + int i; > > DB_PRINT("offset: 0x%04x write: 0x%08x ", (unsigned)offset, > (unsigned)val); > offset >>= 2; > @@ -1134,14 +1177,18 @@ static void gem_write(void *opaque, hwaddr offset, > uint64_t val, > switch (offset) { > case GEM_NWCTRL: > if (val & GEM_NWCTRL_RXENA) { > - gem_get_rx_desc(s); > + for (i = 0; i < s->num_priority_queues; ++i) { > + gem_get_rx_desc(s, i); > + } > } > if (val & GEM_NWCTRL_TXSTART) { > gem_transmit(s); > } > if (!(val & GEM_NWCTRL_TXENA)) { > /* Reset to start of Q when transmit disabled. */ > - s->tx_desc_addr[0] = s->regs[GEM_TXQBASE]; > + for (i = 0; i < s->num_priority_queues; i++) { > + s->tx_desc_addr[i] = s->regs[GEM_TXQBASE]; > + } > } > if (gem_can_receive(qemu_get_queue(s->nic))) { > qemu_flush_queued_packets(qemu_get_queue(s->nic)); > @@ -1154,9 +1201,15 @@ static void gem_write(void *opaque, hwaddr offset, > uint64_t val, > case GEM_RXQBASE: > s->rx_desc_addr[0] = val; > break; > + case GEM_RECEIVE_Q1_PTR ... GEM_RECEIVE_Q15_PTR: > + s->rx_desc_addr[offset - GEM_RECEIVE_Q1_PTR + 1] = val; > + break; > case GEM_TXQBASE: > s->tx_desc_addr[0] = val; > break; > + case GEM_TRANSMIT_Q1_PTR ... GEM_TRANSMIT_Q15_PTR: > + s->tx_desc_addr[offset - GEM_TRANSMIT_Q1_PTR + 1] = val; > + break; > case GEM_RXSTATUS: > gem_update_int_status(s); > break; > @@ -1164,10 +1217,26 @@ static void gem_write(void *opaque, hwaddr offset, > uint64_t val, > s->regs[GEM_IMR] &= ~val; > gem_update_int_status(s); > break; > + case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE: > + s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val; > + gem_update_int_status(s); > + break; > + case GEM_INT_Q8_ENABLE ... GEM_INT_Q15_ENABLE: > + s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_ENABLE] &= ~val; > + gem_update_int_status(s); > + break; > case GEM_IDR: > s->regs[GEM_IMR] |= val; > gem_update_int_status(s); > break; > + case GEM_INT_Q1_DISABLE ... GEM_INT_Q7_DISABLE: > + s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_DISABLE] |= val; > + gem_update_int_status(s); > + break; > + case GEM_INT_Q8_DISABLE ... GEM_INT_Q15_DISABLE: > + s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_DISABLE] |= val; > + gem_update_int_status(s); > + break; > case GEM_SPADDR1LO: > case GEM_SPADDR2LO: > case GEM_SPADDR3LO: > @@ -1204,8 +1273,11 @@ static const MemoryRegionOps gem_ops = { > > static void gem_set_link(NetClientState *nc) > { > + CadenceGEMState *s = qemu_get_nic_opaque(nc); > + > DB_PRINT("\n"); > - phy_update_link(qemu_get_nic_opaque(nc)); > + phy_update_link(s); > + gem_update_int_status(s); > } > > static NetClientInfo net_gem_info = { > @@ -1219,8 +1291,11 @@ static NetClientInfo net_gem_info = { > static void gem_realize(DeviceState *dev, Error **errp) > { > CadenceGEMState *s = CADENCE_GEM(dev); > + int i; > > - sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[0]); > + for (i = 0; i < s->num_priority_queues; ++i) { > + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[i]); > + } > > qemu_macaddr_default_if_unset(&s->conf.macaddr); > > @@ -1260,6 +1335,8 @@ static const VMStateDescription vmstate_cadence_gem = { > > static Property gem_properties[] = { > DEFINE_NIC_PROPERTIES(CadenceGEMState, conf), > + DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState, > + num_priority_queues, 1), This should go in the same patch as adding the field to the CadenceGEMState struct (don't care whether you move the prop define or the field addition). > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h > index 77e0582..60b3ab0 100644 > --- a/include/hw/net/cadence_gem.h > +++ b/include/hw/net/cadence_gem.h > @@ -30,7 +30,7 @@ > #include "net/net.h" > #include "hw/sysbus.h" > > -#define CADENCE_GEM_MAXREG (0x00000640/4) /* Last valid GEM address */ > +#define CADENCE_GEM_MAXREG (0x00000800 / 4) /* Last valid GEM address > */ Changing this define changes the migration format (because it has an array of this size in it) so you need a version bump. > #define MAX_PRIORITY_QUEUES 8 > > -- > 2.7.4 thanks -- PMM