Re: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-02 Thread Geert Uytterhoeven 
Hi Chris,

On Thu, Dec 1, 2016 at 7:53 PM, Chris Brandt  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 -- ge...@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

RE: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-01 Thread Chris Brandt 
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

Re: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-01 Thread Sergei Shtylyov 

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

RE: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-01 Thread Chris Brandt 
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

Re: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-01 Thread Sergei Shtylyov 

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:
[] sh_eth_interrupt
Disabling IRQ #21

Fixes: db893473d313a4ad ("sh_eth: Add support for r7s72100")
Signed-off-by: Chris Brandt 


Acked-by: Sergei Shtylyov 


   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

Re: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-01 Thread Sergei Shtylyov 

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

Re: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-01 Thread Sergei Shtylyov 

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:
[] sh_eth_interrupt
Disabling IRQ #21

Fixes: db893473d313a4ad ("sh_eth: Add support for r7s72100")
Signed-off-by: Chris Brandt 


Acked-by: Sergei Shtylyov 

MBR, Sergei

Re: [PATCH v2] sh_eth: remove unchecked interrupts

2016-12-01 Thread Geert Uytterhoeven 
Hi Chris,

On Thu, Dec 1, 2016 at 3:06 PM, Chris Brandt  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 -- ge...@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