diff mbox

[v2] sh_eth: remove unchecked interrupts

Message ID 20161201140642.28583-1-chris.brandt@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Chris Brandt Dec. 1, 2016, 2:06 p.m. UTC
When streaming a lot of data and the RZ can't keep up, some status bits
will get set that are not being checked or cleared which cause the
following messages and the Ethernet driver to stop working. This
patch fixes that issue.

irq 21: nobody cared (try booting with the "irqpoll" option)
handlers:
[<c036b71c>] sh_eth_interrupt
Disabling IRQ #21

Fixes: db893473d313a4ad ("sh_eth: Add support for r7s72100")
Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
* switched from modifying eesr_err_check to modifying eesipr_value
---
 drivers/net/ethernet/renesas/sh_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Uytterhoeven Dec. 1, 2016, 2:42 p.m. UTC | #1
Hi Chris,

On Thu, Dec 1, 2016 at 3:06 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -518,7 +518,7 @@ static struct sh_eth_cpu_data r7s72100_data = {
>
>         .ecsr_value     = ECSR_ICD,
>         .ecsipr_value   = ECSIPR_ICDIP,
> -       .eesipr_value   = 0xff7f009f,
> +       .eesipr_value   = 0xe77f009f,

Comment not directly related to the merits of this patch: the EESIPR bit
definitions seem to be identical to those for EESR, so we can get rid of the
hardcoded values here?

>
>         .tx_check       = EESR_TC1 | EESR_FTC,
>         .eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Sergei Shtylyov Dec. 1, 2016, 5:36 p.m. UTC | #2
On 12/01/2016 05:06 PM, Chris Brandt wrote:

> When streaming a lot of data and the RZ can't keep up, some status bits
> will get set that are not being checked or cleared which cause the
> following messages and the Ethernet driver to stop working. This
> patch fixes that issue.
>
> irq 21: nobody cared (try booting with the "irqpoll" option)
> handlers:
> [<c036b71c>] sh_eth_interrupt
> Disabling IRQ #21
>
> Fixes: db893473d313a4ad ("sh_eth: Add support for r7s72100")
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei
Sergei Shtylyov Dec. 1, 2016, 5:37 p.m. UTC | #3
On 12/01/2016 05:42 PM, Geert Uytterhoeven wrote:

>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -518,7 +518,7 @@ static struct sh_eth_cpu_data r7s72100_data = {
>>
>>         .ecsr_value     = ECSR_ICD,
>>         .ecsipr_value   = ECSIPR_ICDIP,
>> -       .eesipr_value   = 0xff7f009f,
>> +       .eesipr_value   = 0xe77f009f,
>
> Comment not directly related to the merits of this patch: the EESIPR bit
> definitions seem to be identical to those for EESR, so we can get rid of the
> hardcoded values here?

    Do you mean using the @define's? We have EESIPR bits also declared, see 
enum DMAC_IM_BIT,

[...]

MBR, Sergei
Sergei Shtylyov Dec. 1, 2016, 5:40 p.m. UTC | #4
On 12/01/2016 08:36 PM, Sergei Shtylyov wrote:

>> When streaming a lot of data and the RZ can't keep up, some status bits
>> will get set that are not being checked or cleared which cause the
>> following messages and the Ethernet driver to stop working. This
>> patch fixes that issue.
>>
>> irq 21: nobody cared (try booting with the "irqpoll" option)
>> handlers:
>> [<c036b71c>] sh_eth_interrupt
>> Disabling IRQ #21
>>
>> Fixes: db893473d313a4ad ("sh_eth: Add support for r7s72100")
>> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
>
> Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

    One thing you've missed so far is mentioning R7S72100 (RZ/A1) in the 
subject. This driver supports many SoCs, you're only fixing one of them...

MBR, Sergei
Chris Brandt Dec. 1, 2016, 6:24 p.m. UTC | #5
On 12/1/2016, Sergei Shtylyov wrote:
>     One thing you've missed so far is mentioning R7S72100 (RZ/A1) in the

> subject. This driver supports many SoCs, you're only fixing one of them...


For the last sh_eth.c patch I submitted, I had:

"net: ethernet: renesas: sh_eth: add POST registers for rz"


Should I resend the patch as:

"[PATCH v3] sh_eth: remove unchecked interrupts for RZ/A1"


??

Chris
Sergei Shtylyov Dec. 1, 2016, 6:28 p.m. UTC | #6
On 12/01/2016 09:24 PM, Chris Brandt wrote:

>>     One thing you've missed so far is mentioning R7S72100 (RZ/A1) in the
>> subject. This driver supports many SoCs, you're only fixing one of them...
>
> For the last sh_eth.c patch I submitted, I had:
>
> "net: ethernet: renesas: sh_eth: add POST registers for rz"
>
>
> Should I resend the patch as:
>
> "[PATCH v3] sh_eth: remove unchecked interrupts for RZ/A1"
>
>
> ??

    Yes, that will do.

> Chris

MBR, Sergei
Chris Brandt Dec. 1, 2016, 6:53 p.m. UTC | #7
Hi Geert,

On 12/1/2016, Sergei Shtylyov wrote:
> 

> On 12/01/2016 05:42 PM, Geert Uytterhoeven wrote:

> 

> >> --- a/drivers/net/ethernet/renesas/sh_eth.c

> >> +++ b/drivers/net/ethernet/renesas/sh_eth.c

> >> @@ -518,7 +518,7 @@ static struct sh_eth_cpu_data r7s72100_data = {

> >>

> >>         .ecsr_value     = ECSR_ICD,

> >>         .ecsipr_value   = ECSIPR_ICDIP,

> >> -       .eesipr_value   = 0xff7f009f,

> >> +       .eesipr_value   = 0xe77f009f,

> >

> > Comment not directly related to the merits of this patch: the EESIPR

> > bit definitions seem to be identical to those for EESR, so we can get

> > rid of the hardcoded values here?

> 

>     Do you mean using the @define's? We have EESIPR bits also declared,

> see enum DMAC_IM_BIT,



Is your idea to get rid of .eesipr_value for devices that have values
that are the same as .eesr_err_check?


For example in sh_eth_dev_init():

	sh_eth_modify(ndev, EESR, 0, 0);
	mdp->irq_enabled = true;
-	sh_eth_write(ndev, mdp->cd->eesipr_value, EESIPR);
+	if (mdp->cd->eesipr_value)
+		sh_eth_write(ndev, mdp->cd->eesipr_value, EESIPR);
+	else
+		sh_eth_write(ndev, mdp->cd->eesr_err_check, EESIPR);


Chris
Geert Uytterhoeven Dec. 2, 2016, 12:18 p.m. UTC | #8
Hi Chris,

On Thu, Dec 1, 2016 at 7:53 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On 12/1/2016, Sergei Shtylyov wrote:
>>
>> On 12/01/2016 05:42 PM, Geert Uytterhoeven wrote:
>>
>> >> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> >> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> >> @@ -518,7 +518,7 @@ static struct sh_eth_cpu_data r7s72100_data = {
>> >>
>> >>         .ecsr_value     = ECSR_ICD,
>> >>         .ecsipr_value   = ECSIPR_ICDIP,
>> >> -       .eesipr_value   = 0xff7f009f,
>> >> +       .eesipr_value   = 0xe77f009f,
>> >
>> > Comment not directly related to the merits of this patch: the EESIPR
>> > bit definitions seem to be identical to those for EESR, so we can get
>> > rid of the hardcoded values here?
>>
>>     Do you mean using the @define's? We have EESIPR bits also declared,
>> see enum DMAC_IM_BIT,

Yes, that's what I meant.
Unfortunately the DMAC_IM_BIT enum doesn't cover all bits.

> Is your idea to get rid of .eesipr_value for devices that have values
> that are the same as .eesr_err_check?
>
>
> For example in sh_eth_dev_init():
>
>         sh_eth_modify(ndev, EESR, 0, 0);
>         mdp->irq_enabled = true;
> -       sh_eth_write(ndev, mdp->cd->eesipr_value, EESIPR);
> +       if (mdp->cd->eesipr_value)
> +               sh_eth_write(ndev, mdp->cd->eesipr_value, EESIPR);
> +       else
> +               sh_eth_write(ndev, mdp->cd->eesr_err_check, EESIPR);

No, my intention was to just get rid of the hardcoded values when
initializing .eesipr_value.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox

Patch

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 05b0dc5..1a92de7 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -518,7 +518,7 @@  static struct sh_eth_cpu_data r7s72100_data = {
 
 	.ecsr_value	= ECSR_ICD,
 	.ecsipr_value	= ECSIPR_ICDIP,
-	.eesipr_value	= 0xff7f009f,
+	.eesipr_value	= 0xe77f009f,
 
 	.tx_check	= EESR_TC1 | EESR_FTC,
 	.eesr_err_check	= EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |