[PATCH 0/1] e1000e: Added ICR clearing by corresponding IMS bit.

[PATCH 0/1] e1000e: Added ICR clearing by corresponding IMS bit.

[andrew]

From: Andrew Melnychenko <andrew@daynix.com>

Added E1000_ICR_ASSERTED check.

Andrew Melnychenko (1):
  e1000e: Added ICR clearing by corresponding IMS bit.

 hw/net/e1000e_core.c | 10 ++++++++++
 hw/net/trace-events  |  1 +
 2 files changed, 11 insertions(+)

-- 
2.26.2

[PATCH 1/1] e1000e: Added ICR clearing by corresponding IMS bit.

[andrew]

From: Andrew Melnychenko <andrew@daynix.com>

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
Added ICR clearing if there is IMS bit - according to the note by
section 13.3.27 of the 8257X developers manual.

Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
---
 hw/net/e1000e_core.c | 10 ++++++++++
 hw/net/trace-events  |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index d5676871fa..10212d7932 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2624,6 +2624,16 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
         e1000e_clear_ims_bits(core, core->mac[IAM]);
     }
 
+    /*
+     * PCIe* GbE Controllers Open Source Software Developer's Manual
+     * 13.3.27 Interrupt Cause Read Register
+     */
+    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
+        (core->mac[ICR] & core->mac[IMS])) {
+        trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], core->mac[IMS]);
+        core->mac[ICR] = 0;
+    }
+
     trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
     e1000e_update_interrupt_state(core);
     return ret;
diff --git a/hw/net/trace-events b/hw/net/trace-events
index e18f883cfd..46e40fcfa9 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -237,6 +237,7 @@ e1000e_irq_icr_read_entry(uint32_t icr) "Starting ICR read. Current ICR: 0x%x"
 e1000e_irq_icr_read_exit(uint32_t icr) "Ending ICR read. Current ICR: 0x%x"
 e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
 e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
+e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
 e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
 e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
 e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"
-- 
2.26.2

Re: [PATCH 1/1] e1000e: Added ICR clearing by corresponding IMS bit.

[Jason Wang]

On 2020/5/13 下午7:31, andrew@daynix.com wrote:
> From: Andrew Melnychenko <andrew@daynix.com>
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> Added ICR clearing if there is IMS bit - according to the note by
> section 13.3.27 of the 8257X developers manual.
>
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> ---
>   hw/net/e1000e_core.c | 10 ++++++++++
>   hw/net/trace-events  |  1 +
>   2 files changed, 11 insertions(+)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index d5676871fa..10212d7932 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2624,6 +2624,16 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>           e1000e_clear_ims_bits(core, core->mac[IAM]);
>       }
>   
> +    /*
> +     * PCIe* GbE Controllers Open Source Software Developer's Manual
> +     * 13.3.27 Interrupt Cause Read Register
> +     */
> +    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
> +        (core->mac[ICR] & core->mac[IMS])) {
> +        trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], core->mac[IMS]);
> +        core->mac[ICR] = 0;
> +    }
> +


Hi Andrew:

So my comments still. I think we need to implement 82574l behavior (if 
you go through e1000e.c all chapters it mentioned is for 82574l 
datasheet not the one you pointed to me).

And actually the 82574l behavior is much more simpler.

Thanks


>       trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
>       e1000e_update_interrupt_state(core);
>       return ret;
> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index e18f883cfd..46e40fcfa9 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -237,6 +237,7 @@ e1000e_irq_icr_read_entry(uint32_t icr) "Starting ICR read. Current ICR: 0x%x"
>   e1000e_irq_icr_read_exit(uint32_t icr) "Ending ICR read. Current ICR: 0x%x"
>   e1000e_irq_icr_clear_zero_ims(void) "Clearing ICR on read due to zero IMS"
>   e1000e_irq_icr_clear_iame(void) "Clearing ICR on read due to IAME"
> +e1000e_irq_icr_clear_icr_bit_ims(uint32_t icr, uint32_t ims) "Clearing ICR on read due corresponding IMS bit: 0x%x & 0x%x"
>   e1000e_irq_iam_clear_eiame(uint32_t iam, uint32_t cause) "Clearing IMS due to EIAME, IAM: 0x%X, cause: 0x%X"
>   e1000e_irq_icr_clear_eiac(uint32_t icr, uint32_t eiac) "Clearing ICR bits due to EIAC, ICR: 0x%X, EIAC: 0x%X"
>   e1000e_irq_ims_clear_set_imc(uint32_t val) "Clearing IMS bits due to IMC write 0x%x"

Re: [PATCH 1/1] e1000e: Added ICR clearing by corresponding IMS bit.

