[PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Ben Dooks]

The current behaviour of the sh_eth driver is not to use the RNC bit
for the receive ring. This means that every packet recieved is not only
generating an IRQ but it also stops the receive ring DMA as well until
the driver re-enables it after unloading the packet.

This means that a number of the following errors are generated due to
the receive packet FIFO overflowing due to nowhere to put packets:

	net eth0: Receive FIFO Overflow

I have tested the RMCR_RNC configuration with NFS root filesystem and
the driver has not failed yet.  There are further test reports from
Sergei Shtylov and others for both the R8A7790 and R8A7791.

There is also feedback fron Cao Minh Hiep[1] which reports the
same issue in (http://comments.gmane.org/gmane.linux.network/316285)
showing this fixes issues with losing UDP datagrams under iperf.

Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

---
Cc: nobuhiro.iwamatsu.yj@renesas.com
Cc: magnus.damn@opensource.se
Cc: horms@verge.net.au
Cc: yoshihiro.shimoda.uh@renesas.com
Cc: cm-hiep@jinso.co.jp

Changes since v1:
	- Fixed title and body to contain R8A7791
	- Updated list of CC:
---
 drivers/net/ethernet/renesas/sh_eth.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 6a9509c..1e3e5d2 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -468,6 +468,7 @@ static struct sh_eth_cpu_data r8a779x_data = {
 	.ecsr_value	= ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
 	.ecsipr_value	= ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
 	.eesipr_value	= 0x01ff009f,
+	.rmcr_value	= RMCR_RNC,
 
 	.tx_check	= EESR_FTC | EESR_CND | EESR_DLC | EESR_CD | EESR_RTO,
 	.eesr_err_check	= EESR_TWB | EESR_TABT | EESR_RABT | EESR_RFE |
-- 
2.0.0.rc2

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[David Miller]

From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Mon,  2 Jun 2014 18:17:36 +0100

> The current behaviour of the sh_eth driver is not to use the RNC bit
> for the receive ring. This means that every packet recieved is not only
> generating an IRQ but it also stops the receive ring DMA as well until
> the driver re-enables it after unloading the packet.
> 
> This means that a number of the following errors are generated due to
> the receive packet FIFO overflowing due to nowhere to put packets:
> 
> 	net eth0: Receive FIFO Overflow
> 
> I have tested the RMCR_RNC configuration with NFS root filesystem and
> the driver has not failed yet.  There are further test reports from
> Sergei Shtylov and others for both the R8A7790 and R8A7791.
> 
> There is also feedback fron Cao Minh Hiep[1] which reports the
> same issue in (http://comments.gmane.org/gmane.linux.network/316285)
> showing this fixes issues with losing UDP datagrams under iperf.
> 
> Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Given the description, I can't fathom a reason why this wouldn't be
set always, for every chip.

Do some chips not implement this bit at all?

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Ben Dooks]

On 02/06/14 19:53, David Miller wrote:
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> Date: Mon,  2 Jun 2014 18:17:36 +0100
> 
>> The current behaviour of the sh_eth driver is not to use the RNC bit
>> for the receive ring. This means that every packet recieved is not only
>> generating an IRQ but it also stops the receive ring DMA as well until
>> the driver re-enables it after unloading the packet.
>>
>> This means that a number of the following errors are generated due to
>> the receive packet FIFO overflowing due to nowhere to put packets:
>>
>> 	net eth0: Receive FIFO Overflow
>>
>> I have tested the RMCR_RNC configuration with NFS root filesystem and
>> the driver has not failed yet.  There are further test reports from
>> Sergei Shtylov and others for both the R8A7790 and R8A7791.
>>
>> There is also feedback fron Cao Minh Hiep[1] which reports the
>> same issue in (http://comments.gmane.org/gmane.linux.network/316285)
>> showing this fixes issues with losing UDP datagrams under iperf.
>>
>> Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> 
> Given the description, I can't fathom a reason why this wouldn't be
> set always, for every chip.
> 
> Do some chips not implement this bit at all?

I do not have access to enough of the sh-eth capable data-sheets
to know why this is.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Sergei Shtylyov]

Hello.

On 06/02/2014 10:53 PM, David Miller wrote:

>> The current behaviour of the sh_eth driver is not to use the RNC bit
>> for the receive ring. This means that every packet recieved is not only
>> generating an IRQ but it also stops the receive ring DMA as well until
>> the driver re-enables it after unloading the packet.

>> This means that a number of the following errors are generated due to
>> the receive packet FIFO overflowing due to nowhere to put packets:

>> 	net eth0: Receive FIFO Overflow

>> I have tested the RMCR_RNC configuration with NFS root filesystem and
>> the driver has not failed yet.  There are further test reports from
>> Sergei Shtylov and others for both the R8A7790 and R8A7791.

>> There is also feedback fron Cao Minh Hiep[1] which reports the
>> same issue in (http://comments.gmane.org/gmane.linux.network/316285)
>> showing this fixes issues with losing UDP datagrams under iperf.

>> Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

> Given the description, I can't fathom a reason why this wouldn't be
> set always, for every chip.

> Do some chips not implement this bit at all?

    Looks like the early SH2/3 SoCs didn't implement the whole register.
Despite that, sh_eth_dev_init() always writes to this register... :-/
So far, the RMCR.RNC bit was mostly set for the Gigabit-capable controllers, 
however that rule wasn't strictly followed. Well, this driver is still a mess, 
and it's hard to deal with it without the necessary documentation.

WBR, Sergei

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 02 Jun 2014 23:33:47 +0400

>    Looks like the early SH2/3 SoCs didn't implement the whole register.
> Despite that, sh_eth_dev_init() always writes to this register... :-/
> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
> controllers, however that rule wasn't strictly followed. Well, this
> driver is still a mess, and it's hard to deal with it without the
> necessary documentation.

Why don't we therefore:

1) Skip the register write if the per-chip value is zero.

2) Add the RNC bit to all of the gigabit capable controllers.

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Sergei Shtylyov]

