* [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
@ 2017-11-28 2:42 Michael Davidsaver
2017-11-29 1:35 ` David Gibson
2018-07-09 6:20 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
0 siblings, 2 replies; 8+ messages in thread
From: Michael Davidsaver @ 2017-11-28 2:42 UTC (permalink / raw)
To: Alexander Graf, Jason Wang, David Gibson; +Cc: qemu-ppc, qemu-devel
Interrupt conditions occurring while masked are not being
signaled when later unmasked.
The fix is to raise/lower IRQs when IMASK is changed.
To avoid problems like this in future, consolidate
IRQ pin update logic in one function.
Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
and update IRQ pins on reset.
Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
hw/net/fsl_etsec/etsec.c | 68 +++++++++++++++++++++++---------------------
hw/net/fsl_etsec/etsec.h | 2 ++
hw/net/fsl_etsec/registers.h | 10 +++++++
hw/net/fsl_etsec/rings.c | 12 +-------
4 files changed, 49 insertions(+), 43 deletions(-)
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 9da1932970..0b66274ce3 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -49,6 +49,28 @@ static const int debug_etsec;
} \
} while (0)
+/* call after any change to IEVENT or IMASK */
+void etsec_update_irq(eTSEC *etsec)
+{
+ uint32_t ievent = etsec->regs[IEVENT].value;
+ uint32_t imask = etsec->regs[IMASK].value;
+ uint32_t active = ievent & imask;
+
+ int tx = !!(active & IEVENT_TX_MASK);
+ int rx = !!(active & IEVENT_RX_MASK);
+ int err = !!(active & IEVENT_ERR_MASK);
+
+ DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
+ __func__, ievent, imask,
+ tx ? 'T' : '_',
+ rx ? 'R' : '_',
+ err ? 'E' : '_');
+
+ qemu_set_irq(etsec->tx_irq, tx);
+ qemu_set_irq(etsec->rx_irq, rx);
+ qemu_set_irq(etsec->err_irq, err);
+}
+
static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
{
eTSEC *etsec = opaque;
@@ -139,31 +161,6 @@ static void write_rbasex(eTSEC *etsec,
etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
}
-static void write_ievent(eTSEC *etsec,
- eTSEC_Register *reg,
- uint32_t reg_index,
- uint32_t value)
-{
- /* Write 1 to clear */
- reg->value &= ~value;
-
- if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
- qemu_irq_lower(etsec->tx_irq);
- }
- if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
- qemu_irq_lower(etsec->rx_irq);
- }
-
- if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
- IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
- IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
- IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
- IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
- IEVENT_MMRW))) {
- qemu_irq_lower(etsec->err_irq);
- }
-}
-
static void write_dmactrl(eTSEC *etsec,
eTSEC_Register *reg,
uint32_t reg_index,
@@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC *etsec,
} else {
/* Graceful receive stop now */
etsec->regs[IEVENT].value |= IEVENT_GRSC;
- if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
- qemu_irq_raise(etsec->err_irq);
- }
+ etsec_update_irq(etsec);
}
}
@@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC *etsec,
} else {
/* Graceful transmit stop now */
etsec->regs[IEVENT].value |= IEVENT_GTSC;
- if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
- qemu_irq_raise(etsec->err_irq);
- }
+ etsec_update_irq(etsec);
}
}
@@ -222,7 +215,16 @@ static void etsec_write(void *opaque,
switch (reg_index) {
case IEVENT:
- write_ievent(etsec, reg, reg_index, value);
+ /* Write 1 to clear */
+ reg->value &= ~value;
+
+ etsec_update_irq(etsec);
+ break;
+
+ case IMASK:
+ reg->value = value;
+
+ etsec_update_irq(etsec);
break;
case DMACTRL:
@@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
MII_SR_EXTENDED_STATUS | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
MII_SR_10T_HD_CAPS | MII_SR_10T_FD_CAPS | MII_SR_100X_HD_CAPS |
MII_SR_100X_FD_CAPS | MII_SR_100T4_CAPS;
+
+ etsec_update_irq(etsec);
}
static ssize_t etsec_receive(NetClientState *nc,
diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index 30c828e241..877988572e 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr base,
qemu_irq rx_irq,
qemu_irq err_irq);
+void etsec_update_irq(eTSEC *etsec);
+
void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
index c4ed2b9d62..f085537ecd 100644
--- a/hw/net/fsl_etsec/registers.h
+++ b/hw/net/fsl_etsec/registers.h
@@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
#define IEVENT_RXC (1 << 30)
#define IEVENT_BABR (1 << 31)
+/* Mapping between interrupt pin and interrupt flags */
+#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
+#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
+#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
+ IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
+ IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
+ IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
+ IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
+ IEVENT_MMRW)
+
#define IMASK_RXFEN (1 << 7)
#define IMASK_GRSCEN (1 << 8)
#define IMASK_RXBEN (1 << 15)
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index d0f93eebfc..337a55fc95 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -152,17 +152,7 @@ static void ievent_set(eTSEC *etsec,
{
etsec->regs[IEVENT].value |= flags;
- if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
- || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
- qemu_irq_raise(etsec->tx_irq);
- RING_DEBUG("%s Raise Tx IRQ\n", __func__);
- }
-
- if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
- || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
- qemu_irq_raise(etsec->rx_irq);
- RING_DEBUG("%s Raise Rx IRQ\n", __func__);
- }
+ etsec_update_irq(etsec);
}
static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
2017-11-28 2:42 [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking Michael Davidsaver
@ 2017-11-29 1:35 ` David Gibson
2018-07-07 18:06 ` Michael Davidsaver
2018-07-09 6:20 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
1 sibling, 1 reply; 8+ messages in thread
From: David Gibson @ 2017-11-29 1:35 UTC (permalink / raw)
To: Michael Davidsaver; +Cc: Alexander Graf, Jason Wang, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7753 bytes --]
On Mon, Nov 27, 2017 at 08:42:13PM -0600, Michael Davidsaver wrote:
> Interrupt conditions occurring while masked are not being
> signaled when later unmasked.
> The fix is to raise/lower IRQs when IMASK is changed.
>
> To avoid problems like this in future, consolidate
> IRQ pin update logic in one function.
>
> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> and update IRQ pins on reset.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
Looks sane, but I would like to get an R-b from someone who knows FSL
better than me before applying.
> ---
> hw/net/fsl_etsec/etsec.c | 68 +++++++++++++++++++++++---------------------
> hw/net/fsl_etsec/etsec.h | 2 ++
> hw/net/fsl_etsec/registers.h | 10 +++++++
> hw/net/fsl_etsec/rings.c | 12 +-------
> 4 files changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 9da1932970..0b66274ce3 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -49,6 +49,28 @@ static const int debug_etsec;
> } \
> } while (0)
>
> +/* call after any change to IEVENT or IMASK */
> +void etsec_update_irq(eTSEC *etsec)
> +{
> + uint32_t ievent = etsec->regs[IEVENT].value;
> + uint32_t imask = etsec->regs[IMASK].value;
> + uint32_t active = ievent & imask;
> +
> + int tx = !!(active & IEVENT_TX_MASK);
> + int rx = !!(active & IEVENT_RX_MASK);
> + int err = !!(active & IEVENT_ERR_MASK);
> +
> + DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
> + __func__, ievent, imask,
> + tx ? 'T' : '_',
> + rx ? 'R' : '_',
> + err ? 'E' : '_');
> +
> + qemu_set_irq(etsec->tx_irq, tx);
> + qemu_set_irq(etsec->rx_irq, rx);
> + qemu_set_irq(etsec->err_irq, err);
> +}
> +
> static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
> {
> eTSEC *etsec = opaque;
> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC *etsec,
> etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
> }
>
> -static void write_ievent(eTSEC *etsec,
> - eTSEC_Register *reg,
> - uint32_t reg_index,
> - uint32_t value)
> -{
> - /* Write 1 to clear */
> - reg->value &= ~value;
> -
> - if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> - qemu_irq_lower(etsec->tx_irq);
> - }
> - if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> - qemu_irq_lower(etsec->rx_irq);
> - }
> -
> - if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> - IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> - IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> - IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> - IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> - IEVENT_MMRW))) {
> - qemu_irq_lower(etsec->err_irq);
> - }
> -}
> -
> static void write_dmactrl(eTSEC *etsec,
> eTSEC_Register *reg,
> uint32_t reg_index,
> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC *etsec,
> } else {
> /* Graceful receive stop now */
> etsec->regs[IEVENT].value |= IEVENT_GRSC;
> - if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> - qemu_irq_raise(etsec->err_irq);
> - }
> + etsec_update_irq(etsec);
> }
> }
>
> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC *etsec,
> } else {
> /* Graceful transmit stop now */
> etsec->regs[IEVENT].value |= IEVENT_GTSC;
> - if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> - qemu_irq_raise(etsec->err_irq);
> - }
> + etsec_update_irq(etsec);
> }
> }
>
> @@ -222,7 +215,16 @@ static void etsec_write(void *opaque,
>
> switch (reg_index) {
> case IEVENT:
> - write_ievent(etsec, reg, reg_index, value);
> + /* Write 1 to clear */
> + reg->value &= ~value;
> +
> + etsec_update_irq(etsec);
> + break;
> +
> + case IMASK:
> + reg->value = value;
> +
> + etsec_update_irq(etsec);
> break;
>
> case DMACTRL:
> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
> MII_SR_EXTENDED_STATUS | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
> MII_SR_10T_HD_CAPS | MII_SR_10T_FD_CAPS | MII_SR_100X_HD_CAPS |
> MII_SR_100X_FD_CAPS | MII_SR_100T4_CAPS;
> +
> + etsec_update_irq(etsec);
> }
>
> static ssize_t etsec_receive(NetClientState *nc,
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 30c828e241..877988572e 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr base,
> qemu_irq rx_irq,
> qemu_irq err_irq);
>
> +void etsec_update_irq(eTSEC *etsec);
> +
> void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
> void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
> ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> index c4ed2b9d62..f085537ecd 100644
> --- a/hw/net/fsl_etsec/registers.h
> +++ b/hw/net/fsl_etsec/registers.h
> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
> #define IEVENT_RXC (1 << 30)
> #define IEVENT_BABR (1 << 31)
>
> +/* Mapping between interrupt pin and interrupt flags */
> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> + IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> + IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> + IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> + IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> + IEVENT_MMRW)
> +
> #define IMASK_RXFEN (1 << 7)
> #define IMASK_GRSCEN (1 << 8)
> #define IMASK_RXBEN (1 << 15)
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d0f93eebfc..337a55fc95 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC *etsec,
> {
> etsec->regs[IEVENT].value |= flags;
>
> - if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> - || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> - qemu_irq_raise(etsec->tx_irq);
> - RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> - }
> -
> - if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> - || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> - qemu_irq_raise(etsec->rx_irq);
> - RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> - }
> + etsec_update_irq(etsec);
> }
>
> static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
2017-11-29 1:35 ` David Gibson
@ 2018-07-07 18:06 ` Michael Davidsaver
2018-07-09 3:56 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: Michael Davidsaver @ 2018-07-07 18:06 UTC (permalink / raw)
To: David Gibson; +Cc: Alexander Graf, Jason Wang, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7856 bytes --]
On 11/28/2017 05:35 PM, David Gibson wrote:
> On Mon, Nov 27, 2017 at 08:42:13PM -0600, Michael Davidsaver wrote:
>> Interrupt conditions occurring while masked are not being
>> signaled when later unmasked.
>> The fix is to raise/lower IRQs when IMASK is changed.
>>
>> To avoid problems like this in future, consolidate
>> IRQ pin update logic in one function.
>>
>> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
>> and update IRQ pins on reset.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>
> Looks sane, but I would like to get an R-b from someone who knows FSL
> better than me before applying.
Any progress on finding a candidate to comment?
>> ---
>> hw/net/fsl_etsec/etsec.c | 68 +++++++++++++++++++++++---------------------
>> hw/net/fsl_etsec/etsec.h | 2 ++
>> hw/net/fsl_etsec/registers.h | 10 +++++++
>> hw/net/fsl_etsec/rings.c | 12 +-------
>> 4 files changed, 49 insertions(+), 43 deletions(-)
>>
>> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
>> index 9da1932970..0b66274ce3 100644
>> --- a/hw/net/fsl_etsec/etsec.c
>> +++ b/hw/net/fsl_etsec/etsec.c
>> @@ -49,6 +49,28 @@ static const int debug_etsec;
>> } \
>> } while (0)
>>
>> +/* call after any change to IEVENT or IMASK */
>> +void etsec_update_irq(eTSEC *etsec)
>> +{
>> + uint32_t ievent = etsec->regs[IEVENT].value;
>> + uint32_t imask = etsec->regs[IMASK].value;
>> + uint32_t active = ievent & imask;
>> +
>> + int tx = !!(active & IEVENT_TX_MASK);
>> + int rx = !!(active & IEVENT_RX_MASK);
>> + int err = !!(active & IEVENT_ERR_MASK);
>> +
>> + DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
>> + __func__, ievent, imask,
>> + tx ? 'T' : '_',
>> + rx ? 'R' : '_',
>> + err ? 'E' : '_');
>> +
>> + qemu_set_irq(etsec->tx_irq, tx);
>> + qemu_set_irq(etsec->rx_irq, rx);
>> + qemu_set_irq(etsec->err_irq, err);
>> +}
>> +
>> static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
>> {
>> eTSEC *etsec = opaque;
>> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC *etsec,
>> etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
>> }
>>
>> -static void write_ievent(eTSEC *etsec,
>> - eTSEC_Register *reg,
>> - uint32_t reg_index,
>> - uint32_t value)
>> -{
>> - /* Write 1 to clear */
>> - reg->value &= ~value;
>> -
>> - if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
>> - qemu_irq_lower(etsec->tx_irq);
>> - }
>> - if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
>> - qemu_irq_lower(etsec->rx_irq);
>> - }
>> -
>> - if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
>> - IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
>> - IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
>> - IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
>> - IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
>> - IEVENT_MMRW))) {
>> - qemu_irq_lower(etsec->err_irq);
>> - }
>> -}
>> -
>> static void write_dmactrl(eTSEC *etsec,
>> eTSEC_Register *reg,
>> uint32_t reg_index,
>> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC *etsec,
>> } else {
>> /* Graceful receive stop now */
>> etsec->regs[IEVENT].value |= IEVENT_GRSC;
>> - if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
>> - qemu_irq_raise(etsec->err_irq);
>> - }
>> + etsec_update_irq(etsec);
>> }
>> }
>>
>> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC *etsec,
>> } else {
>> /* Graceful transmit stop now */
>> etsec->regs[IEVENT].value |= IEVENT_GTSC;
>> - if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
>> - qemu_irq_raise(etsec->err_irq);
>> - }
>> + etsec_update_irq(etsec);
>> }
>> }
>>
>> @@ -222,7 +215,16 @@ static void etsec_write(void *opaque,
>>
>> switch (reg_index) {
>> case IEVENT:
>> - write_ievent(etsec, reg, reg_index, value);
>> + /* Write 1 to clear */
>> + reg->value &= ~value;
>> +
>> + etsec_update_irq(etsec);
>> + break;
>> +
>> + case IMASK:
>> + reg->value = value;
>> +
>> + etsec_update_irq(etsec);
>> break;
>>
>> case DMACTRL:
>> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
>> MII_SR_EXTENDED_STATUS | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
>> MII_SR_10T_HD_CAPS | MII_SR_10T_FD_CAPS | MII_SR_100X_HD_CAPS |
>> MII_SR_100X_FD_CAPS | MII_SR_100T4_CAPS;
>> +
>> + etsec_update_irq(etsec);
>> }
>>
>> static ssize_t etsec_receive(NetClientState *nc,
>> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
>> index 30c828e241..877988572e 100644
>> --- a/hw/net/fsl_etsec/etsec.h
>> +++ b/hw/net/fsl_etsec/etsec.h
>> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr base,
>> qemu_irq rx_irq,
>> qemu_irq err_irq);
>>
>> +void etsec_update_irq(eTSEC *etsec);
>> +
>> void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
>> void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
>> ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
>> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
>> index c4ed2b9d62..f085537ecd 100644
>> --- a/hw/net/fsl_etsec/registers.h
>> +++ b/hw/net/fsl_etsec/registers.h
>> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
>> #define IEVENT_RXC (1 << 30)
>> #define IEVENT_BABR (1 << 31)
>>
>> +/* Mapping between interrupt pin and interrupt flags */
>> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
>> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
>> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
>> + IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
>> + IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
>> + IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
>> + IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
>> + IEVENT_MMRW)
>> +
>> #define IMASK_RXFEN (1 << 7)
>> #define IMASK_GRSCEN (1 << 8)
>> #define IMASK_RXBEN (1 << 15)
>> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
>> index d0f93eebfc..337a55fc95 100644
>> --- a/hw/net/fsl_etsec/rings.c
>> +++ b/hw/net/fsl_etsec/rings.c
>> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC *etsec,
>> {
>> etsec->regs[IEVENT].value |= flags;
>>
>> - if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
>> - || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
>> - qemu_irq_raise(etsec->tx_irq);
>> - RING_DEBUG("%s Raise Tx IRQ\n", __func__);
>> - }
>> -
>> - if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
>> - || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
>> - qemu_irq_raise(etsec->rx_irq);
>> - RING_DEBUG("%s Raise Rx IRQ\n", __func__);
>> - }
>> + etsec_update_irq(etsec);
>> }
>>
>> static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking
2018-07-07 18:06 ` Michael Davidsaver
@ 2018-07-09 3:56 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2018-07-09 3:56 UTC (permalink / raw)
To: Michael Davidsaver; +Cc: Alexander Graf, Jason Wang, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 8602 bytes --]
On Sat, Jul 07, 2018 at 11:06:05AM -0700, Michael Davidsaver wrote:
> On 11/28/2017 05:35 PM, David Gibson wrote:
> > On Mon, Nov 27, 2017 at 08:42:13PM -0600, Michael Davidsaver wrote:
> >> Interrupt conditions occurring while masked are not being
> >> signaled when later unmasked.
> >> The fix is to raise/lower IRQs when IMASK is changed.
> >>
> >> To avoid problems like this in future, consolidate
> >> IRQ pin update logic in one function.
> >>
> >> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> >> and update IRQ pins on reset.
> >>
> >> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> >
> > Looks sane, but I would like to get an R-b from someone who knows FSL
> > better than me before applying.
>
> Any progress on finding a candidate to comment?
I'm afraid not, I was kind of hoping someone would respond on list.
>
>
> >> ---
> >> hw/net/fsl_etsec/etsec.c | 68 +++++++++++++++++++++++---------------------
> >> hw/net/fsl_etsec/etsec.h | 2 ++
> >> hw/net/fsl_etsec/registers.h | 10 +++++++
> >> hw/net/fsl_etsec/rings.c | 12 +-------
> >> 4 files changed, 49 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> >> index 9da1932970..0b66274ce3 100644
> >> --- a/hw/net/fsl_etsec/etsec.c
> >> +++ b/hw/net/fsl_etsec/etsec.c
> >> @@ -49,6 +49,28 @@ static const int debug_etsec;
> >> } \
> >> } while (0)
> >>
> >> +/* call after any change to IEVENT or IMASK */
> >> +void etsec_update_irq(eTSEC *etsec)
> >> +{
> >> + uint32_t ievent = etsec->regs[IEVENT].value;
> >> + uint32_t imask = etsec->regs[IMASK].value;
> >> + uint32_t active = ievent & imask;
> >> +
> >> + int tx = !!(active & IEVENT_TX_MASK);
> >> + int rx = !!(active & IEVENT_RX_MASK);
> >> + int err = !!(active & IEVENT_ERR_MASK);
> >> +
> >> + DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
> >> + __func__, ievent, imask,
> >> + tx ? 'T' : '_',
> >> + rx ? 'R' : '_',
> >> + err ? 'E' : '_');
> >> +
> >> + qemu_set_irq(etsec->tx_irq, tx);
> >> + qemu_set_irq(etsec->rx_irq, rx);
> >> + qemu_set_irq(etsec->err_irq, err);
> >> +}
> >> +
> >> static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
> >> {
> >> eTSEC *etsec = opaque;
> >> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC *etsec,
> >> etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
> >> }
> >>
> >> -static void write_ievent(eTSEC *etsec,
> >> - eTSEC_Register *reg,
> >> - uint32_t reg_index,
> >> - uint32_t value)
> >> -{
> >> - /* Write 1 to clear */
> >> - reg->value &= ~value;
> >> -
> >> - if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> >> - qemu_irq_lower(etsec->tx_irq);
> >> - }
> >> - if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> >> - qemu_irq_lower(etsec->rx_irq);
> >> - }
> >> -
> >> - if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> >> - IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> >> - IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> >> - IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> >> - IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> >> - IEVENT_MMRW))) {
> >> - qemu_irq_lower(etsec->err_irq);
> >> - }
> >> -}
> >> -
> >> static void write_dmactrl(eTSEC *etsec,
> >> eTSEC_Register *reg,
> >> uint32_t reg_index,
> >> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC *etsec,
> >> } else {
> >> /* Graceful receive stop now */
> >> etsec->regs[IEVENT].value |= IEVENT_GRSC;
> >> - if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> >> - qemu_irq_raise(etsec->err_irq);
> >> - }
> >> + etsec_update_irq(etsec);
> >> }
> >> }
> >>
> >> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC *etsec,
> >> } else {
> >> /* Graceful transmit stop now */
> >> etsec->regs[IEVENT].value |= IEVENT_GTSC;
> >> - if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> >> - qemu_irq_raise(etsec->err_irq);
> >> - }
> >> + etsec_update_irq(etsec);
> >> }
> >> }
> >>
> >> @@ -222,7 +215,16 @@ static void etsec_write(void *opaque,
> >>
> >> switch (reg_index) {
> >> case IEVENT:
> >> - write_ievent(etsec, reg, reg_index, value);
> >> + /* Write 1 to clear */
> >> + reg->value &= ~value;
> >> +
> >> + etsec_update_irq(etsec);
> >> + break;
> >> +
> >> + case IMASK:
> >> + reg->value = value;
> >> +
> >> + etsec_update_irq(etsec);
> >> break;
> >>
> >> case DMACTRL:
> >> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
> >> MII_SR_EXTENDED_STATUS | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
> >> MII_SR_10T_HD_CAPS | MII_SR_10T_FD_CAPS | MII_SR_100X_HD_CAPS |
> >> MII_SR_100X_FD_CAPS | MII_SR_100T4_CAPS;
> >> +
> >> + etsec_update_irq(etsec);
> >> }
> >>
> >> static ssize_t etsec_receive(NetClientState *nc,
> >> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> >> index 30c828e241..877988572e 100644
> >> --- a/hw/net/fsl_etsec/etsec.h
> >> +++ b/hw/net/fsl_etsec/etsec.h
> >> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr base,
> >> qemu_irq rx_irq,
> >> qemu_irq err_irq);
> >>
> >> +void etsec_update_irq(eTSEC *etsec);
> >> +
> >> void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
> >> void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
> >> ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> >> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> >> index c4ed2b9d62..f085537ecd 100644
> >> --- a/hw/net/fsl_etsec/registers.h
> >> +++ b/hw/net/fsl_etsec/registers.h
> >> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
> >> #define IEVENT_RXC (1 << 30)
> >> #define IEVENT_BABR (1 << 31)
> >>
> >> +/* Mapping between interrupt pin and interrupt flags */
> >> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> >> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> >> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> >> + IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> >> + IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> >> + IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> >> + IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> >> + IEVENT_MMRW)
> >> +
> >> #define IMASK_RXFEN (1 << 7)
> >> #define IMASK_GRSCEN (1 << 8)
> >> #define IMASK_RXBEN (1 << 15)
> >> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> >> index d0f93eebfc..337a55fc95 100644
> >> --- a/hw/net/fsl_etsec/rings.c
> >> +++ b/hw/net/fsl_etsec/rings.c
> >> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC *etsec,
> >> {
> >> etsec->regs[IEVENT].value |= flags;
> >>
> >> - if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> >> - || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> >> - qemu_irq_raise(etsec->tx_irq);
> >> - RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> >> - }
> >> -
> >> - if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> >> - || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> >> - qemu_irq_raise(etsec->rx_irq);
> >> - RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> >> - }
> >> + etsec_update_irq(etsec);
> >> }
> >>
> >> static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
> >
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH] etsec: fix IRQ (un)masking
2017-11-28 2:42 [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking Michael Davidsaver
2017-11-29 1:35 ` David Gibson
@ 2018-07-09 6:20 ` Cédric Le Goater
2018-07-09 6:25 ` David Gibson
1 sibling, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2018-07-09 6:20 UTC (permalink / raw)
To: Michael Davidsaver, Alexander Graf, Jason Wang, David Gibson
Cc: qemu-ppc, qemu-devel
On 11/28/2017 03:42 AM, Michael Davidsaver wrote:
> Interrupt conditions occurring while masked are not being
> signaled when later unmasked.
> The fix is to raise/lower IRQs when IMASK is changed.
>
> To avoid problems like this in future, consolidate
> IRQ pin update logic in one function.
it makes sense.
> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> and update IRQ pins on reset.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
> hw/net/fsl_etsec/etsec.c | 68 +++++++++++++++++++++++---------------------
> hw/net/fsl_etsec/etsec.h | 2 ++
> hw/net/fsl_etsec/registers.h | 10 +++++++
> hw/net/fsl_etsec/rings.c | 12 +-------
> 4 files changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 9da1932970..0b66274ce3 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -49,6 +49,28 @@ static const int debug_etsec;
> } \
> } while (0)
>
> +/* call after any change to IEVENT or IMASK */
> +void etsec_update_irq(eTSEC *etsec)
> +{
> + uint32_t ievent = etsec->regs[IEVENT].value;
> + uint32_t imask = etsec->regs[IMASK].value;
> + uint32_t active = ievent & imask;
> +
> + int tx = !!(active & IEVENT_TX_MASK);
> + int rx = !!(active & IEVENT_RX_MASK);
> + int err = !!(active & IEVENT_ERR_MASK);
you could make these bools. this is minor.
> + DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c\n",
> + __func__, ievent, imask,
> + tx ? 'T' : '_',
> + rx ? 'R' : '_',
> + err ? 'E' : '_');
> +
> + qemu_set_irq(etsec->tx_irq, tx);
> + qemu_set_irq(etsec->rx_irq, rx);
> + qemu_set_irq(etsec->err_irq, err);
> +}
> +
> static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
> {
> eTSEC *etsec = opaque;
> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC *etsec,
> etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
> }
>
> -static void write_ievent(eTSEC *etsec,
> - eTSEC_Register *reg,
> - uint32_t reg_index,
> - uint32_t value)
> -{
> - /* Write 1 to clear */
> - reg->value &= ~value;
> -
> - if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> - qemu_irq_lower(etsec->tx_irq);
> - }
> - if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> - qemu_irq_lower(etsec->rx_irq);
> - }
> -
> - if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> - IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> - IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> - IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> - IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> - IEVENT_MMRW))) {
> - qemu_irq_lower(etsec->err_irq);
> - }
> -}
> -
> static void write_dmactrl(eTSEC *etsec,
> eTSEC_Register *reg,
> uint32_t reg_index,
> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC *etsec,
> } else {
> /* Graceful receive stop now */
> etsec->regs[IEVENT].value |= IEVENT_GRSC;
> - if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> - qemu_irq_raise(etsec->err_irq);
> - }
> + etsec_update_irq(etsec);
> }
> }
>
> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC *etsec,
> } else {
> /* Graceful transmit stop now */
> etsec->regs[IEVENT].value |= IEVENT_GTSC;
> - if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> - qemu_irq_raise(etsec->err_irq);
> - }
> + etsec_update_irq(etsec);
> }
> }
>
> @@ -222,7 +215,16 @@ static void etsec_write(void *opaque,
>
> switch (reg_index) {
> case IEVENT:
> - write_ievent(etsec, reg, reg_index, value);
> + /* Write 1 to clear */
> + reg->value &= ~value;
> +
> + etsec_update_irq(etsec);
> + break;
> +
> + case IMASK:
> + reg->value = value;
> +
> + etsec_update_irq(etsec);
> break;
>
> case DMACTRL:
> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
> MII_SR_EXTENDED_STATUS | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
> MII_SR_10T_HD_CAPS | MII_SR_10T_FD_CAPS | MII_SR_100X_HD_CAPS |
> MII_SR_100X_FD_CAPS | MII_SR_100T4_CAPS;
> +
> + etsec_update_irq(etsec);>
> }
>
> static ssize_t etsec_receive(NetClientState *nc,
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 30c828e241..877988572e 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr base,
> qemu_irq rx_irq,
> qemu_irq err_irq);
>
> +void etsec_update_irq(eTSEC *etsec);
> +
> void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
> void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
> ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> index c4ed2b9d62..f085537ecd 100644
> --- a/hw/net/fsl_etsec/registers.h
> +++ b/hw/net/fsl_etsec/registers.h
> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
> #define IEVENT_RXC (1 << 30)
> #define IEVENT_BABR (1 << 31)
>
> +/* Mapping between interrupt pin and interrupt flags */
> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> + IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> + IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> + IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> + IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> + IEVENT_MMRW)
> +
> #define IMASK_RXFEN (1 << 7)
> #define IMASK_GRSCEN (1 << 8)
> #define IMASK_RXBEN (1 << 15)
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d0f93eebfc..337a55fc95 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC *etsec,
> {
> etsec->regs[IEVENT].value |= flags;
>
> - if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> - || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> - qemu_irq_raise(etsec->tx_irq);
> - RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> - }
> -
> - if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> - || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> - qemu_irq_raise(etsec->rx_irq);
> - RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> - }
> + etsec_update_irq(etsec);
> }
>
> static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH] etsec: fix IRQ (un)masking
2018-07-09 6:20 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
@ 2018-07-09 6:25 ` David Gibson
2018-07-12 21:00 ` [Qemu-devel] [PATCH 1/1] " Michael Davidsaver
0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2018-07-09 6:25 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Michael Davidsaver, Alexander Graf, Jason Wang, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 915 bytes --]
On Mon, Jul 09, 2018 at 08:20:03AM +0200, Cédric Le Goater wrote:
11;rgb:ffff/ffff/ffff> On 11/28/2017 03:42 AM, Michael Davidsaver wrote:
> > Interrupt conditions occurring while masked are not being
> > signaled when later unmasked.
> > The fix is to raise/lower IRQs when IMASK is changed.
> >
> > To avoid problems like this in future, consolidate
> > IRQ pin update logic in one function.
>
> it makes sense.
>
> > Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> > and update IRQ pins on reset.
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks for the review Cédric. Unfortunately, I've mislaid the
original patch.
Michael, can you resend with Cédric's R-b included, please.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 1/1] etsec: fix IRQ (un)masking
2018-07-09 6:25 ` David Gibson
@ 2018-07-12 21:00 ` Michael Davidsaver
2018-07-13 0:36 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: Michael Davidsaver @ 2018-07-12 21:00 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, Alexander Graf, Jason Wang, qemu-ppc,
qemu-devel, Michael Davidsaver
Interrupt conditions occurring while masked are not being
signaled when later unmasked.
The fix is to raise/lower IRQs when IMASK is changed.
To avoid problems like this in future, consolidate
IRQ pin update logic in one function.
Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
and update IRQ pins on reset.
Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
hw/net/fsl_etsec/etsec.c | 68 +++++++++++++++++++++++---------------------
hw/net/fsl_etsec/etsec.h | 2 ++
hw/net/fsl_etsec/registers.h | 10 +++++++
hw/net/fsl_etsec/rings.c | 12 +-------
4 files changed, 49 insertions(+), 43 deletions(-)
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 9da1932970..0b66274ce3 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -49,6 +49,28 @@ static const int debug_etsec;
} \
} while (0)
+/* call after any change to IEVENT or IMASK */
+void etsec_update_irq(eTSEC *etsec)
+{
+ uint32_t ievent = etsec->regs[IEVENT].value;
+ uint32_t imask = etsec->regs[IMASK].value;
+ uint32_t active = ievent & imask;
+
+ int tx = !!(active & IEVENT_TX_MASK);
+ int rx = !!(active & IEVENT_RX_MASK);
+ int err = !!(active & IEVENT_ERR_MASK);
+
+ DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c",
+ __func__, ievent, imask,
+ tx ? 'T' : '_',
+ rx ? 'R' : '_',
+ err ? 'E' : '_');
+
+ qemu_set_irq(etsec->tx_irq, tx);
+ qemu_set_irq(etsec->rx_irq, rx);
+ qemu_set_irq(etsec->err_irq, err);
+}
+
static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
{
eTSEC *etsec = opaque;
@@ -139,31 +161,6 @@ static void write_rbasex(eTSEC *etsec,
etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
}
-static void write_ievent(eTSEC *etsec,
- eTSEC_Register *reg,
- uint32_t reg_index,
- uint32_t value)
-{
- /* Write 1 to clear */
- reg->value &= ~value;
-
- if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
- qemu_irq_lower(etsec->tx_irq);
- }
- if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
- qemu_irq_lower(etsec->rx_irq);
- }
-
- if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
- IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
- IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
- IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
- IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
- IEVENT_MMRW))) {
- qemu_irq_lower(etsec->err_irq);
- }
-}
-
static void write_dmactrl(eTSEC *etsec,
eTSEC_Register *reg,
uint32_t reg_index,
@@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC *etsec,
} else {
/* Graceful receive stop now */
etsec->regs[IEVENT].value |= IEVENT_GRSC;
- if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
- qemu_irq_raise(etsec->err_irq);
- }
+ etsec_update_irq(etsec);
}
}
@@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC *etsec,
} else {
/* Graceful transmit stop now */
etsec->regs[IEVENT].value |= IEVENT_GTSC;
- if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
- qemu_irq_raise(etsec->err_irq);
- }
+ etsec_update_irq(etsec);
}
}
@@ -222,7 +215,16 @@ static void etsec_write(void *opaque,
switch (reg_index) {
case IEVENT:
- write_ievent(etsec, reg, reg_index, value);
+ /* Write 1 to clear */
+ reg->value &= ~value;
+
+ etsec_update_irq(etsec);
+ break;
+
+ case IMASK:
+ reg->value = value;
+
+ etsec_update_irq(etsec);
break;
case DMACTRL:
@@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
MII_SR_EXTENDED_STATUS | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
MII_SR_10T_HD_CAPS | MII_SR_10T_FD_CAPS | MII_SR_100X_HD_CAPS |
MII_SR_100X_FD_CAPS | MII_SR_100T4_CAPS;
+
+ etsec_update_irq(etsec);
}
static ssize_t etsec_receive(NetClientState *nc,
diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index 30c828e241..877988572e 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr base,
qemu_irq rx_irq,
qemu_irq err_irq);
+void etsec_update_irq(eTSEC *etsec);
+
void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
index c4ed2b9d62..f085537ecd 100644
--- a/hw/net/fsl_etsec/registers.h
+++ b/hw/net/fsl_etsec/registers.h
@@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
#define IEVENT_RXC (1 << 30)
#define IEVENT_BABR (1 << 31)
+/* Mapping between interrupt pin and interrupt flags */
+#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
+#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
+#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
+ IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
+ IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
+ IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
+ IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
+ IEVENT_MMRW)
+
#define IMASK_RXFEN (1 << 7)
#define IMASK_GRSCEN (1 << 8)
#define IMASK_RXBEN (1 << 15)
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index d0f93eebfc..337a55fc95 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -152,17 +152,7 @@ static void ievent_set(eTSEC *etsec,
{
etsec->regs[IEVENT].value |= flags;
- if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
- || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
- qemu_irq_raise(etsec->tx_irq);
- RING_DEBUG("%s Raise Tx IRQ\n", __func__);
- }
-
- if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
- || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
- qemu_irq_raise(etsec->rx_irq);
- RING_DEBUG("%s Raise Rx IRQ\n", __func__);
- }
+ etsec_update_irq(etsec);
}
static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] etsec: fix IRQ (un)masking
2018-07-12 21:00 ` [Qemu-devel] [PATCH 1/1] " Michael Davidsaver
@ 2018-07-13 0:36 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2018-07-13 0:36 UTC (permalink / raw)
To: Michael Davidsaver
Cc: Cédric Le Goater, Alexander Graf, Jason Wang, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7720 bytes --]
On Thu, Jul 12, 2018 at 02:00:52PM -0700, Michael Davidsaver wrote:
> Interrupt conditions occurring while masked are not being
> signaled when later unmasked.
> The fix is to raise/lower IRQs when IMASK is changed.
>
> To avoid problems like this in future, consolidate
> IRQ pin update logic in one function.
>
> Also fix probable typo "IEVENT_TXF | IEVENT_TXF",
> and update IRQ pins on reset.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
Applied to ppc-for-3.0.
> ---
> hw/net/fsl_etsec/etsec.c | 68 +++++++++++++++++++++++---------------------
> hw/net/fsl_etsec/etsec.h | 2 ++
> hw/net/fsl_etsec/registers.h | 10 +++++++
> hw/net/fsl_etsec/rings.c | 12 +-------
> 4 files changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 9da1932970..0b66274ce3 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -49,6 +49,28 @@ static const int debug_etsec;
> } \
> } while (0)
>
> +/* call after any change to IEVENT or IMASK */
> +void etsec_update_irq(eTSEC *etsec)
> +{
> + uint32_t ievent = etsec->regs[IEVENT].value;
> + uint32_t imask = etsec->regs[IMASK].value;
> + uint32_t active = ievent & imask;
> +
> + int tx = !!(active & IEVENT_TX_MASK);
> + int rx = !!(active & IEVENT_RX_MASK);
> + int err = !!(active & IEVENT_ERR_MASK);
> +
> + DPRINTF("%s IRQ ievent=%"PRIx32" imask=%"PRIx32" %c%c%c",
> + __func__, ievent, imask,
> + tx ? 'T' : '_',
> + rx ? 'R' : '_',
> + err ? 'E' : '_');
> +
> + qemu_set_irq(etsec->tx_irq, tx);
> + qemu_set_irq(etsec->rx_irq, rx);
> + qemu_set_irq(etsec->err_irq, err);
> +}
> +
> static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
> {
> eTSEC *etsec = opaque;
> @@ -139,31 +161,6 @@ static void write_rbasex(eTSEC *etsec,
> etsec->regs[RBPTR0 + (reg_index - RBASE0)].value = value & ~0x7;
> }
>
> -static void write_ievent(eTSEC *etsec,
> - eTSEC_Register *reg,
> - uint32_t reg_index,
> - uint32_t value)
> -{
> - /* Write 1 to clear */
> - reg->value &= ~value;
> -
> - if (!(reg->value & (IEVENT_TXF | IEVENT_TXF))) {
> - qemu_irq_lower(etsec->tx_irq);
> - }
> - if (!(reg->value & (IEVENT_RXF | IEVENT_RXF))) {
> - qemu_irq_lower(etsec->rx_irq);
> - }
> -
> - if (!(reg->value & (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC |
> - IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC |
> - IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ |
> - IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE |
> - IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD |
> - IEVENT_MMRW))) {
> - qemu_irq_lower(etsec->err_irq);
> - }
> -}
> -
> static void write_dmactrl(eTSEC *etsec,
> eTSEC_Register *reg,
> uint32_t reg_index,
> @@ -178,9 +175,7 @@ static void write_dmactrl(eTSEC *etsec,
> } else {
> /* Graceful receive stop now */
> etsec->regs[IEVENT].value |= IEVENT_GRSC;
> - if (etsec->regs[IMASK].value & IMASK_GRSCEN) {
> - qemu_irq_raise(etsec->err_irq);
> - }
> + etsec_update_irq(etsec);
> }
> }
>
> @@ -191,9 +186,7 @@ static void write_dmactrl(eTSEC *etsec,
> } else {
> /* Graceful transmit stop now */
> etsec->regs[IEVENT].value |= IEVENT_GTSC;
> - if (etsec->regs[IMASK].value & IMASK_GTSCEN) {
> - qemu_irq_raise(etsec->err_irq);
> - }
> + etsec_update_irq(etsec);
> }
> }
>
> @@ -222,7 +215,16 @@ static void etsec_write(void *opaque,
>
> switch (reg_index) {
> case IEVENT:
> - write_ievent(etsec, reg, reg_index, value);
> + /* Write 1 to clear */
> + reg->value &= ~value;
> +
> + etsec_update_irq(etsec);
> + break;
> +
> + case IMASK:
> + reg->value = value;
> +
> + etsec_update_irq(etsec);
> break;
>
> case DMACTRL:
> @@ -337,6 +339,8 @@ static void etsec_reset(DeviceState *d)
> MII_SR_EXTENDED_STATUS | MII_SR_100T2_HD_CAPS | MII_SR_100T2_FD_CAPS |
> MII_SR_10T_HD_CAPS | MII_SR_10T_FD_CAPS | MII_SR_100X_HD_CAPS |
> MII_SR_100X_FD_CAPS | MII_SR_100T4_CAPS;
> +
> + etsec_update_irq(etsec);
> }
>
> static ssize_t etsec_receive(NetClientState *nc,
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 30c828e241..877988572e 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -163,6 +163,8 @@ DeviceState *etsec_create(hwaddr base,
> qemu_irq rx_irq,
> qemu_irq err_irq);
>
> +void etsec_update_irq(eTSEC *etsec);
> +
> void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
> void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
> ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> diff --git a/hw/net/fsl_etsec/registers.h b/hw/net/fsl_etsec/registers.h
> index c4ed2b9d62..f085537ecd 100644
> --- a/hw/net/fsl_etsec/registers.h
> +++ b/hw/net/fsl_etsec/registers.h
> @@ -74,6 +74,16 @@ extern const eTSEC_Register_Definition eTSEC_registers_def[];
> #define IEVENT_RXC (1 << 30)
> #define IEVENT_BABR (1 << 31)
>
> +/* Mapping between interrupt pin and interrupt flags */
> +#define IEVENT_RX_MASK (IEVENT_RXF | IEVENT_RXB)
> +#define IEVENT_TX_MASK (IEVENT_TXF | IEVENT_TXB)
> +#define IEVENT_ERR_MASK (IEVENT_MAG | IEVENT_GTSC | IEVENT_GRSC | IEVENT_TXC | \
> + IEVENT_RXC | IEVENT_BABR | IEVENT_BABT | IEVENT_LC | \
> + IEVENT_CRL | IEVENT_FGPI | IEVENT_FIR | IEVENT_FIQ | \
> + IEVENT_DPE | IEVENT_PERR | IEVENT_EBERR | IEVENT_TXE | \
> + IEVENT_XFUN | IEVENT_BSY | IEVENT_MSRO | IEVENT_MMRD | \
> + IEVENT_MMRW)
> +
> #define IMASK_RXFEN (1 << 7)
> #define IMASK_GRSCEN (1 << 8)
> #define IMASK_RXBEN (1 << 15)
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d0f93eebfc..337a55fc95 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -152,17 +152,7 @@ static void ievent_set(eTSEC *etsec,
> {
> etsec->regs[IEVENT].value |= flags;
>
> - if ((flags & IEVENT_TXB && etsec->regs[IMASK].value & IMASK_TXBEN)
> - || (flags & IEVENT_TXF && etsec->regs[IMASK].value & IMASK_TXFEN)) {
> - qemu_irq_raise(etsec->tx_irq);
> - RING_DEBUG("%s Raise Tx IRQ\n", __func__);
> - }
> -
> - if ((flags & IEVENT_RXB && etsec->regs[IMASK].value & IMASK_RXBEN)
> - || (flags & IEVENT_RXF && etsec->regs[IMASK].value & IMASK_RXFEN)) {
> - qemu_irq_raise(etsec->rx_irq);
> - RING_DEBUG("%s Raise Rx IRQ\n", __func__);
> - }
> + etsec_update_irq(etsec);
> }
>
> static void tx_padding_and_crc(eTSEC *etsec, uint32_t min_frame_len)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-13 1:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-28 2:42 [Qemu-devel] [PATCH] etsec: fix IRQ (un)masking Michael Davidsaver
2017-11-29 1:35 ` David Gibson
2018-07-07 18:06 ` Michael Davidsaver
2018-07-09 3:56 ` David Gibson
2018-07-09 6:20 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
2018-07-09 6:25 ` David Gibson
2018-07-12 21:00 ` [Qemu-devel] [PATCH 1/1] " Michael Davidsaver
2018-07-13 0:36 ` David Gibson
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.