| Message ID | 1401729456-23514-1-git-send-email-ben.dooks@codethink.co.uk |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 |