On 06/03/2014 12:49 AM, David Miller wrote:

>>     Looks like the early SH2/3 SoCs didn't implement the whole register.
>> Despite that, sh_eth_dev_init() always writes to this register... :-/
>> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
>> controllers, however that rule wasn't strictly followed. Well, this
>> driver is still a mess, and it's hard to deal with it without the
>> necessary documentation.

> Why don't we therefore:

> 1) Skip the register write if the per-chip value is zero.

    I rather thought about not writing when the register is not implemented.
I'll probably look into this when I have time.

> 2) Add the RNC bit to all of the gigabit capable controllers.

    I probably misspoke -- all the Gigabit controllers already have it set, 
it's just that some 100 MBbps ones have it set, but most don't.

WBR, Sergei

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Tue, 03 Jun 2014 00:55:38 +0400

> On 06/03/2014 12:49 AM, David Miller wrote:
> 
>>>     Looks like the early SH2/3 SoCs didn't implement the whole register.
>>> Despite that, sh_eth_dev_init() always writes to this register... :-/
>>> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
>>> controllers, however that rule wasn't strictly followed. Well, this
>>> driver is still a mess, and it's hard to deal with it without the
>>> necessary documentation.
> 
>> Why don't we therefore:
> 
>> 1) Skip the register write if the per-chip value is zero.
> 
>    I rather thought about not writing when the register is not
>    implemented.
> I'll probably look into this when I have time.
> 
>> 2) Add the RNC bit to all of the gigabit capable controllers.
> 
>    I probably misspoke -- all the Gigabit controllers already have it
>    set, it's just that some 100 MBbps ones have it set, but most don't.

So these chips that do not implement the register, they only process
one RX descriptor at a time until the interrupt handler re-enables
DMA receive?

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Sergei Shtylyov]

Hello.

On 06/03/2014 01:05 AM, David Miller wrote:

>>>>      Looks like the early SH2/3 SoCs didn't implement the whole register.
>>>> Despite that, sh_eth_dev_init() always writes to this register... :-/
>>>> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
>>>> controllers, however that rule wasn't strictly followed. Well, this
>>>> driver is still a mess, and it's hard to deal with it without the
>>>> necessary documentation.

