| Message ID | 20161201140642.28583-1-chris.brandt@renesas.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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 --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 |
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(-)