[Jason Wang]

On 2020/5/29 下午3:18, Jason Wang wrote:
>
> On 2020/5/13 下午7:31, andrew@daynix.com wrote:
>> From: Andrew Melnychenko <andrew@daynix.com>
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>> Added ICR clearing if there is IMS bit - according to the note by
>> section 13.3.27 of the 8257X developers manual.
>>
>> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>> ---
>>   hw/net/e1000e_core.c | 10 ++++++++++
>>   hw/net/trace-events  |  1 +
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> index d5676871fa..10212d7932 100644
>> --- a/hw/net/e1000e_core.c
>> +++ b/hw/net/e1000e_core.c
>> @@ -2624,6 +2624,16 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
>>           e1000e_clear_ims_bits(core, core->mac[IAM]);
>>       }
>>   +    /*
>> +     * PCIe* GbE Controllers Open Source Software Developer's Manual
>> +     * 13.3.27 Interrupt Cause Read Register
>> +     */
>> +    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>> +        (core->mac[ICR] & core->mac[IMS])) {
>> + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR], 
>> core->mac[IMS]);
>> +        core->mac[ICR] = 0;
>> +    }
>> +
>
>
> Hi Andrew:
>
> So my comments still. I think we need to implement 82574l behavior (if 
> you go through e1000e.c all chapters it mentioned is for 82574l 
> datasheet not the one you pointed to me).
>
> And actually the 82574l behavior is much more simpler.


To be more specific.

See chapter 7.4.5 which describes the ICR clearing.

It has three methods for clearing: auto-clear, clear-on-write and 
clear-on-read.

And in the part of "Read to clear" it said:

"""
All bits in the ICR register are cleared on a read to ICR.

"""

So there's no need to IMS and other stuffs here.

Thanks


>
> Thanks

Re: [PATCH 1/1] e1000e: Added ICR clearing by corresponding IMS bit.

[Andrew Melnichenko]

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

As I understand it, the e1000e.c was implemented by 82574L spec(
https://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
).
In the same spec there is 10.2.4 paragraph which provides more details when
ICR should be cleared.

> • Case 1 - Interrupt Mask register equals 0x0000 (mask all): ICR content
> is cleared.
> • Case 2 - Interrupt was asserted (ICR.INT_ASSERT=1) and auto mask is
> active: ICR
> content is cleared, and the IAM register is written to the IMC register.
> • Case 3 - Interrupt was not asserted (ICR.INT_ASSERT=0): Read has no side
> affect.


On Fri, May 29, 2020 at 10:35 AM Jason Wang <jasowang@redhat.com> wrote:

>
> On 2020/5/29 下午3:18, Jason Wang wrote:
> >
> > On 2020/5/13 下午7:31, andrew@daynix.com wrote:
> >> From: Andrew Melnychenko <andrew@daynix.com>
> >>
> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
> >> Added ICR clearing if there is IMS bit - according to the note by
> >> section 13.3.27 of the 8257X developers manual.
> >>
> >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> >> ---
> >>   hw/net/e1000e_core.c | 10 ++++++++++
> >>   hw/net/trace-events  |  1 +
> >>   2 files changed, 11 insertions(+)
> >>
> >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> >> index d5676871fa..10212d7932 100644
> >> --- a/hw/net/e1000e_core.c
> >> +++ b/hw/net/e1000e_core.c
> >> @@ -2624,6 +2624,16 @@ e1000e_mac_icr_read(E1000ECore *core, int index)
> >>           e1000e_clear_ims_bits(core, core->mac[IAM]);
> >>       }
> >>   +    /*
> >> +     * PCIe* GbE Controllers Open Source Software Developer's Manual
> >> +     * 13.3.27 Interrupt Cause Read Register
> >> +     */
> >> +    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
> >> +        (core->mac[ICR] & core->mac[IMS])) {
> >> + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
> >> core->mac[IMS]);
> >> +        core->mac[ICR] = 0;
> >> +    }
> >> +
> >
> >
> > Hi Andrew:
> >
> > So my comments still. I think we need to implement 82574l behavior (if
> > you go through e1000e.c all chapters it mentioned is for 82574l
> > datasheet not the one you pointed to me).
> >
> > And actually the 82574l behavior is much more simpler.
>
>
> To be more specific.
>
> See chapter 7.4.5 which describes the ICR clearing.
>
> It has three methods for clearing: auto-clear, clear-on-write and
> clear-on-read.
>
> And in the part of "Read to clear" it said:
>
> """
> All bits in the ICR register are cleared on a read to ICR.
>
> """
>
> So there's no need to IMS and other stuffs here.
>
> Thanks
>
>
> >
> > Thanks
>
>