>>> Why don't we therefore:

>>> 1) Skip the register write if the per-chip value is zero.

>>     I rather thought about not writing when the register is not
>>     implemented.
>> I'll probably look into this when I have time.

>>> 2) Add the RNC bit to all of the gigabit capable controllers.

>>     I probably misspoke -- all the Gigabit controllers already have it
>>     set, it's just that some 100 MBbps ones have it set, but most don't.

> So these chips that do not implement the register, they only process
> one RX descriptor at a time until the interrupt handler re-enables
> DMA receive?

    I just don't know. Looks like the driver is broken on SH2/3 even more than 
I thought: it always reads the EDRRR register in sh_eth_rx() trying to 
understand if the reception has been stopped but that register doesn't seem to 
exist on SH2/3. Moreover, sh_eth_interrupt() reads EESR in order to determine 
the interrupt status but that register doesn't seem to exist on SH2/3 either!

WBR, Sergei

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Sergei Shtylyov]

Hello.

On 06/03/2014 01:17 AM, Sergei Shtylyov wrote:

>>>>>      Looks like the early SH2/3 SoCs didn't implement the whole register.
>>>>> Despite that, sh_eth_dev_init() always writes to this register... :-/
>>>>> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
>>>>> controllers, however that rule wasn't strictly followed. Well, this
>>>>> driver is still a mess, and it's hard to deal with it without the
>>>>> necessary documentation.

>>>> Why don't we therefore:

>>>> 1) Skip the register write if the per-chip value is zero.

>>>     I rather thought about not writing when the register is not
>>>     implemented.
>>> I'll probably look into this when I have time.

>>>> 2) Add the RNC bit to all of the gigabit capable controllers.

>>>     I probably misspoke -- all the Gigabit controllers already have it
>>>     set, it's just that some 100 MBbps ones have it set, but most don't.

>> So these chips that do not implement the register, they only process
>> one RX descriptor at a time until the interrupt handler re-enables
>> DMA receive?

>     I just don't know. Looks like the driver is broken on SH2/3 even more than
> I thought: it always reads the EDRRR register in sh_eth_rx() trying to
> understand if the reception has been stopped but that register doesn't seem to
> exist on SH2/3. Moreover, sh_eth_interrupt() reads EESR in order to determine
> the interrupt status but that register doesn't seem to exist on SH2/3 either!

    OK, I've chased down the commit that broke SH2/3 support 3+ years ago; 
here it is:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4a55530f38e4eeee3afb06093e81309138fe8360

    All the registers I've mentioned did exist on SH2/3, they just got missed 
in the mapping arrays.

WBR, Sergei

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Yoshihiro Shimoda]

Hi David,

(2014/06/03 5:49), David Miller wrote:
> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Date: Mon, 02 Jun 2014 23:33:47 +0400
> 
>>    Looks like the early SH2/3 SoCs didn't implement the whole register.
>> Despite that, sh_eth_dev_init() always writes to this register... :-/
>> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
>> controllers, however that rule wasn't strictly followed. Well, this
>> driver is still a mess, and it's hard to deal with it without the
>> necessary documentation.
> 
> Why don't we therefore:
> 
> 1) Skip the register write if the per-chip value is zero.
> 
> 2) Add the RNC bit to all of the gigabit capable controllers.
> 

Thank you for the suggestion.

I checked all of datasheets about the following LSIs.
 - sh7619, sh7710, sh7724, sh7734, sh7757(ether, gther),
   sh7763, r7s72100, r8a7740, r8a7778, r8a7790, r8a7791
   (There are in the "static struct platform_device_id sh_eth_id_table[]".)

And then, I found all of these LSIs have the "RNC" bit in the register.
Accurately some LSIs have a difference bit name like "RNR" or "RR". (I don't know why...)
However, the behavior of the bit is the same as all of these LSIs.

So, I think we are able to:
 - add the RNC bit to all of the ethernet controllers.
 - remove ".rmcr_value" in the "struct sh_eth_cpu_data".

Best regargds,
Yoshihiro Shimoda

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Ben Dooks]

