* [Qemu-devel] [PATCH v1 0/5] Improve the Cadence GEM multi-queue support
@ 2017-04-04 23:40 Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor Alistair Francis
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Alistair Francis @ 2017-04-04 23:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell; +Cc: alistair.francis, alistair23
Improve the Cadence GEM multi-queue support. This fixes a few bugs
which were hanging around from the initial implementation.
Alistair Francis (5):
cadence_gem: Read the correct queue descriptor
cadence_gem: Correct the multi-queue can rx logic
cadence_gem: Only trigger interrupts if the status register is set
cadence_gem: Make the revision a property
xlnx-zynqmp: Set the Cadence GEM revision
hw/arm/xlnx-zynqmp.c | 6 +++++-
hw/net/cadence_gem.c | 30 +++++++++++++++++++-----------
include/hw/net/cadence_gem.h | 1 +
3 files changed, 25 insertions(+), 12 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor
2017-04-04 23:40 [Qemu-devel] [PATCH v1 0/5] Improve the Cadence GEM multi-queue support Alistair Francis
@ 2017-04-04 23:40 ` Alistair Francis
2017-04-05 10:54 ` Philippe Mathieu-Daudé
2017-04-10 12:39 ` Peter Maydell
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 2/5] cadence_gem: Correct the multi-queue can rx logic Alistair Francis
` (3 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Alistair Francis @ 2017-04-04 23:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell; +Cc: alistair.francis, alistair23
Read the correct descriptor instead of hardcoding the first (q=0).
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/net/cadence_gem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index d4de8ad..17c229d 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -790,8 +790,8 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
{
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]));
+ cpu_physical_memory_read(s->rx_desc_addr[q],
+ (uint8_t *)s->rx_desc[q], sizeof(s->rx_desc[q]));
/* Descriptor owned by software ? */
if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 2/5] cadence_gem: Correct the multi-queue can rx logic
2017-04-04 23:40 [Qemu-devel] [PATCH v1 0/5] Improve the Cadence GEM multi-queue support Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor Alistair Francis
@ 2017-04-04 23:40 ` Alistair Francis
2017-04-10 12:37 ` Peter Maydell
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set Alistair Francis
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Alistair Francis @ 2017-04-04 23:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell; +Cc: alistair.francis, alistair23
Correct the buffer descriptor busy logic to work correctly when using
multiple queues.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/net/cadence_gem.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 17c229d..3e37665 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -481,14 +481,18 @@ static int gem_can_receive(NetClientState *nc)
}
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;
+ if (rx_desc_get_ownership(s->rx_desc[i]) != 1) {
+ break;
+ }
+ };
+
+ if (i == s->num_priority_queues) {
+ 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;
}
if (s->can_rx_state != 0) {
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set
2017-04-04 23:40 [Qemu-devel] [PATCH v1 0/5] Improve the Cadence GEM multi-queue support Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 2/5] cadence_gem: Correct the multi-queue can rx logic Alistair Francis
@ 2017-04-04 23:40 ` Alistair Francis
2017-04-05 11:00 ` Philippe Mathieu-Daudé
2017-04-10 12:44 ` Peter Maydell
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the Cadence GEM revision Alistair Francis
4 siblings, 2 replies; 18+ messages in thread
From: Alistair Francis @ 2017-04-04 23:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell; +Cc: alistair.francis, alistair23
Only trigger multi-queue GEM interrupts if the interrupt status register
is set. This logic was already used for non multi-queue interrupts but
it also applies to multi-queue interrupts.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/net/cadence_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 3e37665..b9eaed4 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
}
for (i = 0; i < s->num_priority_queues; ++i) {
- if (s->regs[GEM_INT_Q1_STATUS + i]) {
+ if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
DB_PRINT("asserting int. (q=%d)\n", i);
qemu_set_irq(s->irq[i], 1);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property
2017-04-04 23:40 [Qemu-devel] [PATCH v1 0/5] Improve the Cadence GEM multi-queue support Alistair Francis
` (2 preceding siblings ...)
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set Alistair Francis
@ 2017-04-04 23:40 ` Alistair Francis
2017-04-05 10:57 ` Philippe Mathieu-Daudé
2017-04-10 12:45 ` Peter Maydell
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the Cadence GEM revision Alistair Francis
4 siblings, 2 replies; 18+ messages in thread
From: Alistair Francis @ 2017-04-04 23:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell; +Cc: alistair.francis, alistair23
Expose the Cadence GEM revision as a property.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/net/cadence_gem.c | 6 +++++-
include/hw/net/cadence_gem.h | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index b9eaed4..7047e02 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -300,6 +300,8 @@
#define DESC_1_RX_SOF 0x00004000
#define DESC_1_RX_EOF 0x00008000
+#define GEM_MODID_VALUE 0x00020118
+
static inline unsigned tx_desc_get_buffer(unsigned *desc)
{
return desc[0];
@@ -1213,7 +1215,7 @@ static void gem_reset(DeviceState *d)
s->regs[GEM_TXPAUSE] = 0x0000ffff;
s->regs[GEM_TXPARTIALSF] = 0x000003ff;
s->regs[GEM_RXPARTIALSF] = 0x000003ff;
- s->regs[GEM_MODID] = 0x00020118;
+ s->regs[GEM_MODID] = s->revision;
s->regs[GEM_DESCONF] = 0x02500111;
s->regs[GEM_DESCONF2] = 0x2ab13fff;
s->regs[GEM_DESCONF5] = 0x002f2145;
@@ -1512,6 +1514,8 @@ static const VMStateDescription vmstate_cadence_gem = {
static Property gem_properties[] = {
DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
+ DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
+ GEM_MODID_VALUE),
DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
num_priority_queues, 1),
DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
index c469ffe..35de622 100644
--- a/include/hw/net/cadence_gem.h
+++ b/include/hw/net/cadence_gem.h
@@ -50,6 +50,7 @@ typedef struct CadenceGEMState {
uint8_t num_priority_queues;
uint8_t num_type1_screeners;
uint8_t num_type2_screeners;
+ uint32_t revision;
/* GEM registers backing store */
uint32_t regs[CADENCE_GEM_MAXREG];
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the Cadence GEM revision
2017-04-04 23:40 [Qemu-devel] [PATCH v1 0/5] Improve the Cadence GEM multi-queue support Alistair Francis
` (3 preceding siblings ...)
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property Alistair Francis
@ 2017-04-04 23:40 ` Alistair Francis
2017-04-10 12:38 ` Peter Maydell
4 siblings, 1 reply; 18+ messages in thread
From: Alistair Francis @ 2017-04-04 23:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell; +Cc: alistair.francis, alistair23
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/arm/xlnx-zynqmp.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index bc4e66b..e41b6fe 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -30,6 +30,8 @@
#define ARM_PHYS_TIMER_PPI 30
#define ARM_VIRT_TIMER_PPI 27
+#define GEM_REVISION 0x40070106
+
#define GIC_BASE_ADDR 0xf9000000
#define GIC_DIST_ADDR 0xf9010000
#define GIC_CPU_ADDR 0xf9020000
@@ -334,8 +336,10 @@ 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]), GEM_REVISION, "revision",
+ &error_abort);
object_property_set_int(OBJECT(&s->gem[i]), 2, "num-priority-queues",
- &error_abort);
+ &error_abort);
object_property_set_bool(OBJECT(&s->gem[i]), true, "realized", &err);
if (err) {
error_propagate(errp, err);
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor Alistair Francis
@ 2017-04-05 10:54 ` Philippe Mathieu-Daudé
2017-04-10 12:39 ` Peter Maydell
1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-05 10:54 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, peter.maydell; +Cc: alistair23
On 04/04/2017 08:40 PM, Alistair Francis wrote:
> Read the correct descriptor instead of hardcoding the first (q=0).
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> hw/net/cadence_gem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d4de8ad..17c229d 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -790,8 +790,8 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
> {
> 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]));
> + cpu_physical_memory_read(s->rx_desc_addr[q],
> + (uint8_t *)s->rx_desc[q], sizeof(s->rx_desc[q]));
>
> /* Descriptor owned by software ? */
> if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property Alistair Francis
@ 2017-04-05 10:57 ` Philippe Mathieu-Daudé
2017-04-10 12:45 ` Peter Maydell
1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-05 10:57 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, peter.maydell; +Cc: alistair23
On 04/04/2017 08:40 PM, Alistair Francis wrote:
> Expose the Cadence GEM revision as a property.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> hw/net/cadence_gem.c | 6 +++++-
> include/hw/net/cadence_gem.h | 1 +
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index b9eaed4..7047e02 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -300,6 +300,8 @@
> #define DESC_1_RX_SOF 0x00004000
> #define DESC_1_RX_EOF 0x00008000
>
> +#define GEM_MODID_VALUE 0x00020118
> +
> static inline unsigned tx_desc_get_buffer(unsigned *desc)
> {
> return desc[0];
> @@ -1213,7 +1215,7 @@ static void gem_reset(DeviceState *d)
> s->regs[GEM_TXPAUSE] = 0x0000ffff;
> s->regs[GEM_TXPARTIALSF] = 0x000003ff;
> s->regs[GEM_RXPARTIALSF] = 0x000003ff;
> - s->regs[GEM_MODID] = 0x00020118;
> + s->regs[GEM_MODID] = s->revision;
> s->regs[GEM_DESCONF] = 0x02500111;
> s->regs[GEM_DESCONF2] = 0x2ab13fff;
> s->regs[GEM_DESCONF5] = 0x002f2145;
> @@ -1512,6 +1514,8 @@ static const VMStateDescription vmstate_cadence_gem = {
>
> static Property gem_properties[] = {
> DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
> + DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
> + GEM_MODID_VALUE),
> DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
> num_priority_queues, 1),
> DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
> index c469ffe..35de622 100644
> --- a/include/hw/net/cadence_gem.h
> +++ b/include/hw/net/cadence_gem.h
> @@ -50,6 +50,7 @@ typedef struct CadenceGEMState {
> uint8_t num_priority_queues;
> uint8_t num_type1_screeners;
> uint8_t num_type2_screeners;
> + uint32_t revision;
>
> /* GEM registers backing store */
> uint32_t regs[CADENCE_GEM_MAXREG];
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set Alistair Francis
@ 2017-04-05 11:00 ` Philippe Mathieu-Daudé
2017-04-10 12:44 ` Peter Maydell
1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-05 11:00 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, peter.maydell; +Cc: alistair23
On 04/04/2017 08:40 PM, Alistair Francis wrote:
> Only trigger multi-queue GEM interrupts if the interrupt status register
> is set. This logic was already used for non multi-queue interrupts but
> it also applies to multi-queue interrupts.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> hw/net/cadence_gem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3e37665..b9eaed4 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
> }
>
> for (i = 0; i < s->num_priority_queues; ++i) {
> - if (s->regs[GEM_INT_Q1_STATUS + i]) {
> + if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
> DB_PRINT("asserting int. (q=%d)\n", i);
> qemu_set_irq(s->irq[i], 1);
> }
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/5] cadence_gem: Correct the multi-queue can rx logic
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 2/5] cadence_gem: Correct the multi-queue can rx logic Alistair Francis
@ 2017-04-10 12:37 ` Peter Maydell
2017-04-10 20:25 ` Alistair Francis
0 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2017-04-10 12:37 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, Alistair Francis
On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Correct the buffer descriptor busy logic to work correctly when using
> multiple queues.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/net/cadence_gem.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 17c229d..3e37665 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -481,14 +481,18 @@ static int gem_can_receive(NetClientState *nc)
> }
>
> 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;
> + if (rx_desc_get_ownership(s->rx_desc[i]) != 1) {
> + break;
> + }
> + };
> +
> + if (i == s->num_priority_queues) {
> + 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]);
This looks a little odd -- surely i isn't the right index to use
into rx_desc_addr[] any more now we're outside the loop and i
is always larger than the largest valid queue number? It looks
like the debug print should be rephrased somehow.
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the Cadence GEM revision
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the Cadence GEM revision Alistair Francis
@ 2017-04-10 12:38 ` Peter Maydell
0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2017-04-10 12:38 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, Alistair Francis
On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/arm/xlnx-zynqmp.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index bc4e66b..e41b6fe 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -30,6 +30,8 @@
> #define ARM_PHYS_TIMER_PPI 30
> #define ARM_VIRT_TIMER_PPI 27
>
> +#define GEM_REVISION 0x40070106
> +
> #define GIC_BASE_ADDR 0xf9000000
> #define GIC_DIST_ADDR 0xf9010000
> #define GIC_CPU_ADDR 0xf9020000
> @@ -334,8 +336,10 @@ 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]), GEM_REVISION, "revision",
> + &error_abort);
> object_property_set_int(OBJECT(&s->gem[i]), 2, "num-priority-queues",
> - &error_abort);
> + &error_abort);
> object_property_set_bool(OBJECT(&s->gem[i]), true, "realized", &err);
> if (err) {
> error_propagate(errp, err);
> --
> 2.9.3
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor Alistair Francis
2017-04-05 10:54 ` Philippe Mathieu-Daudé
@ 2017-04-10 12:39 ` Peter Maydell
1 sibling, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2017-04-10 12:39 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, Alistair Francis
On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Read the correct descriptor instead of hardcoding the first (q=0).
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/net/cadence_gem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d4de8ad..17c229d 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -790,8 +790,8 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
> {
> 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]));
> + cpu_physical_memory_read(s->rx_desc_addr[q],
> + (uint8_t *)s->rx_desc[q], sizeof(s->rx_desc[q]));
>
> /* Descriptor owned by software ? */
> if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
> --
> 2.9.3
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set Alistair Francis
2017-04-05 11:00 ` Philippe Mathieu-Daudé
@ 2017-04-10 12:44 ` Peter Maydell
2017-04-10 22:23 ` Alistair Francis
1 sibling, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2017-04-10 12:44 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, Alistair Francis
On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Only trigger multi-queue GEM interrupts if the interrupt status register
> is set. This logic was already used for non multi-queue interrupts but
> it also applies to multi-queue interrupts.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/net/cadence_gem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3e37665..b9eaed4 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
> }
>
> for (i = 0; i < s->num_priority_queues; ++i) {
> - if (s->regs[GEM_INT_Q1_STATUS + i]) {
> + if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
> DB_PRINT("asserting int. (q=%d)\n", i);
> qemu_set_irq(s->irq[i], 1);
> }
With this change, if s->regs[GEM_ISR] is zero then the entire
function never does anything, so you could hoist that up to
the start, rather than testing it inside the loop and in the
previous conditional.
Also the comment says "raise or lower interrupt based on current
status", but the code will only ever do qemu_set_irq(..., 1),
never 0. Which is right?
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property Alistair Francis
2017-04-05 10:57 ` Philippe Mathieu-Daudé
@ 2017-04-10 12:45 ` Peter Maydell
1 sibling, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2017-04-10 12:45 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers, Alistair Francis
On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Expose the Cadence GEM revision as a property.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/net/cadence_gem.c | 6 +++++-
> include/hw/net/cadence_gem.h | 1 +
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index b9eaed4..7047e02 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -300,6 +300,8 @@
> #define DESC_1_RX_SOF 0x00004000
> #define DESC_1_RX_EOF 0x00008000
>
> +#define GEM_MODID_VALUE 0x00020118
> +
> static inline unsigned tx_desc_get_buffer(unsigned *desc)
> {
> return desc[0];
> @@ -1213,7 +1215,7 @@ static void gem_reset(DeviceState *d)
> s->regs[GEM_TXPAUSE] = 0x0000ffff;
> s->regs[GEM_TXPARTIALSF] = 0x000003ff;
> s->regs[GEM_RXPARTIALSF] = 0x000003ff;
> - s->regs[GEM_MODID] = 0x00020118;
> + s->regs[GEM_MODID] = s->revision;
> s->regs[GEM_DESCONF] = 0x02500111;
> s->regs[GEM_DESCONF2] = 0x2ab13fff;
> s->regs[GEM_DESCONF5] = 0x002f2145;
> @@ -1512,6 +1514,8 @@ static const VMStateDescription vmstate_cadence_gem = {
>
> static Property gem_properties[] = {
> DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
> + DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
> + GEM_MODID_VALUE),
> DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
> num_priority_queues, 1),
> DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
> index c469ffe..35de622 100644
> --- a/include/hw/net/cadence_gem.h
> +++ b/include/hw/net/cadence_gem.h
> @@ -50,6 +50,7 @@ typedef struct CadenceGEMState {
> uint8_t num_priority_queues;
> uint8_t num_type1_screeners;
> uint8_t num_type2_screeners;
> + uint32_t revision;
>
> /* GEM registers backing store */
> uint32_t regs[CADENCE_GEM_MAXREG];
> --
> 2.9.3
>
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/5] cadence_gem: Correct the multi-queue can rx logic
2017-04-10 12:37 ` Peter Maydell
@ 2017-04-10 20:25 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2017-04-10 20:25 UTC (permalink / raw)
To: Peter Maydell; +Cc: Alistair Francis, QEMU Developers
On Mon, Apr 10, 2017 at 5:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Correct the buffer descriptor busy logic to work correctly when using
>> multiple queues.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>> hw/net/cadence_gem.c | 18 +++++++++++-------
>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 17c229d..3e37665 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -481,14 +481,18 @@ static int gem_can_receive(NetClientState *nc)
>> }
>>
>> 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;
>> + if (rx_desc_get_ownership(s->rx_desc[i]) != 1) {
>> + break;
>> + }
>> + };
>> +
>> + if (i == s->num_priority_queues) {
>> + 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]);
>
> This looks a little odd -- surely i isn't the right index to use
> into rx_desc_addr[] any more now we're outside the loop and i
> is always larger than the largest valid queue number? It looks
> like the debug print should be rephrased somehow.
Yeah you are right. It means that they are all busy, we can either
iterate over all of them and print out this or just print one
statement saying that. Somehow I ended up half way between both.
I'll update it to just print that they are all busy. I don't see why
we need every address printed.
Thanks,
Alistair
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set
2017-04-10 12:44 ` Peter Maydell
@ 2017-04-10 22:23 ` Alistair Francis
2017-04-11 9:05 ` Peter Maydell
0 siblings, 1 reply; 18+ messages in thread
From: Alistair Francis @ 2017-04-10 22:23 UTC (permalink / raw)
To: Peter Maydell; +Cc: Alistair Francis, QEMU Developers
On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Only trigger multi-queue GEM interrupts if the interrupt status register
>> is set. This logic was already used for non multi-queue interrupts but
>> it also applies to multi-queue interrupts.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>> hw/net/cadence_gem.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 3e37665..b9eaed4 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
>> }
>>
>> for (i = 0; i < s->num_priority_queues; ++i) {
>> - if (s->regs[GEM_INT_Q1_STATUS + i]) {
>> + if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
>> DB_PRINT("asserting int. (q=%d)\n", i);
>> qemu_set_irq(s->irq[i], 1);
>> }
>
> With this change, if s->regs[GEM_ISR] is zero then the entire
> function never does anything, so you could hoist that up to
> the start, rather than testing it inside the loop and in the
> previous conditional.
Yep, I have moved it to the top.
>
> Also the comment says "raise or lower interrupt based on current
> status", but the code will only ever do qemu_set_irq(..., 1),
> never 0. Which is right?
This is a little confusing. The interrupts are lowered when the ISR is
read, so the assumption was that we never need to clear them in the
gem_update_int_status(). Although then if we perform a reset nothing
will clear the interrupts until the ISR is read from. So that is a
bug. I'll update this patch to fix this up in V2.
Thanks,
Alistair
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set
2017-04-10 22:23 ` Alistair Francis
@ 2017-04-11 9:05 ` Peter Maydell
2017-04-11 16:00 ` Alistair Francis
0 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2017-04-11 9:05 UTC (permalink / raw)
To: Alistair Francis; +Cc: QEMU Developers
On 10 April 2017 at 23:23, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Also the comment says "raise or lower interrupt based on current
>> status", but the code will only ever do qemu_set_irq(..., 1),
>> never 0. Which is right?
>
> This is a little confusing. The interrupts are lowered when the ISR is
> read, so the assumption was that we never need to clear them in the
> gem_update_int_status(). Although then if we perform a reset nothing
> will clear the interrupts until the ISR is read from.
On QEMU reset the other end will be reset anyway so it will
update its idea of whether the irq is asserted (and calling
qemu_set_irq in a reset function is generally not a good idea).
If the device has guest-programmable reset of some kind you'd
need to clear the irq lines then, though.
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set
2017-04-11 9:05 ` Peter Maydell
@ 2017-04-11 16:00 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2017-04-11 16:00 UTC (permalink / raw)
To: Peter Maydell; +Cc: Alistair Francis, QEMU Developers
On Tue, Apr 11, 2017 at 2:05 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 April 2017 at 23:23, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Also the comment says "raise or lower interrupt based on current
>>> status", but the code will only ever do qemu_set_irq(..., 1),
>>> never 0. Which is right?
>>
>> This is a little confusing. The interrupts are lowered when the ISR is
>> read, so the assumption was that we never need to clear them in the
>> gem_update_int_status(). Although then if we perform a reset nothing
>> will clear the interrupts until the ISR is read from.
>
> On QEMU reset the other end will be reset anyway so it will
> update its idea of whether the irq is asserted (and calling
> qemu_set_irq in a reset function is generally not a good idea).
> If the device has guest-programmable reset of some kind you'd
> need to clear the irq lines then, though.
Ok, that explains why it was always working.
I still like the idea of all irq updates (raise or lower) being done
in a single function. Instead of how it was previously where the
raises where in one function and the lower was in the read function.
That way if in some future IP version the ISR is cleared somewhere
else the interrupt logic is ready to handle it.
I did make one small error in my V2 though, so I'll send a V3 with the
consolidation still in it.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-04-11 16:01 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 23:40 [Qemu-devel] [PATCH v1 0/5] Improve the Cadence GEM multi-queue support Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 1/5] cadence_gem: Read the correct queue descriptor Alistair Francis
2017-04-05 10:54 ` Philippe Mathieu-Daudé
2017-04-10 12:39 ` Peter Maydell
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 2/5] cadence_gem: Correct the multi-queue can rx logic Alistair Francis
2017-04-10 12:37 ` Peter Maydell
2017-04-10 20:25 ` Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set Alistair Francis
2017-04-05 11:00 ` Philippe Mathieu-Daudé
2017-04-10 12:44 ` Peter Maydell
2017-04-10 22:23 ` Alistair Francis
2017-04-11 9:05 ` Peter Maydell
2017-04-11 16:00 ` Alistair Francis
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 4/5] cadence_gem: Make the revision a property Alistair Francis
2017-04-05 10:57 ` Philippe Mathieu-Daudé
2017-04-10 12:45 ` Peter Maydell
2017-04-04 23:40 ` [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Set the Cadence GEM revision Alistair Francis
2017-04-10 12:38 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.