[-- Attachment #2: Type: text/html, Size: 3968 bytes --]

Re: [PATCH 1/1] e1000e: Added ICR clearing by corresponding IMS bit.

[Jason Wang]

On 2020/6/2 上午12:47, Andrew Melnichenko wrote:
> As I understand it, the e1000e.c was implemented by 82574L 
> spec(https://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf).
> In the same spec there is 10.2.4 paragraph which provides more details 
> when ICR should be cleared.
>
>     • Case 1 - Interrupt Mask register equals 0x0000 (mask all): ICR
>     content is cleared.
>     • Case 2 - Interrupt was asserted (ICR.INT_ASSERT=1) and auto mask
>     is active: ICR
>     content is cleared, and the IAM register is written to the IMC
>     register.
>     • Case 3 - Interrupt was not asserted (ICR.INT_ASSERT=0): Read has
>     no side affect.
>

Thanks for the pointer, so it looks to me the current implementation is 
fine ?

static uint32_t
e1000e_mac_icr_read(E1000ECore *core, int index)
{
     uint32_t ret = core->mac[ICR];
     trace_e1000e_irq_icr_read_entry(ret);

     if (core->mac[IMS] == 0) {
         trace_e1000e_irq_icr_clear_zero_ims();
         core->mac[ICR] = 0;
     }


// This is the case 1)


     if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
         (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
         trace_e1000e_irq_icr_clear_iame();
         core->mac[ICR] = 0;
         trace_e1000e_irq_icr_process_iame();
         e1000e_clear_ims_bits(core, core->mac[IAM]);
     }


// This is the case 2) and case 3)

     trace_e1000e_irq_icr_read_exit(core->mac[ICR]);
     e1000e_update_interrupt_state(core);
     return ret;
}


Thanks


>
> On Fri, May 29, 2020 at 10:35 AM Jason Wang <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/5/29 下午3:18, Jason Wang wrote:
>     >
>     > On 2020/5/13 下午7:31, andrew@daynix.com
>     <mailto:andrew@daynix.com> wrote:
>     >> From: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     >>
>     >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1707441
>     >> Added ICR clearing if there is IMS bit - according to the note by
>     >> section 13.3.27 of the 8257X developers manual.
>     >>
>     >> Signed-off-by: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     >> ---
>     >>   hw/net/e1000e_core.c | 10 ++++++++++
>     >>   hw/net/trace-events  |  1 +
>     >>   2 files changed, 11 insertions(+)
>     >>
>     >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>     >> index d5676871fa..10212d7932 100644
>     >> --- a/hw/net/e1000e_core.c
>     >> +++ b/hw/net/e1000e_core.c
>     >> @@ -2624,6 +2624,16 @@ e1000e_mac_icr_read(E1000ECore *core,
>     int index)
>     >>           e1000e_clear_ims_bits(core, core->mac[IAM]);
>     >>       }
>     >>   +    /*
>     >> +     * PCIe* GbE Controllers Open Source Software Developer's
>     Manual
>     >> +     * 13.3.27 Interrupt Cause Read Register
>     >> +     */
>     >> +    if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
>     >> +        (core->mac[ICR] & core->mac[IMS])) {
>     >> + trace_e1000e_irq_icr_clear_icr_bit_ims(core->mac[ICR],
>     >> core->mac[IMS]);
>     >> +        core->mac[ICR] = 0;
>     >> +    }
>     >> +
>     >
>     >
>     > Hi Andrew:
>     >
>     > So my comments still. I think we need to implement 82574l
>     behavior (if
>     > you go through e1000e.c all chapters it mentioned is for 82574l
>     > datasheet not the one you pointed to me).
>     >
>     > And actually the 82574l behavior is much more simpler.
>
>
>     To be more specific.
>
>     See chapter 7.4.5 which describes the ICR clearing.
>
>     It has three methods for clearing: auto-clear, clear-on-write and
>     clear-on-read.
>
>     And in the part of "Read to clear" it said:
>
>     """
>     All bits in the ICR register are cleared on a read to ICR.
>
>     """
>
>     So there's no need to IMS and other stuffs here.
>
>     Thanks
>
>
>     >
>     > Thanks
>

[PATCH 0/1] e1000e: Added ICR clearing by corresponding IMS bit.

[andrew]

From: Andrew Melnychenko <andrew@daynix.com>

Added E1000_ICR_ASSERTED check.

Andrew Melnychenko (1):
  e1000e: Added ICR clearing by corresponding IMS bit.

 hw/net/e1000e_core.c | 9 +++++++++
 hw/net/trace-events  | 1 +
 2 files changed, 10 insertions(+)

-- 
2.26.2