* [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM
2016-07-11 23:20 [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues Alistair Francis
@ 2016-07-11 23:20 ` Alistair Francis
2016-07-25 15:20 ` Peter Maydell
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 2/5] cadence_gem: Arrayify Alistair Francis
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-11 23:20 UTC (permalink / raw)
To: qemu-devel, qemu-arm; +Cc: alistair.francis, crosthwaitepeter, peter.maydell
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/net/cadence_gem.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 8a4be1e..9d64644 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1214,24 +1214,29 @@ static NetClientInfo net_gem_info = {
.link_status_changed = gem_set_link,
};
-static int gem_init(SysBusDevice *sbd)
+static void gem_realize(DeviceState *dev, Error **errp)
{
- DeviceState *dev = DEVICE(sbd);
CadenceGEMState *s = CADENCE_GEM(dev);
- DB_PRINT("\n");
+ sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
- gem_init_register_masks(s);
- memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
- "enet", sizeof(s->regs));
- sysbus_init_mmio(sbd, &s->iomem);
- sysbus_init_irq(sbd, &s->irq);
qemu_macaddr_default_if_unset(&s->conf.macaddr);
s->nic = qemu_new_nic(&net_gem_info, &s->conf,
object_get_typename(OBJECT(dev)), dev->id, s);
+}
+
+static void gem_init(Object *obj)
+{
+ CadenceGEMState *s = CADENCE_GEM(obj);
+ DeviceState *dev = DEVICE(obj);
+
+ DB_PRINT("\n");
- return 0;
+ gem_init_register_masks(s);
+ memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
+ "enet", sizeof(s->regs));
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
}
static const VMStateDescription vmstate_cadence_gem = {
@@ -1257,9 +1262,8 @@ static Property gem_properties[] = {
static void gem_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
- SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
- sdc->init = gem_init;
+ dc->realize = gem_realize;
dc->props = gem_properties;
dc->vmsd = &vmstate_cadence_gem;
dc->reset = gem_reset;
@@ -1269,6 +1273,7 @@ static const TypeInfo gem_info = {
.name = TYPE_CADENCE_GEM,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(CadenceGEMState),
+ .instance_init = gem_init,
.class_init = gem_class_init,
};
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM Alistair Francis
@ 2016-07-25 15:20 ` Peter Maydell
2016-07-25 16:38 ` Alistair Francis
0 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2016-07-25 15:20 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, qemu-arm, Peter Crosthwaite
On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/net/cadence_gem.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 8a4be1e..9d64644 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1214,24 +1214,29 @@ static NetClientInfo net_gem_info = {
> .link_status_changed = gem_set_link,
> };
>
> -static int gem_init(SysBusDevice *sbd)
> +static void gem_realize(DeviceState *dev, Error **errp)
> {
> - DeviceState *dev = DEVICE(sbd);
> CadenceGEMState *s = CADENCE_GEM(dev);
>
> - DB_PRINT("\n");
> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>
> - gem_init_register_masks(s);
> - memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
> - "enet", sizeof(s->regs));
> - sysbus_init_mmio(sbd, &s->iomem);
> - sysbus_init_irq(sbd, &s->irq);
> qemu_macaddr_default_if_unset(&s->conf.macaddr);
>
> s->nic = qemu_new_nic(&net_gem_info, &s->conf,
> object_get_typename(OBJECT(dev)), dev->id, s);
> +}
> +
> +static void gem_init(Object *obj)
> +{
> + CadenceGEMState *s = CADENCE_GEM(obj);
> + DeviceState *dev = DEVICE(obj);
> +
> + DB_PRINT("\n");
>
> - return 0;
> + gem_init_register_masks(s);
> + memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
> + "enet", sizeof(s->regs));
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> }
I don't understand the logic behind which things are
in init and which in realize here -- why is
sysbus_init_mmio() in init but sysbus_init_irq() in
realize ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM
2016-07-25 15:20 ` Peter Maydell
@ 2016-07-25 16:38 ` Alistair Francis
2016-07-25 16:42 ` Alistair Francis
0 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-25 16:38 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Peter Crosthwaite, qemu-arm, QEMU Developers
On Mon, Jul 25, 2016 at 8:20 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>> hw/net/cadence_gem.c | 27 ++++++++++++++++-----------
>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 8a4be1e..9d64644 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -1214,24 +1214,29 @@ static NetClientInfo net_gem_info = {
>> .link_status_changed = gem_set_link,
>> };
>>
>> -static int gem_init(SysBusDevice *sbd)
>> +static void gem_realize(DeviceState *dev, Error **errp)
>> {
>> - DeviceState *dev = DEVICE(sbd);
>> CadenceGEMState *s = CADENCE_GEM(dev);
>>
>> - DB_PRINT("\n");
>> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>>
>> - gem_init_register_masks(s);
>> - memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
>> - "enet", sizeof(s->regs));
>> - sysbus_init_mmio(sbd, &s->iomem);
>> - sysbus_init_irq(sbd, &s->irq);
>> qemu_macaddr_default_if_unset(&s->conf.macaddr);
>>
>> s->nic = qemu_new_nic(&net_gem_info, &s->conf,
>> object_get_typename(OBJECT(dev)), dev->id, s);
>> +}
>> +
>> +static void gem_init(Object *obj)
>> +{
>> + CadenceGEMState *s = CADENCE_GEM(obj);
>> + DeviceState *dev = DEVICE(obj);
>> +
>> + DB_PRINT("\n");
>>
>> - return 0;
>> + gem_init_register_masks(s);
>> + memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
>> + "enet", sizeof(s->regs));
>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>> }
>
> I don't understand the logic behind which things are
> in init and which in realize here -- why is
> sysbus_init_mmio() in init but sysbus_init_irq() in
> realize ?
That was just a mistake. I have moved all of the *_init_* functions to
the main init function.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM
2016-07-25 16:38 ` Alistair Francis
@ 2016-07-25 16:42 ` Alistair Francis
2016-07-25 16:55 ` Peter Maydell
0 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-25 16:42 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, Peter Crosthwaite, qemu-arm, QEMU Developers
On Mon, Jul 25, 2016 at 9:38 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Mon, Jul 25, 2016 at 8:20 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>> hw/net/cadence_gem.c | 27 ++++++++++++++++-----------
>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>> index 8a4be1e..9d64644 100644
>>> --- a/hw/net/cadence_gem.c
>>> +++ b/hw/net/cadence_gem.c
>>> @@ -1214,24 +1214,29 @@ static NetClientInfo net_gem_info = {
>>> .link_status_changed = gem_set_link,
>>> };
>>>
>>> -static int gem_init(SysBusDevice *sbd)
>>> +static void gem_realize(DeviceState *dev, Error **errp)
>>> {
>>> - DeviceState *dev = DEVICE(sbd);
>>> CadenceGEMState *s = CADENCE_GEM(dev);
>>>
>>> - DB_PRINT("\n");
>>> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>>>
>>> - gem_init_register_masks(s);
>>> - memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
>>> - "enet", sizeof(s->regs));
>>> - sysbus_init_mmio(sbd, &s->iomem);
>>> - sysbus_init_irq(sbd, &s->irq);
>>> qemu_macaddr_default_if_unset(&s->conf.macaddr);
>>>
>>> s->nic = qemu_new_nic(&net_gem_info, &s->conf,
>>> object_get_typename(OBJECT(dev)), dev->id, s);
>>> +}
>>> +
>>> +static void gem_init(Object *obj)
>>> +{
>>> + CadenceGEMState *s = CADENCE_GEM(obj);
>>> + DeviceState *dev = DEVICE(obj);
>>> +
>>> + DB_PRINT("\n");
>>>
>>> - return 0;
>>> + gem_init_register_masks(s);
>>> + memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
>>> + "enet", sizeof(s->regs));
>>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>>> }
>>
>> I don't understand the logic behind which things are
>> in init and which in realize here -- why is
>> sysbus_init_mmio() in init but sysbus_init_irq() in
>> realize ?
>
> That was just a mistake. I have moved all of the *_init_* functions to
> the main init function.
As soon as I sent that mail I remembered why it was like that.
Eventually the sysbus_init_irq() function depends on the number of
queues, which is a property so it can't be set in the init.
Do you think the sysbus_init_mmio() should be moved to the realise as well then?
Thanks,
Alistair
>
> Thanks,
>
> Alistair
>
>>
>> thanks
>> -- PMM
>>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM
2016-07-25 16:42 ` Alistair Francis
@ 2016-07-25 16:55 ` Peter Maydell
0 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2016-07-25 16:55 UTC (permalink / raw)
To: Alistair Francis; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers
On 25 July 2016 at 17:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Mon, Jul 25, 2016 at 9:38 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Mon, Jul 25, 2016 at 8:20 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> ---
>>>>
>>>> hw/net/cadence_gem.c | 27 ++++++++++++++++-----------
>>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>>> index 8a4be1e..9d64644 100644
>>>> --- a/hw/net/cadence_gem.c
>>>> +++ b/hw/net/cadence_gem.c
>>>> @@ -1214,24 +1214,29 @@ static NetClientInfo net_gem_info = {
>>>> .link_status_changed = gem_set_link,
>>>> };
>>>>
>>>> -static int gem_init(SysBusDevice *sbd)
>>>> +static void gem_realize(DeviceState *dev, Error **errp)
>>>> {
>>>> - DeviceState *dev = DEVICE(sbd);
>>>> CadenceGEMState *s = CADENCE_GEM(dev);
>>>>
>>>> - DB_PRINT("\n");
>>>> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>>>>
>>>> - gem_init_register_masks(s);
>>>> - memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
>>>> - "enet", sizeof(s->regs));
>>>> - sysbus_init_mmio(sbd, &s->iomem);
>>>> - sysbus_init_irq(sbd, &s->irq);
>>>> qemu_macaddr_default_if_unset(&s->conf.macaddr);
>>>>
>>>> s->nic = qemu_new_nic(&net_gem_info, &s->conf,
>>>> object_get_typename(OBJECT(dev)), dev->id, s);
>>>> +}
>>>> +
>>>> +static void gem_init(Object *obj)
>>>> +{
>>>> + CadenceGEMState *s = CADENCE_GEM(obj);
>>>> + DeviceState *dev = DEVICE(obj);
>>>> +
>>>> + DB_PRINT("\n");
>>>>
>>>> - return 0;
>>>> + gem_init_register_masks(s);
>>>> + memory_region_init_io(&s->iomem, OBJECT(s), &gem_ops, s,
>>>> + "enet", sizeof(s->regs));
>>>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>>>> }
>>>
>>> I don't understand the logic behind which things are
>>> in init and which in realize here -- why is
>>> sysbus_init_mmio() in init but sysbus_init_irq() in
>>> realize ?
>>
>> That was just a mistake. I have moved all of the *_init_* functions to
>> the main init function.
>
> As soon as I sent that mail I remembered why it was like that.
>
> Eventually the sysbus_init_irq() function depends on the number of
> queues, which is a property so it can't be set in the init.
>
> Do you think the sysbus_init_mmio() should be moved to the realise as well then?
Oh, I see. This is fine, then, but put a note in the commit message
about why the sysbus_init_irq() is where it is.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v1 2/5] cadence_gem: Arrayify
2016-07-11 23:20 [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues Alistair Francis
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM Alistair Francis
@ 2016-07-11 23:20 ` Alistair Francis
2016-07-25 15:22 ` Peter Maydell
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Add queue support Alistair Francis
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-11 23:20 UTC (permalink / raw)
To: qemu-devel, qemu-arm; +Cc: alistair.francis, crosthwaitepeter, peter.maydell
Arrayify the Cadence GEM device to prepare for adding priority queue support.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
This patch has no real effect, I used it to help debug some issues so I
figured I would leave it in.
hw/net/cadence_gem.c | 78 +++++++++++++++++++++++---------------------
include/hw/net/cadence_gem.h | 13 +++++---
2 files changed, 50 insertions(+), 41 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 9d64644..2dabad5 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -428,18 +428,18 @@ static int gem_can_receive(NetClientState *nc)
return 0;
}
- if (rx_desc_get_ownership(s->rx_desc) == 1) {
+ 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);
+ s->rx_desc_addr[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);
+ DB_PRINT("can receive 0x%x\n", s->rx_desc_addr[0]);
}
return 1;
}
@@ -452,7 +452,7 @@ 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, 1);
+ qemu_set_irq(s->irq[0], 1);
}
}
@@ -603,15 +603,15 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
static void gem_get_rx_desc(CadenceGEMState *s)
{
- DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
+ DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[0]);
/* read current descriptor */
- cpu_physical_memory_read(s->rx_desc_addr,
- (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
+ 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) == 1) {
+ if (rx_desc_get_ownership(s->rx_desc[0]) == 1) {
DB_PRINT("descriptor 0x%x owned by sw.\n",
- (unsigned)s->rx_desc_addr);
+ (unsigned)s->rx_desc_addr[0]);
s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
/* Handle interrupt consequences */
@@ -718,54 +718,56 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
}
DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize),
- rx_desc_get_buffer(s->rx_desc));
+ rx_desc_get_buffer(s->rx_desc[0]));
/* Copy packet data to emulated DMA buffer */
- cpu_physical_memory_write(rx_desc_get_buffer(s->rx_desc) + rxbuf_offset,
+ cpu_physical_memory_write(rx_desc_get_buffer(s->rx_desc[0]) +
+ rxbuf_offset,
rxbuf_ptr, MIN(bytes_to_copy, rxbufsize));
rxbuf_ptr += MIN(bytes_to_copy, rxbufsize);
bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
/* Update the descriptor. */
if (first_desc) {
- rx_desc_set_sof(s->rx_desc);
+ rx_desc_set_sof(s->rx_desc[0]);
first_desc = false;
}
if (bytes_to_copy == 0) {
- rx_desc_set_eof(s->rx_desc);
- rx_desc_set_length(s->rx_desc, size);
+ rx_desc_set_eof(s->rx_desc[0]);
+ rx_desc_set_length(s->rx_desc[0], size);
}
- rx_desc_set_ownership(s->rx_desc);
+ rx_desc_set_ownership(s->rx_desc[0]);
switch (maf) {
case GEM_RX_PROMISCUOUS_ACCEPT:
break;
case GEM_RX_BROADCAST_ACCEPT:
- rx_desc_set_broadcast(s->rx_desc);
+ rx_desc_set_broadcast(s->rx_desc[0]);
break;
case GEM_RX_UNICAST_HASH_ACCEPT:
- rx_desc_set_unicast_hash(s->rx_desc);
+ rx_desc_set_unicast_hash(s->rx_desc[0]);
break;
case GEM_RX_MULTICAST_HASH_ACCEPT:
- rx_desc_set_multicast_hash(s->rx_desc);
+ rx_desc_set_multicast_hash(s->rx_desc[0]);
break;
case GEM_RX_REJECT:
abort();
default: /* SAR */
- rx_desc_set_sar(s->rx_desc, maf);
+ rx_desc_set_sar(s->rx_desc[0], maf);
}
/* Descriptor write-back. */
- cpu_physical_memory_write(s->rx_desc_addr,
- (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
+ cpu_physical_memory_write(s->rx_desc_addr[0],
+ (uint8_t *)s->rx_desc[0],
+ sizeof(s->rx_desc[0]));
/* Next descriptor */
- if (rx_desc_get_wrap(s->rx_desc)) {
+ if (rx_desc_get_wrap(s->rx_desc[0])) {
DB_PRINT("wrapping RX descriptor list\n");
- s->rx_desc_addr = s->regs[GEM_RXQBASE];
+ s->rx_desc_addr[0] = s->regs[GEM_RXQBASE];
} else {
DB_PRINT("incrementing RX descriptor list\n");
- s->rx_desc_addr += 8;
+ s->rx_desc_addr[0] += 8;
}
gem_get_rx_desc(s);
}
@@ -855,7 +857,7 @@ static void gem_transmit(CadenceGEMState *s)
total_bytes = 0;
/* read current descriptor */
- packet_desc_addr = s->tx_desc_addr;
+ packet_desc_addr = s->tx_desc_addr[0];
DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
cpu_physical_memory_read(packet_desc_addr,
@@ -902,18 +904,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, (uint8_t *)desc_first,
+ cpu_physical_memory_read(s->tx_desc_addr[0], (uint8_t *)desc_first,
sizeof(desc_first));
tx_desc_set_used(desc_first);
- cpu_physical_memory_write(s->tx_desc_addr, (uint8_t *)desc_first,
+ cpu_physical_memory_write(s->tx_desc_addr[0], (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 = s->regs[GEM_TXQBASE];
+ s->tx_desc_addr[0] = s->regs[GEM_TXQBASE];
} else {
- s->tx_desc_addr = packet_desc_addr + 8;
+ s->tx_desc_addr[0] = packet_desc_addr + 8;
}
- DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr);
+ DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[0]);
s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
@@ -1076,7 +1078,7 @@ 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);
+ qemu_set_irq(s->irq[0], 0);
break;
case GEM_PHYMNTNC:
if (retval & GEM_PHYMNTNC_OP_R) {
@@ -1139,7 +1141,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
}
if (!(val & GEM_NWCTRL_TXENA)) {
/* Reset to start of Q when transmit disabled. */
- s->tx_desc_addr = s->regs[GEM_TXQBASE];
+ s->tx_desc_addr[0] = s->regs[GEM_TXQBASE];
}
if (gem_can_receive(qemu_get_queue(s->nic))) {
qemu_flush_queued_packets(qemu_get_queue(s->nic));
@@ -1150,10 +1152,10 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
gem_update_int_status(s);
break;
case GEM_RXQBASE:
- s->rx_desc_addr = val;
+ s->rx_desc_addr[0] = val;
break;
case GEM_TXQBASE:
- s->tx_desc_addr = val;
+ s->tx_desc_addr[0] = val;
break;
case GEM_RXSTATUS:
gem_update_int_status(s);
@@ -1218,7 +1220,7 @@ static void gem_realize(DeviceState *dev, Error **errp)
{
CadenceGEMState *s = CADENCE_GEM(dev);
- sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
+ sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[0]);
qemu_macaddr_default_if_unset(&s->conf.macaddr);
@@ -1247,8 +1249,10 @@ static const VMStateDescription vmstate_cadence_gem = {
VMSTATE_UINT32_ARRAY(regs, CadenceGEMState, CADENCE_GEM_MAXREG),
VMSTATE_UINT16_ARRAY(phy_regs, CadenceGEMState, 32),
VMSTATE_UINT8(phy_loop, CadenceGEMState),
- VMSTATE_UINT32(rx_desc_addr, CadenceGEMState),
- VMSTATE_UINT32(tx_desc_addr, CadenceGEMState),
+ VMSTATE_UINT32_ARRAY(rx_desc_addr, CadenceGEMState,
+ MAX_PRIORITY_QUEUES),
+ VMSTATE_UINT32_ARRAY(tx_desc_addr, CadenceGEMState,
+ MAX_PRIORITY_QUEUES),
VMSTATE_BOOL_ARRAY(sar_active, CadenceGEMState, 4),
VMSTATE_END_OF_LIST(),
}
diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
index f2e08e3..77e0582 100644
--- a/include/hw/net/cadence_gem.h
+++ b/include/hw/net/cadence_gem.h
@@ -32,6 +32,8 @@
#define CADENCE_GEM_MAXREG (0x00000640/4) /* Last valid GEM address */
+#define MAX_PRIORITY_QUEUES 8
+
typedef struct CadenceGEMState {
/*< private >*/
SysBusDevice parent_obj;
@@ -40,7 +42,10 @@ typedef struct CadenceGEMState {
MemoryRegion iomem;
NICState *nic;
NICConf conf;
- qemu_irq irq;
+ qemu_irq irq[MAX_PRIORITY_QUEUES];
+
+ /* Static properties */
+ uint8_t num_priority_queues;
/* GEM registers backing store */
uint32_t regs[CADENCE_GEM_MAXREG];
@@ -59,12 +64,12 @@ typedef struct CadenceGEMState {
uint8_t phy_loop; /* Are we in phy loopback? */
/* The current DMA descriptor pointers */
- uint32_t rx_desc_addr;
- uint32_t tx_desc_addr;
+ uint32_t rx_desc_addr[MAX_PRIORITY_QUEUES];
+ uint32_t tx_desc_addr[MAX_PRIORITY_QUEUES];
uint8_t can_rx_state; /* Debug only */
- unsigned rx_desc[2];
+ unsigned rx_desc[MAX_PRIORITY_QUEUES][2];
bool sar_active[4];
} CadenceGEMState;
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 2/5] cadence_gem: Arrayify
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 2/5] cadence_gem: Arrayify Alistair Francis
@ 2016-07-25 15:22 ` Peter Maydell
0 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2016-07-25 15:22 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, qemu-arm, Peter Crosthwaite
On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Arrayify the Cadence GEM device to prepare for adding priority queue support.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> This patch has no real effect, I used it to help debug some issues so I
> figured I would leave it in.
An expanded commit message here would be useful, something along
the lines of the hardware having N priority queues and that
the behaviour doesn't yet change because we only use queue 0,
or whatever. The subject line is also a bit cryptic.
> @@ -1247,8 +1249,10 @@ static const VMStateDescription vmstate_cadence_gem = {
> VMSTATE_UINT32_ARRAY(regs, CadenceGEMState, CADENCE_GEM_MAXREG),
> VMSTATE_UINT16_ARRAY(phy_regs, CadenceGEMState, 32),
> VMSTATE_UINT8(phy_loop, CadenceGEMState),
> - VMSTATE_UINT32(rx_desc_addr, CadenceGEMState),
> - VMSTATE_UINT32(tx_desc_addr, CadenceGEMState),
> + VMSTATE_UINT32_ARRAY(rx_desc_addr, CadenceGEMState,
> + MAX_PRIORITY_QUEUES),
> + VMSTATE_UINT32_ARRAY(tx_desc_addr, CadenceGEMState,
> + MAX_PRIORITY_QUEUES),
> VMSTATE_BOOL_ARRAY(sar_active, CadenceGEMState, 4),
> VMSTATE_END_OF_LIST(),
> }
You need to bump the vmstate version numbers if you change the format.
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v1 3/5] cadence_gem: Add queue support
2016-07-11 23:20 [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues Alistair Francis
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: QOMify Cadence GEM Alistair Francis
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 2/5] cadence_gem: Arrayify Alistair Francis
@ 2016-07-11 23:20 ` Alistair Francis
2016-07-25 15:46 ` Peter Maydell
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Correct indentation Alistair Francis
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-11 23:20 UTC (permalink / raw)
To: qemu-devel, qemu-arm; +Cc: alistair.francis, crosthwaitepeter, peter.maydell
Signed-off-by: Alistair Francis <alistair.francis@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);
+ }
}
}
@@ -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) {
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;
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;
/* 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),
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 */
#define MAX_PRIORITY_QUEUES 8
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Add queue support
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Add queue support Alistair Francis
@ 2016-07-25 15:46 ` Peter Maydell
2016-07-25 23:01 ` Alistair Francis
0 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2016-07-25 15:46 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, qemu-arm, Peter Crosthwaite
On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Add queue support
2016-07-25 15:46 ` Peter Maydell
@ 2016-07-25 23:01 ` Alistair Francis
0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2016-07-25 23:01 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Peter Crosthwaite, qemu-arm, QEMU Developers
On Mon, Jul 25, 2016 at 8:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@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 ?
Yeah, this is wrong, I have updated it.
>
>> + }
>> }
>> }
>>
>> @@ -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.)
Another weird thing, fixed.
>
>> 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?
Woops, I missed out on the screening logic. Added it in.
>
>>
>> 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?
No reason, it's an int now.
>
>>
>> /* 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).
Agreed, I have moved it to an earlier patch.
>
>> 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.
Done
Thanks,
Alistair
>
>> #define MAX_PRIORITY_QUEUES 8
>>
>> --
>> 2.7.4
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v1 4/5] cadence_gem: Correct indentation
2016-07-11 23:20 [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues Alistair Francis
` (2 preceding siblings ...)
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Add queue support Alistair Francis
@ 2016-07-11 23:20 ` Alistair Francis
2016-07-25 15:48 ` Peter Maydell
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the number of priority queues Alistair Francis
2016-07-12 14:25 ` [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM " Peter Maydell
5 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-11 23:20 UTC (permalink / raw)
To: qemu-devel, qemu-arm; +Cc: alistair.francis, crosthwaitepeter, peter.maydell
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/net/cadence_gem.c | 182 +++++++++++++++++++++++++--------------------------
1 file changed, 91 insertions(+), 91 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index c80e833..1c09756 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -894,111 +894,111 @@ static void gem_transmit(CadenceGEMState *s)
total_bytes = 0;
for (q = s->num_priority_queues - 1; q >= 0; q--) {
- /* read current descriptor */
- 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,
- (uint8_t *)desc, sizeof(desc));
- /* Handle all descriptors owned by hardware */
- while (tx_desc_get_used(desc) == 0) {
+ /* read current descriptor */
+ packet_desc_addr = s->tx_desc_addr[q];
- /* Do nothing if transmit is not enabled. */
- if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) {
- return;
- }
- print_gem_tx_desc(desc, q);
-
- /* The real hardware would eat this (and possibly crash).
- * For QEMU let's lend a helping hand.
- */
- if ((tx_desc_get_buffer(desc) == 0) ||
- (tx_desc_get_length(desc) == 0)) {
- DB_PRINT("Invalid TX descriptor @ 0x%x\n",
- (unsigned)packet_desc_addr);
- break;
- }
-
- if (tx_desc_get_length(desc) > sizeof(tx_packet) - (p - tx_packet)) {
- DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space 0x%x\n",
- (unsigned)packet_desc_addr,
- (unsigned)tx_desc_get_length(desc),
- sizeof(tx_packet) - (p - tx_packet));
- break;
- }
-
- /* Gather this fragment of the packet from "dma memory" to our contig.
- * buffer.
- */
- cpu_physical_memory_read(tx_desc_get_buffer(desc), p,
- tx_desc_get_length(desc));
- p += tx_desc_get_length(desc);
- total_bytes += tx_desc_get_length(desc);
+ DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
+ cpu_physical_memory_read(packet_desc_addr,
+ (uint8_t *)desc, sizeof(desc));
+ /* Handle all descriptors owned by hardware */
+ while (tx_desc_get_used(desc) == 0) {
- /* Last descriptor for this packet; hand the whole thing off */
- if (tx_desc_get_last(desc)) {
- unsigned desc_first[2];
+ /* Do nothing if transmit is not enabled. */
+ if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) {
+ return;
+ }
+ print_gem_tx_desc(desc, q);
- /* Modify the 1st descriptor of this packet to be owned by
- * the processor.
+ /* The real hardware would eat this (and possibly crash).
+ * For QEMU let's lend a helping hand.
*/
- 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[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[q] = s->regs[GEM_TXQBASE];
- } else {
- s->tx_desc_addr[q] = packet_desc_addr + 8;
+ if ((tx_desc_get_buffer(desc) == 0) ||
+ (tx_desc_get_length(desc) == 0)) {
+ DB_PRINT("Invalid TX descriptor @ 0x%x\n",
+ (unsigned)packet_desc_addr);
+ break;
}
- 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]);
- /* Handle interrupt consequences */
- gem_update_int_status(s);
-
- /* Is checksum offload enabled? */
- if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
- net_checksum_calculate(tx_packet, total_bytes);
+ if (tx_desc_get_length(desc) > sizeof(tx_packet) - (p - tx_packet)) {
+ DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space 0x%x\n",
+ (unsigned)packet_desc_addr,
+ (unsigned)tx_desc_get_length(desc),
+ sizeof(tx_packet) - (p - tx_packet));
+ break;
}
- /* Update MAC statistics */
- gem_transmit_updatestats(s, tx_packet, total_bytes);
+ /* Gather this fragment of the packet from "dma memory" to our contig.
+ * buffer.
+ */
+ cpu_physical_memory_read(tx_desc_get_buffer(desc), p,
+ tx_desc_get_length(desc));
+ p += tx_desc_get_length(desc);
+ total_bytes += tx_desc_get_length(desc);
+
+ /* Last descriptor for this packet; hand the whole thing off */
+ if (tx_desc_get_last(desc)) {
+ unsigned desc_first[2];
+
+ /* Modify the 1st descriptor of this packet to be owned by
+ * the processor.
+ */
+ 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[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[q] = s->regs[GEM_TXQBASE];
+ } else {
+ s->tx_desc_addr[q] = packet_desc_addr + 8;
+ }
+ 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]);
+
+ /* Handle interrupt consequences */
+ gem_update_int_status(s);
+
+ /* Is checksum offload enabled? */
+ if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
+ net_checksum_calculate(tx_packet, total_bytes);
+ }
+
+ /* Update MAC statistics */
+ gem_transmit_updatestats(s, tx_packet, total_bytes);
+
+ /* Send the packet somewhere */
+ if (s->phy_loop || (s->regs[GEM_NWCTRL] & GEM_NWCTRL_LOCALLOOP)) {
+ gem_receive(qemu_get_queue(s->nic), tx_packet, total_bytes);
+ } else {
+ qemu_send_packet(qemu_get_queue(s->nic), tx_packet,
+ total_bytes);
+ }
+
+ /* Prepare for next packet */
+ p = tx_packet;
+ total_bytes = 0;
+ }
- /* Send the packet somewhere */
- if (s->phy_loop || (s->regs[GEM_NWCTRL] & GEM_NWCTRL_LOCALLOOP)) {
- gem_receive(qemu_get_queue(s->nic), tx_packet, total_bytes);
+ /* read next descriptor */
+ if (tx_desc_get_wrap(desc)) {
+ tx_desc_set_last(desc);
+ packet_desc_addr = s->regs[GEM_TXQBASE];
} else {
- qemu_send_packet(qemu_get_queue(s->nic), tx_packet,
- total_bytes);
+ packet_desc_addr += 8;
}
-
- /* Prepare for next packet */
- p = tx_packet;
- total_bytes = 0;
+ DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
+ cpu_physical_memory_read(packet_desc_addr,
+ (uint8_t *)desc, sizeof(desc));
}
- /* read next descriptor */
- if (tx_desc_get_wrap(desc)) {
- tx_desc_set_last(desc);
- packet_desc_addr = s->regs[GEM_TXQBASE];
- } else {
- packet_desc_addr += 8;
+ if (tx_desc_get_used(desc)) {
+ s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_USED;
+ s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
+ gem_update_int_status(s);
}
- DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
- cpu_physical_memory_read(packet_desc_addr,
- (uint8_t *)desc, sizeof(desc));
- }
-
- if (tx_desc_get_used(desc)) {
- s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_USED;
- s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
- gem_update_int_status(s);
- }
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 4/5] cadence_gem: Correct indentation
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Correct indentation Alistair Francis
@ 2016-07-25 15:48 ` Peter Maydell
2016-07-25 16:34 ` Alistair Francis
0 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2016-07-25 15:48 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, qemu-arm, Peter Crosthwaite
On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/net/cadence_gem.c | 182 +++++++++++++++++++++++++--------------------------
> 1 file changed, 91 insertions(+), 91 deletions(-)
I assume this patch comes out empty if you use 'git show -w';
would be nice to say so specifically in the commit message if true.
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/5] cadence_gem: Correct indentation
2016-07-25 15:48 ` Peter Maydell
@ 2016-07-25 16:34 ` Alistair Francis
2016-07-25 16:55 ` Peter Maydell
0 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-25 16:34 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Peter Crosthwaite, qemu-arm, QEMU Developers
On Mon, Jul 25, 2016 at 8:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>> hw/net/cadence_gem.c | 182 +++++++++++++++++++++++++--------------------------
>> 1 file changed, 91 insertions(+), 91 deletions(-)
>
> I assume this patch comes out empty if you use 'git show -w';
> would be nice to say so specifically in the commit message if true.
It is empty, I'll update the commit message.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/5] cadence_gem: Correct indentation
2016-07-25 16:34 ` Alistair Francis
@ 2016-07-25 16:55 ` Peter Maydell
0 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2016-07-25 16:55 UTC (permalink / raw)
To: Alistair Francis; +Cc: Peter Crosthwaite, qemu-arm, QEMU Developers
On 25 July 2016 at 17:34, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Mon, Jul 25, 2016 at 8:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>> hw/net/cadence_gem.c | 182 +++++++++++++++++++++++++--------------------------
>>> 1 file changed, 91 insertions(+), 91 deletions(-)
>>
>> I assume this patch comes out empty if you use 'git show -w';
>> would be nice to say so specifically in the commit message if true.
>
> It is empty, I'll update the commit message.
In that case
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the number of priority queues
2016-07-11 23:20 [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues Alistair Francis
` (3 preceding siblings ...)
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Correct indentation Alistair Francis
@ 2016-07-11 23:20 ` Alistair Francis
2016-07-25 15:50 ` Peter Maydell
2016-07-12 14:25 ` [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM " Peter Maydell
5 siblings, 1 reply; 19+ messages in thread
From: Alistair Francis @ 2016-07-11 23:20 UTC (permalink / raw)
To: qemu-devel, qemu-arm; +Cc: alistair.francis, crosthwaitepeter, peter.maydell
Set the ZynqMP number of priority queues to 2.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/arm/xlnx-zynqmp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 23c7199..0d86ba3 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -332,6 +332,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
qdev_set_nic_properties(DEVICE(&s->gem[i]), nd);
}
+ object_property_set_int(OBJECT(&s->gem[i]), 2, "num-priority-queues",
+ &error_abort);
object_property_set_bool(OBJECT(&s->gem[i]), true, "realized", &err);
if (err) {
error_propagate(errp, err);
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the number of priority queues
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the number of priority queues Alistair Francis
@ 2016-07-25 15:50 ` Peter Maydell
0 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2016-07-25 15:50 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, qemu-arm, Peter Crosthwaite
On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Set the ZynqMP number of priority queues to 2.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/arm/xlnx-zynqmp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 23c7199..0d86ba3 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -332,6 +332,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
> qdev_set_nic_properties(DEVICE(&s->gem[i]), nd);
> }
> + object_property_set_int(OBJECT(&s->gem[i]), 2, "num-priority-queues",
> + &error_abort);
> object_property_set_bool(OBJECT(&s->gem[i]), true, "realized", &err);
> if (err) {
> error_propagate(errp, err);
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues
2016-07-11 23:20 [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues Alistair Francis
` (4 preceding siblings ...)
2016-07-11 23:20 ` [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the number of priority queues Alistair Francis
@ 2016-07-12 14:25 ` Peter Maydell
2016-07-12 16:34 ` Alistair Francis
5 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2016-07-12 14:25 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, qemu-arm, Peter Crosthwaite
On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
> This patch series adds initial priority queue support to the Cadence GEM
> device. This is based on original work by Peter C, that has been ported
> to the latest version of QEMU.
>
> There is more GEM work that I'd like to upstream after this, but I
> figured this a good place to start. I have done limited testing on the
> Xilinx machine, including running some of our GEM regressions, although
> they don't cover everything. I can't really think of too many other test
> cases that I can run at the moment.
Hi; I'll get to a proper review at some point in the future, but
for the moment I just wanted to let you know that I'm classifying
this as "not for 2.7", given where we are in the release process.
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM priority queues
2016-07-12 14:25 ` [Qemu-devel] [PATCH v1 0/5] Add support for Cadence GEM " Peter Maydell
@ 2016-07-12 16:34 ` Alistair Francis
0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2016-07-12 16:34 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Peter Crosthwaite, qemu-arm, QEMU Developers
On Tue, Jul 12, 2016 at 7:25 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> This patch series adds initial priority queue support to the Cadence GEM
>> device. This is based on original work by Peter C, that has been ported
>> to the latest version of QEMU.
>>
>> There is more GEM work that I'd like to upstream after this, but I
>> figured this a good place to start. I have done limited testing on the
>> Xilinx machine, including running some of our GEM regressions, although
>> they don't cover everything. I can't really think of too many other test
>> cases that I can run at the moment.
>
> Hi; I'll get to a proper review at some point in the future, but
> for the moment I just wanted to let you know that I'm classifying
> this as "not for 2.7", given where we are in the release process.
That's fine, I should have sent it earlier.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 19+ messages in thread