On 02/06/14 23:26, Sergei Shtylyov wrote:
> Hello.
> 
> On 06/03/2014 01:17 AM, Sergei Shtylyov wrote:
> 
>>>>>>      Looks like the early SH2/3 SoCs didn't implement the whole
>>>>>> register.
>>>>>> Despite that, sh_eth_dev_init() always writes to this register... :-/
>>>>>> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
>>>>>> controllers, however that rule wasn't strictly followed. Well, this
>>>>>> driver is still a mess, and it's hard to deal with it without the
>>>>>> necessary documentation.
> 
>>>>> Why don't we therefore:
> 
>>>>> 1) Skip the register write if the per-chip value is zero.
> 
>>>>     I rather thought about not writing when the register is not
>>>>     implemented.
>>>> I'll probably look into this when I have time.
> 
>>>>> 2) Add the RNC bit to all of the gigabit capable controllers.
> 
>>>>     I probably misspoke -- all the Gigabit controllers already have it
>>>>     set, it's just that some 100 MBbps ones have it set, but most
>>>> don't.
> 
>>> So these chips that do not implement the register, they only process
>>> one RX descriptor at a time until the interrupt handler re-enables
>>> DMA receive?
> 
>>     I just don't know. Looks like the driver is broken on SH2/3 even
>> more than
>> I thought: it always reads the EDRRR register in sh_eth_rx() trying to
>> understand if the reception has been stopped but that register doesn't
>> seem to
>> exist on SH2/3. Moreover, sh_eth_interrupt() reads EESR in order to
>> determine
>> the interrupt status but that register doesn't seem to exist on SH2/3
>> either!
> 
>    OK, I've chased down the commit that broke SH2/3 support 3+ years
> ago; here it is:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4a55530f38e4eeee3afb06093e81309138fe8360
> 
> 
>    All the registers I've mentioned did exist on SH2/3, they just got
> missed in the mapping arrays.

I suppose it would be a good idea to submit a patch to add these then.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH v2] sh_eth: use RNC mode for R8A7790/R87791

[Sergei Shtylyov]

Hello.

On 06/03/2014 03:19 PM, Ben Dooks wrote:

>>>>>>>       Looks like the early SH2/3 SoCs didn't implement the whole
>>>>>>> register.
>>>>>>> Despite that, sh_eth_dev_init() always writes to this register... :-/
>>>>>>> So far, the RMCR.RNC bit was mostly set for the Gigabit-capable
>>>>>>> controllers, however that rule wasn't strictly followed. Well, this
>>>>>>> driver is still a mess, and it's hard to deal with it without the
>>>>>>> necessary documentation.

>>>>>> Why don't we therefore:

>>>>>> 1) Skip the register write if the per-chip value is zero.

>>>>>      I rather thought about not writing when the register is not
>>>>>      implemented.
>>>>> I'll probably look into this when I have time.

>>>>>> 2) Add the RNC bit to all of the gigabit capable controllers.

>>>>>      I probably misspoke -- all the Gigabit controllers already have it
>>>>> set, it's just that some 100 MBbps ones have it set, but most
>>>>> don't.

>>>> So these chips that do not implement the register, they only process
>>>> one RX descriptor at a time until the interrupt handler re-enables
>>>> DMA receive?

>>>      I just don't know. Looks like the driver is broken on SH2/3 even
>>> more than
>>> I thought: it always reads the EDRRR register in sh_eth_rx() trying to
>>> understand if the reception has been stopped but that register doesn't
>>> seem to
>>> exist on SH2/3. Moreover, sh_eth_interrupt() reads EESR in order to
>>> determine
>>> the interrupt status but that register doesn't seem to exist on SH2/3
>>> either!

>>     OK, I've chased down the commit that broke SH2/3 support 3+ years
>> ago; here it is:

>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4a55530f38e4eeee3afb06093e81309138fe8360

>>     All the registers I've mentioned did exist on SH2/3, they just got
>> missed in the mapping arrays.

> I suppose it would be a good idea to submit a patch to add these then.

    Not that I'm supposed to fix the old SH machines now but I've just posted 
the patch. :-)

WBR, Sergei